Use of EscapeUriString in URL formation leads to docs that can be created but not deleted

[abhijitpai]

Describe the bug https://docs.microsoft.com/en-us/dotnet/api/system.uri.escapeuristring?view=net-6.0 EscapeUriString (obsolete in recent .net versions) is used in https://github.com/Azure/azure-cosmos-dotnet-v3/blob/d331683a3937cc435002f291cd1998f8c5b93c46/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs#L1207 and similar to form URLs that doesn't encode parameters correctly. These should instead use EscapeDataString.

To Reproduce Direct mode: Item item1 = new Item() { Id = "abcd/", Partitionkey = "pk1" }; await container.CreateItemAsync(item1, new PartitionKey(item1.Partitionkey)); await container.DeleteItemAsync("abcd/", new PartitionKey(item1.Partitionkey));

Expected behavior Both operations succeed.

Actual behavior Create succeeds but delete throws 404 because the service is sent the document name "abcd" without any trace of "/".

Environment summary SDK Version: 3.23.0 OS Version (e.g. Windows, Linux, MacOSX): Windows.

Additional context Add any other context about the problem here (for example, complete stack traces or logs).

[ealsur]

@abhijitpai We used to have / documented as an unsupported character for ids?

EDIT: Here is the REST API documentation for the unsupported characters: https://docs.microsoft.com/en-us/dotnet/api/microsoft.azure.documents.resource.id?view=azure-dotnet#remarks

How does the REST API handle deletes for items with / on the Uri when using named routes?

Reference: https://docs.microsoft.com/en-us/rest/api/cosmos-db/delete-a-document

[abhijitpai]

It is likely that our REST API implementation does not handle it correctly. But by sticking to the obsolete method, we are having users need to find other much costlier workarounds after they create such documents, so I don't see why this should not be fixed.

[ealsur]

Agree that if the method is obsolete, it should be addressed. The only thing is we don't target NET 6, but NET Standard 2.0, and it's not obsolete there:

[abhijitpai]

We are talking about reducing customer pain here.

[ealsur]

Not disagreeing, but then the fact that it's obsolete on NET 6 is not relevant, because we don't use NET 6.

If we then want to talk about the characters, if we make the change on the code to use the other method, will the REST API work? If the answer is yes, then I guess we can do the change. If the answer is no, then the REST API needs to change first in any case and making the SDK change is not needed until that happens because the other reason (obsolescence) is not currently valid for NET Standard 2.0.

[abhijitpai]

It is relevant due to the fact it is made obsolete because it doesn't encode properly, but anyway we can let that be. REST API is not in my control, and neither is it in yours - but the SDK is. I am not sure why it matters that we need to fix the REST API to resolve the customer problem when we can make this fix in the SDK.

[ealsur]

@abhijitpai I changed the method in the code and did a simple test:

CosmosClient cosmosClient = new CosmosClient("<connectionstring>", new CosmosClientOptions() { ConnectionMode = ConnectionMode.Gateway });

            var container = cosmosClient.GetContainer("test", "items");
            var item1 = new { Id = "abcd/", Partitionkey = "pk1" };
            await container.CreateItemAsync(item1, new PartitionKey(item1.Partitionkey));
            await container.DeleteItemAsync<dynamic>("abcd/", new PartitionKey(item1.Partitionkey));

Gateway returns a 401:

The problem is way deeper than just the EscapeDataString, it is deep in the PathsHelper, the code does this:

It calls Uri.UnescapeDataString which basically removes the escaping:

Looks like in order to support this scenario, it will also require changes in the PathHelper and Resource parsing.

[ealsur]

I hacked it around to avoid the Unescape so the request uses the escaped Url, but Gateway still fails with a 401, would this mean this is not supported by the service even if we did manage to change the code?

[abhijitpai]

Please try it in direct mode.

[ealsur]

Direct mode yields a 404 after hacking the code to have the EscapeDataString and avoid the unescaping on the PathHelper.

Could it be because really the document's id is not abcd%2F but abcd/?

[ealsur]

The issue might need both ends (client and backend) to use EscapeData and UnescapeData in order to work, because I can have 2 documents like:

[abhijitpai]

Yes, the DocumentName you have above is incorrect - it should match the user specified document name which is "abcd/". And that also explains how we address both the documents in your last image - for the second document, the DocumentName that needs to be passed is "abcd%2F". Let me also research this once more to see if there are other ambiguous things in this flow - I thought I'd got the flow working with something like just the fix for the issue I mentioned in the original post, but I will recheck that.