sylvainpolletvillard / ObjectModel

Strong Dynamically Typed Object Modeling for JavaScript
http://objectmodel.js.org
MIT License
467 stars 30 forks source link

Question: Is it possible to skip more detailed checks if initial type check fails? #156

Closed eponymous301 closed 1 year ago

eponymous301 commented 1 year ago

e.g. the following generates 3 errors:

import {ObjectModel} from 'objectmodel'

const PersonModel = new ObjectModel({
  FirstName: String,
  LastName: String,
})

const x = PersonModel(42)

exception output:

TypeError: expecting Object, got Number 42
expecting FirstName to be String, got undefined
expecting LastName to be String, got undefined

Given that we did not get an object at all, the 2nd and 3rd errors are a bit superfluous, and for large and/or nested models this can get spammy. A contrived example:

import {ObjectModel} from 'objectmodel'

const BirthdateModel = new ObjectModel({
  Day: Number,
  Month: Number,
  Year: Number
})

const PersonModel = new ObjectModel({
  FirstName: String,
  LastName: String,
  Birthdate: BirthdateModel
})

const x = PersonModel(42)

output:

TypeError: expecting Object, got Number 42
expecting FirstName to be String, got undefined
expecting LastName to be String, got undefined
expecting Birthdate to be {
        Day: Number, 
        Month: Number, 
        Year: Number 
}, got undefined

It would be nice to be able to short-circuit more detailed assertions if there is a gross failure at start of model validation like wrong type or undefined - I notice that the above nested BirthdateModel validation DID skip validating Day/Month/Year - but printed out model definition rather than saying e.g. "expecting Birthdate to be Object, got undefined")

To achieve similar effect for models that are not objects, I've been coding my custom assertions to first check that value is present and passes a less-restrictive model validation so that I don't get e.g.:

TypeError: expecting Number, got String "foo"
assertion "isPositive" returned false for value "foo"

(My custom 'isPositive' assertion function returns true if value fails BasicModel(Number)(value))

sylvainpolletvillard commented 1 year ago

Hi ! Agree. I'm going to try to short-circuit validation in this scenario.

sylvainpolletvillard commented 1 year ago

Released in v4.3.1

const PersonModel = new ObjectModel({
  FirstName: String,
  LastName: String,
})

const x = PersonModel(42)

now only returns:

expecting {
    FirstName: String, 
    LastName: String 
}, got Number 42

Regarding your second suggestion to display "Object" vs the whole model definition, I still believe it is more useful to show the model definition compared to just "Object", despite being more verbose.

eponymous301 commented 1 year ago

Excellent, many thanks!

eponymous301 commented 1 year ago

Apologies for pestering - is it also possible to skip field-level checks if undefined is passed in?

Currently:

const PersonModel = new ObjectModel({
  FirstName: String,
  LastName: String
}).as("PersonModel")

PersonModel(undefined)

generates

TypeError: expecting FirstName to be String, got undefined
expecting LastName to be String, got undefined

I tried to fork and modify:

        if (!isObject(obj) && !isFunction(obj) && obj !== undefined) {
            // short circuit validation if not receiving an object as expected
            return obj
        }

by removing && obj !== undefined - this did change error to:

TypeError: expecting {
        FirstName: String, 
        LastName: String 
}, got undefined

but also had side effect of causing test failures for Object Models > Cyclic detection and Object Models > edge cases of constructors

sylvainpolletvillard commented 1 year ago

This is due to null-safe object traversal: https://objectmodel.js.org/#doc-null-safe Also, the default value of the model will be applied when undefined is passed.

So in order to be safe to short circuit validation, it is required to have no default value and no behavior relying on null-safe object traversal. I can easily check the first condition but the second one is a bit more tricky. Contrary to the previous release, this will be a breaking change ; not a big one, the impact is probably low, but still, I'm not sure if I want to do that.

Apart from noisy output, do you have a particular reason for wanting this change?

eponymous301 commented 1 year ago

No reason other than reducing noisy output.

Testing with nested models, it looks like the short-circuiting works fine if a nested object is undefined, which is the main error scenario I encounter (processing JSON data from outside):

const PersonModel = new ObjectModel({
  FirstName: String,
  LastName: String
}).as("PersonModel")

const CastModel = new ObjectModel({
  Luke: PersonModel,
  Obiwan: PersonModel
})

CastModel({Luke:{FirstName: "Mark", LastName: "Hamill"}})

// TypeError: expecting Obiwan to be {
//        FirstName: String, 
//        LastName: String 
// }, got undefined

Apologies, I didn't realize nested worked fine when I commented - I can protect against top level being undefined before feeding in to ObjectModel in the first place, so not a problem!