Skip to content

Enum based json serialization #990

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

Closed
lipchev opened this issue Nov 20, 2021 · 6 comments
Closed

Enum based json serialization #990

lipchev opened this issue Nov 20, 2021 · 6 comments

Comments

@lipchev
Copy link
Collaborator

lipchev commented Nov 20, 2021

Now that #985 is out- lets discuss the enum based schema:

  1. What name would you give it
  2. Which of the following two versions would you prefer:
    a) Similar to the AbbreviatedUnitsConverter: {"Value" : 1.2, "Unit": "Milligram", "Type": "Mass"}
    b) Similar to the UnitsNetIQuantityJsonConverter: {"Value" : 1.2, "Unit": "MassUnit.Milligram"}
@lipchev
Copy link
Collaborator Author

lipchev commented Nov 20, 2021

If you think it is still actually needed, that is.

@angularsen
Copy link
Owner

Sure, I think it would be useful to have.
I am still maturing this idea in my head, but I mostly want to replace UnitsNetIQuantityJsonConverter and its JSON schema with DecimalValue.

But now that you remind me of the enum schema, I just had a thought.

Maybe this should all be refactored to:

  • A single new main JSON converter
  • Options to choose between enum or abbreviation representation, defaults to enum, which is closer to the old serializer schemas.
  • Name it something new like DefaultUnitsNetJsonConverter or UnitsNetJsonConverter2.
  • After major version bump, we can remove the old serializers and name it UnitsNetJsonConverter, which will be our single main converter. Simple.

Schema:

{
	"Value" : 1.2, 
	"UnitAbbreviation": "mg",  // Either this..
    "Unit": "Milligram"        // ..or this, not both.
	"Quantity": "Mass"         // Renamed from Type.
}

What do you think?

@angularsen
Copy link
Owner

angularsen commented Nov 21, 2021

Also, supporting IComparable could be yet another option, instead of having a separate serializer for it. I just think it is simpler from the consumer perspective to have a single converter with options than multiple special purpose converters.

@lipchev
Copy link
Collaborator Author

lipchev commented Nov 21, 2021

Maybe this should all be refactored to:

* A single new main JSON converter

* Options to choose between enum or abbreviation representation, defaults to enum, which is closer to the old serializer schemas.

In my previous implementation I had it setup as an abstract converter and specific implementations for the different schemas (this is probably what I would have proposed for option a)).
We could also have most of the logic refactored in a sort of 'IUnitSerializer' that is constructed according to some UnitSerializationStrategy (Enum) that's provided to the main converter.

Pros:

  1. we don't have to solve the hard problem of naming things
  2. we could have the interface & implementations for the IUnitSerializer shared between all serializers (JsonNet, System.Text etc)

Cons:

  1. We'd have to make sure that both the main converter and the unit serializer work on the same map of QuantityInfos
  2. We introduce coupling between a converter and the (default) implementations of the IUnitSerializer
  3. Additional configuration options (e.g. AllowIntegerValues) might be a little tricky to configure (and implement, but that's a common concern)
  4. The IUnitSerializer would make for some weird looking interface (that would have to reside in some public location, if we'd want to re-use it among multiple nugets)
* Name it something new like `DefaultUnitsNetJsonConverter` or `UnitsNetJsonConverter2`.

* After major version bump, we can remove the old serializers and name it `UnitsNetJsonConverter`, which will be our single main converter. Simple.

I think it would be better if we picked up a stable name for the default converter as early as possible. We wouldn't want to introduce a breaking change for both old and new users, if we can avoid it.

Schema:

{
	"Value" : 1.2, 
	"UnitAbbreviation": "mg",  // Either this..
    "Unit": "Milligram"        // ..or this, not both.
	"Quantity": "Mass"         // Renamed from Type.
}

What do you think?

I prefer Unit over UnitAbbreviation and Type over Quantity as it is closer to the concept of type information while on the other hand, if I was going to have an IQuantity in a data contract- "Quantity" is precisely the name I would give it - making for some weird looking json.

@angularsen
Copy link
Owner

angularsen commented Dec 1, 2021

Do we need a common interface though? I'm not sure I see the benefit, but I see drawbacks with it.

Let's say we arrive at 3 strategies, just an example:

  • Legacy: "Unit": "LengthUnit.Meter"
  • Enum: "Unit": "Meter"
  • Abbreviation: "UnitAbbreviation": "mg"

Let's say we implement implement all 3 in newtonsoft, the last 2 in system.text.json and only enum based for protobuf.

It would then probably throw an NotImplementedException in some cases, which I don't like.

Also, let's imagine with protobuf, there is a fourth strategy that only makes sense for binary formats or maybe even solely for protobuf. I can't think of an example, but in this case the enum would have to be extended with a protobuf-only strategy. It feels weird.

I think it's better that each converter offers its own options based on what it actually can do, and we name similar schemes the same so you can recognize them across converters. So compatible JSON schemas would have similarly named options.

Something like this (ignore the dummy schemas I implemented and ignore the converter names):

void Main()
{
    var person = new { Weight = Mass.FromKilograms(80) };
    
    Newtonsoft.Json.JsonConvert.SerializeObject(person, new JsonSerializerSettings
    {
        Formatting = Newtonsoft.Json.Formatting.Indented,
        Converters = 
        { 
            new NewtonsoftUnitsNetJsonConverter(new NewtonsoftUnitsNetJsonConverterOptions { Schema = NewtonsoftUnitsNetJsonSchema.Enum })  
        }
    }).Dump("newtonsoft");
    
    System.Text.Json.JsonSerializer.Serialize(person, new JsonSerializerOptions 
    {
        WriteIndented = true,
        Converters = 
        { 
            new SystemTextUnitsNetJsonConverter(new SystemTextUnitsNetJsonConverterOptions { Schema = SystemTextUnitsNetJsonSchema.Enum }) 
        }
    })
    .Dump("system.text.json");
}

public enum NewtonsoftUnitsNetJsonSchema { Legacy, Enum, Abbreviation }
public enum SystemTextUnitsNetJsonSchema { Legacy, Enum, Abbreviation }

public class NewtonsoftUnitsNetJsonConverterOptions
{
    public NewtonsoftUnitsNetJsonSchema Schema { get; set; } = NewtonsoftUnitsNetJsonSchema.Enum;
}

public class SystemTextUnitsNetJsonConverterOptions
{
    public SystemTextUnitsNetJsonSchema Schema { get; set;} = SystemTextUnitsNetJsonSchema.Enum;
}

public class NewtonsoftUnitsNetJsonConverter : Newtonsoft.Json.JsonConverter
{
    public NewtonsoftUnitsNetJsonConverter(NewtonsoftUnitsNetJsonConverterOptions options) { }
    
    public override bool CanConvert(Type objectType) { return objectType == typeof(Mass); }

    public override object ReadJson(JsonReader reader, Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
    {
        return Mass.Parse(reader.ReadAsString(), CultureInfo.InvariantCulture);
    }

    public override void WriteJson(JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
    {
        var quantity = (Mass)value;
        writer.WriteValue(quantity.ToString(CultureInfo.InvariantCulture));
    }
}

public class SystemTextUnitsNetJsonConverter : System.Text.Json.Serialization.JsonConverter<Mass>
{
    public SystemTextUnitsNetJsonConverter(SystemTextUnitsNetJsonConverterOptions options) { }
    public override Mass Read(
        ref Utf8JsonReader reader,
        Type typeToConvert,
        JsonSerializerOptions options) => Mass.Parse(reader.GetString(), CultureInfo.InvariantCulture);

    public override void Write(
        Utf8JsonWriter writer,
        Mass quantity,
        JsonSerializerOptions options) => writer.WriteStringValue(quantity.ToString(CultureInfo.InvariantCulture));
}

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 2, 2022
@stale stale bot closed this as completed Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants