Support conflicting partition key predicates in Cosmos queries#38240
Support conflicting partition key predicates in Cosmos queries#38240lorenacr wants to merge 5 commits into
Conversation
- Detect conflicting partition key predicates during read-item extraction - Add coverage for conflicting partition key predicate scenarios - Update Cosmos provider resources/strings Fixes dotnet#38238
|
@dotnet-policy-service agree |
1 similar comment
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Fixes Cosmos ReadItem query translation to detect and handle partition key predicates that conflict with values supplied via WithPartitionKey, preventing incorrect results due to predicate being ignored in the ReadItem optimization path.
Changes:
- Added conflict detection between
WithPartitionKey(...)values and partition key predicates during ReadItem extraction. - Added functional test coverage for conflicting/non-conflicting partition key predicate scenarios.
- Introduced a new Cosmos provider resource string for the conflict error.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTestBase.cs | Adds tests asserting conflict throws and same-value predicate does not throw. |
| src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs | Adds validation to detect WithPartitionKey vs predicate conflicts. |
| src/EFCore.Cosmos/Properties/CosmosStrings.resx | Adds new localized string for conflict error. |
| src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs | Adds generated accessor for the new resource string. |
Files not reviewed (1)
- src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Language not supported
- Introduce a snapshot of predicate partition key values to validate conflicts when using ReadItem. - Update the ValidateNoWithPartitionKeyConflict method to accept the new snapshot for improved accuracy in conflict detection. - Ensure that partition key comparisons are validated before clearing the partition key property values.
…itionKeyQueryTestBase - Include the Microsoft.EntityFrameworkCore.Cosmos.Internal namespace to enhance access to internal Cosmos features. - This change prepares the test base for future enhancements related to partition key queries.
- Update the partition key validation logic to ensure conflicts are detected even when the query does not use ReadItem. - Introduce new tests to verify behavior when conflicting partition key predicates are present. - Refactor partition key comparison methods for improved clarity and functionality.
- Introduce UnwrapShaperForReadItem method to streamline the extraction of partition keys and IDs. - Enhance validation of partition key conflicts when using ReadItem optimization. - Update tests to reflect changes in query behavior regarding conflicting partition key predicates. Fixes dotnet#38238.
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
This is quite a lot of additional code and complexity just for getting an exception.
But more importantly... The checking here will produce false positives in some cases where the same value is specified in WithPartitionKey and in the Where. The problem is that the comparison is done on the expression nodes, but these may be parameterized; so two SqlParameterExpressions with the same value will trigger an exception, although they shouldn't. To see this, try tweaking ReadItem_with_WithPartitionKey_and_partition_key_predicate_same_parameter_does_not_throw to now reuse the same local variable, but rather two local variables.
In more general terms, at the point where this tries to check for value equality, we intentionally have no access to parameter values; this is done so that we can cache the query. Access to parameterized values is only available much later, and flowing all the necessary information to that point in order to throw on conflicting values will likely be a pretty complicated thing.
@AndriySvyryd let's rediscuss, but I don't think we should pursue this. It's really trivial to simply have the query return zero results - probably by simply avoiding the ReadItem transformation for this case.
|
|
||
| _rootAlias = rootSource.Alias; | ||
|
|
||
| Expression UnwrapShaperForReadItem(Expression shaper) |
There was a problem hiding this comment.
Any reason for this rename and move of Unwrap()? At least keep it in the same place to reduce the diff?
| await AssertQuery( | ||
| async: true, | ||
| ss => ss.Set<SinglePartitionKeyEntity>().WithPartitionKey(partitionKey) | ||
| .Where(e => e.Id == Guid.Parse("B29BCED8-E1E5-420E-82D7-1C7A51703D34") && e.PartitionKey == partitionKey)); |
There was a problem hiding this comment.
I'm pretty sure that if you don't reference the same local variable, but two local variables with the same value (or a local variable and a constant), this test would fail.
Fixes #38238