Skip to content

Commit 811f5df

Browse files
koenbeukclaude
andcommitted
fix: address Copilot review feedback on PR #31
- Reject static backing fields in Pattern B of ProjectablePatternRecognizer. A static field would share materialized state across all instances, breaking per-entity semantics. Adds a snapshot test for the EXP0022 diagnostic. - Register ExpressiveMongoIgnoreConvention from the AsExpressive extension path (not just the ExpressiveMongoCollection<T> constructor). Document the ordering constraint with MongoDB's eager class-map caching, and recommend calling ExpressiveMongoIgnoreConvention.EnsureRegistered() at startup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 90c7a82 commit 811f5df

7 files changed

Lines changed: 109 additions & 16 deletions

File tree

docs/guide/integrations/mongodb.md

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,24 @@ No custom MQL is emitted — MongoDB's own translator does all the heavy lifting
4545

4646
## `[Expressive]` Properties Are Unmapped from BSON
4747

48-
ExpressiveSharp registers a MongoDB `IClassMapConvention` that unmaps every `[Expressive]`-decorated property from the BSON class map, so the property's backing field is not persisted to documents. This matters most for [Projectable properties](../../reference/projectable-properties), which have a writable `init` accessor and would otherwise be serialized as a real BSON field.
48+
ExpressiveSharp provides a MongoDB `IClassMapConvention` that unmaps every `[Expressive]`-decorated property from the BSON class map, so the property's backing field is not persisted to documents. This matters most for [Projectable properties](../../reference/projectable-properties), which have a writable `init` accessor and would otherwise be serialized as a real BSON field.
4949

50-
The convention is registered automatically the first time you construct an `ExpressiveMongoCollection<T>` or call `.AsExpressive()` on a collection. No opt-in is required.
50+
::: warning Ordering constraint
51+
MongoDB builds and caches a class map the first time you call `IMongoDatabase.GetCollection<T>()` for a given `T`. A convention registered *after* that call does not apply to the cached map. If any of your document types use `[Expressive]`, register the convention before the first `GetCollection<T>` call:
52+
53+
```csharp
54+
using ExpressiveSharp.MongoDB.Infrastructure;
55+
56+
// At application startup, before any GetCollection<T>:
57+
ExpressiveMongoIgnoreConvention.EnsureRegistered();
58+
59+
var client = new MongoClient(connectionString);
60+
var db = client.GetDatabase("shop");
61+
var customers = db.GetCollection<Customer>("customers"); // class map built now
62+
```
63+
64+
The convention is also registered automatically when you construct `ExpressiveMongoCollection<T>` or call `collection.AsExpressive()` — but only if that happens before any `GetCollection<T>` call for a type with `[Expressive]` properties. The explicit `EnsureRegistered()` call is the most reliable pattern.
65+
:::
5166

5267
## Async Methods
5368

src/ExpressiveSharp.Generator/Interpretation/ProjectablePatternRecognizer.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,11 @@ private static bool MatchesPatternAOrB(
148148
return true;
149149
}
150150

151-
// Pattern B: manually declared private nullable backing field on the same type.
152-
if (field.DeclaredAccessibility != Accessibility.Private
151+
// Pattern B: manually declared private *instance* nullable backing field on the same type.
152+
// A static backing field is explicitly rejected because it would share materialized state
153+
// across all instances of the containing type, breaking per-entity materialization semantics.
154+
if (field.IsStatic
155+
|| field.DeclaredAccessibility != Accessibility.Private
153156
|| !SymbolEqualityComparer.Default.Equals(field.ContainingType, property.ContainingType))
154157
{
155158
ReportGetAccessorPattern(context, diagnosticLocation,

src/ExpressiveSharp.MongoDB/ExpressiveMongoCollection.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,10 @@ public ExpressiveMongoCollection(IMongoCollection<TDocument> inner, ExpressiveOp
4040
_inner = inner ?? throw new ArgumentNullException(nameof(inner));
4141
_options = options ?? MongoExpressiveOptions.CreateDefault();
4242

43-
// Unmap [Expressive] (including Projectable) properties from BSON serialization
44-
// so the backing field is not persisted to documents.
43+
// Idempotent registration; belt-and-braces for code paths that access
44+
// `Inner` directly for writes (InsertOne/ReplaceOne/…) without ever
45+
// going through `AsQueryable`. The `AsExpressive` extension registers
46+
// the convention on the query path.
4547
ExpressiveMongoIgnoreConvention.EnsureRegistered();
4648
}
4749

src/ExpressiveSharp.MongoDB/Extensions/MongoExpressiveExtensions.cs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ public static IExpressiveMongoQueryable<T> AsExpressive<T>(
2525
this IQueryable<T> source,
2626
ExpressiveOptions? options = null)
2727
{
28+
// Idempotent; cheap on subsequent calls via an Interlocked.Exchange guard.
29+
// Registering here (rather than in ExpressiveMongoCollection's constructor)
30+
// guarantees the BSON class-map convention fires on the common
31+
// `collection.AsExpressive()` entry point too.
32+
ExpressiveMongoIgnoreConvention.EnsureRegistered();
33+
2834
var mongoProvider = source.Provider as IMongoQueryProvider
2935
?? throw new ArgumentException(
3036
"The source queryable's Provider must implement IMongoQueryProvider. " +
@@ -52,6 +58,13 @@ public static IExpressiveMongoQueryable<T> AsExpressive<T>(
5258
ExpressiveOptions? options = null,
5359
AggregateOptions? aggregateOptions = null)
5460
{
61+
// MongoDB's AsQueryable() builds the BSON class map eagerly to prepare the
62+
// aggregation pipeline. We must register our ignore convention *before* that
63+
// call, otherwise the class map for T is constructed with the default auto-map
64+
// behavior that includes writable [Expressive] properties as BSON fields —
65+
// persisting backing-field state to the document.
66+
ExpressiveMongoIgnoreConvention.EnsureRegistered();
67+
5568
var queryable = aggregateOptions is not null
5669
? collection.AsQueryable(aggregateOptions)
5770
: collection.AsQueryable();

src/ExpressiveSharp.MongoDB/Infrastructure/ExpressiveMongoIgnoreConvention.cs

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,20 +34,30 @@ public sealed class ExpressiveMongoIgnoreConvention : ConventionBase, IClassMapC
3434

3535
public ExpressiveMongoIgnoreConvention() : base("ExpressiveSharpIgnore") { }
3636

37+
/// <summary>
38+
/// Runs during class-map construction. Inspects the class's CLR properties directly
39+
/// (instead of <see cref="BsonClassMap.DeclaredMemberMaps"/>, which may not be populated
40+
/// yet at this stage) and unmaps any that carry <see cref="ExpressiveAttribute"/>.
41+
/// </summary>
3742
public void Apply(BsonClassMap classMap)
3843
{
39-
if (classMap is null) throw new ArgumentNullException(nameof(classMap));
44+
ArgumentNullException.ThrowIfNull(classMap);
4045

41-
// Walk the class map's already-auto-mapped members and remove any whose underlying
42-
// PropertyInfo carries [Expressive]. Using DeclaredMemberMaps (not AllMemberMaps) to
43-
// only touch members declared on this specific class; inherited ones will be unmapped
44-
// when the base class's map is built.
45-
foreach (var memberMap in classMap.DeclaredMemberMaps.ToArray())
46+
foreach (var property in classMap.ClassType.GetProperties(
47+
BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic))
4648
{
47-
if (memberMap.MemberInfo is not PropertyInfo property) continue;
48-
if (property.GetCustomAttribute<ExpressiveAttribute>(inherit: false) is null) continue;
49+
// Only unmap properties declared on this exact type; inherited ones are handled
50+
// by the base class's own class-map convention pass.
51+
if (property.DeclaringType != classMap.ClassType)
52+
{
53+
continue;
54+
}
55+
if (property.GetCustomAttribute<ExpressiveAttribute>(inherit: false) is null)
56+
{
57+
continue;
58+
}
4959

50-
classMap.UnmapMember(memberMap.MemberInfo);
60+
classMap.UnmapProperty(property.Name);
5161
}
5262
}
5363

@@ -60,10 +70,25 @@ public void Apply(BsonClassMap classMap)
6070
/// <see cref="ConventionRegistry"/>. Subsequent calls are no-ops.
6171
/// </summary>
6272
/// <remarks>
73+
/// <para>
74+
/// <b>Ordering matters.</b> MongoDB builds and caches a <see cref="BsonClassMap"/> for a
75+
/// document type on the first call to <c>IMongoDatabase.GetCollection&lt;T&gt;()</c>. A
76+
/// convention registered <i>after</i> that call does not apply to the cached map; the
77+
/// <c>[Expressive]</c> properties will still be serialized to BSON.
78+
/// </para>
79+
/// <para>
80+
/// Call this method once at application startup, before any <c>GetCollection&lt;T&gt;</c>
81+
/// call for a type that has <c>[Expressive]</c> properties. Alternatively, wrap the
82+
/// collection in <see cref="ExpressiveMongoCollection{TDocument}"/> or call
83+
/// <c>collection.AsExpressive()</c> before any other collection handle is obtained; both
84+
/// of those paths call this method.
85+
/// </para>
86+
/// <para>
6387
/// The filter predicate returns <c>true</c> for every type — the convention's
6488
/// <see cref="Apply(BsonClassMap)"/> is a no-op for classes without <c>[Expressive]</c>
6589
/// properties, so applying it globally is harmless and avoids subtle ordering issues
6690
/// where a type-level predicate is evaluated before attribute metadata is visible.
91+
/// </para>
6792
/// </remarks>
6893
public static void EnsureRegistered()
6994
{

tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ProjectableTests.cs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,36 @@ public string FullName
263263
"Expected either EXP0022 (pattern mismatch) or EXP0025 (backing field type mismatch)");
264264
}
265265

266+
[TestMethod]
267+
public void StaticBackingField_EXP0022()
268+
{
269+
// A static backing field would share materialized state across all instances.
270+
// It must be rejected so per-entity semantics are preserved.
271+
var compilation = CreateCompilation(
272+
"""
273+
namespace Foo {
274+
class User {
275+
public string FirstName { get; set; } = "";
276+
public string LastName { get; set; } = "";
277+
278+
private static string? _shared;
279+
280+
[Expressive(Projectable = true)]
281+
public string FullName
282+
{
283+
get => _shared ?? (LastName + ", " + FirstName);
284+
init => _shared = value;
285+
}
286+
}
287+
}
288+
""");
289+
var result = RunExpressiveGenerator(compilation);
290+
291+
Assert.IsTrue(
292+
result.Diagnostics.Any(d => d.Id == "EXP0022"),
293+
"Static backing fields must be rejected with EXP0022 (pattern mismatch)");
294+
}
295+
266296
[TestMethod]
267297
public void RequiredModifier_EXP0026()
268298
{

tests/ExpressiveSharp.MongoDB.IntegrationTests/Tests/ProjectableMongoIgnoreTests.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,12 @@ public async Task InitMongo()
2626
if (!MongoContainerFixture.IsDockerAvailable)
2727
Assert.Inconclusive("Docker not available");
2828

29-
// Register the ignore convention BEFORE any class map could be implicitly built.
29+
// IMPORTANT: The ignore convention must be registered BEFORE the first call to
30+
// IMongoDatabase.GetCollection<T>(), which builds and caches the BSON class map for
31+
// T eagerly. A convention registered afterward would not apply to the cached map.
32+
// Users who want [Expressive] properties unmapped from BSON must either call
33+
// EnsureRegistered() before accessing collections, or go through
34+
// ExpressiveMongoCollection<T>/AsExpressive() *before* the first GetCollection<T>.
3035
ExpressiveMongoIgnoreConvention.EnsureRegistered();
3136

3237
_client = new MongoClient(MongoContainerFixture.ConnectionString);

0 commit comments

Comments
 (0)