Azure / azure-cosmos-dotnet-v3

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

[V4] ReplaceItemAsync with abstract class => Exception in ItemResponse #1255

Open Mortana89 opened 4 years ago

Mortana89 commented 4 years ago

We use the UnitOfWork pattern to buffer our changes during processing of a single command/event/message. At the end, we do a commit which will:

A unit of work can be used for all entities extending from an abstract class (AggregateEntity). As such, we have a List toUpdateEntities which buffers changes. Upon committing, we, simplified, do: foreach(var entity in toUpdateEntities) { var resp = container.ReplaceItemAsync(entity, entity.Id, ...) }

problem however, is that the ReplaceItemAsync sees the type of entity as the AggregateEntity, instead of the actual type. As such, it's trying to cast the response to a type of ItemResponse<AggregateEntity> instead of e.g. ItemResponse<Car>. This causes a deserialization exception (cannot deserialize to an abstract class).

What if we can add a type parameter to the ReplaceItemAsync method?

ealsur commented 4 years ago

Deserializing abstract classes is an issue even with Newtonsoft.Json. There is no way for the serializer to know which class it should serialize.

ReplaceItemAsync already has <T> adding another Type I don't think it makes sense.

What you can do, is, similar to this solution for Newtonsoft.Json, has a JsonConverter that can cast the abstract class and understand which concrete type it is and serialize/deserialize accordingly.

Mortana89 commented 4 years ago

Yes, I know, but we are not serializing an abstract class, we have a list of classes that extend an abstract class. Cosmonaut/newtonsoft was able to accept entries of this list and serialize/deserialize to the concrete type. But thé replaceitem takes the T argument as the type instead of the concrete type of the entity being parsed. As such we can only use a MakeGeneric method within reflection to do this correctly...

ealsur commented 4 years ago

Do you need any information from the ItemResponse? If not, why not call ReplaceItemStreamAsync with the Stream serialization of the payload?

j82w commented 4 years ago

There is another issue which describes some alternatives. This is meant for the v3 SDK, but should work with v4 implementation.

Mortana89 commented 4 years ago

Do you need any information from the ItemResponse? If not, why not call ReplaceItemStreamAsync with the Stream serialization of the payload?

Yes, we need the ETag and the id property primarily from the response. We now use reflection to cast it, but I'll see if we could use dynamic for this. Do I keep this open or are you happy with the dynamic approach as the way forward

j82w commented 4 years ago

I think this is worth discussing further. While throwing because it can't create an interface makes since it seems like a bad user experience. The SDK should be able to serialize back to the same type it was deserialized from. If there is a performant fix then it should be done in my opinion.

@kirankumarkolli and @ealsur what are your thoughts?

Mortana89 commented 4 years ago
public static async Task<ItemResponse<T>> CreateItemAsync<T>(this CosmosContainer container, T entity, CancellationToken cancellationToken = default)
            where T : AggregateEntity
        {
            if (entity == null)
                throw new ArgumentNullException(nameof(entity));

            var realType = entity.GetType();
            var cosmosType = typeof(CosmosContainer);
            var method = cosmosType.GetMethod(nameof(CosmosContainer.CreateItemAsync));
            var genericMethod = method.MakeGenericMethod(realType);

            var parameters = new object[]
            {
                entity,
                new PartitionKey(entity.Discriminator),
                null, // request options
                cancellationToken
            };

            var task = (Task)genericMethod.Invoke(container, parameters);
            await task.ConfigureAwait(false);
            var resultProperty = task.GetType().GetProperty("Result");
            var obj = resultProperty.GetValue(task);

            if (realType != typeof(T))
            {
                // We can actually get the value as aggregate entity by reflection
                var valueProperty = obj.GetType().GetProperty("Value");
                var propValue = (AggregateEntity)valueProperty.GetValue(obj);
                SetAggregateEntityFields(propValue, entity);

                return null; // We can't cast, so return null
            }

            var response = obj as ItemResponse<T>;
            SetAggregateEntityFields(response.Value, entity);
            return response;
        }
 private static void SetAggregateEntityFields(AggregateEntity dbResponse, AggregateEntity userProvidedEntity)
        {
            userProvidedEntity.Id = dbResponse.Id;
            userProvidedEntity.ETag = dbResponse.ETag;
            userProvidedEntity.CreatedDateTime = dbResponse.CreatedDateTime;
            userProvidedEntity.TimeToLive = dbResponse.TimeToLive;
        }

This is the extension method we use now. Don't mind AggregateEntity, it's our abstract class that all domain entities extend, which hold common props like Id, ETag etc...