Skip to content

Don't transform Cosmos query to ReadItem when WithPartitionKey conflicts with predicate partition key#38439

Open
Copilot wants to merge 8 commits into
mainfrom
copilot/fix-conflicting-partition-key-filter
Open

Don't transform Cosmos query to ReadItem when WithPartitionKey conflicts with predicate partition key#38439
Copilot wants to merge 8 commits into
mainfrom
copilot/fix-conflicting-partition-key-filter

Conversation

Copilot AI commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Combining WithPartitionKey(...) with a predicate that also compares the partition key produced a ReadItem using the WithPartitionKey value, silently discarding the predicate. A conflicting value returned the WithPartitionKey document instead of zero results.

// Document exists at PK1 with Id == sharedId
context.Entities
    .WithPartitionKey("PK1")
    .Where(e => e.Id == sharedId && e.PartitionKey == "PK2")
    .SingleOrDefault();
// Before: ReadItem(["PK1"], sharedId) -> returns the PK1 document
// After:  regular query -> returns null (PK1 != PK2)

Changes

  • CosmosReadItemAndPartitionKeysExtractor: skip the ReadItem transform only when partition key values supplied via WithPartitionKey() actually conflict with partition key comparisons in the predicate. Matching values remain eligible for ReadItem, while conflicting values execute as a regular query so the predicate is preserved and yields zero results.
  • Fix repeated JSON id / partition-key comparison checks to compare expression values structurally, so equivalent repeated comparisons remain compatible with extraction and ReadItem eligibility.
  • Conflict detection applies predicateValue.TypeMapping to the WithPartitionKey() value before comparing (since WithPartitionKey() arguments are translated with applyDefaultTypeMapping:false), ensuring identical constant/parameter pairs are correctly recognized as non-conflicting. A special case handles the SqlParameterExpression vs SqlConstantExpression mismatch that arises when a literal is passed to WithPartitionKey() (funcletized to a parameter) but written inline in the predicate (stays a constant): when all partition key properties are also part of the JSON ID, ReadItem is safe regardless.

Behavior change

  • Queries that use both WithPartitionKey() and an equivalent predicate partition-key comparison (same values) continue to translate to ReadItem.
  • Queries that use both WithPartitionKey() and conflicting predicate partition-key comparisons no longer translate to ReadItem; they execute as regular queries so the conflict is honored and no result is returned.
  • Queries that use WithPartitionKey(pkVar) and PartitionKey == differentPkVar where both are different local variables holding the same runtime value cannot be recognized as equal at translate time and fall through to a full query (the result is still correct).

Tests

  • Added WithPartitionKey_and_predicate_with_id_and_conflicting_partition_key (+ _leaf variant) covering the conflicting case (asserts empty).
  • Added WithPartitionKey_and_predicate_with_id_two_locals_same_value documenting the behavior when WithPartitionKey and the predicate use two different local variables that happen to hold the same value — these cannot be recognized as equal at translate time, so the query falls through to a full SQL query (result is still correct).

…icate partition key

Fixes #38238

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix conflicting partition key filter in ReadItem case Don't transform Cosmos query to ReadItem when WithPartitionKey conflicts with predicate partition key Jun 17, 2026
Copilot AI requested a review from AndriySvyryd June 17, 2026 04:58
@AndriySvyryd AndriySvyryd requested a review from Copilot June 17, 2026 20:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an incorrect Cosmos optimization where a query could be transformed into a ReadItem call even when WithPartitionKey(...) conflicts with partition-key comparisons in the predicate—causing the predicate partition-key to be silently ignored and returning the wrong document. The change prevents the ReadItem transform in that scenario so the query executes normally and conflicting values correctly yield zero results.

Changes:

  • Update CosmosReadItemAndPartitionKeysExtractor to skip the ReadItem transform when partition key values come from WithPartitionKey() and the predicate also contains partition key comparison(s).
  • Add new functional tests for the conflicting WithPartitionKey + (id && partitionKey) predicate case (including leaf/inheritance variants).
  • Update multiple Cosmos functional test SQL baselines to reflect the new (non-ReadItem) query shape in same-value scenarios.

Reviewed changes

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

Show a summary per file
File Description
src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs Adds a predicate-partition-key-comparison flag and blocks ReadItem transform when WithPartitionKey() may conflict with predicate PK comparisons.
test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTestBase.cs Adds a new base test covering conflicting WithPartitionKey + predicate partition-key comparison.
test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryInheritanceTestBase.cs Adds a leaf/inheritance variant of the conflicting-case test.
test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTest.cs Updates baselines to expect regular query SQL (not ReadItem) and adds baseline for the new conflicting-case test.
test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryRootDiscriminatorInIdTest.cs Updates baselines for non-ReadItem query shape and adds baselines for the new conflicting-case tests.
test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryNoDiscriminatorInIdTest.cs Updates baselines for non-ReadItem query shape and adds baselines for the new conflicting-case tests.
test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryDiscriminatorInIdTest.cs Updates baselines for non-ReadItem query shape and adds baselines for the new conflicting-case tests.

Comment thread src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs Outdated
Comment thread src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs Outdated
Copilot AI and others added 2 commits June 17, 2026 20:37
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs Outdated
…ng LINQ + ApplyTypeMapping

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…titionKey + same predicate

When a literal is passed to WithPartitionKey() it gets funcletized to a SqlParameterExpression
(outside a lambda), while the same literal in a Where() stays as a SqlConstantExpression.
SqlExpression.Equals() returned false for different subtypes even with identical values, so the
conflict detection incorrectly disabled ReadItem for same-value cases.

Fix: when withPkValue is a parameter and predicateValue is a constant, and all partition key
properties are encoded in the JSON ID, ReadItem is safe regardless of runtime value — a wrong
partition key returns null rather than the wrong document.

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
ss => ss.Set<SinglePartitionKeyEntity>().Where(e => e.PartitionKey == "PK1")
.Where(e => e.Id == Guid.Parse("B29BCED8-E1E5-420E-82D7-1C7A51703D34") && e.PartitionKey == "PK2"),
assertEmpty: true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Also add

    [Fact]
    public virtual Task WithPartitionKey_and_predicate_with_id_two_locals_same_value()
    {
        var pkForWith = "PK1";
        var pkForWhere = "PK1";

        return AssertQuery(
            async: true,
            ss => ss.Set<SinglePartitionKeyEntity>().WithPartitionKey(pkForWith)
                .Where(e => e.Id == Guid.Parse("B29BCED8-E1E5-420E-82D7-1C7A51703D34") && e.PartitionKey == pkForWhere),
            ss => ss.Set<SinglePartitionKeyEntity>().Where(e => e.PartitionKey == pkForWith)
                .Where(e => e.Id == Guid.Parse("B29BCED8-E1E5-420E-82D7-1C7A51703D34") && e.PartitionKey == pkForWhere));
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in the latest commit. The test is in the base class and overridden in all four concrete test files with SQL baselines.

Since pkForWith and pkForWhere are two different local variables (even though both hold "PK1"), they produce two different SqlParameterExpressions at translate time (@pkForWith and @pkForWhere). The conflict check compares by expression equality (which uses parameter names), so they are not recognized as equal → ReadItem is skipped and the query runs as a full SQL query with @pkForWhere in the WHERE clause. The SQL baselines reflect this behavior.

…oop, shorten comments

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
@AndriySvyryd AndriySvyryd marked this pull request as ready for review June 18, 2026 06:27
@AndriySvyryd AndriySvyryd requested a review from a team as a code owner June 18, 2026 06:27
Copilot AI review requested due to automatic review settings June 18, 2026 06:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants