azure-sdk-for-net icon indicating copy to clipboard operation
azure-sdk-for-net copied to clipboard

[BUG] DictionaryTableExtensions.ToOdataAnnotatedDictionary assumes ETag values will always have the key '

Open P-Nathan opened this issue 3 years ago • 6 comments

Library name and version

Azure.Data.Tables 12.6.1

Describe the bug

Hey there!

I've discovered that line 26 of DictionaryTableExtensions.cs doesn't account for scenarios where the value for the ETag in the dictionary might have the key 'Etag' and not 'odata.etag'. Thereby, failing to remove the ETag from the dictionary and resulting in a NotSupportedException being thrown when the client eventually goes to write the ETag to the table store as JSON.

This means that any implementation of ITableEntity that also implements IDictionary<string, object> needs to account for the fact the package is expecting the Etag value in the dictionary to have the key "odata.etag" for it to be correctly handled by ToOdataAnnotatedDictionary.

I've encountered this issue myself as I'm using a custom ExpandoObject that implements ITableEntity and maps the interfaced properties to a dictionary that stores the values using the property names as keys. This allows me to handle the entity as a dictionary internally. This is a common way of implementing custom ExpandoObjects that implement interfaces. See the following stack overflow link for an example: (Adding Interface Implementation to ExpandoObject)

This has resulted in me having to write quite a bit of workaround code into my custom ExpandoObject to account for this scenario.

Links to source code:

Expected behavior

ToOdataAnnotatedDictionary should correctly strip the ETag value from the dictionary in scenarios where the IDictionary's value for Etag is stored under the key "Etag".

Just a suggestion, but maybe it's worth either checking for an existence of an Etag value with key "odata.etag" in the dictionary, and strip that one out if it's present. Otherwise, strip out the value with the key "Etag'.

Another suggestion could just be to strip out all values that are Etags, as they can't be handled by the JSONWriter anyway.

Actual behavior

ETag value is left in the dictionary as it has the key "Etag" and ToOdataAnnotatedDictionary expects it to have the key "odata.etag".

An exception is then thrown in TableRestClient.cs at line 896 in CreateInsertEntityRequest with the following stack trace: image

Reproduction Steps

Recreate Bug

  1. Create implementation of ITableEntity that also implements IDictionary<string, object>
  2. Etag value in dictionary has the key 'Etag'.
  3. Attempt to push entity to table storage using TableClient.
  4. NotSupportedException thrown.

Recreate Workaround

  1. Create implementation of ITableEntity that also implements IDictionary<string, object>
  2. Etag value in dictionary has the key 'odata.etag'.
  3. Attempt to push entity to table storage using TableClient.
  4. Entity added to table storage.

Environment

  • Windows 11
  • Visual Studio 2022
  • .NET 6

P-Nathan avatar Aug 24 '22 15:08 P-Nathan

Thank you for your feedback. Tagging and routing to the team member best able to assist.

jsquire avatar Aug 24 '22 16:08 jsquire

Hi @P-Nathan - I agree this error is confusing. Could you talk a bit more about why your scenario requires a property named ETag that is of the type Azure.ETag?

christothes avatar Sep 09 '22 18:09 christothes

Hi @christothes,

Apologies for the delayed response, I've just returned from leave.

Regarding our scenario where we require a property named ETag that is of type Azure.Etag on our table entity, we're using a custom expando object that maps the properties of the ITableEntity interface to values in a dictionary.

This is done as we can't rely on us knowing the structure of the objects we're pushing up to table storage at compile time, so we're using dynamics to provide us with a single class that can handle every structure of data we're pushing up to table storage.

The problem with using a dictionary of property name to property value is that ToOdataAnnotatedDictionary tries to strip the etag from the properties before serialization, but doesn't look for the property under the key "Etag" even though that would be expected to be the key in the property dictionary as it's enforced by the ITableEntity interface.

Does the above description assist at all?

P-Nathan avatar Sep 14 '22 08:09 P-Nathan

@christothes

Just to add:

Because ToOdataAnnotatedDictionary is expecting to find the ETag under "odata.etag" we're having to override the property key in the dictionary, and have interactions with ITableEntity.Etag on the dynamic object be directed to odata.etag in the property dictionary

P-Nathan avatar Sep 14 '22 08:09 P-Nathan

Hi @P-Nathan Thanks for the additional context. Unfortunately, I don't think we can change this behavior, as it would be a behavioral breaking change. For example, if you had entities that had a property named ETag of type string, this would get serialized currently. Silently removing that property during serialization would cause unexpected data loss.

Considering that you are already customizing how you deal with dynamic entities, is there a reason that the workaround isn't acceptable? Note that another workaround would be to change the type of your existing property to string - which can be produced from the ETag type via its .ToString() method.

christothes avatar Sep 14 '22 14:09 christothes

Hey @christothes

The workaround works for our usecase, and should be good for our usage in the forseeable future. Though, I should note that this workaround relies on a hard-coding of odata.etag as the key within our code so is not resilient to changes in this key on your end.

The potential issue I see is that someone may come across this issue in the future themselves. I had to read through the project's source code to understand why the package wasn't working with my implementation and truly understand the errors I was recieving, resulting in the workaround being discovered. I wouldn't want someone else to have to do the same.

I believe there is a valid usecase for having an ITableEntity also implement IDictionary, and think this bug is a limitation in other engineers achieving this behaviour as it's not directly communicated that the ITableEntity implementing IDictionary will change the serialization behaviour of the package.

P-Nathan avatar Sep 14 '22 15:09 P-Nathan