[BUG] Azure.AI.OpenAI deserialization does not account for null-valued JSON properties and throws InvalidOperationException.

CharlieDigital commented 4 months ago

Library name and version

Azure.AI.OpenAI 1.0.0-beta.17

Describe the bug

When processing streaming JSON responses from an OpenAI compatible API endpoint, the Azure.AI.OpenAI.StreamingChatCompletionsUpdate.StreamingDeltaData.DeserializeStreamingDeltaData throws an InvalidOperationException when encountering a null-valued property:

System.InvalidOperationException: The requested operation requires an element of type 'Array', but the target element has type 'Null'.
   at System.Text.Json.ThrowHelper.ThrowJsonElementWrongTypeException(JsonTokenType expectedType, JsonTokenType actualType)
   at System.Text.Json.JsonElement.EnumerateArray()
   at Azure.AI.OpenAI.StreamingChatCompletionsUpdate.StreamingDeltaData.DeserializeStreamingDeltaData(JsonElement element, ModelReaderWriterOptions _)
   at Azure.AI.OpenAI.StreamingChatCompletionsUpdate.StreamingChoiceData.DeserializeStreamingChoiceData(JsonElement element, ModelReaderWriterOptions _)
   at Azure.AI.OpenAI.StreamingChatCompletionsUpdate.DeserializeStreamingChatCompletionsUpdates(JsonElement element)
   at Azure.Core.Sse.SseAsyncEnumerator`1.EnumerateFromSseStream(Stream stream, Func`2 multiElementDeserializer, CancellationToken cancellationToken)+MoveNext()
   at Azure.Core.Sse.SseAsyncEnumerator`1.EnumerateFromSseStream(Stream stream, Func`2 multiElementDeserializer, CancellationToken cancellationToken)+System.Threading.Tasks.Sources.IValueTaskSource<System.Boolean>.GetResult()

This error can occur if the provider includes null-valued property tokens in the response stream. For example, Together.ai's implementation.

Expected behavior

When processing streaming JSON responses from an OpenAI compatible API endpoint, the Azure.AI.OpenAI.StreamingChatCompletionsUpdate.StreamingDeltaData.DeserializeStreamingDeltaData correctly handles null values.

Actual behavior

When the response has a null value, this causes an InvalidOperationException to be thrown.

This occurs because the code for deserialization does not account for null values. For example: https://github.com/Azure/azure-sdk-for-net/blob/bed3a44beecff00a5aefc88709f1155d1ba24cd4/sdk/openai/Azure.AI.OpenAI/src/Custom/ChatCompletions/StreamingChatCompletionsUpdate.Serialization.cs#L239-L245

if (deltaProperty.NameEquals("tool_calls"))
{
    // deltaProperty.Value could be a JSON `null`
    foreach (JsonElement toolCallElement in deltaProperty.Value.EnumerateArray())
    {
        result.ToolCallUpdates.Add(StreamingToolCallUpdate.DeserializeStreamingToolCallUpdate(toolCallElement));
    }
 }

Will cause an exception because the deltaProperty.Value could be null if the JSON has a null value for the attribute:

{
    "tool_calls": null
}

Reproduction Steps

using System.Text.Json;

var json = "{ \"tool_calls\": null }";

var js = JsonDocument.Parse(json);

foreach (JsonProperty property in js.RootElement.EnumerateObject())
{
  if (property.NameEquals("tool_calls"))
  {
    foreach (JsonElement toolCallElement in property.Value.EnumerateArray())
    {
      Console.WriteLine("Won't reach here because of exception");
    }
  }
}

This will produce the same error.

Proposed fix:

using System.Text.Json;

var json = "{ \"tool_calls\": null }";

var js = JsonDocument.Parse(json);

foreach (JsonProperty property in js.RootElement.EnumerateObject())
{
  // Check if the raw value is `null`
  if (property.NameEquals("tool_calls") && property.Value.GetRawText() != "null")
  {
    foreach (JsonElement toolCallElement in property.Value.EnumerateArray())
    {
      Console.WriteLine("Won't reach here because of exception");
    }
  }
}

Alternatively, modify the code here to allow null values: https://github.com/Azure/azure-sdk-for-net/blob/bed3a44beecff00a5aefc88709f1155d1ba24cd4/sdk/core/Azure.Core/src/DynamicData/MutableJsonElement.cs#L1397-L1403

        private void EnsureArray()
        {
            // Allow `JsonValueKind.Null`
            if (_element.ValueKind != JsonValueKind.Array && _element.ValueKind != JsonValueKind.Null)
            {
                throw new InvalidOperationException($"Expected an 'Array' type but was '{_element.ValueKind}'.");
            }
        }

Environment

.NET SDK:
 Version:           8.0.201
 Commit:            4c2d78f037
 Workload version:  8.0.200-manifests.3097af8b

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  14.2
 OS Platform: Darwin
 RID:         osx-arm64
 Base Path:   /usr/local/share/dotnet/sdk/8.0.201/

.NET workloads installed:
There are no installed workloads to display.

Host:
  Version:      8.0.2
  Architecture: arm64
  Commit:       1381d5ebd2

.NET SDKs installed:
  6.0.300 [/usr/local/share/dotnet/sdk]
  7.0.402 [/usr/local/share/dotnet/sdk]
  8.0.100 [/usr/local/share/dotnet/sdk]
  8.0.101 [/usr/local/share/dotnet/sdk]
  8.0.201 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.5 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.12 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.1 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.2 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 6.0.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.12 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.0 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.1 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.2 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

Other architectures found:
  x64   [/usr/local/share/dotnet/x64]
    registered at [/etc/dotnet/install_location_x64]

Environment variables:
  Not set

global.json file:
  Not found

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
  https://aka.ms/dotnet/download

github-actions[bot] commented 4 months ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @jpalvarezl @trrwilson.

JasonYeMSFT commented 2 months ago

I have seen the same issue with responses where the "role" property is null. My team kinda needs a fix for this null dereference issue soon. Can I make a PR to let the parser ignore values that are null? @trrwilson

trrwilson commented 2 months ago

@CharlieDigital @JasonYeMSFT -- thanks for the report and corroboration.

I don't have any specific objection to loosening the checks for streaming payloads to permit nulls; the tricky thing is that this is not compliant with the OpenAI API specification; as you can see in the .yaml file, properties like tool_calls and role on ChatCompletionStreamResponseDelta are optional, but non-nullable. Strictly speaking, "tool_calls": null or "role": null is not valid payload.

I'd still advocate for us being more permissive because I don't see enough relative value in being "schema cop" for nullability, but the team will need to discuss and ensure we're being consistent.

Azure / azure-sdk-for-net