Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PatchItemAsync is ignoring CosmosClientOptions.SerializerOptions.PropertyNamingPolicy when set to CamelCase #4733

Closed
calloncampbell opened this issue Sep 28, 2024 · 8 comments
Assignees
Labels
customer-reported Issue created by a customer needs-investigation
Milestone

Comments

@calloncampbell
Copy link

Describe the bug
When using the PatchItem operation and I've configured the serialization to CamelCase, my results that show up in Cosmos DB for the patched item are in standard C# case (not CamelCase). When I use the CreateItem or UpsertItem, the data shows up in Cosmos DB using the desired serialization of CamelCase.

To Reproduce

  1. I have a .NET 8 Azure Function in Isolated worker with an HTTP trigger which has a PATCH endpoint.
  2. in my Program.cs I've configured my CosmosClient:
services.AddSingleton((s) =>
{
    CosmosClientBuilder configurationBuilder = new CosmosClientBuilder(accountEndpoint: cosmosEndpoint, tokenCredential: tokenCredential)
        .WithSerializerOptions(new CosmosSerializationOptions
        {
            PropertyNamingPolicy = CosmosPropertyNamingPolicy.CamelCase
        })
        .WithConnectionModeDirect(openTcpConnectionTimeout: TimeSpan.FromSeconds(10))
        .WithApplicationRegion(regionName)
        .WithRequestTimeout(TimeSpan.FromMinutes(1))
        .WithThrottlingRetryOptions(new TimeSpan(0, 0, 0, 0, 500), 3);

    return configurationBuilder.Build();
});
  1. My method for patching the item:
    public async Task<OrderModel> PatchOrderAsync(string orderId, List<PatchOperationModel> items, CancellationTokenSource? cancellationTokenSource = default)
    {
        List<PatchOperation> patchOperations = new List<PatchOperation>
        {
            PatchOperation.Replace("/modifiedTimeUtc", DateTime.UtcNow)
        };

        foreach (var operation in items)
        {
            if (operation.OperationType == Patch.Add)
            {
                patchOperations.Add(PatchOperation.Add(operation.Path, operation.Value));
            }
            else if (operation.OperationType == Patch.Remove)
            {
                patchOperations.Add(PatchOperation.Remove(operation.Path));
            }
            else if (operation.OperationType == Patch.Replace)
            {
                patchOperations.Add(PatchOperation.Replace(operation.Path, operation.Value));
            }
            else
            {
                _logger.LogWarning("Unsupported operation type: {0}", operation.OperationType);
                throw new Exception($"Unsupported operation type: {operation.OperationType}");
            }
        }

        var response = await _container.PatchItemAsync<OrderModel>(
            id: orderId,
            partitionKey: new PartitionKey(universalCallId),
            patchOperations: patchOperations,
            cancellationToken: cancellationTokenSource?.Token ?? CancellationToken.None);

        if (response.Diagnostics != null)
        {
            _logger.LogInformation("\nCosmos DB query diagnostics for PatchCallLogAsync: {Diagnostics}", response.Diagnostics);
        }

        _logger.LogInformation("Successfully patched Order for OrderId {0} ({1} RUs)",
            orderId,
            response.RequestCharge);

        return response.Resource;
    }

Sample:

PATCH http://localhost:7188/api/orders/c4a50add-52a4-417b-990f-6f3259335f36
Content-Type: application/json-patch+json

[
    {
        "op": "replace",
        "path": "/orderDetails",
        "value": {
            "OrderId": 2429319024,
            "OrderName": "Mini Wheats",
            "OrderDescription": "",
            "OrderType": "Renewal"
        }
    }
]

Expected behavior
When patching items, I expect it to work similarly to Create and Upsert where it uses the PropertyNamingPolicy which in my case has a value of CamelCase.

Actual behavior
Data is patched in Cosmos container using the Default PropertyNamingPolicy.

Expected result in cosmos

{
    "id": "c4a50add-52a4-417b-990f-6f3259335f36",
    "orderDate": "2024-09-27T20:38:41",
    "orderDetails": {
        "orderId": 2429319024,
        "orderName": "Mini Wheats",
        "orderDescription": "",
        "orderType": "Renewal"
    }
}

Actual result in cosmos

{
    "id": "c4a50add-52a4-417b-990f-6f3259335f36",
    "orderDate": "2024-09-27T20:38:41",
    "orderDetails": {
        "OrderId": 2429319024,
        "OrderName": "Mini Wheats",
        "OrderDescription": "",
        "OrderType": "Renewal"
    }
}

Environment summary
Environment:
Azure Functions v4 - .NET 8 - Isolated worker
VS2022 17.11.4
Nuget Pakcges:

  • Microsoft.AspNetCore.App
  • AspNetCore.HealthChecks.CosmosDb Version="8.0.1"
  • Azure.Identity Version="1.12.1"
  • Microsoft.Azure.Cosmos Version="3.43.1"
  • Microsoft.Azure.Functions.Worker Version="1.23.0"
  • Microsoft.Azure.Functions.Worker.Extensions.Http Version="3.2.0"
  • Microsoft.Azure.Functions.Worker.Extensions.Http.AspNetCore Version="1.3.2"
  • Microsoft.Azure.Functions.Worker.Extensions.OpenApi Version="1.5.1"
  • Microsoft.Azure.Functions.Worker.Extensions.Warmup Version="4.0.2"
  • Microsoft.Azure.Functions.Worker.Sdk Version="1.18.0"
  • Microsoft.ApplicationInsights.WorkerService Version="2.22.0"
  • Microsoft.Azure.Functions.Worker.ApplicationInsights Version="1.4.0"
  • Microsoft.Azure.WebJobs.Extensions Version="5.0.0"
  • Microsoft.Azure.AppConfiguration.Functions.Worker Version="7.3.0"
  • Microsoft.Extensions.Configuration.AzureAppConfiguration Version="7.3.0"
  • Microsoft.Extensions.Configuration.UserSecrets Version="8.0.0"

Additional context
Happy to provide additional code if needed.

@kirankumarkolli
Copy link
Member

@philipthomas-MSFT can you please look into it?

@kirankumarkolli
Copy link
Member

NamingPolicy is a pure SDK concept not service concept.

Patch API's all take string as path argument and application is expected to provide with the right cased value.

@calloncampbell
Copy link
Author

calloncampbell commented Oct 2, 2024

I got this working by reading the Object in the patch and then reserializing back as a camelCase object. This works, but it would be great if the Cosmos DB SDK would do this using the same CosmosPropertyNamingPolicy which is what the Create and Upsert use.

@philipthomas-MSFT
Copy link
Contributor

I got this working by reading the Object in the patch and then reserializing back as a camelCase object. This works, but it would be great if the Cosmos DB SDK would do this using the same CosmosPropertyNamingPolicy which is what the Create and Upsert use.

@calloncampbell Do you mind sharing your final solution. Also, agree with @kirankumarkolli on path being a string. We can consider a typed solution in the future.

@kirankumarkolli
Copy link
Member

@calloncampbell it's an interesting idea to explore. Your solution will definitely help us learn and improve this API.

The API can take Field/Property, but it will be scoped to a single name only, where the data model might be more nested.
May be LINQ expression might be an alternative?

@philipthomas-MSFT
Copy link
Contributor

philipthomas-MSFT commented Oct 8, 2024

I think I am leaning toward a PatchOperationBuilder that gets passed into an overloaded PatchItemAsync

Current Container.PatchItemAsync

public abstract Task<ItemResponse<T>> PatchItemAsync<T>(
    string id,
    PartitionKey partitionKey,
    IReadOnlyList<PatchOperation> patchOperations,
    PatchItemRequestOptions requestOptions = null,
    CancellationToken cancellationToken = default);

Future Container.PatchItemAsync overloaded with PatchOperationBuilder

public abstract Task<ItemResponse<T>> PatchItemAsync<T>(
    string id,
    PartitionKey partitionKey,
    PatchOperationBuilder<T> patchOperationBuilder,
    PatchItemRequestOptions requestOptions = null,
    CancellationToken cancellationToken = default);

Since all Container's operations have access to the CosmosSerializer, can use deferred execution to do the serialization after you create the builder and pass it into the overloaded PatchItemAsync.

Maybe implementation could look like this

ToDoActivity toDoActivity = new ToDoActivity { Address = new Address1 { City = "Seattle" } };

PatchOperationBuilder<ToDoActivity> builder = new PatchOperationBuilder<ToDoActivity>(
    toDoActivity,
    op => op.SetOperationType(PatchOperationType.Add)
            .Property(p => p.Address.City)
);

CosmosJsonDotNetSerializer serializer = new CosmosJsonDotNetSerializer(new CosmosSerializationOptions
{
    PropertyNamingPolicy = CosmosPropertyNamingPolicy.CamelCase,
});

List<PatchOperation> patchOperations = builder.Build(serializer);

Now the builder.Build(serializer); is something you would not have to execute unless you wanted to, but really the overloaded PatchItemAsync would handle that by using the serializer that is configured in the CosmosClientBuilder.WithSerializerOptions. Just a thought. If we wanted to just pass the serializer into builder.Build(serializer), then there is no need to overload PatchItemAsync and we can just use the existing one, but you would have to pass it in twice.

Also, I would obviously need to spend more on ensuring that all variations of the supported operations are supported and tested (Add, Replace, etc.).

I think this would be a good addition to the SDK.

@philipthomas-MSFT philipthomas-MSFT added this to the Backlog milestone Oct 8, 2024
@philipthomas-MSFT philipthomas-MSFT moved this from In Progress to NotApproved in Azure Cosmos SDKs Oct 8, 2024
@github-project-automation github-project-automation bot moved this from NotApproved to Done in Azure Cosmos SDKs Nov 12, 2024
@calloncampbell
Copy link
Author

Thanks for your investigation and efforts.

@calloncampbell
Copy link
Author

I got this working by reading the Object in the patch and then reserializing back as a camelCase object. This works, but it would be great if the Cosmos DB SDK would do this using the same CosmosPropertyNamingPolicy which is what the Create and Upsert use.

@calloncampbell Do you mind sharing your final solution. Also, agree with @kirankumarkolli on path being a string. We can consider a typed solution in the future.

Here is the solution I went with...

I added this...
object camelCaseValue = _serializationService.CreateCamelCaseObject(operation.value);

public List<PatchOperationModel> CreatePatchOperation(string requestBody)
{
    if (string.IsNullOrWhiteSpace(requestBody))
    {
        throw new Exception("RequestBody is null or empty");
    }

    // Use the Newtonsoft.Json.JsonConvert.DeserializeObject method to deserialize a JsonPatchDocument, and use
    // System.Text.Json for all other JSON requests and responses.
    JsonPatchDocument<CallLogModel> jsonPatchDocument = _serializationService.Deserialize<JsonPatchDocument<CallLogModel>>(requestBody);
    if (jsonPatchDocument is null)
    {
        throw new Exception("Deserialization exception of request body into JsonPatchDocument.");
    }
   
    List<PatchOperationModel> patchOperations = new();                

    foreach (var operation in jsonPatchDocument.Operations)
    {
        if (operation.OperationType == OperationType.Add)
        {
            var camelCasePath = CamelCaseConverter.ToCamelCase(operation.path);
            object camelCaseValue = _serializationService.CreateCamelCaseObject(operation.value);

            patchOperations.Add(new()
            {
                OperationType = Domain.Enums.PatchOperationType.Add,
                Path = camelCasePath,
                Value = camelCaseValue
            });
        }
        else if (operation.OperationType == OperationType.Replace)
        {
            var camelCasePath = CamelCaseConverter.ToCamelCase(operation.path);
            object camelCaseValue = _serializationService.CreateCamelCaseObject(operation.value);

            patchOperations.Add(new()
            {
                OperationType = Domain.Enums.PatchOperationType.Replace,
                Path = camelCasePath,
                Value = camelCaseValue
            });
        }
        else
        {
            throw new Exception($"Unsupported operation type: {operation.OperationType}");
        }
    }

    return patchOperations;
}

In my serialization service, using the Newtonsoft JSON library, I used the ExpandoObject when reserializing. Not what I desired to do, but does the job.

        public object CreateCamelCaseObject<T>(T objectToExpandoObject)
        {
            if (objectToExpandoObject is null)
            {
                return default;
            }

            try
            {
                if (objectToExpandoObject is string)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is int)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is long)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is decimal)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is bool)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is float)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is double)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is DateTime)
                {
                    return objectToExpandoObject;
                }
                else if (objectToExpandoObject is DateTimeOffset)
                {
                    return objectToExpandoObject;
                }
                else
                {
                    var jObjectIn = JObject.FromObject(objectToExpandoObject, JsonSerializer.Create());
                    var jObjectOut = JObject.FromObject(jObjectIn.ToObject<ExpandoObject>(), JsonSerializer.Create(_readJsonSerializerOptions));
                    return jObjectOut;
                }
            }
            catch
            {
                return default;
            }
        }

Hope this helps.

Callon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issue created by a customer needs-investigation
Projects
Status: Done
Development

No branches or pull requests

4 participants