assertion message not matching description

[gotjoshua]

For the following model definition, I get an error message as if i did not send a date, if i send a Date that is not at High Noon.

export const DateAtHighNoon = BasicModel(Date)
  .extend()
  .assert(function isAtHighNoon(d) {
    return d.getUTCHours() === 12 && d.getUTCMinutes() === 0 && d.getUTCSeconds() === 0;
  }, "Date isn't at HighNoon (12:00 UTC) - try using Utils.truncateTimeAndZone or Utils.toDateAtHighNoon")
  .as('DateAtHighNoon');

Here is the console.log that shows clearly the description of the assertion is correct but below it says message: "expecting departureDate to be Date, got Date Thu Oct 24 2019 01:00:00 GMT+0100 (Western European Summer Time)"

[29066.12, 19208.21] ParticipationsViewService.Participations: _om.test failed: 
[{…}]
0:
expected: Array(1)
0: ƒ ()
constructor: ƒ Q(t)
definition: ƒ Date()
assertions: Array(1)
0: ƒ isAtHighNoon(d)
length: 1
name: "isAtHighNoon"
arguments: null
caller: null
prototype: {constructor: ƒ}
description: "Date isn't at HighNoon (12:00 UTC) - try using Utils.truncateTimeAndZone or Utils.toDateAtHighNoon"
__proto__: ƒ ()
[[FunctionLocation]]: BasicModels.js:84
[[Scopes]]: Scopes[2]
length: 1
__proto__: Array(0)
length: 0
arguments: null
caller: null
prototype: t {constructor: ƒ}
errors: []
name: "DateAtHighNoon"
__proto__: ƒ ()
[[FunctionLocation]]: objectmodel-4.0.3.js:1
[[Scopes]]: Scopes[5]
length: 1
__proto__: Array(0)
received: Thu Oct 24 2019 01:00:00 GMT+0100 (Western European Summer Time) {}
path: "departureDate"
message: "expecting departureDate to be Date, got Date Thu Oct 24 2019 01:00:00 GMT+0100 (Western European Summer Time)"
__proto__: Object
length: 1
__proto__: 

[sylvainpolletvillard]

Hi ! What value did you test for this model to get this error message ?

[gotjoshua]

it says it in the console log in received: and also in message: Thu Oct 24 2019 01:00:00 GMT+0100 (Western European Summer Time)

[sylvainpolletvillard]

do you have some code with the value tested ? It is probably a String

[gotjoshua]

I am trying to make a code sandbox and i doubt that it was a string, because then Object Model gives this:

TypeError: expecting highNoonDate to be Date, got String "Wed Dec 12 2012 12:00:00 GMT+0000 (Western European Standard Time)"

what i found the most strange was that the message indicates that it is indeed a date:

message: "expecting departureDate to be Date, got Date Thu Oct 24 2019 01:00:00 GMT+0100 (Western European Summer Time)"

so far i don't manage to get the same behavior in the sandbox got it: if the DateAtHighNoon is optional in the OM definition, then the error is not using the description! Check out the code sandbox ...

[sylvainpolletvillard]

Okay got it ! Minimal reproduction case:

const PositiveNumber = BasicModel(Number).assert(function isPositive(n) { return n >= 0 })
const User = ObjectModel({ age: [PositiveNumber] })
User({ age: -1 })
// Uncaught TypeError: expecting age to be Number, got Number -1

Working on a fix

[sylvainpolletvillard]

Okay so I made a minor fix to get the initially intended message here, which should be:

expecting quantity to be Number or undefined or null, got Number -1

When a model definition is an union type, we enumerate the allowed types in the error messages, without accounting the custom assertions that may be added to these types. Optional properties are basically union types with null and undefined values.

I'm not sure how I could improve these messages for these cases ? If it is an union type with several models having each several assertions, things could quickly become unreadable.

What do you think of

expecting quantity to be Number (with assertions) or undefined or null, got Number -1

Another option would be to show custom names when given to a model with assertions, in your case:

expecting departureDate be DateAtHighNoon or undefined or null, got Date Thu Oct 24 2019 01:00:00 GMT+0100

but that would only fix the issue if people are naming their models like you did with .as("DateAtHighNoon")

[sylvainpolletvillard]

Another option would be to show custom names when given to a model with assertions

this is done and released in v4.0.4, it makes total sense to me

but that would only fix the issue if people are naming their models

So I'm still looking for a better error message when dealing with unnamed models with custom assertions

1. expecting Number (with assertions)
2. expecting Number*
3. expecting Number (2 assertions)
4. expecting Number(isPositive) - requires named functions as assertions
5. expecting Number*** - number of stars is the number of assertions

My current preference goes to 2., but I'm open to other suggestions

[gotjoshua]

Awesome that you fixed it for named extended models! Thanks for the prompt action (as usual)...

Hmmm, about unnamed models... I guess I don't understand why the error collector can't know which assertion failed and include the first assertion description that failed. But if it is impossible then why bother putting the description for the custom assertion? Am I missing something here?

Should we define an explicitly MaybeNullDateAtHighNoon that doesn't extend anything and can be put in the definition as non-optional?

Of the options you offered, I like a union of 4. and 5. So if the assertion is not a named function then it looks like 5.

Number***

and if it is named then like 4.

Number(isPositive)

explicitly exposing the internals of a union type (when from the user perspective we are just creating an optional value) feels a bit messy.

expecting departureDate be DateAtHighNoon or undefined or null,

So for my case this would be my preference:

expecting departureDate be [DateAtHighNoon], got Date Thu Oct 24 2019 01:00:00 GMT+0100:
"Date isn't at HighNoon (12:00 UTC) - try using Utils.truncateTimeAndZone or Utils.toDateAtHighNoon"

[gotjoshua]

also, what is the timelilne for pika to publish the new v4.0.4 in UMD? ( still needed for using your lib with Meteor : -( )

[sylvainpolletvillard]

also, what is the timelilne for pika to publish the new v4.0.4 in UMD?

I'm not responsible of the pika platform, I see that 4.0.4 is queued but I don't have more information than you. Note that it is quite easy to build this UMD version yourself with rollup CLI:

npm install --global rollup
rollup objectmodel.js --file objectmodel.umd.js --format umd --name "window"

https://rollupjs.org/guide/en/

[sylvainpolletvillard]

I guess I don't understand why the error collector can't know which assertion failed and include the first assertion description that failed.

Actually the collector has this info, but the error formatter is not "smart" enough to show you the most appropriate message due to union types (as explained before, optional types are basically union types with null and undefined). Let me explain with this example:

const NumberHigherThan10 = Model(Number).assert(function isHigherThan10(n) { return n > 10 });
const NumberLowerThan5 = Model(Number).assert(function isLowerThan5(n) { return n < 5 });
const NumberNotBetween5And10 = Model([NumberHigherThan10, NumberLowerThan5 ]) // <-- union

NumberHigherThan10(7) // TypeError: assertion "isHigherThan10" returned false for value 7
NumberLowerThan5(7) // TypeError: assertion "isLowerThan5" returned false for value 7
NumberNotBetween5And10(7) // <---- which message should be displayed ?

If the error formatter was smart enough, it would print something like: "TypeError: None of the types accepted for NumberNotBetween5And10 have been found to be valid for value 7. The first type NumberHigherThan10 is not valid because assertion "isHigherThan10" returned false. The second type NumberLowerThan5 is not valid because assertion "isLowerThan5" returned false"

As you can see it is quite verbose to explain exactly what is going on, and it is a very basic example :) For union types with 3, 4, 5 different types or more, it would be really messy.

The current error message is : TypeError: expecting Number or Number, got Number 7 ; which is quite bad obviously

If models are named, the message becomes: TypeError: expecting NumberHigherThan10 or NumberLowerThan5 , got Number 7 ; which is much better

If models are not named, but assertions functions are named, it could be: TypeError: expecting Number(isHigherThan10) or Number(isLowerThan5) , got Number 7 ; but that's an ideal case where all assertions have a descriptive name, and a relatively small number of assertions. What if a model has 4 assertions with 3 anonymous functions ?

Should we define an explicitly MaybeNullDateAtHighNoon that doesn't extend anything and can be put in the definition as non-optional?

Yes that would work, but it's a hacky workaround, let's try to find something better :)

for my case this would be my preference

what you suggest is that the error formatter could autodetect optional types (check that the union type has exactly 3 different accepted types being X, null and undefined), and display [X] instead with the validation errors of X exclusively. That's a bit complicated but I could do it. But I would rather find a solution that works for all union types, instead of a specific one for optionals.

[sylvainpolletvillard]

the error formatter could autodetect optional types

I found another simpler way, I shorten the validation path for optional types with an additional pre-check. This way you get an appropriate error message, and a slight performance gain when checking optionals. Will release later today, I'm still looking for a solution for other union types.

[gotjoshua]

nice! thanks again : -D

[gotjoshua]

there is no release nor tag for 4.0.4 here on github... but i see on npm that it is released, is that intentional?

[sylvainpolletvillard]

yeah I usually do Github releases for major or minor versions, and ignore small bugfixes and improvements like those. I can add a tag for 4.0.5 if you want, so that 4.0.4 is tag not found 👓

[sylvainpolletvillard]

Okay I doubt I can get a better result than this. It displays the model name, or the assertion function name, or the assertion description, or the inner function code. It will probably become too verbose for some users, but I can enforce as a best practice to always name models when composed.

[sylvainpolletvillard]

Released in v4.0.5

[gotjoshua]

rollup objectmodel.js --file objectmodel.umd.js --format umd --name "window"

I gave this a try with rollup objectmodel.js --file objectmodel.umd.js --format umd --name "window"

objectmodel.js → objectmodel.umd.js... [!] Error: Could not resolve entry module (objectmodel.js)

I know debugging rollup isn't your responsibility but I didn't manage yet (can also wait for Pika to publish it)

[gotjoshua]

this seemed to have worked for the umd build: rollup main=build/bundle-entry.js --dir ./objectmodel.umd --format umd --name "window"

[gotjoshua]

The test implementation in our app, looks great...

If I catch the error with this fx:

omTestErrorFx = errors => {
    const {message:msg, path:prop} = errors[0];
    LV > 3 && console.warn(this.l(`_om.test failed for ${prop}: ${msg}`), errors);
  }

_om.test(clonedUpdatedOMish, this.omTestErrorFx);

This is the output: ParticipationsViewService.Participations: _om.test failed for departureDate: assertion "Date isn't at HighNoon (12:00 UTC) - try using Utils.truncateTimeAndZone or Utils.toDateAtHighNoon" returned false for departureDate = Wed Dec 11 2019 00:00:00 GMT+0000 (Western European Standard Time)

[sylvainpolletvillard]

Okay so it's all good for you ?

[gotjoshua]

brilliant, many thanks !