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

Nullable types on a per-property basis #58

Closed
dleavitt opened this issue Mar 8, 2018 · 19 comments
Closed

Nullable types on a per-property basis #58

dleavitt opened this issue Mar 8, 2018 · 19 comments
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@dleavitt
Copy link

dleavitt commented Mar 8, 2018

Is there a way to specify that a particular field is nullable?

It seems there's a way to allow all fields to be nullable, and a way to allow individual fields to be undefined, but no way for individual fields to be made nullable. Would it make sense to avoid throwing errors when properties with isOptional receive null values? Perhaps behind a flag set at the converter level if needed?

@andreas-aeschlimann andreas-aeschlimann self-assigned this Mar 9, 2018
@andreas-aeschlimann andreas-aeschlimann added the question Further information is requested label Mar 9, 2018
@andreas-aeschlimann
Copy link
Member

Idea:

export class Message {
    @JsonConvert("text", String, true) // makes it optional
    text: string = null;
}

Then:

deserialize({}, Message);
would return the object Message { text: null }

deserialize({ text: "Hello" }, Message);
would return the object Message { text: "Hello" }

@andreas-aeschlimann
Copy link
Member

Would it make sense to avoid throwing errors when properties with isOptional receive null values?

This could be this discussed, though.

@andreas-aeschlimann andreas-aeschlimann added the enhancement New feature or request label Mar 9, 2018
@dleavitt
Copy link
Author

Sorry, not sure I described the issue (as I see it) well. Here's what I see as the issue:

const jsonConvert = new JsonConvert(OperationMode.ENABLE)

@JsonObject
class Message {
  @JsonProperty("text", String, true)
  text?: string | null = undefined
  // or text?: string = undefined
  // or text: string | null = null
}

// works
jsonConvert.deserialize({ text: undefined }, Message)
jsonConvert.deserialize({}, Message)

// doesn't work
jsonConvert.deserialize({ text: null }, Message)

A lot of APIs return null for various optional fields - it would be great if these could be treated the same way that undefined is treated now.

Happy to take a stab at a PR if you agree!

@andreas-aeschlimann
Copy link
Member

It's true. Can you not manipulate the API so that it omits null? Many guides even suggest you to omit null because it is bad practice passing non-existent data.

I recognize the need of this and I will have a look at it soon. However, it is not clear whether a null value in JSON should be ignored or taken, in case if you set the flag to "optional=true" and the configuration is not set to allow null on your property.

@dleavitt
Copy link
Author

Yeah, agree, but a lot of APIs (Twitter, Github, etc.) still use null pretty extensively.

Ideas on ways to implement:

  1. In the places you're currently checking for undefined, instead check for undefined OR null.
  2. Rather than allowing null values, set them to undefined (or unset the property.) This could make sense for deserialization (typescript generally likes undefined better than null.) Might make less sense for serialization where the upstream API may be expecting null rather than a missing field / undefined.

Possible approaches to configuration:

  • a. Always allow optional values to be null, irrespective of ValueCheckingMode.
  • b. Retain current behavior by default, but add an additional ValueCheckingMode that only allows optional fields to be null (ALLOW_OPTIONAL_NULL or something.)
  • c. Add a new flag to the converter or decorator that controls the above.

Approach 1+a certainly seems best and simplest to me (I think it's like two lines of code?), but is debatably a breaking change.

@andreas-aeschlimann
Copy link
Member

andreas-aeschlimann commented Mar 16, 2018

I guess the way to go is this: Make an additional flag ALLOW_JSON_OPTIONAL_NULL - independent of ValueCheckingMode - then work it as follows:

Deserialize:
If a property is set to optional and the json value is null, then first try to assign the null value. If that is not allowed (ValueCheckingMode.DISALLOW_NULL for example), then ignore instead of throwing an exception. If it is indeed allowed for the given property, then assign null anyway.

Serialize:
If a property is set to optional and is null, only include it in the json if null was supposed to be allowed in deserialization as well.

@cristianmiranda
Copy link

Hi guys. Any updates on this?

@andreas-aeschlimann
Copy link
Member

I will take care of all open issues within the next few weeks. I was dealing with another project for the last few months, so it was on hold.

@m-architek
Copy link

I must say the null handling here is not intuitive for me too.

E.g.

@JsonObject("Foo")
export class Foo {

    @JsonProperty("notOptional", String, false)
    notOptional: string = undefined as any;

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

I was expected that in case of notOptional = null deserialization will rise an error (also when ValueCheckingMode.ALLOW_NULL is selected). But it won't, what is bad for strict null checking.

@andreas-aeschlimann
Copy link
Member

I don't follow. Why do you want to allow null but throw an exception if a null value appears? After all, a property (for example a foreign key) could be set to null on purpose.

@m-architek
Copy link

Because optional means for me null or undefined. So I want to allow null, but only on optional fields.
But I understand your point, that no value is not necessarily equal to null value (although I've never made API contract this way).

I believe this ALLOW_JSON_OPTIONAL_NULL flag you mention above could do the trick for me.
Or maybe we need some additional type check, somthing like:

@JsonObject("Foo")
export class Foo {

    @JsonProperty("notOptional")
    @JsonType(String)
    notOptional: string = undefined as any;

    @JsonProperty("optional")
    @JsonType(String, Null)
    optional: string | null = null;
}

or

@JsonObject("Foo")
export class Foo {

    @JsonProperty("notOptional", notNull(String), false)
    notOptional: string = undefined as any;

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

or whatever. ;)

@ankitbko
Copy link

ankitbko commented Oct 8, 2018

Any update on this?

@brandon-sigao
Copy link

I'd also be interested in an update. We've been getting around it by using "Any" in the JsonProperty decorator, but its seems like bad practice to do that. Some properties really should throw an error if they're ever null, and some properties absolutely need to be nullable.

@andreas-aeschlimann
Copy link
Member

andreas-aeschlimann commented Feb 21, 2019

I am planning a new version (2.x) with some deeper changes to make configuration easier, but before that I suggest the following:

@JsonProperty("middleName", String, true)
middleName: string = "";

middlename is an optional json param and automatically ignored if the JSON value or local value is null. This means that jsonConvert.deserializeObject(json, Person).middleName will print "" and not null (and also not throw an error) as before.

For serialization, it would work the same way: If the local value is actually null and the property is flagged optional, the resulting json object will not include the value.

If you @dleavitt @m-architek have any time, could you try this new version (1.2.0) that implements this? I have to make some further tests as well before publishing to NPM. Here it is:
https://github.com/dhlab-basel/json2typescript/tree/develop/andreas

@m-architek
Copy link

m-architek commented Feb 25, 2019

It still not covers my case when I want to allow null, but only on optional field.

Example:

import {JsonConvert, JsonObject, JsonProperty, ValueCheckingMode} from "./index";


let jsonConvert: JsonConvert = new JsonConvert();
jsonConvert.valueCheckingMode = ValueCheckingMode.ALLOW_NULL;


@JsonObject("Foo")
export class Foo {

    @JsonProperty("notOptional", String, false)
        notOptional: string = undefined as any;

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


function doJob(input: any) {
    let output: Foo = jsonConvert.deserializeObject(input, Foo);
    console.log(output);
}


const validInput = {
    notOptional: "some value",
    optional: null
};

// prints Foo { notOptional: 'some value', optional: null } - it's ok
doJob(validInput);

const invalidInput = {
    notOptional: null
};

// prints Foo { notOptional: null, optional: null } - should throw an error instead
doJob(invalidInput);

@m-architek
Copy link

Ok, I can see it works this way without ALLOW_NULL flag. Thank you for this change. :)

import {JsonConvert, JsonObject, JsonProperty, ValueCheckingMode} from "./index";


let jsonConvert: JsonConvert = new JsonConvert();
// jsonConvert.valueCheckingMode = ValueCheckingMode.ALLOW_NULL;


@JsonObject("Foo")
export class Foo {

    @JsonProperty("notOptional", String, false)
        notOptional: string = undefined as any;

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


function doJob(input: any) {
    let output: Foo = jsonConvert.deserializeObject(input, Foo);
    console.log(output);
}

const validInput = {
    notOptional: "some value",
    optional: null
};
doJob(validInput); // Foo { notOptional: 'some value', optional: null }

const otherValidInput = {
    notOptional: "some value"
};
doJob(otherValidInput); // Foo { notOptional: 'some value', optional: null }

const invalidInput = {
    notOptional: null
};
doJob(invalidInput); // error

@andreas-aeschlimann
Copy link
Member

Exactly.

If you globally allow null, the first property will actually get a null value. You then need to make sure to tell the compiler that through your type definition!

If you disallow null, the first property indeed must not be null. If the second is null in the json, nothing is done and your default in the class is taken. This default could also be something not null, like „default“.

What is new (and does not work anymore): If you had default value „default“, but wanted it to become null in case the json had null/missin the value, then it would not change the value. I hope there are no people relying on this behavior, even though it is a little odd.

@andreas-aeschlimann
Copy link
Member

I'll go ahead and close this issue, I consider it fixed for now with version 1.2.0.

There is still room for improvements (like different behavior for each property), but this will be postponed for a future 2.0.0.

@dleavitt
Copy link
Author

@andreas-aeschlimann thanks so much for doing this! I've been going through my projects and setting DISALLOW_NULL and it's turning up lots of potential bugs in my typedefs/apis. Super helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants