fix: case-insensitive SessionId and reserved-header handling for ASB (#4054)

[iancooper]

What

Fixes #4054 — an ASB SessionId set from a Brighter Outbox was silently dropped, and camelCased reserved keys leaked into ApplicationProperties.

Root cause

Brighter's Outbox serializes the header Bag with JsonSerialisationOptions.Options, whose PropertyNamingPolicy rewrites bag keys (see DictionaryStringObjectJsonConverter.Write). Under the default JsonNamingPolicy.CamelCase, a key written as "SessionId" comes back as "sessionId" after a round-trip.

In AzureServiceBusMessagePublisher:

• Bag.TryGetValue("SessionId", …) is an exact match, so it missed the camelCased "sessionId" key and the session was never set on the outgoing message.
• The reserved-header filter ReservedHeaders.Contains(h.Key) was also case-sensitive, so the camelCased reserved keys (sessionId, messageType, handledCount, replyTo) leaked into ApplicationProperties.

Fix

Resolve bag keys using the same naming policy that serialized them, rather than a fixed case comparison.

A first pass used OrdinalIgnoreCase, but that only works for the default camelCase policy (camelCase differs from PascalCase by leading case only). JsonSerialisationOptions.Options has a public setter, so a user can override PropertyNamingPolicy — under e.g. SnakeCaseLower, "SessionId" round-trips as "session_id" and a case-insensitive match fails again, reintroducing the bug.

ASBConstants.IsBagKey / IsReservedHeader now match a bag key against either the raw reserved key or the key as the active PropertyNamingPolicy renders it (read at call time, so a later Options override is honoured), case-insensitive on top. The publisher uses these for both the SessionId lookup and the reserved-header filter. This is correct under camelCase, snake_case, a custom policy, or none.

Tests

When_Converting_A_Message_With_A_Session_Id.cs is now a [Theory] over CamelCase, SnakeCaseLower, and no policy. Each case round-trips the Bag through JsonSerialisationOptions.Options exactly as the Outbox does (serialize → deserialize), so the key is mangled by the real policy rather than a hand-written string, then asserts the SessionId is set and the reserved key is kept out of ApplicationProperties. The tests live in a parallelism-disabled collection that snapshots and restores Options in a finally, so mutating the global policy can't leak into other tests. The SnakeCaseLower case fails the earlier OrdinalIgnoreCase approach and passes the policy-aware one.

Scope / follow-up

Kept to the ASB publisher. A survey found the same exact-match pattern elsewhere; those are tracked in #4214 (RocketMQ Keys/Tag broken under camelCase; AWS messageDeduplicationId and core ProducerTopic latent under a non-casing policy; AzureServiceBusScheduler same smell but not reproducible — it converts an empty freshly-built bag). #4214 also proposes lifting a shared policy-aware lookup into Paramore.Brighter core.

🤖 Generated with Claude Code

[codescene-delta-analysis]

Our agent can fix these. Install it.

Gates Passed
4 Quality Gates Passed

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[iancooper]

@Jonny-Freemarket Any thoughts?

[claude]

Code Review — PR #4205: case-insensitive SessionId & reserved-header handling (ASB)

Reviewed the diff against the surrounding code and traced the serialization path. Overall: correct, well-scoped fix with an accurate root-cause diagnosis. I verified the claim independently — DictionaryStringObjectJsonConverter.Write (line 57) applies options.PropertyNamingPolicy.ConvertName to every bag key, and JsonSerialisationOptions sets CamelCase, so a bag key written as SessionId does round-trip back as sessionId. The JsonSerialisationOptions XML doc even calls this out explicitly. Good detective work, and the in-code comments make the non-obvious behavior discoverable for the next maintainer.

Correctness ✅

• FirstOrDefault(...).Value on the Dictionary<string,object> returns default(KeyValuePair<…>) = (null, null) when no match, so the is not null guard is safe — no KeyNotFoundException/NRE risk.
• Stripping reserved headers case-insensitively and then re-adding MessageType/HandledCount/ReplyTo from the canonical header properties is the right ordering: the camelCased duplicates are removed and the authoritative values re-added.

Suggestions (non-blocking)

1. Test coverage gap vs. the stated fix scope. The PR body says messageType, handledCount, and replyTo also leaked into ApplicationProperties when camelCased, and the reserved-header filter change addresses that — but the new test only exercises SessionId/sessionId. Since the reserved-header leak is half the fix, consider a [Theory] (or extra cases) asserting that camelCased messageType/handledCount/replyTo bag keys do not appear in ApplicationProperties. Otherwise a future regression to the ReservedHeaders.Contains comparer would slip through untested.

2. Behavior change worth noting. Reserved-header stripping is now case-insensitive across the board, so a user-supplied custom property whose key differs from a reserved name only by case (e.g. sessionid, Replyto) will now be stripped where it previously passed through. This is consistent with the fix's intent and almost certainly desirable, but it is a (minor) observable change for that edge case.

3. Minor: ambiguous-casing edge case. A Dictionary<string,object> can legitimately hold both SessionId and sessionId simultaneously. FirstOrDefault then resolves to whichever the dictionary enumerates first, which isn't a guaranteed order. Extremely unlikely in practice (a round-trip produces only the camelCased form), so not worth code — just flagging.

4. Micro-perf (negligible). The lookup moved from an O(1) TryGetValue to an O(n) FirstOrDefault scan, and the bag is now iterated twice. The header bag is tiny, so this is irrelevant — mentioning only for completeness.

Conventions ✅

• New test follows repo naming (When_Converting_… file, AzureServiceBusMessagePublisher…Tests class, [Trait("Category","ASB")]), matching siblings like When_Converting_A_Message_With_Local_Headers.cs.
• A test was added with the fix, consistent with the repo's TDD guidance.

Nice fix — the one thing I'd genuinely push on before merge is adding the reserved-header-leak assertions (suggestion #1) so the second half of the change is actually covered by a test.

🤖 Generated with Claude Code

[iancooper]

A note to self here, I wonder if the issue is more that we need to look at the options, which the user could override, and use those when we look into the bag. Otherwise, if someone changes the JsonNamingPolicy, we can't be sure that we won't have a different bug

[claude]

Code Review — PR #4205: case-insensitive SessionId & reserved-header handling for ASB

Reviewed against CLAUDE.md conventions. Overall: solid, well-diagnosed fix. The root cause holds up under inspection and the change is minimal and correctly scoped.

✅ Root cause is confirmed by the code

The PR's diagnosis is accurate. DictionaryStringObjectJsonConverter.Write explicitly runs each bag key through the naming policy:

var convertedPropName = options.PropertyNamingPolicy?.ConvertName(propertyName) ?? propertyName;

with PropertyNamingPolicy = JsonNamingPolicy.CamelCase set in JsonSerialisationOptions. So a bag key written as "SessionId" genuinely comes back as "sessionId" after an Outbox serialize/deserialize round-trip. The old Bag.TryGetValue("SessionId", …) exact match would miss it, and the case-sensitive ReservedHeaders.Contains(h.Key) would leak the camelCased reserved keys into ApplicationProperties. Both are addressed by moving to OrdinalIgnoreCase. This matches the "confirm the theory first" note in the PR description — good.

✅ Fix is a robustness improvement, not just a rename

Worth calling out: the new code is strictly safer than the original for the null case. The old path did value.ToString() immediately after TryGetValue, which would throw NullReferenceException if the bag held a null value under that key. The new if (sessionId is not null) guard removes that latent NRE. Nice side benefit.

✅ Scoping is correct — the consumer side is unaffected

I checked AzureServiceBusMessageCreator (the read path). It reads MessageType/HandledCount/ReplyTo from native ASB ApplicationProperties, which the publisher writes directly with PascalCase consts (lines 79–81) — those never pass through Brighter's JSON bag serialization, so there's no symmetric casing bug to fix there. The problem is genuinely specific to the JSON-serialized Bag, and the fix is placed exactly where it belongs.

🟡 Minor observations (non-blocking)

1. Broadened behavior for all reserved headers. Switching the filter to OrdinalIgnoreCase means any casing variant of LockToken, SequenceNumber, MessageType, HandledCount, ReplyTo, SessionId is now stripped from ApplicationProperties, not just SessionId. This is the desired behavior (they're reserved), but it's a slightly wider change than the SessionId headline suggests — worth noting for anyone reading the changelog.

2. FirstOrDefault(...).Value is a linear scan vs. the old O(1) dictionary lookup. The bag is small so this is negligible, but if you wanted to keep the dictionary-lookup fast-path you could try the exact key first and fall back to the scan only on a miss. Not worth changing for typical bag sizes.

3. Test covers the fix point but not the full round-trip. The [Theory] directly seeds "SessionId"/"sessionId" bag keys and asserts on ConvertToServiceBusMessage — this precisely exercises the changed code and asserts both that SessionId is set and that the key stays out of ApplicationProperties. Good coverage of the unit. It intentionally does not exercise an actual JSON round-trip; a test that serializes/deserializes a Message through JsonSerialisationOptions.Options and confirms the key emerges camelCased would nail the whole chain for [Bug] Do Case insensitive check for ASB sessionID #4054 — though that's arguably the JSON converter's own test responsibility and optional here.

Style / conventions

• Class name AzureServiceBusMessagePublisherSessionIdTests matches the sibling AzureServiceBusMessagePublisherLocalHeaderTests convention. ✔
• License header preserved; new using System; is justified by StringComparison/StringComparer. ✔
• Explanatory comments on the fix are clear and point future readers at the camelCase policy. ✔

Verdict: approve. Correct root cause, minimal targeted fix, added test, no regressions to the read path. The minor points above are optional.

🤖 Generated with Claude Code

[claude]

Code Review — PR #4205 (fix: case-insensitive SessionId & reserved-header handling for ASB, #4054)

Overall this is a well-diagnosed, well-tested fix. The root-cause analysis (camelCase JsonNamingPolicy mangling bag keys during an Outbox round-trip) is correct, and generalizing the lookup to any configured naming policy — rather than just handling camelCase — is a genuinely more robust choice than the originally-suggested case-insensitive-only fix. The XML docs and inline comments explain the "why" clearly, and the parametrized test covering camelCase / snake_case / no-policy is exactly the right shape to lock the behavior in.

A few observations, mostly minor:

Correctness

• ✅ The FirstOrDefault(...).Value pattern is safe: no match yields default(KeyValuePair<string,object>) whose .Value is null, so the is not null guard correctly skips setting SessionId.
• ✅ Stripping the reserved sessionId/messageType/etc. keys via IsReservedHeader fixes the real leak — previously the camelCased keys survived the case-sensitive ReservedHeaders.Contains and were duplicated into ApplicationProperties alongside the PascalCase re-adds at lines 78–80.
• Minor edge case: IsBagKey compares against the currently configured policy (JsonSerialisationOptions.Options.PropertyNamingPolicy). If a message is written to the Outbox under one policy and published after the global policy is reconfigured, the mangled key won't be recognized. Unusual mid-flight-config-change scenario, probably not worth handling — just noting it.

Performance

• IsReservedHeader calls IsBagKey for each of the 6 reserved headers per bag entry, and each IsBagKey re-reads the global static and calls namingPolicy.ConvertName(reservedKey), which allocates a fresh string every time. For a bag with N entries that's up to ~6N ConvertName allocations on the publish path. Bags are usually tiny so the cost is negligible, but if you wanted to tidy it you could compute the policy-converted reserved-key set once per ConvertToServiceBusMessage call rather than per-entry-per-key. Not blocking.

Scope

• Issue [Bug] Do Case insensitive check for ASB sessionID #4054 is specifically about the default camelCase policy; the fix broadens to arbitrary policies (incl. snake_case). This is a reasonable, well-justified generalization, but note it's slightly beyond the literal ask — flagging only because CLAUDE.md's "Change Scope" guidance leans conservative. I'd keep it; the snake_case test case demonstrates real added value.

Tests

• ✅ Good isolation: mutating the global JsonSerialisationOptions.Options is correctly contained by the DisableParallelization collection + try/finally restore. Since a collection is xUnit's unit of parallelism and this one won't run concurrently with other collections, the shared-state mutation can't bleed into other tests in the assembly.
• ✅ The test round-trips the Bag through the real serializer (not a hand-mangled key), which faithfully reproduces the Outbox behavior — nice.
• Possible additional coverage (optional): assert that a non-reserved user header still passes through to ApplicationProperties after the round-trip, to guard against IsReservedHeader over-stripping in a future change.

Nice, focused fix with a diagnosis-first approach. 👍

(Automated review by Claude)