CSHARP-5816: Insonsistent behavior of EnsureNoMemberMapConflicts for discriminator convention

[papafe]

No description provided.

[Copilot]

Pull request overview

Fixes CSHARP-5816 by making BsonClassMap.GetDiscriminatorConvention() behave consistently when the discriminator element name conflicts with an existing member mapping, and adds a regression test to prevent reintroducing the issue.

Changes:

• Update BsonClassMap.GetDiscriminatorConvention() to cache the discriminator convention only after conflict validation succeeds.
• Add a unit test that asserts repeated calls to GetDiscriminatorConvention() consistently throw when a member uses the discriminator element name.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/MongoDB.Bson/Serialization/BsonClassMap.cs	Moves discriminator convention caching to occur after EnsureNoMemberMapConflicts validation passes.
tests/MongoDB.Bson.Tests/Serialization/BsonClassMapTests.cs	Adds a regression test covering repeated GetDiscriminatorConvention() calls when a member conflicts with the discriminator element name.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ajcvickers]

Claude review

This is the output from running the review-areas skill being worked on in #1993

• CSHARP-5816: Insonsistent behavior of EnsureNoMemberMapConflicts for discriminator convention #1991
• diff range: upstream/main...16b5b54

PR summary

Description: This PR touches BsonClassMap in the BSON serialization layer (src/MongoDB.Bson/Serialization/BsonClassMap.cs). The change addresses an inconsistency in GetDiscriminatorConvention: previously, the resolved discriminator convention was cached in _discriminatorConvention before EnsureNoMemberMapConflicts ran its validation check. That meant the first call would throw a BsonSerializationException as expected, but a second call would find the (invalid) convention already cached and skip the conflict check entirely, silently returning the bad convention instead of throwing again. The fix is a one-line reorder — move the assignment _discriminatorConvention = discriminatorConvention to after the validation call — so caching only happens when validation passes. Referenced as CSHARP-5816. The PR is small and focused (34 additions, 1 deletion across 2 files), and the code change is accompanied by a regression test.

Assessment: The problem is real and the diff confirms it clearly: the original code assigned before validating, so the second call hit the early-return if (_discriminatorConvention != null) guard and bypassed the error. The fix is the minimal correct change. The regression test directly reproduces the bug by calling GetDiscriminatorConvention() twice and asserting both throw, and uses a minimal custom discriminator convention that conflicts with a mapped member — this covers exactly the reported scenario. The scope is appropriately narrow. One subtle caveat: the comment added says "only cache if validation succeeds," but the code also suppresses caching when discriminatorConvention is null. That is correct behavior — null is a valid result meaning no convention is registered — but the comment is slightly misleading. This is a minor comment-quality issue, not a correctness problem. No async, security, or API-surface concerns are present.

Verdict: looks good

Summary

5 reviewers ran (1 area + 3 cross-cutting + 1 PR summary). 4 approved, 1 flagged, 0 escalated. (PR-summary verdict is not counted in those totals.)

Escalations

None.

Cross-cutting findings

• security-reviewer — approve — No credentials, crypto, TLS, or deserialization-safety concerns. The fix tightens discriminator safety by ensuring an invalid convention is never cached; it does not introduce new deserialization surface or bypass the conflict check.
• api-stability-reviewer — approve — GetDiscriminatorConvention is internal; no public or protected signatures were changed. SetDiscriminatorConvention (the one public method touching _discriminatorConvention) is unchanged. No [Obsolete], enum, or interface changes.
• async-reviewer — approve — No async constructs anywhere in the diff. The change is a synchronous line reorder with no threading implications beyond the pre-existing race on the cache write.

Area findings

bson-reviewer — flag

• src/MongoDB.Bson/Serialization/BsonClassMap.cs:1334 — When LookupDiscriminatorConvention() returns null, the new code assigns null back to _discriminatorConvention, so no caching of the null result occurs and every call re-runs the lookup walk — this is pre-existing behavior preserved by the fix, not a regression, but the comment "only cache if validation succeeds" is slightly misleading since caching null is also suppressed; consider guarding the null case separately (e.g. a dedicated sentinel or a bool _discriminatorConventionResolved flag) if the repeated lookup ever becomes a performance concern in hot paths.
• tests/MongoDB.Bson.Tests/Serialization/BsonClassMapTests.cs:743 — The test calls BsonSerializer.RegisterDiscriminatorConvention(typeof(Foo), ...) against static global state with no teardown; since Foo is a private nested type it is unlikely to clash with other tests, but using [Collection(nameof(BsonSerializerTests))] or otherwise isolating the test would be more robust in a parallelism-unsafe suite — nit only, does not affect correctness.

Tests run (by bson-reviewer):

• dotnet test tests/MongoDB.Bson.Tests/MongoDB.Bson.Tests.csproj -f net10.0 --filter "FullyQualifiedName~BsonClassMap" — Passed (110 tests)
• dotnet test tests/MongoDB.Bson.Tests/MongoDB.Bson.Tests.csproj -f net10.0 --filter "FullyQualifiedName~Serialization" — Passed (3565 tests)

Unmapped changes

• tests/MongoDB.Bson.Tests/Serialization/BsonClassMapTests.cs — under tests/MongoDB.Bson.Tests/, not matched by src/MongoDB.Bson/** or any spec-conformance pattern; reviewed contextually by bson-reviewer above.

[ajcvickers]

Claude: When LookupDiscriminatorConvention() returns null, the new code assigns null back to _discriminatorConvention, so no caching of the null result occurs and every call re-runs the lookup walk — this is pre-existing behavior preserved by the fix, not a regression, but the comment "only cache if validation succeeds" is slightly misleading since caching null is also suppressed; consider guarding the null case separately (e.g. a dedicated sentinel or a bool _discriminatorConventionResolved flag) if the repeated lookup ever becomes a performance concern in hot paths.

[papafe]

Reasonable comment, but I wanted to do the minimum amount of changes to fix this.

[ajcvickers]

LGTM