[BUG] Deserializing `null` BoundingBox throws an exception

[goncalo-oliveira]

Library name and version

Azure.Core 1.40.0

Describe the bug

Consider the following GeoJson point

    {
        "coordinates": [
            -1.740572,
            52.487343
        ],
        "type": "Point",
        "crs": null,
        "bbox": null
    }

Deserializing this into a GeoObject should result in a successful operation and a valid object. The BoundingBox property is after all, nullable.

public abstract class GeoObject
{
    // ...

    public GeoBoundingBox? BoundingBox { get; }

    // ...
}

The code used to deserialize the JSON string

var options = new JsonSerializerOptions
{
    PropertyNameCaseInsensitive = true
};

var point = JsonSerializer.Deserialize<GeoObject>( json, options );

Looking at the source code, the deserializer allows a null value if the property doesn't exist, but if the property exists, it expects it to be an array.

Expected behavior

The expected result would be a successful result and a valid instance of a GeoObject with BoundingBox set as null.

Actual behavior

The actual result is an exception being through because the deserializer is expecting an array.

tem.Text.Json.JsonException : The JSON value could not be converted to Azure.Core.GeoJson.GeoObject. Path: $ | LineNumber: 8 | BytePositionInLine: 1.
---- System.InvalidOperationException : The requested operation requires an element of type 'Array', but the target element has type 'Null'.
   at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, Utf8JsonReader& reader, Exception ex)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo`1 jsonTypeInfo, Nullable`1 actualByteCount)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo`1 jsonTypeInfo)
   at ...

Reproduction Steps

As described before, using the following json

    {
        "coordinates": [
            -1.740572,
            52.487343
        ],
        "type": "Point",
        "crs": null,
        "bbox": null
    }

And the code snippet to deserialize it.

var options = new JsonSerializerOptions
{
    PropertyNameCaseInsensitive = true
};

var point = JsonSerializer.Deserialize<GeoObject>( json, options );

Environment

.NET SDK:
 Version:           8.0.100
 Commit:            57efcf1350
 Workload version:  8.0.100-manifests.6c33ef20

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

.NET workloads installed:
 Workload version: 8.0.100-manifests.6c33ef20
There are no installed workloads to display.

Host:
  Version:      8.0.0
  Architecture: arm64
  Commit:       5535e31a71

.NET SDKs installed:
  6.0.420 [/usr/local/share/dotnet/sdk]
  7.0.400 [/usr/local/share/dotnet/sdk]
  8.0.100 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.28 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.10 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 6.0.28 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.10 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.0 [/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

[jsquire]

Hi @goncalo-oliveira. Thanks for reaching out and we regret that you're experiencing difficulties. Tagging and routing to the team member best able to assist.

[goncalo-oliveira]

I'd rewrite the function as follows

private static GeoBoundingBox? ReadBoundingBox( in JsonElement element )
{
    if ( !element.TryGetProperty( BBoxProperty, out JsonElement bboxElement ) )
    {
        return null;
    }

    if ( bboxElement.ValueKind == JsonValueKind.Null )
    {
        return null;
    }

    if ( bboxElement.ValueKind != JsonValueKind.Array )
    {
        throw new JsonException( "Bounding box must be an array" );
    }

    var arrayLength = bboxElement.GetArrayLength();

    return arrayLength switch
    {
        4 => new GeoBoundingBox(
            bboxElement[0].GetDouble(),
            bboxElement[1].GetDouble(),
            bboxElement[2].GetDouble(),
            bboxElement[3].GetDouble()
        ),
        6 => new GeoBoundingBox(
            bboxElement[0].GetDouble(),
            bboxElement[1].GetDouble(),
            bboxElement[3].GetDouble(),
            bboxElement[4].GetDouble(),
            bboxElement[2].GetDouble(),
            bboxElement[5].GetDouble()
        ),
        _ => throw new JsonException( "Only 2 or 3 element coordinates supported" )
    };
}

[m-redding]

@goncalo-oliveira Thank you so much for this detailed description. I will get this addressed.

[m-redding]

@goncalo-oliveira It turns out that the current behavior is expected according to the spec:

5. Bounding Box A GeoJSON object MAY have a member named "bbox" to include information on the coordinate range for its Geometries, Features, or FeatureCollections. The value of the bbox member MUST be an array of length 2*n where n is the number of dimensions represented in the contained geometries, with all axes of the most southwesterly point followed by all axes of the more northeasterly point. The axes order of a bbox follows the axes order of geometries.

So "bbox" doesn't have to be included, but if it is it must be an array.

[goncalo-oliveira]

Hi @m-redding,

Thanks for checking it out. The spec does say that MAY have a member named "bbox" yes, which means if it’s omitted, it implies it can be null because it’s not required for the GeoJSON object to be valid.

Also, if I'm consuming data from a source that serializes JSON objects without ignoring null values, I'm going to get a null bbox value. As such,

Note that since GeoJson objects only have the option of including a bounding box JSON element, the bbox value returned by a GeoJson object might be null.

[m-redding]

@goncalo-oliveira Yeah it doesn't need to be included, if it's omitted then GeoObject.BoundingBox will be null, which is why the property is nullable. But to me, the spec implies that the JSON cannot have a null value for "bbox" if one is included:

The value of the bbox member MUST be an array

And for this piece:

Also, if I'm consuming data from a source that serializes JSON objects without ignoring null values, I'm going to get a null bbox value.

Yes, that is true, and I agree that is not a great experience, but I do think it's an error on the serialization side, rather than the deserialization side. So, this library (I think this is where the quote is from) seems to be serializing incorrectly according to the spec. In Azure.Core, GeoJSON serialization doesn't write a "bbox" property if BoundingBox is null (ref if you're curious).

[goncalo-oliveira]

@m-redding I understand your view. I think the spec is not very specific on this matter; in my understanding, as I said, is that if it's optional, then it does imply it can be null as it's not required to be a valid GeoJSON or JSON object. Nullable means optional.

It would be great to get feedback from the authors, really...

That quote is from that particular library from Mapbox, yes, but it's unrelated to my experience. We consume mapping data from different locations and that means that sometimes we can't control the deserialization process. If this isn't fixed, I'll have to use a different library or write one myself. We can't refuse a source just because they send a null bbox.

[m-redding]

@goncalo-oliveira I started a discussion with some of our HTTP experts to see if it's acceptable to allow null in this case. Out of curiosity, what Azure service are you using? Azure services shouldn't be returning null for values, so if one is that should be fixed regardless of whether we're able to change this in the SDK.

[goncalo-oliveira]

@m-redding the issue is not with any of Azure's services, it's with third-party data providers. But since we already use Azure.Core in the context of, well, Azure services, we thought it was best not to introduce yet another GeoJson library dependency.

Thanks for extending the discussion, btw.