Repository navigation
EF-477: Fix stack overflow in QueryableEncryptionSchemaGenerator for owned types with WithOwner - #379
Open
ajcvickers wants to merge 2 commits into
Open
ajcvickers wants to merge 2 commits into
ajcvickers wants to merge 2 commits into
Conversation
…owned types with WithOwner ## Problem QueryableEncryptionSchemaGenerator.AddSchemaFields walked every navigation returned by GetNavigations() and recursed into any whose target entity type is owned. For an owned type, GetNavigations() also returns the dependent-to-principal (WithOwner) navigation. When the owner is itself owned, that back-navigation passes the IsOwned() check and the recursion never terminates, so the process dies with a StackOverflow that cannot be caught. Confirmed on main, each shape tested in isolation against the unfixed code: - Root owner, owned type with WithOwner: works (the root is not owned). - Nested owned types, each with WithOwner: stack overflow. - Owned collection (OwnsMany) with WithOwner: stack overflow. - Encrypted owned navigation whose owned child has WithOwner: stack overflow. The bug is reachable through EnsureCreated via the public GenerateSchemas entry point. ## Fix Skip navigations on the dependent (if navigation.IsOnDependent continue) so only owner-to-dependent navigations are walked. Ownership is a tree, so the walk terminates by construction rather than relying on a visited-set. ## Compatibility Not a break. The generator file is byte-identical to v8.4.4, v9.1.4 and v10.0.4, and the only models whose output changes are those that previously crashed. The skipped navigations never produced schema output, and Equality/Range on an owned navigation still throws NotSupportedException. ## Other navigation walks Checked the other GetNavigations() walks in src/ (MongoUpdate, MongoModelValidator, Query visitors, LookupExpression). None has this defect; they already skip the dependent side, filter with IsEmbedded(), or do not recurse. ## Tests New QueryableEncryptionSchemaGeneratorTests (unit, no database) assert full field documents (keyId, path, bsonType, queries) for root, nested and collection shapes, an encrypted owned navigation at root and nested prefixes, the same owned type on two navigations, and Equality/Range on an owned navigation still throwing. Mutation testing confirmed the tests discriminate: removing the guard crashes the test host, and changes to recursion, path prefix or collection handling fail with visible assertions. Note: the Queryable Encryption and CSFLE functional tests skip unless CRYPT_SHARED_LIB_PATH is set, and were not run with the fix. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Review and mutation testing showed three mutations of the generator survived the original tests: dropping the recursion in the encrypted-owned-navigation branch, changing its path separator, and skipping owned collections. - Assert full field documents (keyId, path, bsonType, queries), not just paths. - Encrypt properties under an encrypted owned navigation, at root and nested prefixes, and pin the children-before-object-field order. - Pin owned-collection behavior: the model validator rejects encrypted properties in arrays, and the generator itself walks collections. - Cover the same owned type on two navigations, Equality/Range on an owned navigation still throwing, and an Equality property in a nested owned type. - Document which tests crash the host if the IsOnDependent guard regresses. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The traversal guard correctly prevents the ownership cycle while preserving owner-to-dependent schema generation, with comprehensive regression coverage.
0 open findings
What changed in this PR
Fixes infinite recursion when generating Queryable Encryption schemas for nested owned types with WithOwner back-references.
Changes:
- Skips dependent-to-principal navigations during schema traversal.
- Adds comprehensive unit coverage for nested, collection, encrypted-navigation, and validation scenarios.
| File | Description |
|---|---|
src/MongoDB.EntityFrameworkCore/Infrastructure/QueryableEncryptionSchemaGenerator.cs |
Prevents traversal back to owned principals. |
tests/MongoDB.EntityFrameworkCore.UnitTests/Infrastructure/QueryableEncryptionSchemaGeneratorTests.cs |
Adds regression and schema-output tests. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
QueryableEncryptionSchemaGenerator.AddSchemaFields walked every navigation
returned by GetNavigations() and recursed into any whose target entity type is
owned. For an owned type, GetNavigations() also returns the dependent-to-principal
(WithOwner) navigation. When the owner is itself owned, that back-navigation
passes the IsOwned() check and the recursion never terminates, so the process
dies with a StackOverflow that cannot be caught.
Confirmed on main, each shape tested in isolation against the unfixed code:
The bug is reachable through EnsureCreated via the public GenerateSchemas entry
point.
Fix
Skip navigations on the dependent (if navigation.IsOnDependent continue) so only
owner-to-dependent navigations are walked. Ownership is a tree, so the walk
terminates by construction rather than relying on a visited-set.
Compatibility
Not a break. The generator file is byte-identical to v8.4.4, v9.1.4 and
v10.0.4, and the only models whose output changes are those that previously
crashed. The skipped navigations never produced schema output, and
Equality/Range on an owned navigation still throws NotSupportedException.
Other navigation walks
Checked the other GetNavigations() walks in src/ (MongoUpdate,
MongoModelValidator, Query visitors, LookupExpression). None has this defect;
they already skip the dependent side, filter with IsEmbedded(), or do not
recurse.
Tests
New QueryableEncryptionSchemaGeneratorTests (unit, no database) assert full
field documents (keyId, path, bsonType, queries) for root, nested and
collection shapes, an encrypted owned navigation at root and nested prefixes,
the same owned type on two navigations, and Equality/Range on an owned
navigation still throwing. Mutation testing confirmed the tests discriminate:
removing the guard crashes the test host, and changes to recursion, path prefix
or collection handling fail with visible assertions.
Note: the Queryable Encryption and CSFLE functional tests skip unless
CRYPT_SHARED_LIB_PATH is set, and were not run with the fix.