New optional flags for property

[andreas-aeschlimann]

As of now (v1.4.1), we can set the third parameter of @JsonProperty to false or true. The current default is false.

I suggest a new approach with multiple enum values:

/**
 * Enum for the property converting mode of a property (de)serialized with JsonConvert.
 *
 * The values should be used as follows:
 * - NEVER_OPTIONAL: the property is never optional
 * - ALWAYS_OPTIONAL: the property is always optional
 * - SERIALIZE_OPTIONAL: the property is optional in serialization only
 * - DESERIALIZE_OPTIONAL: the property is optional in deserialization only
 */
export enum PropertyConvertingMode {
    NEVER_OPTIONAL = 0,
    ALWAYS_OPTIONAL = 1,
    SERIALIZE_OPTIONAL = 2,
    DESERIALIZE_OPTIONAL = 3
};

Thanks to the way I chose the enum values, this approach would be downward compatible with the current version:

1) Assume the json object {} and define this

@JsonProperty("name", String, true)
name: string | null = null;

Deserialization and serialization is 1-1 and there will be never an error. Null is treated as if the property was not there.

2) Now again assume the json object {} and define this

@JsonProperty("name", String, PropertyConvertingMode.DESERIALIZE_OPTIONAL)
name: string | null = null;

When I deserialize the empty json object now, I will still not get an error because the property is flagged optional in the deserialization. JsonConvert does not set the value because the class has a default value already. This is the equal behavior to 1).

However, when I serialize this object, it will create the json object { name: null }. While the local value is null, we do not assume that this means it is optional anymore. This can be a desired result because sometimes you may want to tell the server to reset a specific property to null.

---

TLDR: When this PR is merged, we can have a different treatment of optional (null) values in deserialization as in serialization.

[andreas-aeschlimann]

Also happy to hear input @tobiasschweizer @cmolodo

[cmolodo]

Ah very nice! This is sort of the flip side of the issue that led to my adding the flag, and is a much more elegant solution. Funnily enough, I actually also ran into this issue, but resolved it by simply making properties no longer optional where possible! I like this much better.

[andreas-aeschlimann]

Ah very nice! This is sort of the flip side of the issue that led to my adding the flag, and is a much more elegant solution. Funnily enough, I actually also ran into this issue, but resolved it by simply making properties no longer optional where possible! I like this much better.

The problem is that my server sometimes sends null values. If I set the property non-optional, it will throw an Error with strict null mode.

For example, I have an instance of type Page with the property chapter_id, but the page does not belong to any chapter (no foreign key), hence chapter_id is null.

@JsonProperty("chapter_id", Number, true)
chapter_id: number|null = null

When I edit an existing page, I might want to remove it from a chapter. I need to let the server know that chapter_id is null. This is not possible with an optional property and serialization.

To be honest, the more I think about it, it might have been a mistake to mix the optional flag with null values. After all, null and optional can be two different things. I need a nullable flag too. My property is not optional, but it is nullable.

[jmesa-sistel]

@andreas-aeschlimann As you have said, for me optional and nullable are two different things. Maybe two flags for property make thing easier to understand than one flag with all combinations.

[tobiasschweizer]

I will try you have a look today!

[cmolodo]

Ah, I see - I think I misread it earlier, sorry! I thought the issue was only with wanting to serialize nulls, not with also deserializing nulls. (My back end just doesn't send null properties, with my setup it wouldn't be useful.)

Actually, I'm not sure that I agree with splitting it into 2 flags. I guess I still think of it as being along an axis:

1. True optional (ignore if undefined or null)
2. Optional nullable (ignore if undefined, transmit null)
3. Required nullable (error if undefined, transmit null)
4. Required non-null (error if undefined or null)

This covers the possible cases, right?

If you split it into 2 flags, how do you define the nullable flag for case 1? Technically, the property is still nullable...just that you ignore nulls instead of sending them. Alternatively, I guess you could split it, but then the second "null" flag isn't a boolean, it would need to be an enum of ignore/send/error, since there are 3 different ways to handle nulls. I think it would still be simpler to have one combined enum.

In some ways, I think it would make more sense to have one enum that covers these 4 cases, but then have 2 settings, one for serialization and one for deserialization. (Of course that's because I need separate settings for serialization and deserialization! But I recognize that my use-case is a little peculiar. I can create a separate pull request to do that later.)

[tobiasschweizer]

As you have said, for me optional and nullable are two different things.

I agree. I wouldn't mix undefined and null since it is not the same, see https://stackoverflow.com/questions/5076944/what-is-the-difference-between-null-and-undefined-in-javascript

[cmolodo]

@tobiasschweizer They're definitely not the same! And the code already handles them differently, even before this pull request. I guess my thinking is that these flags are less about the explicit undefined and null values themselves, and more how you handle such values, if that makes any sense.

As I said above, you can split undefined and null handling, but then if you want null handling to be independent of the "optional" flag you need 3 different choices for it - ignore, include, or error. Or else you have to use the combination of both "optional" and "nullable" flags when deciding how to handle nulls. At that point having a separate flag doesn't seem that helpful, since you need them both anyway.

Not to mention, if you eventually want to allow different behavior for serialization and deserialization...then we have 4 flags for each property. Yikes!

I still think it would be simpler to have a combined enum that defines handling of both undefined and null, something similar to what I outlined above.

[tobiasschweizer]

@cmolodo In our API format, we avoid using null. I think you can have endless discussions whether to use it or not. I understand that you try to handle it in a way so that it makes sense for you or your use case at hand. However, I think if you want to offer the flexibility you mentioned for serialising and deserialising, the options should not obfuscate a really simple use case or lead to wrong usage of the lib because the configuration is too complicated.

And I would rather opt for a non backward compatible way than trying to save what has been there before, if already inconsistent. I haven't looked at this PR in detail yet, so maybe I should spend some time next week to get a better understanding (so before complaining, I should do my homework properly ;-)).

[cmolodo]

@tobiasschweizer I agree that everyone will have different use cases for null vs undefined, etc! And I definitely don't think everyone should have to use null - use whatever makes sense for your projects/APIs, (though in a consistent way). But I would argue that there is a need to support usage of both null and undefined for this library.

In any case, I got a little sidetracked with the null vs undefined discussion, sorry @andreas-aeschlimann. We should be discussing different behavior for serialization vs deserialization, as the pull request actually specifies! :)

After all the above I now think this would be better handled by having 2 separate parameters, one for serialization and one for deserialization. I personally would find it easier to understand and configure them separately, because you could use the same option values for each parameter. Also, for the simpler use case where you don't need to separate the behavior, you can just provide one parameter and fall back on that for both modes.

[andreas-aeschlimann]

After all the above I now think this would be better handled by having 2 separate parameters, one for serialization and one for deserialization. I personally would find it easier to understand and configure them separately, because you could use the same option values for each parameter. Also, for the simpler use case where you don't need to separate the behavior, you can just provide one parameter and fall back on that for both modes.

I agree, but falling back would be more difficult. Do you have an idea how this could be properly done?

I still think the two problems are somewhat connected:

@tobiasschweizer I agree that everyone will have different use cases for null vs undefined, etc! And I definitely don't think everyone should have to use null - use whatever makes sense for your projects/APIs, (though in a consistent way). But I would argue that there is a need to support usage of both null and undefined for this library.

The main problem is that in TS, you could have undefined and null. In JSON, you could have null and inexistent. Now let's look what happens when having these values currently:

deserialize          null                 inexistent 
--------------------------------------------------------------
isOptional=true      default TS value     default TS value  
isOptional=false     null/error           null/error

serialize            null                 undefined 
--------------------------------------------------------------
isOptional=true      inexistent           inexistent  
isOptional=false     null                 null

I have to admit, the longer I think about it, I am not happy that we treat null the same way as inexistent/undefined. This should be user configurable.

[cmolodo]

I agree, but falling back would be more difficult. Do you have an idea how this could be properly done?

I'd suggest changing MappingOptions to make the current isOptional serialization-only and adding a deserialization property to MappingOptions. Users could still define minimally serialization (or if not defined it would fall back to the global default). Then if deserialization isn't defined you just copy the value from serialization-only. This of course only works if they have the same type, whether boolean or an enum, but I'd think we'd want that anyway, to reduce confusion for the user.

Then you'd access the mode-specific mapping option in JsonConvert.
Hopefully that made sense?

[cmolodo]

Now let's look what happens when having these values currently:

Actually, I think your table of the current behavior incorrectly suggests that null and undefined/missing values are always treated the same whether isOptional is true or false. However, that's only true if certain global flags are set. If the value checking mode is set to allow nulls and the ignoreRequiredCheck flag is set to false, for example, the actual behavior differs:

deserialize          null                 inexistent 
--------------------------------------------------------------
isOptional=false     null                 error

serialize            null                 undefined 
--------------------------------------------------------------
isOptional=false     null                 error

My point is that it's already treating null and undefined/missing object properties differently, even under the default conditions.

I'll also note that the valueCheckingMode and ignoreRequredCheck global properties confuse the issue a bit, since they both affect how nulls are handled, but only for properties with isOptional = false. Neither has any effect on optional properties.

To really deal with undefined vs null handling, I'd suggest a truly breaking change and get rid of the value checking JsonConvert global option and the JsonProperty isOptional flag. Possibly remove ignoreRequiredCheck as well (though I think I'd actually still want the ability to circumvent existing mappings as needed, to deal with serializing partial/patch objects vs full objects).

Instead add 2 new decorators to specify handling for different types of values. One would define undefined/missing property value handling, and the other would define null property value handling (@JsonUndefinedHandling() and @JsonNullHandling()?). Each one would take at least one argument to define handling, or two if the user wanted different handling for serialization/deserialization.

Additionally, there could be new global default undefined/null handling properties for the JsonConvert option. Then if the user didn't put the new decorators on a property, it would fall back to the JsonConvert global values. Not sure if we'd need separate null handling properties for primitives vs objects, to match the current valueCheckingMode granularity?

There could be an enum for the possible values for both decorators and the global JsonConvert properties:

• Ignore (meaning don't include if serializing and use default value if deserializing)
• Transmit (meaning include if serializing and replace the default value if deserializing)
• Error

I think that covers the possible handling options? Though using an enum would probably make it a little easier to extend later to add some new handling option.

Thoughts? Would it be worth pulling this into a separate issue for discussion?

[andreas-aeschlimann]

I know that some time has passed, I came to the following conclusion:

1) The current handling of null/undefined is not very concise. The only way to properly fix this issue is by introducing true breaking changes, as @cmolodo suggested. This should be done in a version 2.0.0.

2) This PR here makes sense in terms how version 1.x.x handle null/undefined values. I will go ahead and merge this PR for a version 1.5.0. The advantage is that we have no breaking change and add some more flexibility.

[andreas-aeschlimann]

I need to give you an update on this. I added some tests and it looks like the implementation is really bugged in some special cases. This needs further investigation and probably another solution, even for the minor update 1.5.0.

[andreas-aeschlimann]

Okay guys @cmolodo @tobiasschweizer , I think I finally found a good solution. I would appreciate some feedback!

Undefined/null issue:

From now on, json2typescript sees a missing JSON property as undefined. A TypeScript value undefined is the same as a missing JSON property.

I added a boolean property mapUndefinedToNull that automatically converts every undefined value to null. If you set this property to true, json2typescript will basically see null as the same as undefined for all conversions.

Optional issue:

I deprecated the setting ignoreRequiredCheck introduced by @cmolodo and instead added the setting propertyConvertingMode. This is a global setting for what happens with all properties in your project. The very same setting can be applied to each individual property, too.

The options are:

• PropertyConvertingMode.MAP_NULLABLE: the mapper is applied, type is checked
• PropertyConvertingMode.IGNORE_NULLABLE: the mapper is not applied if the property is missing, undefined or null; the property is not added to the result
• PropertyConvertingMode.PASS_NULLABLE: the mapper is not applied if the property is missing, undefined or null; the property is added with its value to the result

The first option is default and exactly behaves like isOptional = false in the past. The second option behaves exactly like isOptional = true in the past. Thus, these changes are nicely downward compatible. The third option is useful because it does not use the mapper, but just passes the value that is given before the mapping.

Summary

All my changes should be downward compatible, but nevertheless I will first release a RC version 1.5.0 so we can test it in projects first. If we come accross a breaking change that I was not aware of, I will bump the version to 2.0.0 instead.

[andreas-aeschlimann]

You can try json2typescript 1.5.0-rc.0 if you would like to test it. Feel free to report back your input here or in a new issue.

Happy holidays!