fix: preserve MessageHeader.Bag keys verbatim through serialization (#4151)

[iancooper]

What

Fixes #4151 at the root: MessageHeader.Bag keys now round-trip verbatim through Brighter's serialization instead of being rewritten by the camelCase naming policy.

Closes #4151.
Closes #4214.

Root cause

DictionaryStringObjectJsonConverter.Write applied JsonSerialisationOptions.Options.PropertyNamingPolicy (default JsonNamingPolicy.CamelCase) to dictionary keys as well as property names. A bag key written as "SessionId" was therefore serialized as "sessionId". Bag keys are arbitrary user-defined identifiers, not C# property names, so any consumer that looks the key up by its original name missed it — silently.

Note (from review): DictionaryStringObjectJsonConverter.Read never applied the policy — it reads keys via reader.GetString() verbatim. So Write and Read were asymmetric, and even a same-process serialize→deserialize already mangled PascalCase keys. This change makes Write symmetric with the existing Read, rather than introducing new behaviour.

This is the shared root cause behind a cluster of issues/bugs:

• [Bug] Do Case insensitive check for ASB sessionID #4054 / fix: case-insensitive SessionId and reserved-header handling for ASB (#4054) #4205 — ASB SessionId dropped after an Outbox round-trip.
• fix(cloud-events): preserve message header values in CloudEventsTransformer #4132 — Subject put in the Bag arrived camelCased, breaking downstream consumers (e.g. Python Lambdas reading SNS notifications by PascalCase key); that PR worked around it by moving Subject to MessageHeader.Subject.
• Header-bag key lookups are not naming-policy-aware (latent SessionId/#4054-class bugs) #4214 — the survey of other exact-match bag lookups (RocketMQ Keys/Tag, AWS messageDeduplicationId, core ProducerTopic, ASB scheduler).

Origin of the mutation

Traced to the converter's introduction in #1531 — the key transformation was copied from the reference implementation the converter is based on (Josef Ottoson's), not added to fix a specific Brighter problem. So there is no known behaviour that intentionally relies on it.

Fix

Stop applying the naming policy to dictionary keys — write them verbatim. PropertyNamingPolicy still governs real MessageHeader property names, so the property wire format is unchanged; only Bag keys are affected.

Scope

DictionaryStringObjectJsonConverter is registered globally in JsonSerialisationOptions and applies to every Dictionary<string, object?> Brighter serializes (MessageHeader.Bag, RequestContext bags, outbox/scheduler payloads, any internal dictionary) — not just MessageHeader.Bag. This is intended: dictionary keys are data, not property names, and should not be case-mangled. Because Read never applied the policy, no in-process consumer could have depended on the old Write casing.

Why this closes #4214

#4214 catalogued the other exact-match Bag.TryGetValue(constant) lookups that the camelCase policy could break (RocketMQ Keys/Tag, AWS messageDeduplicationId, core ProducerTopic; plus the ASB scheduler, which reads an empty bag and was never live). Now that keys round-trip verbatim under any policy, every one of those lookups is correct as written — the "make each read site policy-aware" direction #4214 proposed is no longer needed. The #4205 policy-aware reader (ASBConstants.IsBagKey) is retained for ASB only, as a belt-and-braces compat shim for messages persisted under the old camelCasing during an upgrade drain.

Tests

• BagKeyRoundTripTests — pins that PascalCase (SessionId, PartitionKey) and lowercase (customer.header) bag keys survive verbatim, both through the converter directly and through a full MessageHeader round-trip.
• When_migrating_subject_from_bag_to_header — previously a characterisation test that asserted the bug (converts_to_camelCase); now asserts the key is preserved. The Header.Subject guidance from fix(cloud-events): preserve message header values in CloudEventsTransformer #4132 remains valid and those tests are unchanged.
• Full Paramore.Brighter.Core.Tests: 849 passed, 0 failed, 7 skipped. The only test that changed behaviour was the characterisation test above.

Risk / reviewer notes

• In-flight wire compatibility (drain window): messages already written to a persistent Outbox/queue under the old camelCasing hold keys like "sessionId"; after this change they are read back verbatim. Code looking for "SessionId" won't match during the drain. ASB is covered by the fix: case-insensitive SessionId and reserved-header handling for ASB (#4054) #4205 policy-aware reader; the other gateways folded in from Header-bag key lookups are not naming-policy-aware (latent SessionId/#4054-class bugs) #4214 (RocketMQ, AWS, core ProducerTopic) have no such shim, so their pre-upgrade in-flight messages would miss the lookup during the drain — transient and niche, but worth a release-notes callout.
• Not exercised here: the persistent-outbox suites (SQL / Mongo / DynamoDB) need live infra. They store the same JSON and read keys back with the same literal, so no key-level regression is expected, but they should be run in CI before merge.

🤖 Generated with Claude Code

[claude]

Review — fix: preserve MessageHeader.Bag keys verbatim (#4215)

Nice, tightly-scoped fix with a clear root-cause writeup. The core change is correct and I'd be comfortable merging it after a couple of small doc cleanups. Details below.

✅ Correctness — the fix is sound

The key observation that makes this safe: Read never applied the naming policy (DictionaryStringObjectJsonConverter.Read uses reader.GetString() verbatim, src/Paramore.Brighter/Serialization/DictionaryStringObjectJsonConverter.cs:35). So before this PR Write and Read were asymmetric — "SessionId" was written as "sessionId" but read back as "sessionId", meaning even a same-process serialize→deserialize round-trip already mangled PascalCase keys. This change makes Write symmetric with Read rather than introducing new behaviour, which is exactly what you want and materially shrinks the blast radius.

I also confirmed real MessageHeader property names are unaffected: MessageHeaderCustomPropertyParityTests still passes (STJ emits topic/replyTo/etc.), because those go through the type's own property serialization, not this dictionary converter. Good — the wire format for properties is genuinely unchanged.

🟠 Stale/misleading XML docs left behind (please fix — in-scope)

Two doc comments now describe behaviour that this PR removes, and they'll actively mislead users into camelCasing their keys defensively:

1. MessageHeader.Bag — src/Paramore.Brighter/MessageHeader.cs:155-158:

   "Use camelCase for the key names if you intend to read it yourself, as when converted to and from Json serializers will tend convert the property name from UpperCase to camelCase"

   This is the exact advice the fix makes obsolete.

2. JsonSerialisationOptions — src/Paramore.Brighter/JsonConverters/JsonSerialisationOptions.cs class summary:

   "The camelCase property will convert a key, such as a header bag key, to camelCase if you use UpperCase..."

Since these comments document the precise behaviour being changed, updating them is part of the fix, not scope creep. Leaving them stale is arguably worse than before, because now the code and the docs disagree.

🟡 Scope is broader than "Bag keys" — worth a reviewer note

DictionaryStringObjectJsonConverter is registered globally in JsonSerialisationOptions and applies to every Dictionary<string, object?> Brighter serializes (RequestContext bags, outbox/scheduler payloads, etc.), not just MessageHeader.Bag. The PR narrative frames it as "only Bag keys," but any internal dictionary payload is affected too. This is almost certainly the right behaviour everywhere (dictionary keys shouldn't be case-mangled), and because Read never applied the policy, no in-process consumer could have depended on the old Write casing anyway — so I don't think there's a hidden regression. But it's worth stating explicitly for reviewers rather than implying the converter is Bag-specific.

🟢 Tests

• BagKeyRoundTripTests is a good pin — [Theory] over PascalCase + already-lowercase keys, asserting verbatim survival.
• Flipping When_migrating_subject_... from characterisation-of-the-bug to assert-the-fix is the correct move, and the updated summary is clear.
• Optional suggestion: the new test pins the converter in isolation via a bare Dictionary<string,object>. Consider one round-trip test that serializes an actual Message/MessageHeader and reads a Bag key back, to guard the real user-facing path against future regressions in how the Bag is wired into serialization.
• Minor nit: Assert.Equal("a-value", roundTripped[key]) compares a string literal against object; it works but doesn't pin the value's runtime type. Not worth changing.

Risk

The drain-window analysis in the PR body is accurate and the decision to keep the #4205 policy-aware reader as a compat shim is the right call. Agree this deserves a release-notes callout, and that the persistent-outbox suites (SQL/Mongo/Dynamo) should run in CI before merge since they weren't exercised here.

Verdict: Approve once the two stale doc comments are updated. Everything else is optional polish.

🤖 Generated with Claude Code

[iancooper]

Thanks — good review, and the Read-was-never-policy-aware observation is a sharper framing of the risk than the PR body had. Addressed in 70f0942:

Required (stale docs) — done:

• MessageHeader.Bag XML doc no longer advises camelCasing keys; it now states keys round-trip verbatim.
• JsonSerialisationOptions class summary now clarifies that PropertyNamingPolicy applies to C# property names only, while dictionary keys are serialized verbatim by DictionaryStringObjectJsonConverter.

Optional test — done:

• Added a MessageHeader-level round-trip test next to the bare-dictionary pin, so the real user-facing path (Bag wired into MessageHeader serialization) is guarded, not just the converter in isolation.

Global-scope note (making it explicit, as requested):
DictionaryStringObjectJsonConverter is registered globally and applies to every Dictionary<string, object?> Brighter serializes — RequestContext bags, outbox/scheduler payloads, any internal dictionary — not just MessageHeader.Bag. This is intended: dictionary keys are data, not property names, and shouldn't be case-mangled. And because Read never applied the policy, no in-process consumer could have depended on the old asymmetric Write casing, so there's no hidden regression — the change makes Write symmetric with the Read that was already there.

Left as-is:

• The Assert.Equal("a-value", roundTripped[key]) string-vs-object nit — agree it's not worth changing.

Still open before merge (flagged, not code): run the persistent-outbox suites (SQL/Mongo/Dynamo) in CI, and a release-notes callout for the drain-window compat (covered by keeping #4205's policy-aware reader).

[claude]

Review: preserve MessageHeader.Bag keys verbatim (#4215)

Reviewed the diff plus the surrounding serialization and ASB read paths. This is a well-reasoned, well-documented root-cause fix — the write-side asymmetry is real (Read already returned keys verbatim at DictionaryStringObjectJsonConverter.cs:44, while Write mangled them), so a single serialize→deserialize through Brighter's own options silently lost any PascalCase bag key. Removing ConvertName on keys restores symmetry, and scoping it to dictionary keys (not MessageHeader property names) is correct. The doc-comment updates and the characterisation-test flip are exactly right.

A few observations, ranked:

1. The #4205 compat shim is no longer exercised by any test (test coverage)

tests/.../AzureServiceBus.Tests/.../When_Converting_A_Message_With_A_Session_Id.cs was designed to prove ASBConstants.IsBagKey resolves a key that serialization mangled — it round-trips the Bag through JsonSerialisationOptions.Options under CamelCase / SnakeCase / null and expects the key to come back transformed.

After this fix, the converter no longer transforms keys, so all three policies now round-trip "SessionId" verbatim. The test still passes, but the SnakeCase and CamelCase branches now test the identical verbatim path — the "mangled key" scenario the shim exists for (reading messages persisted under the old camelCasing during an upgrade drain) is no longer covered anywhere. If someone later deletes IsBagKey as apparent dead code, drain compatibility breaks with no red test.

Since the PR intentionally keeps IsBagKey as belt-and-braces, I'd suggest pinning that behaviour directly — construct a bag whose key is already the mangled literal (e.g. bag["sessionId"] = ... / bag["session_id"] = ...) and assert the publisher still resolves it — rather than relying on the serializer to produce the mangled key (it no longer does).

2. Stale comments in that same ASB test (minor / docs)

Lines 22–24 and 36–38 still assert as current fact that "a bag key written as SessionId round-trips as sessionId" and that "its PropertyNamingPolicy rewrites bag keys on the way out." That's precisely the behaviour this PR removes; the comments now contradict the code. Worth updating alongside #1.

3. Release-note framing of the blast radius (consideration)

JsonNamingPolicy.CamelCase only lowercases the leading character(s), so the only keys whose wire representation actually changes are bag keys beginning with an uppercase letter (SessionId, Subject, PartitionKey, ReceiptHandle, …). Keys already starting lowercase — including paramore.brighter.ProducerTopic and customer.header — were never affected. Stating this scoping in the release notes (alongside the drain-window note you already flagged) will help users reason about exactly which persisted keys shift.

Also, minor and pre-existing: the naming-policy branch of IsBagKey is effectively redundant for the default CamelCase policy, since CamelCase differs from the raw key only in first-character case, which the OrdinalIgnoreCase comparison in the first branch already covers. It earns its keep only for custom snake/kebab policies. Not something to change here — just context for whether the shim is worth its complexity long-term.

4. Nice-to-have: cover the null-value branch (minor)

Write writes the key verbatim in both the value and null branches (writer.WriteNull(propertyName)), but the new tests only exercise non-null values. A single case with a null bag value would pin the WriteNull path too.

Correctness / performance / security

• No correctness concerns in the change itself; the fix is minimal and on-point.
• Dropping the per-key ConvertName call is a marginal perf win — no concern.
• No security implications.

Nice work tracing this back to #1531 and resisting the temptation to also rip out the read-side shims. 🤖