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

Change the default serializer behaviour to emit default values #427

Closed
aaubry opened this issue Aug 15, 2019 · 9 comments · Fixed by #435
Closed

Change the default serializer behaviour to emit default values #427

aaubry opened this issue Aug 15, 2019 · 9 comments · Fixed by #435
Assignees

Comments

@aaubry
Copy link
Owner

aaubry commented Aug 15, 2019

As evidenced by #318, #310, #298, #268, #251, #258, #153, the current behaviour of omitting default values during serialization is misleading and a source of confusion.

I'm opening this issue so that we can discuss the best way to handle this. I suggest the following changes, but feel free to propose alternatives:

  • (1) Change the default behaviour to emit all default values, including null references. This will be a breaking change but it is necessary to move forward.
  • (2) Add an option to not emit any default value. This ensures a simple upgrade path for anyone who depends on the current behaviour.

Additionally, we could offer some or all of the following options:

  • (3) Add an option to not emit any null value. This was the original intention and I still believe that it is a valid use case.
  • (4) Add a property to YamlMemberAttribute to override the emission of defaults on a specific member.
  • (5) Add an option to not emit default values for a specific type. Would this be useful ?

I think that performing changes (1) and (2) is necessary to properly address the issue. I am inclined on also including (3) and (4) because they offer more flexibility. I don't see any use case for (5), but if someone can come up with a relevant one, let's discuss it.

For more complex behaviour, it is already possible to replace DefaultExclusiveObjectGraphVisitor by a custom implementation that can decide whether to emit any value based on arbitrary rules.

@duaneking
Copy link

duaneking commented Aug 15, 2019

My thinking is if you want to make it a pure breaking change, take the opportunity to design forward and change API's as needed so that old code wont run, or at least can be easily seen as "the old version", etc.

Many of the features you speak of are really multiple smaller separate features in my mind and they are split by serializer verses deserializer:

  1. Change the default behavior to emit or consume all values (default or not,) for both reference types and value types, and expect all values to be instanced and raise a serialization exception if a null reference is found, a field name is missing, etc, and the feature flags used do not explicitly request support for it. Call this "strict mode" and make it the Default, but give options to override explicitly so its the dev's choice/responsibility/option.
  2. Add an option to use a "legacy mode" that is equal to the current defaults. This ensures a simple upgrade path for anyone who depends on the current behavior via a simple feature flag, as you noted. This feature would be a combination of features 3 and 4 below in my mind, or at least its feature flag could simply activate both as one binary OR value and get this as the result in the best case. Call this SkipEmittingNullsOrValueTypeDefaultValues or something and set it to (SkipEmittingValueTypeDefaultValues | SkipEmittingNulls ) in the enum.
  3. Add an option to not emit (to skip) serializing any reference type or nullable valuetype fields that have explicit set null values in the serializer as a sort of "if value for this field is null, don't include it in the output, but still include all other types like bool false, int zero, enum default, etc,) so that objects with null child object references can still be supported now that an exception is thrown by default when null or missing fields are encountered by default per item 1 above. Yes, I agree this is a valid feature. Call it SkipEmittingNulls or something.
  4. Add an option to not emit (to skip) any default value for reference or value types, except nulls, otherwise just like the default is right now. Call this SkipEmittingValueTypeDefaultValues or something so people know they still get zero, false,, enum defaults, etc, just not nulls.
  5. Add an option to support and allow explicit nulls as input to the deserializer without error so that they can still be supported now that an exception is thrown when they are encountered by default. You can choose to either silently consume the null value when its found in the object graph being deserialized for all reference types, or you can choose the types this is allowed for so that unexpected nulls can be caught based on the type if you do not want to support fields by name on objects and provide additional debugging or conditions.
  6. Add a property to YamlMemberAttribute or a callback system to allow override tof the skip/emit values on a specific member according toi its type, value, or other data. Yes, this would also improve flexibility and error checking for custom serialization scenarios, and allow that field to either be skipped based on a custom condition or to support custom values generated or calculated at the time of serialization.
  7. Add an option to not emit default values for a specific type. This would effectively be the "skip" option of number 6 above with a condition of being a specific type and having its value be equal to its valuetype's default value, and I see it as valid as a feature. This would honestly power feature number 3 and 4.
  8. Add an option to not emit fields for a specific type when its value is null or has some other known and tested for value. This would effectively be the "skip" option of number 6 above with a condition of being a nullable type and having its value actually be null, and I see it as valid as a feature and easily enabled by the work to support number 6 above.

Honestly the flexibility that #6 would give the library would enable all that and more, imho. This also allows callbacks to be used to control the serialized output/input for very advanced scenarios but still keep you safe from people complaining about defaults they cant change.

Just a few ideas before I go to bed. :)

@aaubry
Copy link
Owner Author

aaubry commented Aug 15, 2019

Thanks for your input, @duaneking.

  1. Change the default behavior to emit or consume all values (default or not,) for both reference types and value types, and expect all values to be instanced and raise a serialization exception if a null reference is found, a field name is missing, etc, and the feature flags used do not explicitly request support for it. Call this "strict mode" and make it the Default, but give options to override explicitly so its the dev's choice/responsibility/option.

I don't think that the serializer should throw exceptions when a null value is encountered. First because the YAML specification defines null values and thus they should be allowed, and second because it is not the responsibility of a serialization library to perform this kind of validation. That responsibility belongs to a properly designed domain model. Other well-known libraries such as Json.NET seem to agree with this.

  1. Add an option to use a "legacy mode" that is equal to the current defaults. This ensures a simple upgrade path for anyone who depends on the current behavior via a simple feature flag, as you noted. This feature would be a combination of features 3 and 4 below in my mind, or at least its feature flag could simply activate both as one binary OR value and get this as the result in the best case. Call this SkipEmittingNullsOrValueTypeDefaultValues or something and set it to (SkipEmittingValueTypeDefaultValues | SkipEmittingNulls ) in the enum.

Agreed, that's equivalent to my suggestion (2).

  1. Add an option to not emit (to skip) serializing any reference type or nullable valuetype fields that have explicit set null values in the serializer as a sort of "if value for this field is null, don't include it in the output, but still include all other types like bool false, int zero, enum default, etc,) so that objects with null child object references can still be supported now that an exception is thrown by default when null or missing fields are encountered by default per item 1 above. Yes, I agree this is a valid feature. Call it SkipEmittingNulls or something.

Agreed, that's equivalent to my suggestion (3).

  1. Add an option to not emit (to skip) any default value for reference or value types, except nulls, otherwise just like the default is right now. Call this SkipEmittingValueTypeDefaultValues or something so people know they still get zero, false,, enum defaults, etc, just not nulls.

We can provide this option for completeness, although I am not convinced that is has that much value.

  1. Add an option to support and allow explicit nulls as input to the deserializer without error so that they can still be supported now that an exception is thrown when they are encountered by default. You can choose to either silently consume the null value when its found in the object graph being deserialized for all reference types, or you can choose the types this is allowed for so that unexpected nulls can be caught based on the type if you do not want to support fields by name on objects and provide additional debugging or conditions.

As for you first point, I don't think that the deserializer should disallow nulls. The domain model should be responsible to validation and ensuring that it doesn't accept invalid values, which is more general than simply disallowing null values. For many use cases, it is simple enough to implement validation based on arbitrary rules.

  1. Add a property to YamlMemberAttribute or a callback system to allow override tof the skip/emit values on a specific member according toi its type, value, or other data. Yes, this would also improve flexibility and error checking for custom serialization scenarios, and allow that field to either be skipped based on a custom condition or to support custom values generated or calculated at the time of serialization.

I agree with extending YamlMemberAttribute. There's no need for a callback system. It is already possible to achieve what you describe by replacing DefaultExclusiveObjectGraphVisitor with a custom implementation (using the WithEmissionPhaseObjectGraphVisitor method).

  1. Add an option to not emit default values for a specific type. This would effectively be the "skip" option of number 6 above with a condition of being a specific type and having its value be equal to its valuetype's default value, and I see it as valid as a feature. This would honestly power feature number 3 and 4.

Sounds good.

  1. Add an option to not emit fields for a specific type when its value is null or has some other known and tested for value. This would effectively be the "skip" option of number 6 above with a condition of being a nullable type and having its value actually be null, and I see it as valid as a feature and easily enabled by the work to support number 6 above.

Sounds good.

I think the only point where we disagree is whether to disallow nulls by default. I even think that this is outside of the scope of this issue, but I'm willing to consider counter-arguments of course.

@duaneking
Copy link

What does the rest of the community say?

@aaubry
Copy link
Owner Author

aaubry commented Aug 19, 2019

I will be away until September. Let's see if anyone has anything to comment, and I will pick this up when I return.

@aaubry
Copy link
Owner Author

aaubry commented Sep 28, 2019

So I have submitted a PR that addresses this issue. It changes the default behaviour to alway emit all properties, regardless of their value. It is then possible to request to omit properties whose value is null, or to omit properties that are set to their default value, be it null, default(T) or another value indicated by the DefaultValue attribute.

There's currently no out-of-the-box way to configure the behaviour for a specific type because it is trivial to extend the serializer to add more complex rules.

As I discussed this does nothing to prevent null values while parsing or emitting documents. I don't think that a serialization library should make this kind of decisions.

Comments are welcome.

@aaubry
Copy link
Owner Author

aaubry commented Oct 19, 2019

YamlDotNet 8.0.0, which includes this change, has just been released.

@ArrowRaider
Copy link

I don't want null properties to be included. How do I get rid of them?

@duaneking
Copy link

I don't want null properties to be included. How do I get rid of them?

That is part of your code, not this library. This bug as created because the old way was a bad design and created a leaky abstraction. You need to have a better handle on your interfaces.

@ArrowRaider
Copy link

ArrowRaider commented Nov 12, 2020

My use case is to accomplish polymorphism by having a composite type hold all of the possible derived classes, with only one property being set, which corresponds to the appropriate class. I have since found the setting though to change it to prune null values: .ConfigureDefaultValuesHandling(DefaultValuesHandling.OmitNull)

My use case matches how Ansible YAML files represent polymorphism, though I don't know what their parsing procedure is. I bring in the raw DTO from the deserialized YAML and then pass it through Automapper to make the concrete model.

Example of such a YAML in case that wasn't clear:

Contents:
- SomethingA:
    Foo: Test
    Bar: 1
- SomethingB:
    Foo: Test2
    Baz: abc

With the YamlDotNet default, it was serializing into this:

Contents:
- SomethingA:
    Foo: Test
    Bar: 1
  SomethingB:
- SomethingA:
  SomethingB:
    Foo: Test2
    Baz: abc

Anyways, I got it working, so I am happy now. Just wanted to point out the reason I wanted this.

PS: I should probably do this with a IYamlTypeConverter instead, but I need to better learn how use that first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants