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

Roadblocks when writing custom JsonConverter instances #2202

Closed
casperOne opened this issue Jan 26, 2020 · 1 comment
Closed

Roadblocks when writing custom JsonConverter instances #2202

casperOne opened this issue Jan 26, 2020 · 1 comment
Assignees
Milestone

Comments

@casperOne
Copy link

casperOne commented Jan 26, 2020

I'm currently writing a JsonConverter<T> to serialize an Exception (the type isn't really important here).

The idea is that I would serialize a common set of properties on the type, as well as insert some additional information (in this case, I want to insert some additional information about the Type of the Exception as a "$Meta" property).

I find myself running into the following problems around composition and lack of access to framework logic that we have to replicate on our own.

Determining converters for any type

In my converter I'd like to write the Data property of the Exception out. The Data property is of type System.Collections.IDictionary.

What I thought I'd be able to do is call the GetConverter method on the JsonConverterOptions passed to the Write overload, something like this:

var converter = (JsonConverter<System.Collections.IDictionary>) options
    .GetConverter(typeof(System.Collections.IDictionary));

// Write the property.
writer.WriteProperty(nameof(exception.Data));

// Write the object out.
converter.Write(writer, exception.Data, options);

Unfortunately, the call to GetConverter returns null.

If I just serialize any Exception with no converters, the Data property correctly serializes the values (in most cases, it's just an empty map in JSON).

Clearly the JsonSerializer class knows how to handle this situation, but if we want to capitalize on that we can't.

Edit:

I realize now I can call the Write method on the JsonSerializer class for some things, but not everything.

As an example, I can use it to write the Data property on an Exception, but if I pass, say, a Guid I get an exception saying there is a possible circular reference, that the depth is too deep.

It's good that we can use JsonSerializer again to get at the logic above, but it seems that for basic types, it may have to be shored up if that's the recommended route.

Writing property names consistently

Currently, all of the methods that take a property name on the Utf8JsonWriter class do not do manipulation on the property name to align with the JsonNamingPolicy that is passed to the JsonSerializer class.

This isn't to say that this is incorrect, but like the previous point, there is no way be consistent with what the JsonSerializer does.

Yes, we have access to the JsonNamingPolicy that is exposed by the JsonSerializerOptions instance, but in this case, where I'm trying to mirror an existing type, I still want to be cognizant of JsonPropertyNameAtrribute instances attached to properties as I reflect through the properties to see what should and shouldn't be serialized.

Ideally there would be some sort of breaking change to the Write method on JsonConverter<T> instances which would take a wrapper around the Utf8JsonWriter class (and still expose the writer) and encapsulate the logic that the JsonSerializer would apply.

The idea of having a wrapper is to minimize allocations; we could do this in every JsonConverter<T> implementation, but that might be difficult.

It could also be exposed through some sort of context passed through the Write method; this would accomplish two goals:

  • Allow new items to be attached to the context without having to change the signature of the Write methods.
  • Minimize allocations for additional logic attached to the context.

Mind you, this doesn't have to be the approach, extension methods on Utf8JsonWriter could be created which would accept a JsonSerializationOptions instance and write out the correct property name.

However, this will become tedious, as it puts the onus of passing those options in each call, which is ceremony that is unneeded (IMO).

Conclusion

Yes, there are two real issues here that I'd like to see addressed (and I'd be happy for pointers on how to address them), but the larger sentiment is that the logic in JsonSerializer needs to be exposed in order to make composition of Jsonconverter<T> instances easier so that we can have consistent application of this logic in our implementations.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jan 26, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Feb 21, 2020
@layomia layomia self-assigned this Feb 21, 2020
@layomia layomia added this to the 5.0 milestone Feb 21, 2020
@layomia
Copy link
Contributor

layomia commented Mar 19, 2020

Unfortunately, the call to GetConverter returns null.

Following work in #2259 and #32669, GetConverter will never return null (but may throw NotSupportedException if the type is not supported by the serializer or doesn't have a compatible custom converter). Converter composition involving dictionaries and objects is now easier.

I still want to be cognizant of JsonPropertyNameAtrribute instances attached to properties as I reflect through the properties to see what should and shouldn't be serialized

Exposing internal logic/state which the serializer uses to perform per-property (de)serialization is being considered to make converter composition easier. See #1562.

Closing - the first concern is addressed, and there's a tracking issue for the second. In the future, please open one specific issue for each concern/work item so we can better track issues. @casperOne

@layomia layomia closed this as completed Mar 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants