Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Text.Json ObjectConverter does not handle reference metadata correctly on deserialization #67171

Closed
Tracked by #63918
eiriktsarpalis opened this issue Mar 25, 2022 · 1 comment · Fixed by #67183

Comments

@eiriktsarpalis
Copy link
Member

The following test fails with both JsonElement and JsonNode type handling.

        [Theory]
        [InlineData(JsonUnknownTypeHandling.JsonElement)]
        [InlineData(JsonUnknownTypeHandling.JsonNode)]
        public void ObjectConverterShouldHandleReferenceMetadata(JsonUnknownTypeHandling typehandling)
        {
            var options = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.Preserve, UnknownTypeHandling = typehandling };
            string json = @"[{ ""$id"" : ""1"" },{ ""$ref"" : ""1""}]";
            object[] deserialized = JsonSerializer.Deserialize<object[]>(json, options);
            Assert.Same(deserialized[0], deserialized[1]);
        }

We need to update the TryGetReferenceFromJsonElement method in ObjectConverter:

// Edge case where we want to lookup for a reference when parsing into typeof(object)
if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve &&
JsonSerializer.TryGetReferenceFromJsonElement(ref state, element, out object? referenceValue))
{
value = referenceValue;
}

so that it handles both $id and $ref metadata. We also need to expose equivalent functionality for JsonNode, which currently is not implemented.

cc @jozkee @steveharter

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Mar 25, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Mar 25, 2022
@ghost
Copy link

ghost commented Mar 25, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

The following test fails with both JsonElement and JsonNode type handling.

        [Theory]
        [InlineData(JsonUnknownTypeHandling.JsonElement)]
        [InlineData(JsonUnknownTypeHandling.JsonNode)]
        public void ObjectConverterShouldHandleReferenceMetadata(JsonUnknownTypeHandling typehandling)
        {
            var options = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.Preserve, UnknownTypeHandling = typehandling };
            string json = @"[{ ""$id"" : ""1"" },{ ""$ref"" : ""1""}]";
            object[] deserialized = JsonSerializer.Deserialize<object[]>(json, options);
            Assert.Same(deserialized[0], deserialized[1]);
        }

We need to update the TryGetReferenceFromJsonElement method in ObjectConverter:

// Edge case where we want to lookup for a reference when parsing into typeof(object)
if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve &&
JsonSerializer.TryGetReferenceFromJsonElement(ref state, element, out object? referenceValue))
{
value = referenceValue;
}

so that it handles both $id and $ref metadata. We also need to expose equivalent functionality for JsonNode, which currently is not implemented.

cc @jozkee @steveharter

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: 7.0.0

@eiriktsarpalis eiriktsarpalis added bug help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Mar 25, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 26, 2022
@eiriktsarpalis eiriktsarpalis removed the help wanted [up-for-grabs] Good issue for external contributors label Mar 26, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant