Elfocrash / Cosmonaut

🌐 A supercharged Azure CosmosDB .NET SDK with ORM support
https://cosmonaut.readthedocs.io
MIT License
341 stars 44 forks source link

JsonSerialization: CosmonautHelpers -> ToCosmonautDocument does not respect JsonSerializerSettings supplied by CosmosStoreSettings #95

Closed ankitvijay closed 5 years ago

ankitvijay commented 5 years ago

I ran into an issue while using Cosmonaunt library. I had defined CosmosStoreSettings as follows:

var cosmosStoreSettings = new CosmosStoreSettings("<Db-Name>","<endpointUri>","<authKey>")
            {
                DefaultCollectionThroughput = 1000,
                IndexingPolicy = new IndexingPolicy
                {
                    IncludedPaths = new Collection<IncludedPath> { new IncludedPath { Path = "/*" } }
                },
                JsonSerializerSettings = new JsonSerializerSettings
                {
                    PreserveReferencesHandling = PreserveReferencesHandling.Objects,
                    ReferenceLoopHandling = ReferenceLoopHandling.Serialize
                }
            };

services.AddCosmosStore<MyEntity>(cosmosStoreSettings);

MyEntity had a self-referential loop. Since, JsonSerializerSettings had been set to NOT error on the reference loop, I expected below line to save the MyEntity object to the CosmosDB collection.

await _cosmosStore.AddAsync(policy);

However, this line throws below error:

Self referencing loop detected for property 'MyEntity' with type ''. Path ''.

Note that when I used the DocumentClient directly to save the MyEntity object, then MyEntity was saved successfully. My expectation was that it should respect the JsonSerializerSettings defined by cosmosStoreSettings when we call AddAsync and not throw an exception.

On digging into the Cosmonaut source code, I found that issue was in the method CosmonautHelpers --> ToCosmonautDocument. This method uses "Global" JsonSerialize settings instead of the one defined in cosmosStoreSettings. Here is the line of code which led to this issue:

var document = JsonConvert.DeserializeObject<dynamic>(JsonConvert.SerializeObject(obj));

When, updating the default global JsonSerializerSettings with the same setting as cosmosStoreSettings the entity was saved successfully.

I do not think i should need to change global JsonSerializerSettings to get this working since I'm supplying the settings already.

Elfocrash commented 5 years ago

Thank you for your report. I will take a look and publish a fix.

Elfocrash commented 5 years ago

@ankitvijay I did identify the issue and I will be committing a fix later today. I will also probably publish a new nuget version as well with the fix today.

Funny enough, this fix shows that there is a bug with the underlying Cosmos DB v2 SDK as well where the JsonSerializationSettings are now being honored during LINQ to SQL conversion, but there is nothing I can do about that.

It's a valid bug here: https://github.com/Azure/azure-cosmos-dotnet-v2/issues/351

Elfocrash commented 5 years ago

Fixed in b61fc0161b02b981408f4bff71ab46a98e4eaa4d. Published in 2.11.1: https://www.nuget.org/packages/Cosmonaut/2.11.1

ankitvijay commented 5 years ago

Waoow.. That was lightning fast. Thanks for the fix :)

irriss commented 5 years ago

Now signature of ToCosmonautDocument method has changed. I use it in many places. Now need to fix all. It is a minor but still breaking change. Would be better to provide a default argument to avoid breaking existing code.

Elfocrash commented 5 years ago

@irriss Sorry for that. You are right I should have added a default one. Ideally I do whenever I can but for some reason I thought this method was marked internal. I will add a default implementation in the next version so if you don't have a need to update you can skip that version.

irriss commented 5 years ago

@Elfocrash not a big issue, thanks for answer and this excellent project!