IEvangelist / azure-cosmos-dotnet-repository

Wraps the .NET SDK for Azure Cosmos DB abstracting away the complexity, exposing a simple CRUD-based repository pattern
https://ievangelist.github.io/azure-cosmos-dotnet-repository
MIT License
300 stars 89 forks source link

DefaultRepository CreateAsync uses Newtonsoft JsonConvert.SerializeObject in TryLogDebugDetails which causes problems on some items. #460

Open cardgeny opened 2 months ago

cardgeny commented 2 months ago

We are continuously addressing and improving the SDK, if possible, make sure the problem persist in the latest SDK version.

Describe the bug When in debug mode TryLogDebugDetails in CreateAsync fails with

Self referencing loop detected for property 'Root' with type 'System.Text.Json.Nodes.JsonValuePrimitive`1[System.Text.Json.JsonElement]'. Path 'Messages[7].ToolCalls[0].Function.Arguments'.

error. If LogLevel is raised to 'Information' that error does not occur.

To Reproduce

Expected behavior

Actual behavior Unhandled exception

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

Additional context at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.CheckForCircularReference(JsonWriter writer, Object value, JsonProperty property, JsonContract contract, JsonContainerContract containerContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeList(JsonWriter writer, IEnumerable values, JsonArrayContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeList(JsonWriter writer, IEnumerable values, JsonArrayContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.Serialize(JsonWriter jsonWriter, Object value, Type objectType) at Newtonsoft.Json.JsonSerializer.SerializeInternal(JsonWriter jsonWriter, Object value, Type objectType) at Newtonsoft.Json.JsonConvert.SerializeObjectInternal(Object value, Type type, JsonSerializer jsonSerializer) at Microsoft.Azure.CosmosRepository.DefaultRepository1.TryLogDebugDetails(ILogger logger, Func1 getMessage) at Microsoft.Azure.CosmosRepository.DefaultRepository1.<CreateAsync>d__7.MoveNext() at System.Threading.Tasks.ValueTask1.get_Result()

IEvangelist commented 2 months ago

What does the code look like, could you share a small project that only includes the code that reproduces the bug? That's usually the best approach for addressing these types of concerns.

I see that there's a call to JsonConvert here:

https://github.com/IEvangelist/azure-cosmos-dotnet-repository/blob/c2221363744c274a6eeb8de605a5b4c7c347fbe2/src/Microsoft.Azure.CosmosRepository/Repositories/DefaultRepository.Create.cs#L28

The helper doesn't seem to be the issue, it's the call to serialize the result. But this only happens when you've specified Debug for the logging level, which I wouldn't expect in production.

https://github.com/IEvangelist/azure-cosmos-dotnet-repository/blob/c2221363744c274a6eeb8de605a5b4c7c347fbe2/src/Microsoft.Azure.CosmosRepository/Repositories/DefaultRepository.cs#L23-L30

cardgeny commented 2 months ago

You're of course correct that it wouldn't happen in production. However it's a bit annoying having to bump up debug level during development.

Attached is a small repro sample with chopped down models. Effectively it's a one of OpenAI response messages that causes the problem.

bug-repro.zip

mateuszkumpf commented 2 months ago

@cardgeny In your example, it is not the serialiser that is the problem but the model you are saving to the database, it is the model that contains the properties that have references to themselves. In this case, it would actually be possible to solve this problem by using ReferenceLoopHandling.Ignore in settings of serializer in CreateAsync and UpdateAsync. @IEvangelist What do you think about that?

cardgeny commented 1 month ago

@mateuszkumpf Sure, it is the data that causes the serialiser to crash. At the same time the item is created in cosmos db just fine. I mean what harm would it be to use it with ReferenceLoopHandling.Ignore option as it doesn't handle such cases anyway?

IEvangelist commented 4 weeks ago

@mateuszkumpf Sure, it is the data that causes the serialiser to crash. At the same time the item is created in cosmos db just fine. I mean what harm would it be to use it with ReferenceLoopHandling.Ignore option as it doesn't handle such cases anyway?

I'm fine with that, as long as it's isolated to that call. @mateuszkumpf and @cardgeny - anyone want to do a PR?