Add NOMKSTREAM support to XADD - #3186
Conversation
Adds a nomkstream option to StreamAdd/StreamAddAsync so callers can append to a stream without implicitly creating it, matching the XADD NOMKSTREAM flag (Redis 6.2+). Existing overloads keep their signatures for binary compatibility; new all-optional overloads carry the flag. Closes StackExchange#3138.
6ccd600 to
9c3822c
Compare
mgravell
left a comment
mgravell
left a comment
There was a problem hiding this comment.
Thanks. The wire mechanics are right: NOMKSTREAM correctly precedes MAXLEN, both GetStreamAddMessage overloads have their totalLength arithmetic updated, and the round-trip test pins the exact bytes, which is the right way to prove it. The issues are in the API shape, plus one demonstrable source break.
1. nomkstream should not be a wire constant, and we already have this concept
We already expose MKSTREAM, on the sibling command:
bool StreamCreateConsumerGroup(RedisKey key, RedisValue groupName, RedisValue? position = null, bool createStream = true, CommandFlags flags = CommandFlags.None);So this is the same flag, on the same data type, spelled two different ways with opposite polarity. Please make it bool createStream = true, so XGROUP CREATE and XADD read identically and neither leaks the wire keyword:
db.StreamAdd(key, "field", "value", createStream: false);(The other candidate is When when = When.Always, with When.Exists meaning NOMKSTREAM, since that is the library-wide vocabulary for NX/XX. I prefer createStream here purely because it matches the sibling stream command, but if you would rather unify both commands on When, say so and we do that instead.)
2. The parameter list is the real problem
Adding one bool costs 4 interface signatures x 2 (sync/async), 4 KeyPrefixed/KeyPrefixedDatabase forwarders, 8 rewritten PublicAPI lines and 8 RS0026/RS0027 suppressions. This signature has already grown twice (int? -> long? plus limit plus trimMode, then the StreamIdempotentId variants), and XADD still has an option we do not expose at all: MINID trimming (we have StreamTrimByMinId for XTRIM, but nothing on XADD), which today's bool useApproximateMaxLength cannot express either. So the next request pays that same multiplier again.
Worth deciding now rather than after this merges, because adding these four overloads and then an options type later leaves us maintaining twelve. Something like:
RedisValue StreamAdd(RedisKey key, RedisValue field, RedisValue value, StreamAddOptions options, CommandFlags flags = CommandFlags.None);
RedisValue StreamAdd(RedisKey key, NameValueEntry[] pairs, StreamAddOptions options, CommandFlags flags = CommandFlags.None);carrying MessageId / IdempotentId / MaxLength / MinId / approximate / Limit / TrimMode / CreateStream, and every future XADD option lands with no new signatures. StreamConfiguration is the local precedent for that shape, and note it is a class, which matters here: as a struct, default would give CreateStream == false, i.e. the unsafe polarity, so either follow StreamConfiguration or store the negative internally.
I am not asking you to build that speculatively - it is a call for me to make. But I would rather make it before we add the overloads than after.
3. Source break: the new parameter is inserted mid-list
nomkstream goes between useApproximateMaxLength and limit, so positional callers no longer compile. Against this branch:
db.StreamAdd(key, "f", "v", (RedisValue?)"1-1", 5L, true, 10L, StreamTrimMode.DeleteReferences);
// error CS1503: Argument 7: cannot convert from 'long' to 'bool'
// error CS1503: Argument 8: cannot convert from 'StackExchange.Redis.StreamTrimMode' to 'long?'That compiles on main and does not here, because the old overload lost its defaults (so it needs all nine arguments) and the new one now wants a bool in slot 7.
#3135 did the same strip-the-defaults manoeuvre for StreamReadGroup, but appended maxCount/maxSize at the end, immediately before flags, which is why it did not break positional callers. Please follow that: put createStream after trimMode.
4. Optional: keep the defaults and use [OverloadResolutionPriority]
AGENTS.md points at this, and RedisValue/RedisChannel already use it. Keeping the defaults on both overloads and putting [OverloadResolutionPriority(1)] on the new one compiles cleanly for every call shape I tried, including the bare three-argument call and the positional forms above, and it leaves PublicAPI.Shipped.txt completely untouched (new lines in Unshipped.txt only), which is a nicer diff to review and to trust. I checked this works for exactly this signature pair before suggesting it. Your call whether we adopt it here or stay with the #3135 pattern.
5. Tests
KeyPrefixedDatabaseTestsandKeyPrefixedTestshave one test per overload (StreamAdd_1,StreamAdd_2,StreamAdd_WithTrimMode_1/_2). This adds four forwarders with no corresponding tests; those forwarders are exactly where a copy-paste slip silently drops the prefix.- The round-trip test only covers the single-pair, non-idempotent builder. Please add the
NameValueEntry[]builder, and a case combiningcreateStream: falsewith aStreamIdempotentId: the relative order ofNOMKSTREAMand the IDMP arguments is currently unverified, and that is precisely the kind of thing a round-trip test exists to pin.
6. Minor
- Six
#pragma warning disable RS0026/RS0027blocks: prefer one region-level suppression carrying a reason, as atIDatabase.cs:3248. docs/Streams.md: worth saying the returned value isRedisValue.Null(and that this needs 6.2 or later), rather than "a nullRedisValue".
Reworks the public surface: instead of inserting a `bool nomkstream` into the
four StreamAdd overload families and their async counterparts, the new options
travel on a required `StreamAddOptions` parameter, adding two overloads per
interface rather than eight.
Because `options` is required it cannot tie with the shipped overloads, so those
keep their signatures *and* their default values - PublicAPI.Shipped.txt is
untouched. That matters: stripping the defaults broke positional callers, e.g.
db.StreamAdd(key, "f", "v", (RedisValue?)"1-1", 5L, true, 10L, StreamTrimMode.DeleteReferences);
which no longer bound to either overload once `nomkstream` sat mid-list.
[OverloadResolutionPriority] would also have disambiguated, but it is only
honoured at LangVersion 13+, so a C# 12 consumer would get CS0121 instead.
The flag is spelled `CreateStream` (default true), matching the MKSTREAM
parameter we already expose as `StreamCreateConsumerGroup(..., createStream,
...)`, rather than leaking the wire keyword. Since the options type takes future
XADD options for free, MINID trimming is included too - previously reachable
only through XTRIM - so XADD's grammar is now fully expressible.
Notes:
- StreamAddOptions is a readonly struct, passed by value publicly and by `in` to
the message builders. CreateStream is stored inverted so that `default` means
"create the stream", matching the command's own default.
- Both builders now share the prefix arithmetic and emission, so NOMKSTREAM,
MAXLEN|MINID [~], LIMIT, the trim-mode keyword and the idempotency arguments
can only be ordered one way.
- The options overloads validate up front (MaxLength xor MinId, MessageId xor
IdempotentId, LIMIT requiring ~ and a threshold); the shipped positional
overloads deliberately do not, preserving their current behaviour of letting
the server rule on odd combinations.
|
Pushed the rework onto this branch rather than asking you to redo the surface twice - the message-builder work, the wire ordering and the round-trip test were the right parts, and they survive. What changed, against the review above: the flag now travels on a required Two corrections to what I wrote earlier:
Tests: the round-trip file now covers both builders, NOMKSTREAM before MAXLEN, MINID exact and approximate, LIMIT, the trim-mode keyword, NOMKSTREAM combined with both One deliberate asymmetry worth knowing about: the options overloads validate up front ( |
2 participants
Summary
nomkstreamoption toStreamAdd/StreamAddAsync(single field/value andNameValueEntry[]shapes, plain andStreamIdempotentIdoverloads) that maps to theXADD ... NOMKSTREAMflag introduced in Redis 6.2 — when set,XADDwill not create the stream if it doesn't already exist and instead returnsnull.StreamAdd/StreamAddAsyncsignatures keep their exact parameter lists for binary compatibility (their optional defaults were removed source-side so calls rebind to the new all-optional overload carryingnomkstream, per the project's back-compat guidance inAGENTS.md).IDatabase/IDatabaseAsync,RedisDatabase, and theKeyspaceIsolation(KeyPrefixedDatabase/KeyPrefixed) forwarders.PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtaccordingly.Closes #3138.
Test plan
dotnet build Build.csproj -c Release /p:CI=true— 0 warnings, 0 errors.dotnet test tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj -f net10.0 --filter "FullyQualifiedName~Stream"— 472 passed, 2 skipped (pre-existing, unrelated), 0 failed, against local docker Redis topology.StreamAddNoMkStreamintegration test inStreamTests.cs(sync/async x single-pair/array) verifying a nonexistent stream stays absent whennomkstream: true, and that adds succeed once the stream exists.StreamAddRoundTripunit test asserting the exact outbound RESP bytes placeNOMKSTREAMbeforeMAXLEN/LIMIT, and that it's omitted whennomkstreamis false.docs/Streams.md.🤖 Generated with Claude Code