Conflicting partition key filter is ignored in ReadItem case

[AndriySvyryd]

Bug description

The below code results in ReadItem with the partition key from WithPartitionKey, but it should've thrown an exception.

Your code

using System.Reflection;
using Microsoft.Azure.Cosmos;
using Microsoft.EntityFrameworkCore;

var databaseName = $"EfCoreCosmosWithPartitionKeyBypass";
const string victimPartitionKey = "PK1";
const string filteredPartitionKey = "PK2";
const string sharedId = "shared-id";

await using var setupContext = new ReproContext(databaseName);
await setupContext.Database.EnsureDeletedAsync();
await setupContext.Database.EnsureCreatedAsync();

setupContext.AddRange(
    new SinglePartitionKeyEntity
    {
        Id = sharedId,
        PartitionKey = victimPartitionKey,
        Payload = "VictimPayload",
    },
    new SinglePartitionKeyEntity
    {
        Id = sharedId,
        PartitionKey = filteredPartitionKey,
        Payload = "FilteredPayload",
    });

await setupContext.SaveChangesAsync();

await using var projectionContext = new ReproContext(databaseName);
var projectionQuery = projectionContext.Entities
    .WithPartitionKey(victimPartitionKey)
    .Where(e => e.Id == sharedId && e.PartitionKey == filteredPartitionKey)
    .Select(e => e.PartitionKey);
Console.WriteLine($"{projectionQuery.ToQueryString().Replace(Environment.NewLine, " ")}");

var projectedPartitionKey = await projectionQuery.SingleOrDefaultAsync();

Console.WriteLine($"projectedPartitionKey={projectedPartitionKey ?? "<null>"}");

class ReproContext(string databaseName) : DbContext
{
    public DbSet<SinglePartitionKeyEntity> Entities => Set<SinglePartitionKeyEntity>();

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder.UseCosmos();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<SinglePartitionKeyEntity>()
            .ToContainer("SinglePartitionKeyEntity")
            .HasPartitionKey(e => e.PartitionKey)
            .HasKey(e => new { e.Id, e.PartitionKey });
    }
}

sealed class SinglePartitionKeyEntity
{
    public string Id { get; set; } = null!;
    public string PartitionKey { get; set; } = null!;
    public string Payload { get; set; } = null!;
}

Stack traces

Verbose output

EF Core version

10.0.0

Database provider

No response

Target framework

No response

Operating system

No response

IDE

No response

[lorenacr]

I was able to reproduce this.

It looks like when WithPartitionKey(...) is present and the query is eligible for the ReadItem optimization, a conflicting partition-key equality in the predicate (e.g. e.PartitionKey == otherValue) is effectively ignored because ReadItem only uses the SDK partition key + resource id and doesn’t evaluate the SQL predicate.

I’m planning to fix this by validating for conflicts between PK values supplied via WithPartitionKey(...) and PK comparisons found in the predicate during CosmosReadItemAndPartitionKeysExtractor. On conflict we should throw InvalidOperationException with a dedicated provider error message; I’ll add a new CosmosStrings entry for this and assert exception type + exact message in a functional test under the existing ReadItemPartitionKeyQuery* coverage.

For parameters, I plan to be conservative: if WithPartitionKey uses a SqlParameterExpression and the predicate PK comparison uses a different parameter (different name), we’ll treat that as a conflict and throw as well (unless they’re the same parameter).

[roji]

The below code results in ReadItem with the partition key from WithPartitionKey, but it should've thrown an exception.

I think returning no results - instead of throwing - would also be reasonable here... WithPartitionKey is logically equivalent to an additional Where clause, and we don't do any sort of detection/throwing for conflicting Wheres; in other words, users are free to write Where(b => b.Id == 1).Where(Ib => b.Id == 2) which is guaranteed to return zero results.

I think I'd rather align with that here... There are also (admittedly edge-case) scenarios where throwing (as opposed to returning nothing) could make life a bit difficult for users, where two layers dynamically add WithPartitionKey and Where.

@AndriySvyryd @lorenacr what do you think?

[AndriySvyryd]

I think returning no results - instead of throwing - would also be reasonable here...

Sure, it just seemed less risky to implement throwing as that was the behavior in 8.0.x

[lorenacr]

I agree with @AndriySvyryd

[roji]

Sure, it just seemed less risky to implement throwing as that was the behavior in 8.0.x

OK, though we already have 9.0 and 10.0 (LTS) out with the problematic behavior... So if we break here, IMHO we might as well implement the behavior we think is most correct rather than take 8.0 into account. If we believe throwing is somehow more correct/justified I'd want to know why (@lorenacr), otherwise if this is purely about aligning with 8.0 I personally don't see much value in that.

@AndriySvyryd since you seem to be OK with both, post your preference between the two options and we'll go with that :)

[AndriySvyryd]

My preference is to throw, because there shouldn't be a case where a user needs to run a query that always returns nothing. We don't throw for other cases because it's impractical to detect all such queries.

[lorenacr]

Agree with you, @AndriySvyryd .

Even though a contradictory PK predicate would just yield no results in the non-ReadItem path, it’s still a strong signal of a bug/misuse, and WithPartitionKey is explicit enough that failing fast is reasonable. We also can’t (and don’t) detect all “always empty” queries, but this specific case is straightforward and high-value to validate, so I’m OK with throwing here.