[BUG] Breaking change in Azure.Data.Table makes it no longer possible to retrieve an entity with an empty partition or row key

[danielmarbach]

Library name and version

Azure.Data.Tables 12.7.0

Describe the bug

Due to legacy reasons, we have a scenario where we might have entities that are stored with string.Empty as a row key and we need to be able to load those. The service accepts such entities without a problem. Here is a simple reproduction code

using Azure.Data.Tables;

var partitionKey = Guid.NewGuid().ToString();
var entity = new TableEntity
{
    RowKey = string.Empty,
    PartitionKey = partitionKey
};
var tableServiceClient = new TableServiceClient(Environment.GetEnvironmentVariable("AzureTableConnectionString"));
var tableClient = tableServiceClient.GetTableClient("AzureDataTableRepro");
await tableClient.CreateIfNotExistsAsync();
await tableClient.AddEntityAsync(entity);

var foundEntity = await tableClient.GetEntityAsync<TableEntity>(partitionKey, string.Empty);

Console.WriteLine($"{foundEntity.Value.PartitionKey}");

This prints the PartitionKey previously generated when using 12.6.1.

When we upgrade to 12.7.0 we get the following stack trace

Unhandled exception. System.ArgumentException: Value cannot be an empty string. (Parameter 'rowKey')
   at Azure.Core.Argument.AssertNotNullOrEmpty(String value, String name)
   at Azure.Data.Tables.TableRestClient.QueryEntityWithPartitionAndRowKeyAsync(String table, String partitionKey, String rowKey, Nullable`1 timeout, String format, String select, String filter, RequestContext context)
   at Azure.Data.Tables.TableClient.GetEntityInternalAsync[T](Boolean async, String partitionKey, String rowKey, Boolean noThrow, IEnumerable`1 select, CancellationToken cancellationToken)
   at Azure.Data.Tables.TableClient.GetEntityAsync[T](String partitionKey, String rowKey, IEnumerable`1 select, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in /home/danielmarbach/Projects/AzureDataTableRepro/ConsoleApp1/ConsoleApp1/Program.cs:line 16
   at Program.<Main>(String[] args)

While historically null was never permitted because the client prevented it with validating against null 12.7.0 introduced validation against empty which is a breaking change which also makes it impossible to retrieve already persisted entities that are using a row key with string.Empty

Expected behavior

Being able to store and retrieve entities with rowKey = string.Empty

Actual behavior

An ArgumentException is thrown

For example QueryEntityWithPartitionAndRowKeyAsync has been changed from

        public async Task<ResponseWithHeaders<IReadOnlyDictionary<string, object>, TableQueryEntityWithPartitionAndRowKeyHeaders>> QueryEntityWithPartitionAndRowKeyAsync(string table, string partitionKey, string rowKey, int? timeout = null, QueryOptions queryOptions = null, CancellationToken cancellationToken = default)
        {
            if (table == null)
            {
                throw new ArgumentNullException(nameof(table));
            }
            if (partitionKey == null)
            {
                throw new ArgumentNullException(nameof(partitionKey));
            }
            if (rowKey == null)
            {
                throw new ArgumentNullException(nameof(rowKey));
            }

to

            Argument.AssertNotNullOrEmpty(table, nameof(table));
            Argument.AssertNotNullOrEmpty(partitionKey, nameof(partitionKey));
            Argument.AssertNotNullOrEmpty(rowKey, nameof(rowKey));

Reproduction Steps

using Azure.Data.Tables;

var partitionKey = Guid.NewGuid().ToString();
var entity = new TableEntity
{
    RowKey = string.Empty,
    PartitionKey = partitionKey
};
var tableServiceClient = new TableServiceClient(Environment.GetEnvironmentVariable("AzureTableConnectionString"));
var tableClient = tableServiceClient.GetTableClient("AzureDataTableRepro");
await tableClient.CreateIfNotExistsAsync();
await tableClient.AddEntityAsync(entity);

var foundEntity = await tableClient.GetEntityAsync<TableEntity>(partitionKey, string.Empty);

Console.WriteLine($"{foundEntity.Value.PartitionKey}");

Environment

No response

[danielmarbach]

@christothes would you mind looking at this? The breaking change makes it for us impossible to upgrade to 12.7.0

[danielmarbach]

FYI

var entity = new TableEntity
{
    RowKey = string.Empty,
    PartitionKey = string.Empty
};

is also supported and would be broken by the new validation. The service accepts perfectly that.

using Azure.Data.Tables;

var entity = new TableEntity
{
    RowKey = string.Empty,
    PartitionKey = string.Empty
};
var tableServiceClient = new TableServiceClient(Environment.GetEnvironmentVariable("AzureTableConnectionString"));
var tableClient = tableServiceClient.GetTableClient("AzureDataTableRepro");
await tableClient.CreateIfNotExistsAsync();
await tableClient.AddEntityAsync(entity);

var foundEntity = await tableClient.GetEntityAsync<TableEntity>(string.Empty, string.Empty);

Console.WriteLine($"{foundEntity.Value.PartitionKey}");

[christothes]

Hi @danielmarbach - Thanks for reporting this. The behavior change was not intentional and was a result of a new generated code path. I'll get a fix out soon.

[danielmarbach]

I think there is a behaviour difference between table API on cosmos vs table storage. On cosmos table API empty row keys are not allowed if I'm not mistaken vs on table storage they are

[christothes]

I think there is a behaviour difference between table API on cosmos vs table storage. On cosmos table API empty row keys are not allowed if I'm not mistaken vs on table storage they are

If so, this is unrelated to why this change was introduced.

[danielmarbach]

Agreed. It was my crappy way of trying to say the validation might make sense depending on the underlying service

[christothes]

Agreed. It was my crappy way of trying to say the validation might make sense depending on the underlying service

I appreciate you giving the benefit of the doubt - but this was just an unexpected behavior change due to some implementation details around how we generated this new code path. 😄

[danielmarbach]

@christothes I saw it has been added to the next minor in the change log. Why is this not considered a patch?

[christothes]

@christothes I saw it has been added to the next minor in the change log. Why is this not considered a patch?

That's the plan. Stay tuned! 😄

[danielmarbach]

I've updated our code and kicked off CI/CD. Will report back

[danielmarbach]

All good. All our test scenarios run now :green_heart: Thanks again for the fix!