Don't escape separator characters in single key values

src/EFCore.Cosmos/Metadata/Internal/JsonIdDefinition.cs

@@ -89,23 +89,24 @@ public virtual string GenerateIdString(IEnumerable<object?> values)
 
         if (_discriminatorProperty != null)
         {
-            AppendValue(_discriminatorProperty!, _discriminatorValue!);
+            AppendValue(_discriminatorProperty!, _discriminatorValue!, singleValue: false);
         }
 
         var i = 0;
+        var multipleValues = _discriminatorProperty != null || values.Skip(1).Any();
         foreach (var value in values)
         {
-            AppendValue(Properties[i++], value);
+            AppendValue(Properties[i++], value, !multipleValues);
         }
 
         builder.Remove(builder.Length - 1, 1);
 
         return builder.ToString();
 
-        void AppendValue(IProperty property, object? value)
+        void AppendValue(IProperty property, object? value, bool singleValue)
         {
             var converter = property.GetTypeMapping().Converter;
-            AppendString(builder, converter == null ? value : converter.ConvertToProvider(value));
+            AppendString(builder, converter == null ? value : converter.ConvertToProvider(value), singleValue);
             builder.Append('|');
         }
     }
@@ -116,23 +117,23 @@ void AppendValue(IProperty property, object? value)
     ///     any release. You should only use it directly in your code with extreme caution and knowing that
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    protected virtual void AppendString(StringBuilder builder, object? propertyValue)
+    protected virtual void AppendString(StringBuilder builder, object? propertyValue, bool singleValue)
     {
         switch (propertyValue)
         {
             case string stringValue:
-                AppendEscape(builder, stringValue);
+                AppendEscape(builder, stringValue, singleValue);
                 return;
             case IEnumerable enumerable:
                 foreach (var item in enumerable)
                 {
-                    AppendEscape(builder, item.ToString()!);
+                    AppendEscape(builder, item.ToString()!, singleValue);
                     builder.Append('|');
                 }
 
                 return;
             case DateTime dateTime:
-                AppendEscape(builder, dateTime.ToString("O"));
+                AppendEscape(builder, dateTime.ToString("O"), singleValue);
                 return;
             default:
                 if (propertyValue == null)
@@ -141,7 +142,7 @@ protected virtual void AppendString(StringBuilder builder, object? propertyValue
                 }
                 else
                 {
-                    AppendEscape(builder, propertyValue.ToString()!);
+                    AppendEscape(builder, propertyValue.ToString()!, singleValue);
                 }
 
                 return;
@@ -154,16 +155,20 @@ protected virtual void AppendString(StringBuilder builder, object? propertyValue
     ///     any release. You should only use it directly in your code with extreme caution and knowing that
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    protected virtual StringBuilder AppendEscape(StringBuilder builder, string stringValue)
+    protected virtual StringBuilder AppendEscape(StringBuilder builder, string stringValue, bool singleValue)
     {
         var startingIndex = builder.Length;
-        return builder.Append(stringValue)
-            // We need this to avoid collisions with the value separator
-            .Replace("|", "^|", startingIndex, builder.Length - startingIndex)
-            // These are invalid characters, see https://docs.microsoft.com/dotnet/api/microsoft.azure.documents.resource.id
-            .Replace("/", "^2F", startingIndex, builder.Length - startingIndex)
-            .Replace("\\", "^5C", startingIndex, builder.Length - startingIndex)
-            .Replace("?", "^3F", startingIndex, builder.Length - startingIndex)
-            .Replace("#", "^23", startingIndex, builder.Length - startingIndex);
+        builder = builder.Append(stringValue);
+
+        return singleValue
+            ? builder
+            : builder
+                // We need this to avoid collisions with the value separator, but only when the key has multiple values.
+                .Replace("|", "^|", startingIndex, builder.Length - startingIndex)
+                // These are invalid characters, see https://docs.microsoft.com/dotnet/api/microsoft.azure.documents.resource.id
+                .Replace("/", "^2F", startingIndex, builder.Length - startingIndex)
+                .Replace("\\", "^5C", startingIndex, builder.Length - startingIndex)
+                .Replace("?", "^3F", startingIndex, builder.Length - startingIndex)
+                .Replace("#", "^23", startingIndex, builder.Length - startingIndex);
     }
 }

test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryDiscriminatorInIdTest.cs

@@ -17,6 +17,34 @@ public ReadItemPartitionKeyQueryDiscriminatorInIdTest(ReadItemPartitionKeyQueryF
     public virtual void Check_all_tests_overridden()
         => TestHelpers.AssertAllMethodsOverridden(GetType());
 
+    public override async Task Key_with_special_characters_1()
+    {
+        await base.Key_with_special_characters_1();
+
+        AssertSql("""ReadItem(["Cat|1"], FancyDiscriminatorEntity|Cat^|1)""");
+    }
+
+    public override async Task Key_with_special_characters_2()
+    {
+        await base.Key_with_special_characters_2();
+
+        AssertSql("""ReadItem(["Cat2||"], FancyDiscriminatorEntity|Cat2^|^|)""");
+    }
+
+    public override async Task Key_with_special_characters_3()
+    {
+        await base.Key_with_special_characters_3();
+
+        AssertSql("""ReadItem(["Cat|3|$|5"], FancyDiscriminatorEntity|Cat^|3^|$^|5)""");
+    }
+
+    public override async Task Key_with_special_characters_4()
+    {
+        await base.Key_with_special_characters_4();
+
+        AssertSql("""ReadItem(["|Cat|"], FancyDiscriminatorEntity|^|Cat^|)""");
+    }
+
     public override async Task Predicate_with_hierarchical_partition_key()
     {
         await base.Predicate_with_hierarchical_partition_key();

test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryFixtureBase.cs

@@ -65,6 +65,11 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
             .HasKey(e => new { e.Id, e.PartitionKey });
 
         modelBuilder.Entity<SharedContainerEntity2Child>();
+
+        modelBuilder.Entity<FancyDiscriminatorEntity>()
+            .ToContainer("Cat35224")
+            .HasPartitionKey(e => e.Id)
+            .HasDiscriminator<string>("Discriminator");
     }
 
     public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
@@ -90,6 +95,7 @@ protected override Task SeedAsync(DbContext context)
         context.AddRange(data.SharedContainerEntities1);
         context.AddRange(data.SharedContainerEntities2);
         context.AddRange(data.SharedContainerEntities2Children);
+        context.AddRange(data.Cat35224Entities);
 
         return context.SaveChangesAsync();
     }
@@ -102,6 +108,7 @@ public virtual ISetSource GetExpectedData()
         { typeof(HierarchicalPartitionKeyEntity), e => ((HierarchicalPartitionKeyEntity?)e)?.Id },
         { typeof(OnlyHierarchicalPartitionKeyEntity), e => ((OnlyHierarchicalPartitionKeyEntity?)e)?.Payload },
         { typeof(SinglePartitionKeyEntity), e => ((SinglePartitionKeyEntity?)e)?.Id },
+        { typeof(FancyDiscriminatorEntity), e => ((FancyDiscriminatorEntity?)e)?.Id },
         { typeof(OnlySinglePartitionKeyEntity), e => ((OnlySinglePartitionKeyEntity?)e)?.Payload },
         { typeof(NoPartitionKeyEntity), e => ((NoPartitionKeyEntity?)e)?.Id },
         { typeof(SharedContainerEntity1), e => ((SharedContainerEntity1?)e)?.Id },
@@ -239,6 +246,22 @@ public virtual ISetSource GetExpectedData()
                     Assert.Equal(ee.ChildPayload, aa.ChildPayload);
                 }
             }
