Setting defaults breaks type check

[klues]

I encounter the problem that setting default values of an ObjectModel breaks the type check. See the following demonstration:

1. https://jsfiddle.net/r4twqge0/3/ --> setting wife to [1,2,3] instead of a valid Person object results in the expected error in the console: Uncaught TypeError: expecting wife[0] to be { name: String, age: [Number] }, got Number 1
2. https://jsfiddle.net/r4twqge0/5/ -->after adding a default value for the Person ObejctModel, suddenly there is no type error anymore for setting wife to [1,2,3].

I tested ObjectModel v3.4.6 in Chrome Version 67.0.3396.79 and Firefox 60.0.2

[klues]

With ObjectModel v2.6.2 it seems to work, see https://jsfiddle.net/yrcnb23h/3/
However the error message is not that clear as in v3.4.6:
Uncaught TypeError: expecting wife.name to be String, got undefined

Edit: on second try of this example I again do not get an error, but I should...

[klues]

Seems to be connected to ArrayModel and Model.Array(). If I am using Model.Array() in v2.6.2 I am also not getting an error, where I should, see https://jsfiddle.net/yrcnb23h/6/

[klues]

Some more tests with v2.6.2, same example as before with Person and Lovers model:

var Person = Model({
    name: String,
    age: [Number]
}).defaults({
    name: "new-name"
});

var Lovers = Model({
    husband: Person,
    wife: Model.Array(Person)
});

with no defaults: on Person

• everything seems to work as expected

with default value for name on model Person:

• husband: 1 gives no error, but should throw error
• husband: "string" gives no error, but should throw error
• husband: null throws error as expected
• husband: new Person() throws no error, as expected
• wife: null throws error, as expected
• wife: new Person() throws error, as expected (should be array)
• wife: [new Person()] no error, as expected
• wife: [1] no error, but should throw error
• wife: ["test"] no error, but should throw error
• wife: [{}] no error, but should throw error --> it seems that just every array filled with any type of objects works
• wife: [null] throws error, as expected

[sylvainpolletvillard]

Oh that's a big one. I'm surprised it didn't get spotted by the 433 unit tests. NEED MORE TESTS !

Thanks for the report, I'm looking at this today

[sylvainpolletvillard]

Ok here is what's happening:

const Person = Model({
	name: String,
	age: [Number]
}).defaults({
	name: "new-name"
});

Person.test(1) // true !

Why is 1 considered a valid Person ? Because object model validation is based on the types of its defined properties. X is a valid Person if X.name is a String and X .age a Number or undefined.

• 1..name is undefined, so we get the default value for it, "new-name". which is a String ✔
• 1..age is undefined, but age is an optional value, so undefined is accepted ✔

So you might say, yes, the definition of its properties is valid, but 1 is not an object ! Well, it can be. JavaScript has this ability to dynamically convert its primitive types to objects when needed. That's why you can call methods on numbers like this : 1..valueOf(). All the other tests you made seem to be related to this one misunderstanding.

Now, what we could do is add an assertion to object models so that they must be objects or derived from objects, without casting. But this is a breaking change and it breaks some existing mechanics like null-safe object traversal, so I need to think about how do to this properly.

[sylvainpolletvillard]

I think I found a proper solution. To be released in v3.5

[klues]

Thanks for your explanation - makes sense!
What I wondered: Why the defaults are also applied on tested objects? So in the line Person.test(1), why are the defaults applied to 1? To my mind it would make more sense if applying the defaults would only be done at creation time of new model objects. So:

1. new Person() <-- here in the constructor the defaults should be applied to the generated object
2. Person.test(1) <-- here the defaults should not be applied to 1

Just my thoughts, I don't know if it makes sense or is possible to implement without breaking something. But since you said you already found a solution - great!
BTW: Thanks for your library, I really like it!

[sylvainpolletvillard]

3.5 has been released, you can test it already. The test I added is relatively safe and sensible, but I expect it to break some part of some project somewhere. We'll see 🙈

The purpose of the test function is to determine if an object can satisfy the model definition, not if an object is already a model instance (x instanceof YourModel). Otherwise you could get false positives where YourModel.test(x) === false while new YourModel(x) works without errors. Remember that the values are automatically casted to their suitable Model (what we call "duck typing" in the docs), so the defaults will always apply.

Also, a common misunderstanding about default values is that they are not set as the properties of the object itself, but instead are defined on the prototype:

const Person = Model({ name: String }).defaults({ name: "Anonymous" });

let rick = new Person({ name: "Rick Sanchez" })
console.log(rick.name); // Rick Sanchez
delete rick.name;
console.log(rick.name); // Anonymous, inherited from Person.prototype.name

[klues]

Ok, then I did not correctly understand the test() method.
But the thing that is a littlebit unlogical to my mind still remains. Assuming the model and instance:

const Person = Model({
	name: [String],
	mother: [Person]
});

let instance = new Person({
	mother: 1,
});

I think this would work without errors, because 1 can satisfy the model definition (only optional properties). However it would be much more logical for me if the property I want to assign to mother would be checked to be instanceof Person.

Thanks for the new release, unfortunately I will not be able to use it, because I do not want to drop IE support on my project, so I'm currently using version 2.6.2.

[klues]

Ah, ok, I think now i understand - its because of the "duck typing".
However, it would maybe an idea to add the possibility to turn "duck typing" on or off and if off to only allow elements that are instanceof the model.

[sylvainpolletvillard]

I'll try to report a similar fix on v2, but I can't promise it will behave exactly the same.

I don't understand what your goal is in turning off the duck-typing/auto-casting feature. To be clear, passing mother: new Person({ name: "Anna" }) or mother: { name: "Anna" } will behave exactly the same since we are casting the object to the suitable model. You can decide to manually construct a Person object and pass it to the parent model constructor, but the result will be the same, and you will get the same errors if any.

My point is that the autocasting provided by ObjectModel doesn't make your type-checking "less strict", it's just more convenient. In the end, it's the same model definition that applies.

[sylvainpolletvillard]

I reported this fix in v2, with the v2.6.3 version. Could you confirm ir works as intended ? Thanks

[klues]

Thanks for also fixing this in v2, great!
I just tested it with several cases here: https://jsfiddle.net/r4twqge0/10/
seems to work - values like 1 or "" are now not valid anymore for Person.
However {} and new Number(1) are still valid, but thats because of the duck typing + applying defaults you already explained. I think its good like it is now. One just has to keep in mind that the defaults are applied always, not only if creating an object using the constructor like new Person().

My point is that the autocasting provided by ObjectModel doesn't make your type-checking "less strict"

But in some cases it would maybe make sense to have a stricter form. Think about this example:

const Person = Model({
	name: String,
	age: [Number]
}).defaults({
	name: "new-name"
});

const Car = Model({
	type: String,
	year: [Number]
});

And instances of these models are serialized and stored as JSON somewhere. Now if in the step of deserializing one would accidentally use a Car instance for creating a Person instance like this:

let carInstsanceJSON = dataService.getCarJSON();
let carInstanceData = JSON.parse(carInstanceJSON);
let person = new Person(carInstanceData); //would not throw an error

However if there would be any kind of stricter parsing, one would be able to detect such an error, e.g.:

let person = Person.parseStrict(carInstanceData); //does not apply defaults, throws error (missing name)

What I was suggesting before, was that the constructor new Person() should do the less strict check (with applying defaults) and the implicit cast should do the stricter check (wihtout applying defaults). However it would be a breaking change now, so its possibly not so clever to do this. Maybe an additional method like parseStrict() would be an alternative.

[sylvainpolletvillard]

I can not imagine a scenario where someone would like to instantiate models, while being certain that his data will not use the default values specified in the model. If you are certain of that, then extend the model and remove the defaults, problem solved.

Also, having a special method that does not apply the defaults would not solve the underlying problem: unless the Car Model is sealed, it is possible to have a Car with a name property that would pass the Person.parseStrict check.

[klues]

ok, I think you are right. I'm just thinking too much about strict type checking like it is possible in typed languages. But it's probably simply not possible to achieve that by dynamic checks in javascript. However your library is a great help, thanks for that!

[sylvainpolletvillard]

Yeah, static and dynamic type-checking are very different, the same concepts can not apply.
Anyway, we have a fix for the primitive autocast oddness

[klues]

yes, very good!