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

Support using any dictionary as JsonExtensionData #31645

Open
kostrse opened this issue Feb 3, 2020 · 14 comments
Open

Support using any dictionary as JsonExtensionData #31645

kostrse opened this issue Feb 3, 2020 · 14 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Milestone

Comments

@kostrse
Copy link

kostrse commented Feb 3, 2020

IMHO, IReadOnlyDictionary should work identical to IDictionary when using with JsonExtensionData attribute.

[JsonExtensionData]
public IReadOnlyDictionary<string, JsonElement> Payload { get; set; }

Currently it fails with error:

System.InvalidOperationException: The data extension property 'Payload' does not match the required signature of IDictionary<string, JsonElement> or IDictionary<string, object>.

This is analogous to collection properties which can be defined as List<T>, IList<T> or IReadOnlyList<T> etc.

@kostrse kostrse changed the title Support IReadOnlyDictionty with JsonExtensionData Support IReadOnlyDictionary with JsonExtensionData Feb 3, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 3, 2020
@ahsonkhan ahsonkhan added this to the 5.0 milestone Feb 21, 2020
@ahsonkhan ahsonkhan added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Feb 21, 2020
@layomia
Copy link
Contributor

layomia commented Feb 21, 2020

Triage: we should generalize this to support all dictionary types that the serializer supports as extension data. Supported types can be found here dotnet/docs#15807 (comment).

Changing the title to reflect this.

@layomia layomia changed the title Support IReadOnlyDictionary with JsonExtensionData Support using any dictionary as JsonExtensionData Feb 21, 2020
@layomia
Copy link
Contributor

layomia commented Feb 21, 2020

Adding request for ImmutableDictionary from @mwikstrom in #32408:

Hi,

I want to use an immutable dictionary to store json extension data.

I tried to see if this is supported by writing the following test in an mstest project targeting netcoreapp3.1:

[TestClass]
public sealed class JsonExtensionDataTest
{
    class HasImmutableExtensionData
    {
        [JsonExtensionData]
        public ImmutableDictionary<string, JsonElement> ExtensionData { get; set; }
    }

    [TestMethod]
    public void Can_deserialize_immutable_extension_data()
    {
        const string json = "{\"foo\":123}";
        var obj = JsonSerializer.Deserialize<HasImmutableExtensionData>(json);
        Assert.IsNotNull(obj.ExtensionData);
        Assert.IsTrue(obj.ExtensionData.ContainsKey("foo"));
    }
}

That test fails with a NullReferenceException which surprised me:

 Can_deserialize_immutable_extension_data
   Source: JsonExtensionDataTest.cs line 42
   Duration: 39 ms

  Message: 
    Test method JsonExtensionDataTest.JsonExtensionDataTest.Can_deserialize_immutable_extension_data threw exception: 
    System.NullReferenceException: Object reference not set to an instance of an object.
  Stack Trace: 
    JsonSerializer.CreateDataExtensionProperty(JsonPropertyInfo jsonPropertyInfo, ReadStack& state)
    JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
    JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
    JsonSerializer.Deserialize(String json, Type returnType, JsonSerializerOptions options)
    JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
    JsonExtensionDataTest.Can_deserialize_immutable_extension_data() line 45

I consider the experienced behavior a bug. Another exception should have been thrown.

I would also like to request future support for using ImmutableDictionary<TKey, TValue> as backer for [JsonExtensionData].

@layomia layomia added the help wanted [up-for-grabs] Good issue for external contributors label Mar 26, 2020
@Romanx
Copy link

Romanx commented Apr 6, 2020

Hey there,

I'd love to pick this up and take a look at getting these types serializing. Could the issue be assigned to me?

@layomia
Copy link
Contributor

layomia commented Apr 7, 2020

@Romanx, certainly. Please let me know if you run into any issue or have any questions.

@layomia layomia removed the help wanted [up-for-grabs] Good issue for external contributors label Apr 7, 2020
@Romanx
Copy link

Romanx commented Apr 7, 2020

@layomia I can't seem to get the project to build.

I've forked and cloned the repo, built everything following this. when I opened vs using build -vs System.Text.Json but i'm getting errors in the ref project about Project System.Memory is not compatible with netcoreapp5.0.

If i unload that project then I get get everything to build but i can't seem to get the tests to run and looking in the test output from VS i'm seeing
None of the specified source(s) 'C:\Projects\dotnet-runtime\artifacts\bin\System.Text.Json.Tests\Debug\System.Text.Json.Tests.dll' is valid. Fix the above errors/warnings and then try again.

Following that path i find that my output is being put into a folder netcoreapp5.0-Debug so prefixed with the TFM.

I may be being dense and have missed something obvious but any help would be appreciated for a first timer!

@layomia
Copy link
Contributor

layomia commented Apr 8, 2020

@Romanx can you share the output if running dotnet --info? Also what version of Visual Studio are you running?

Did you build the repo before running build -vs System.Text.Json as described in this doc? Specifically -

:: From root:
git clean -xdf
git pull upstream master & git push origin master
:: Build Debug libraries on top of Release runtime:
build -subset clr+libs -runtimeConfiguration Release

@Romanx
Copy link

Romanx commented Apr 9, 2020

Hey,

The results from dotnet --info is:

.NET Core SDK (reflecting any global.json):
 Version:   5.0.100-preview.4.20202.8
 Commit:    074b436c3d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.100-preview.4.20202.8\

Host (useful for support):
  Version: 5.0.0-preview.3.20169.1
  Commit:  bc28cbef85.NET Core SDK (reflecting any global.json):
 Version:   5.0.100-preview.4.20202.8
 Commit:    074b436c3d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.100-preview.4.20202.8\

Host (useful for support):
  Version: 5.0.0-preview.3.20169.1
  Commit:  bc28cbef85

I was running VS 16.5.3 but then remembered the post for preview 2 saying you have to use the preview VS so installed that so tried with 16.6 Preview 2.1 although had to open it manually since ./build -vs System.Text.Json didn't open with preview

After building in 16.6 the ref project errors went away but the test error looking for the dll in the wrong directory persisted.

@safern
Copy link
Member

safern commented Apr 10, 2020

'C:\Projects\dotnet-runtime\artifacts\bin\System.Text.Json.Tests\Debug\System.Text.Json.Tests.dll' is valid.

Anipik it seems like the path is missing the TFM, was this fixed with yesterday's PR as well?

I think if you don't open with build -vs ... you won't be able to run the tests because we're setting some environment variables needed for VSTest to run against the lively built shared framework. If it didn't open with preview, you might be missing the Preview devenv in the path, or you're opening a developer command prompt that is not preview, could you make sure that is not the case?

@layomia
Copy link
Contributor

layomia commented Apr 10, 2020

Anipik it seems like the path is missing the TFM, was this fixed with yesterday's PR as well?

cc @Anipik

@Romanx
Copy link

Romanx commented Apr 11, 2020

I managed to get the tests running on my machine after marking sure my default program for .sln files was the preview version of devenv and after pulling from master which seems to have fixed the TFM problem.

I started taking a look using the ImmutableDictionary example from above to help guide me through the codebase. The reason it stops is that jsonPropertyInfo.RuntimeClassInfo.CreateObject value for types such as ImmutableDictionary is null since there is no valid constructor.

The reason that these types work in the context of normal json collections for is that a specific JsonConverter is used and the read items are stored in an intermediate dictionary and then converted into the correct output type when the collection has been fully read.

That method would not work here without work since the items we need to add are not in order since they're the items without a matched property.

It would be good to be able to reuse the JsonConverter method since the conversions are already there and any new types would work however the JsonConverter uses the ReadStack for state which I think would cause the reading to break.

I'm not sure the best course of action from here, creating an interim dictionary and then converting to the output type at the end seems to be right idea however where to start with that is the next question.

Adding a dictionary to the JsonPropertyInfo for the ExtensionData and converting before returning the final result by exposing the collection conversion from the JsonConverter seems like it would work but adding a property to JsonPropertyInfo for this one case feels wrong.

Hopefully that ramble makes some sense 😄

@layomia
Copy link
Contributor

layomia commented Apr 13, 2020

Hopefully that ramble makes some sense 😄

@Romanx, yes it does 😄

Adding a dictionary to the JsonPropertyInfo for the ExtensionData

JsonPropertyInfo and JsonClassInfo are used to cache type metadata & to set/get properties, and can be used by multiple threads at the same time. They thus can't store any (de)serialization specific state as this won't be thread-safe.

ReadStackFrame would be a good place to add a new dictionary property to temporarily hold extension data, as each instance is specific to the deserialization of one object.

converting before returning the final result by exposing the collection conversion from the JsonConverter

After deserialization, logic similar to what you've seen in the dictionary converters can be used to instantiate and populate the desired dictionary type. I expect this conversion to take place in the converter for the object, not one of the dictionary converters.

I managed to get the tests running on my machine

Glad you were able to get this to work.

@Romanx
Copy link

Romanx commented Apr 18, 2020

I'm sure you'll get a notification but i've just pushed up the branch and opened the PR for this. I'm aware there's likely to be changes so I'll wait for them. Thanks for being so kind and helpful with this 👍

I'm not wild about the internal Convert method on TypeConverter to access the GetCreatorDelegate so anything thoughts in that area would be welcome

@layomia layomia modified the milestones: 5.0.0, Future Jun 17, 2020
@eiriktsarpalis eiriktsarpalis added the wishlist Issue we would like to prioritize, but we can't commit we will get to it yet label Oct 19, 2021
@krwq
Copy link
Member

krwq commented Jan 27, 2023

@Romanx I assume you don't work on that anymore. This will be much easier to fix once populating objects is supported. There is a chance I will address this as part of that work but no promises (even if I don't it's going to be much easier to do).

@Romanx
Copy link

Romanx commented Jan 27, 2023

No problem removing me from this @krwq. After the PR got closed I didn't go back to taking a look at this but good to know there may be hope for this yet! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants