mobxjs / serializr

Serialize and deserialize complex object graphs to and from JSON and Javascript classes
MIT License
766 stars 52 forks source link

Add support for required / optional properties #73

Open dsanders1234 opened 6 years ago

dsanders1234 commented 6 years ago

Currently, if the property in the schema does not exist in the json, it is simply skipped/ignored. It would be really useful to be able to mark properties as either optional or required and give an error if the property did not match the expected behavior.

alexggordon commented 6 years ago

You can mostly do that when using class schemas pretty simply.

Class Person {
    @observable @serializable age;
    @observable @serializable firstName = "John"
    @observable @serializable lastName = "Doe"
}

const person = deserialize({age: 20, firstName: "Alex"}, Person)

console.log(person.lastName) // results in "Doe"

does that solve your problem?

dsanders1234 commented 6 years ago

This may help for optional use case where we want the default to be something other than undefined. However what we would also like is to have an error if the property does not exist to ensure that data coming from the server is correct. (We are not using mob-x, just the serializr)

alexggordon commented 6 years ago

I would expect something like that to be enforced outside of Serializr. That's not really in the problem domain that serializr is trying to solve.

However, it sounds like if you're trying to validate json that json-schema might be a good solution for the problem. There are some json-schema validators that are very fast now, so might be a good option.

dsanders1234 commented 6 years ago

Yeah, I guess I see your point. We'll try to use TypeScript decorators or something. I don't want to have two models of my data (a typescript class and a json schema).

alexggordon commented 6 years ago

Sounds good!

dietergeerts commented 6 years ago

I would like this to be reopened, because optional properties can happen, and thus, when not comming from the BE, they shouldn't be added to the deserialized object, and now they do with an undefined value. So an optional option would be great For example, we have an object like this:

/**
 * @typedef {Object} PatientDetails
 * @property {?string} [propA]
 * @property {!boolean} [propB]
 * @property {!PatientCaseC} [propC]
 * @property {!PatientCaseD} [propD]
 * @property {!PatientCaseE} [propE]
 */

All these properties are optional, because not every patient case need each info. In the FE, we can check if a property is there or not, and then we know not to use it or show related things about it.

UPDATE: The actual bug is in the serializer. When you deserialize, the property is skipped, but when you serialize, the property will be there as undefined....

alexggordon commented 6 years ago

So this issue is something I've discussed at length because of an issue we have. There's a couple things that go into it, that have ultimately led us to create app specific serializr types.

If you think of the possible situations in which there could be a undefined type:

  1. Undefined (or key missing) from the backend.
  2. User sets it to undefined (select menu where they can deselect it).

Then, given those to situations, you may either

  1. Not care about the key missing in the serialized json
  2. Care about the key being in the serialized json (if the user sets something to undefined, then undefined is a user value you want to persist to the DB).

The issue is that you have to allow for both of those. For awhile, i've thought that the best way to solve them is just a custom serializr

import { serializable, custom, SKIP } from 'serializr';

@serializable(custom(
   value => value === undefined ? SKIP : value,
   value => value
));

you can then distribute that by storing it as a function:

import { serializable, custom, SKIP } from 'serializr';

const primitiveSkipIfUndefined = custom(
   value => value === undefined ? SKIP : value,
   value => value
)

@serializable(primitiveSkipIfUndefined);

However, I wonder if it would help to add in some more helper functions like SKIP to serializr. There's a similar discussion going on where I've been wondering if a optional null type for arrays could be of use too. That or a strict mode similar to mobx that enforces types.

Anyone have thoughts on either of those?

dietergeerts commented 6 years ago

I have been thinking about this, and I think it would be nice to explicitly tell the schema that it's a real optional value, which means that OR the key is there OR the key isn't there, which has nothing to do with the actual value.

I also see undefined and null differently. For me, undefined is like saying that nothing has been set, null is like saying that it is empty, or has no value, but it is there, it's defined.

So @alexggordon , in your example, for me the user setting to undefined would be the user sets the value to null, as it is defined, but set to null explicitly

alexggordon commented 6 years ago

That's what my example does. if the value is undefined it skips the key entirely. It is omitted from the json.

If you're referring to something different, could you give an example of what it would look like? Value based omission is the only way I could imagine an optional serializr type to work.

And while you're correct, with regards to javascript, unfortunately since this project serializes to json, and there's no difference between undefined and null in json, it's not really a super helpful distinguishment to make :(

chengjianhua commented 6 years ago

Is it ok to have a type called optional ?

In most case, the type user defined are treated as required:

@serializable(list()) strings;

In the above example, strings is required, it could not be null or undefined, if does, the (de)serialize failed.

If we do something like:

@serializable(optional(list())) strings;

If strings is null or undefined, the (de)serialize will not throw error because it's marked as optional.

alexggordon commented 6 years ago

So there's two issues going on so let's me careful not to intermingle them.

First is the possible omission of keys from the serialized json, based on incoming values.

Second is arrays failing serialization because they aren't an "array like" object.

I think your suggestion @chengjianhua solves the second issue, which is more being discussed in #58, but I don't think it solves the issue brought up here. Right? (correct me if I'm wrong?)

dietergeerts commented 6 years ago

I think the optional type would solve both, because:

POST EDITED

For serialization:

For deserialization:

Please correct me if I miss some use cases or logical interpretations.

chengjianhua commented 6 years ago

@alexggordon I think my suggestion aims to solve the second question you described.

@dietergeerts UPDATE: The actual bug is in the serializer. When you deserialize, the property is skipped, but when you serialize, the property will be there as undefined....

In strictly, this is a bug, it behaves inconsistently while deserializing or serializing.

IMO, the behavior while property is undefined should be consistent in deserializing or serializing, after solved this, we can think about the optional further.

dietergeerts commented 6 years ago

User sets it to undefined (select menu where they can deselect it).

Note that if you follow the specs, null means that the value is actually set to nothing, while undefined is something that happens by accident, either the property not being there, or a function that doesn't return anything. So if the user has set the value to nothing, you should use null.

alexggordon commented 6 years ago

I think hiding json key removal in an "optional" field might be a little opaque.

After doing some research, most JSON serializers have the concept of an "ignore if null" that only applies on serialization of an object. Essentially, if the value is undefined or null, it is removed from the serialized object.

I think a class decorator or something to that effect would be useful, and solve this issue in a more transparent way.

References: Json.net OJ, ruby

With regard to the array deserialize/serialize, I think that should be handled separately.

Thoughts on that?

dietergeerts commented 6 years ago

@alexggordon , keep in mind that your first link is for C#, which doesn't have the concept of undefined values. JS does, and they both have a different meaning.

Either way, it's all about a way to define optional/required handling, because a BE will sometimes give back properties with null value and others will not return them at all if they are null in the BE system. So depending on that, you'll want a different behaviour.

If you only look at JS, then the behaviour I described before is most likely the one you want, as it complies to JS as a language. But of course, data comes from the BE, so setting the strategy will be needed, as we aren't always in charge of them ourselves.

alexggordon commented 6 years ago

The issues I have with an optional type is that the only feature based functionality that is changed is serialization in that keys would just be removed if the value is undefined.

The array serialization functionality is a default and validation issue that I think should be solved by the user. If you say that something is an array, is it reasonable for the library to expect it to perform like an array?

Based on those two things is the reason I think it would be better to expand the functionality of the key removal from the serlialized format. What if a user wants to remove a key if the value is 0? That's not something the library should do automatically.

I'm thinking of adding an options object to the serializable decorator/function (and maybe a global one) where

class Dog {
     @serializable({ removeKeyIf: [undefined, 0] }) age;

     @serializable(list(), 
            { removeKeyIf: puppies => !puppies || puppies.length === 0 }
     ) puppies = [];

     @serializable(object(Home),
            { removeKeyIf: home => undefined }
     ) = new Home();
}

there's a removeKeyIf option that allows for a primitive, array, or function to be passed, and if that function returns true, or value matches the primitive passed, or value is one of the values in the array, then the key is removed. I think this exposes a more explicit functionality to a user that is a little easier to understand and to customize. Thoughts?

dietergeerts commented 6 years ago

Though I still believe an optional type would be good with a default working that ties together with what undefined means in JS. But there should be an extra way to decide if it's optional in other cases. So a type where you can give an optional array/function like in your proposal would be nice, so if the user encounters other reasons of what optional means in a certain case, (s)he can configure it.

alexggordon commented 6 years ago

Along that same line, another solution I see in java is a class decorator called @JsonIgnoreType which just wholly ignores keys of the type passed in. That is something we could do also, if we want to start a little more simply than an options hash.

Maybe something also like

@serializrIgnoreType(undefined)
class Person {
 ...
}

@serializrIgnoreType([undefined, null])
class Person {
 ...
}

Upside of this is for a first pass field level property exclusion is probably a little extreme, and most people probably wouldn't use that a ton. Thoughts on either of those? If you're okay with one of those I'll see if I can get a PR up.

chengjianhua commented 6 years ago

@alexggordon The decorator for a class is not accurate enough, if part of the properties should be ignored with undefined or null, but another part of them shouldn't.

alexggordon commented 6 years ago

Can you expand on that? All that would do is remove serialized properties that have the value passed into the decorator. I'm not quite sure what you mean when you say it's not accurate enough.

chengjianhua commented 6 years ago

Sorry, I'll expand on this:

What I mean is that the decorator for class you demonstrated affects all the properties in a class instance, so we can't control only part of them should be removed because If the serialized properties have the value passed into the decorator, you'll remove all of them.

What if I want to remove only part of serialized properties that meet a certain condition but others would be kept still.

pyrogenic commented 5 years ago

104 adds an optional() decorator that, as @alexggordon suggests, skips undefined values on serialization. It has no effect on deserialization, as undefined values are already ignored.