Azure / azure-cosmos-dotnet-v3

.NET SDK for Azure Cosmos DB for the core SQL API
MIT License
731 stars 489 forks source link

PatchItemAsync failing with HTTP400 when using CosmosPropertyNamingPolicy.CamelCase instead of [JsonProperty("id") #4613

Open ssteiner opened 1 month ago

ssteiner commented 1 month ago

My documents all have the Id property named Id (upper case) as is the convention in C#. To get things to work with CosmosDb, I'm using the CosmosClientOptions.SerializerOptions.PropertyNamingPolicy which I'm setting to CosmosPropertyNamingPolicy.CamelCase to have the CosmosDb SDK write my Id as id:

var cosmosOptions = new CosmosClientOptions()
{
    AllowBulkExecution = true,
    SerializerOptions = new CosmosSerializationOptions()
};
cosmosOptions.SerializerOptions.PropertyNamingPolicy = CosmosPropertyNamingPolicy.CamelCase;
var cosmosClient = new CosmosClient(endpoint, key, cosmosOptions);

This has worked fine for me so far - inserting, upserting, deleting, searching all work just fine. It's on Patch where I ran into an issue. Here's a sample to illustrate:

using Microsoft.Azure.Cosmos;

namespace ConsoleApp1;

internal class CosmosDbTest
{
    string endpoint = "endpoint_url";
    string key = "key";

    internal async Task RunTest()
    {
        try
        {
            var cosmosOptions = new CosmosClientOptions()
            {
                AllowBulkExecution = true,
                SerializerOptions = new CosmosSerializationOptions()
            };
            cosmosOptions.SerializerOptions.PropertyNamingPolicy = CosmosPropertyNamingPolicy.CamelCase;
            var cosmosClient = new CosmosClient(endpoint, key, cosmosOptions);
            var createDb = await cosmosClient.CreateDatabaseIfNotExistsAsync("TestDatabase");
            var db = createDb.Database;
            var createContainer = await db.CreateContainerIfNotExistsAsync(new ContainerProperties("TestContainer", "/id"));
            var container = createContainer.Container;
            var testItem = new PhoneBookCategory
            {
                Name = "test object", Id = Guid.NewGuid().ToString(), SubProp = new TestObject { StringProp = "hello world", IntProp = 10 }
            };
            var createRes = await container.CreateItemAsync(testItem, new PartitionKey(testItem.Id));
            if (createRes.StatusCode == System.Net.HttpStatusCode.Created)
            {
                testItem.Name = "renamed by upsert";
                testItem.SubProp.IntProp = 9;
                var upsertRes = await container.UpsertItemAsync(testItem, new PartitionKey(testItem.Id));
                if (upsertRes.StatusCode == System.Net.HttpStatusCode.OK)
                    Console.WriteLine($"Successfully upserted item {upsertRes.Resource.Id}");
                else
                    Console.WriteLine($"Upsert failed - status: {upsertRes.StatusCode}");

                List<PatchOperation> patchOperations =
                [
                     PatchOperation.Add($"/{nameof(PhoneBookCategory.Name)}", "updated name"),
                     PatchOperation.Replace($"/{nameof(PhoneBookCategory.SubProp)}/{nameof(TestObject.IntProp)}", 5)
                ];
                var patchRes = await container.PatchItemAsync<PhoneBookCategory>(testItem.Id, new PartitionKey(testItem.Id), patchOperations);
                Console.WriteLine($"Item modified with id {patchRes.Resource.Id}");
                var deleteRes = await container.DeleteItemAsync<PhoneBookCategory>(testItem.Id, new PartitionKey(testItem.Id));
                if (deleteRes.StatusCode == System.Net.HttpStatusCode.NoContent)
                    Console.WriteLine($"Successfully cleaned up test object");
                else
                    Console.Write($"Unable to clean up test object - status: {deleteRes.StatusCode}");
            }
            else
                Console.WriteLine($"Unable to create test object - status: {createRes.StatusCode}");
        }
        catch (CosmosException ex)
        {
            Console.WriteLine($"Cosmos DB Exception with Status {ex.StatusCode} Message: {ex.Message}");
        }
    }
}

public class PhoneBookCategory
{
    //[JsonProperty(PropertyName = "id")]
    public string Id { get; set; }
    public string Name { get; set; }
    public TestObject SubProp { get; set; }
}
public class TestObject
{
    public string StringProp { get; set; }
    public int? IntProp { get; set; }
}

This will cause the HTTP 400 error on this line

var patchRes = await container.PatchItemAsync<PhoneBookCategory>(testItem.Id, new PartitionKey(testItem.Id), patchOperations);

The exception message being:

Response status code does not indicate success: BadRequest (400); Substatus: 0; ActivityId: bfc4ab22-b363-4cad-970b-63900522677c; Reason: ();

Now uncomment the line

//[JsonProperty(PropertyName = "id")]

and comment out the line

cosmosOptions.SerializerOptions.PropertyNamingPolicy = CosmosPropertyNamingPolicy.CamelCase;

And it'll work. So it seems we have an issue with Patch and CosmosPropertyNamingPolicy.CamelCase. I'd expect the original sample to work and patch the document (it works on all other operations in this sample after all - the insert, the upsert, even the delete). You can even leave the property naming override ([JsonProperty(PropertyName = "id")]) active, as soon as the PropertyNamingPolicy is set, you get the issue.

Environment: Microsoft.Azure.Cosmos 3.42.0 .NET 8.0.7 VS2022 17.10.5

kirankumarkolli commented 1 month ago

@philipthomas-MSFT can you please take a look?

philipthomas-MSFT commented 1 month ago

Seems like it only happens on PatchOperation.Replace. Can repro, so will dig more into the root cause next.

https://github.com/Azure/azure-cosmos-dotnet-v3/pull/4632/files#diff-cceb41a13a0fcd2dfd82902943878d2d699fb966cf72edf2fc2257b4f8802b79

kirankumarkolli commented 1 month ago

FYR: https://learn.microsoft.com/en-us/azure/cosmos-db/partial-document-update#supported-operations

philipthomas-MSFT commented 1 month ago

FYR: https://learn.microsoft.com/en-us/azure/cosmos-db/partial-document-update#supported-operations

Yes, I was going to research that. Thanks. It seems like it makes sense why it fails on Replace so not sure this is something we need to fix. @kirankumarkolli

kirankumarkolli commented 1 month ago

Path API's by-definition taking string as path contracts. => Application needs to provide the right propertyNames based on serialization options/customizations. It's by-design.

In future when Typed APIs are added then SDK will automatically do the required conversion.

ssteiner commented 1 month ago

If you replace the patch operations in my sample with just pure Adds, it still fails. This will still fail:

List<PatchOperation> patchOperations =
[
     PatchOperation.Add($"/{nameof(PhoneBookCategory.Name)}", "updated name"),
     PatchOperation.Add($"/{nameof(PhoneBookCategory.SubProp)}/{nameof(TestObject.IntProp)}", 5)
];

Sorry for providing the initial sample with a Replace, it is indeed not the right syntax to use for this case.

kirankumarkolli commented 1 month ago

PatchOperation.Add($"/{nameof(PhoneBookCategory.SubProp)}/{nameof(TestObject.IntProp)}", 5)

Maps as "/SubProp/IntProp" and path doesn't (case-sensitive) exist in the document.

It's not PatchOperations (i.e. Add/Replace/etc..) the cause but the JSON paths which are case-sensitive.

API contract is string => Application needs to provide a valid JSON path (mapping to the expected serialization)

ssteiner commented 1 month ago

Okay, I get it.

May I propose that the error message be made more useful? Something along the lines of

"property 'Name' does not exist in document 'xyz

That would be a major improvement (and could also be used elsewhere.. e.g. try commenting out cosmosOptions.SerializerOptions.PropertyNamingPolicy = CosmosPropertyNamingPolicy.CamelCase;` and run the sample again.. this time it'll bomb out on the insert with an HTTP 400 without explanation. The explanation in that case should be 'your document has no 'id' property')

kirankumarkolli commented 1 month ago

@philipthomas-MSFT how about update Path API code document in remarks section about the possibilities?

And also can you please validate if the shortlink in error/exception has related context to troubleshoot?