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

boolean value of false serializes incorrectly #153

Closed
thesourcerer opened this issue Nov 19, 2015 · 13 comments
Closed

boolean value of false serializes incorrectly #153

thesourcerer opened this issue Nov 19, 2015 · 13 comments

Comments

@thesourcerer
Copy link

When serializing a value of false, the field is serialized as an empty object if it is not quoted as string.
For example:
yamlfalse

YamlSampleCode.cs.txt
YamlSampleIoutput.txt

@Mitsugaru
Copy link

This is really unfortunate that this hasn't been resolved yet...

@akusei
Copy link

akusei commented Sep 28, 2016

This isn't a bug I don't think. Try specifying EmitDefaults() in SerializerBuilder, that will cause a boolean false to emit correctly.

@dzarda
Copy link

dzarda commented Mar 14, 2017

Confirmed in 4.1.0


This isn't a bug I don't think.

I have to disagree. If it works only in some use cases but doesn't in others, it is clearly a bug.

@dzarda
Copy link

dzarda commented Mar 15, 2017

Failing test here dzarda/YamlDotNet@6c73cb5

@duaneking
Copy link

duaneking commented Aug 14, 2019

Just want to state for the record that every dev I have spoken to about this yesterday/today considers this a defect in the design of this library as it violates the YAML spec.

The YAML spec is very explicit about true and false values; This design doesn't respect the YAML spec, and also schema validation is a very real concern here that this just fully breaks by default due to this design.

The default should be to serialize all values found as correctly as possible. In fact, true and false are EXPLICITLY called out as supported, but they are not by default here.

@aaubry
Copy link
Owner

aaubry commented Aug 14, 2019

As mentioned by @dzarda, this is not a bug. It is a feature that is enabled by default to not emit default values. As indicated, it can be disabled by calling EmitDefaults() on the SerialzerBuilder.

Now I agree that this is an unfortunate default, and I would like to change it to be disabled by default, and also to be able to pick the types for which defaults are to be omitted. I think that a reasonable default would be to omit only reference types (basically not emit null), and emit everything else. Maybe a special case should be made for strings, I am sot sure.

@duaneking
Copy link

May I ask what was the reason for making this the default despite the fact it violates every version of the Yaml Spec I have checked this week?

Or is this just historical code that needs to be removed with fire?

@aaubry
Copy link
Owner

aaubry commented Aug 14, 2019

It doesn't have anything to do with the spec. As far as I know, the spec does not specify how objects should be converted to YAML. If you use the representation model, or the emitter directly, and emit a scalar that equal to false, it will be emitted correctly.
If the default was to always emit defaults, I'm sure that I would get bug reports asking how to get rid of the nulls everywhere and how stupid it is to emit them by default.
The mistake was probably to treat any default(T) the same way, regardless of T being a value type.

@dzarda
Copy link

dzarda commented Aug 14, 2019

At first I was certain that this is a bug. But the more I think about it, @aaubry's logic holds.

But still, I consider the emit everything option to be the better solution.

I now had the same thought about the value types being a sane way to solve this. I'm thinking lowering to primitive types. But such automation seems to bring additional complexity. I'd just emit all the things. Constraints should then be explicit. This would allow specifying granularity for defaultness:

  • NoDefaults()
  • NoDefaults<MyType>()
  • NoDefaultValueTypes()
  • ...

@duaneking
Copy link

duaneking commented Aug 14, 2019

@aaubry With all due respect, I'm not sure how else to say this and be kind, and yet I really want to be kind; A true or false value is explicitly accounted for in the spec. This maps to bools in .Net and so any failure to serialize a false bool value or a zero int value or a enum value that happens to be the default for that type - such as in the case of bool value that is set to false in .net, either by default or explicitly by the user as is the case in my scenario - is an explicit violation of my expectations as a consumer of the library who expects to get spec compliant YAML that I can use to roundtrip the object.

If I set a bool to a value - false or not - and it doesn't serialize, then that is a defect. That is also what is happening now by default. My issue is that bool false, integer 0 of different widths, ENUM Fields that have the default (first) value, and others are always missing by default. Bools are not the only data types affected here, technically.

I get that nulls may be an issue for some people on serialization for incompletely defined types or types with optional values; but that to me means that there is an error in their code, as such things being optional should be explicitly defined in the schema anyway.

And if the issue is nulls generated in RAM on de-serialization of a type without all values set for reference types, then partial type support may be a feature you want to make explicit, as long as the default feature set supports a fully defined type and emits all fields by default.

And if nulls are the issue, why not just check for null? Why does the default value for other types get removed? The default types are 0 for ints, uints, long ints, bytes, chars, and false for bools, or the first (default) value for declared Enum types.for example. I hear you saying you wanted to remove nulls from the output and by mistake removed default enum values, bool:false, int:0, uint:0, long:0, etc:0 and other values that are perfectly legitimate and valid data types, as well. So this is a MUCH bigger deal than you seem to think it is. I consider this a big defect due to the number of fields that are simply lost.

Also: What about compatibility? I can't control how third party code I don't control reads YAML, so if the YAML I'm generating for them does not conform to the spec or by default is missing fields, as is the case here, that's a defect to them and they will complain, or worse, data could be lost forever. How would you design this code if you knew billions of dollars flowed through it? Just so you know, it does. Or does it? I can't confirm or deny, but if we agree its a possibility, does it put this into a better perspective? Because that's what the Yaml spec was defined for, and by parsing Yaml for others and putting this out there, you take on the same responsibilities, duties, and obligations of care from an ethical perspective. Compatibility is huge, because its not just about this lib, its not just about you, its not just about me, its about everybody and everything that could use that generated Yaml Data.. and that is a lot of things! So it has to be full, accurate, and include all fields to be correct! We have an ethical obligation as developers to try to protect people or their data, imho.

The mistake was probably to treat any default(T) the same way, regardless of T being a value type.

YES I agree this is bad and may be the root issue.

@dzarda Emitting everything is the expected functionality of this library from my place as its consumer, given its publicly stated goals. Anything else is a defect. See above

Both of you: I'm happy to answer any questions with the understanding that I'm passionate about this working correctly by default, as I feel that this extra configuration step is harmful and actively wastes peoples time and resources, and puts peoples data at risk of data loss, per the above.

@aaubry
Copy link
Owner

aaubry commented Aug 15, 2019

I disagree with your points. It is common to have contracts that specify default values for parameters, and if that parameter is unspecified, the default value is assumed. See GitVersion for example. There are many parameters that can be configured, but in most cases the defaults are just fine. If I were to design a tool to generate a GitVersion configuration file, I would not want it to specify all the default values, because that makes it more difficult to identify the parts that are actually relevant.

I couldn't care less whether billions of dollars flow through this code. Excluding a few contributions, most of it was developed by me at the expense of my free time. I haven't received a single dollar as compensation for this work, which is expected of course as I chose a permissive license and do not even accept donations. I don't believe that this library is so critical for someone that billions of dollars are at stake, but if that was the case, then maybe the stakeholders should consider hiring me to ensure the maintenance of the library.

(...) I feel that this extra configuration step is harmful and actively wastes peoples time and resources (...)

I totally agree, just by counting the number of issues that were opened due to it. I have created issue #427 to centralize the discussion there and start working on fixing this. Please join the discussion and provide any feedback that might be relevant. Thanks!

@duaneking
Copy link

As I said before, I get the idea of optional values; I should have been more clear about this including default types, however, so really we are on the same page there.

I'm sorry my efforts to show you that the hard work you have put into this are valued is distressing, as that seems to anger you. Text isn't the best medium for communication and if you were local I would buy you a drink/meal and thank you in person, but that sadly was not an option here for me. Just know that the effort is appreciated, and all I can offer at this time is my humble efforts here to help your project grow.

Thank you for agreeing with me about the initial confusion this created; I deeply appreciate your willingness to listen and I hope that that this effort on my part helps.

@EdwardCooke
Copy link
Collaborator

There was another issue opened to track this, I'm closing this issue.

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

No branches or pull requests

7 participants