CSHARP-5757: The problem of filtering by derived types #1812
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses CSHARP-5757 by modifying the discriminator handling logic to allow null discriminators for abstract types when using scalar discriminator conventions, while maintaining the restriction for hierarchical discriminator conventions. The change moves the null discriminator validation from the generic filter builder to the specific hierarchical discriminator implementation.
- Moved null discriminator validation from
FilterDefinitionBuildertoDiscriminatorAstFilter.TypeIsmethod - Added validation that throws
NotSupportedExceptionwhen hierarchical discriminators are null - Added comprehensive test coverage for both hierarchical and scalar discriminator scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Jira/CSharp5757Tests.cs | Adds test cases validating that hierarchical discriminators throw for abstract types while scalar discriminators work correctly with abstract types |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/DiscriminatorAstFilter.cs | Adds null discriminator validation specifically for hierarchical discriminator convention |
| src/MongoDB.Driver/FilterDefinitionBuilder.cs | Removes generic null discriminator check to allow scalar conventions to have null discriminators |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var discriminator = discriminatorConvention.GetDiscriminator(nominalType, actualType); | ||
| if (discriminator == null) | ||
| { | ||
| throw new NotSupportedException($"Hierarchical discriminator convention requires that documents of type {BsonUtils.GetFriendlyTypeName(actualType)} have a discriminator value."); |
There was a problem hiding this comment.
I think we can improve this error message.
A shorter version might be:
throw new NotSupportedException("Discriminator value is null which is not allowed with hierarchical discriminator conventions.");
and a longer version might be:
throw new NotSupportedException($"Discriminator value for type {{BsonUtils.GetFriendlyTypeName(typeof(TDerived))}} is null which is not allowed with hierarchical discriminator conventions.");
| var exception = Record.Exception(() => filter.Render(renderedArgs)); | ||
| exception.Should().BeOfType<NotSupportedException>(); | ||
| exception.Message.Should().Be("Hierarchical discriminator convention requires that documents of type HealthCareWorker have a discriminator value."); | ||
| } |
There was a problem hiding this comment.
These new tests should probably be integration tests like the tests for scalar discriminator conventions.
See CSharp5231Tests.cs for an example of a file that uses two collections for integration tests.
2 participants
This PR allows types that use a scalar discriminator convention to have a null discriminator on some types (for example abstract ones), while disallows it on hierarchical discriminator convention.