+        },
+        {
+            typeof(FancyDiscriminatorEntity), (e, a) =>
+            {
+                Assert.Equal(e == null, a == null);
+
+                if (a != null)
+                {
+                    var ee = (FancyDiscriminatorEntity)e!;
+                    var aa = (FancyDiscriminatorEntity)a;
+
+                    Assert.Equal(ee.Id, aa.Id);
+                    Assert.Equal(ee.Name, aa.Name);
+                    Assert.Equal(ee.Discriminator, aa.Discriminator);
+                }
+            }
         }
     }.ToDictionary(e => e.Key, e => (object)e.Value);
 
@@ -256,6 +279,8 @@ public class PartitionKeyData : ISetSource
         public List<SharedContainerEntity2> SharedContainerEntities2 { get; } = CreateSharedContainerEntities2();
         public List<SharedContainerEntity2Child> SharedContainerEntities2Children { get; } = CreateSharedContainerEntities2Children();
 
+        public List<FancyDiscriminatorEntity> Cat35224Entities { get; } = CreateCat35224Entities();
+
         public virtual IQueryable<TEntity> Set<TEntity>()
             where TEntity : class
         {
@@ -299,6 +324,11 @@ public virtual IQueryable<TEntity> Set<TEntity>()
                 return (IQueryable<TEntity>)SharedContainerEntities2Children.AsQueryable();
             }
 
+            if (typeof(TEntity) == typeof(FancyDiscriminatorEntity))
+            {
+                return (IQueryable<TEntity>)Cat35224Entities.AsQueryable();
+            }
+
             throw new InvalidOperationException("Invalid entity type: " + typeof(TEntity));
         }
 
@@ -480,6 +510,32 @@ private static List<SharedContainerEntity2Child> CreateSharedContainerEntities2C
                     ChildPayload = "Child2"
                 }
             };
+
+        private static List<FancyDiscriminatorEntity> CreateCat35224Entities()
+            => new()
+            {
+                new FancyDiscriminatorEntity
+                {
+                    Id = "Cat|1",
+                    Name = "Smokey"
+                },
+                new FancyDiscriminatorEntity
+                {
+                    Id = "Cat2||",
+                    Name = "Clippy"
+                },
+                new FancyDiscriminatorEntity
+                {
+                    Id = "Cat|3|$|5",
+                    Name = "Sid"
+                },
+                new FancyDiscriminatorEntity
+                {
+                    Id = "|Cat|",
+                    Name = "Killes"
+                }
+            };
+
     }
 }
 
@@ -503,6 +559,13 @@ public class SinglePartitionKeyEntity
     public required string Payload { get; set; }
 }
 
+public class FancyDiscriminatorEntity
+{
+    public string Id { get; set; } = null!;
+    public string? Name { get; set; }
+    public string Discriminator { get; set; } = null!;
+}
+
 // This type is configured with all partition key properties, and nothing else, in the primary key.
 public class OnlyHierarchicalPartitionKeyEntity
 {

test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryNoDiscriminatorInIdTest.cs

@@ -17,6 +17,43 @@ public ReadItemPartitionKeyQueryNoDiscriminatorInIdTest(ReadItemPartitionKeyQuer
     public virtual void Check_all_tests_overridden()
         => TestHelpers.AssertAllMethodsOverridden(GetType());
 
+    public override async Task Key_with_special_characters_1()
+    {
+        await base.Key_with_special_characters_1();
+
+        AssertSql("""ReadItem(["Cat|1"], Cat|1)""");
+    }
+
+    public override async Task Key_with_special_characters_2()
+    {
+        await base.Key_with_special_characters_2();
+
+        AssertSql(
+            """
+ReadItem(["Cat2||"], Cat2||)
+""");
+    }
+
+    public override async Task Key_with_special_characters_3()
+    {
+        await base.Key_with_special_characters_3();
+
+        AssertSql(
+            """
+ReadItem(["Cat|3|$|5"], Cat|3|$|5)
+""");
+    }
+
+    public override async Task Key_with_special_characters_4()
+    {
+        await base.Key_with_special_characters_4();
+
+        AssertSql(
+            """
+ReadItem(["|Cat|"], |Cat|)
+""");
+    }
+
     public override async Task Predicate_with_hierarchical_partition_key()
     {
         await base.Predicate_with_hierarchical_partition_key();

test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryRootDiscriminatorInIdTest.cs

@@ -17,6 +17,34 @@ public ReadItemPartitionKeyQueryRootDiscriminatorInIdTest(ReadItemPartitionKeyQu
     public virtual void Check_all_tests_overridden()
         => TestHelpers.AssertAllMethodsOverridden(GetType());
 
+    public override async Task Key_with_special_characters_1()
+    {
+        await base.Key_with_special_characters_1();
+
+        AssertSql("""ReadItem(["Cat|1"], FancyDiscriminatorEntity|Cat^|1)""");
+    }
+
+    public override async Task Key_with_special_characters_2()
+    {
+        await base.Key_with_special_characters_2();
+
+        AssertSql("""ReadItem(["Cat2||"], FancyDiscriminatorEntity|Cat2^|^|)""");
+    }
+
+    public override async Task Key_with_special_characters_3()
+    {
+        await base.Key_with_special_characters_3();
+
+        AssertSql("""ReadItem(["Cat|3|$|5"], FancyDiscriminatorEntity|Cat^|3^|$^|5)""");
+    }
+
+    public override async Task Key_with_special_characters_4()
+    {
+        await base.Key_with_special_characters_4();
+
+        AssertSql("""ReadItem(["|Cat|"], FancyDiscriminatorEntity|^|Cat^|)""");
+    }
+
     public override async Task Predicate_with_hierarchical_partition_key()
     {
         await base.Predicate_with_hierarchical_partition_key();

test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTest.cs

@@ -17,6 +17,37 @@ public ReadItemPartitionKeyQueryTest(ReadItemPartitionKeyQueryFixture fixture, I
     public virtual void Check_all_tests_overridden()
         => TestHelpers.AssertAllMethodsOverridden(GetType());
 
+    public override async Task Key_with_special_characters_1()
+    {
+        await base.Key_with_special_characters_1();
+
+        AssertSql("""ReadItem(["Cat|1"], Cat|1)""");
+    }
+
+    public override async Task Key_with_special_characters_2()
+    {
+        await base.Key_with_special_characters_2();
+
+        AssertSql("""ReadItem(["Cat2||"], Cat2||)""");
+    }
+
+    public override async Task Key_with_special_characters_3()
+    {
+        await base.Key_with_special_characters_3();
+
+        AssertSql("""ReadItem(["Cat|3|$|5"], Cat|3|$|5)""");
+    }
+
+    public override async Task Key_with_special_characters_4()
+    {
+        await base.Key_with_special_characters_4();
+
+        AssertSql(
+            """
+ReadItem(["|Cat|"], |Cat|)
+""");
+    }
+
     public override async Task Predicate_with_hierarchical_partition_key()
     {
         await base.Predicate_with_hierarchical_partition_key();

test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTestBase.cs

@@ -15,6 +15,30 @@ protected ReadItemPartitionKeyQueryTestBase(TFixture fixture, ITestOutputHelper
         Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
     }
 
+    [ConditionalFact] // Issue #35224
+    public virtual Task Key_with_special_characters_1()
+        => AssertQuery(
+            async: true,
+            ss => ss.Set<FancyDiscriminatorEntity>().Where(c => c.Id == "Cat|1"));
+
+    [ConditionalFact]
+    public virtual Task Key_with_special_characters_2()
+        => AssertQuery(
+            async: true,
+            ss => ss.Set<FancyDiscriminatorEntity>().Where(c => c.Id == "Cat2||"));
+
+    [ConditionalFact]
+    public virtual Task Key_with_special_characters_3()
+        => AssertQuery(
+            async: true,
+            ss => ss.Set<FancyDiscriminatorEntity>().Where(c => c.Id == "Cat|3|$|5"));
+
+    [ConditionalFact]
+    public virtual Task Key_with_special_characters_4()
+        => AssertQuery(
+            async: true,
+            ss => ss.Set<FancyDiscriminatorEntity>().Where(c => c.Id == "|Cat|"));
+
     [ConditionalFact]
     public virtual Task Predicate_with_hierarchical_partition_key()
         => AssertQuery(