ExodusMovement / schemasafe

A reasonably safe JSON Schema validator with draft-04/06/07/2019-09/2020-12 support.
https://npmjs.com/@exodus/schemasafe
MIT License
155 stars 12 forks source link

False positive `requireValidation` check with `minProperties` #149

Open andylizi opened 2 years ago

andylizi commented 2 years ago

Description

{
    "type": "object",
    "minProperties": 1,
    "properties": {
        "phoneNumber": { "type": "string" },
        "emailAddress": { "type": "string" }
    },
    "additionalProperties": false
}

This schema will fail with [requireValidation] if properties is used, required should be specified.

The requirement in this case is not necessary because minProperties is specified and additionalProperties is set to false, so at least one of phoneNumber and emailAddress is already required without explicitly using the required keyword.

Related code

https://github.com/ExodusMovement/schemasafe/blob/81459f3c7162137c19e19d9b869f9e41d0bf8dde/src/compile.js#L1158

ChALkeR commented 2 years ago

The purpose of that check is to ensure that the schema author does not forget to specify required when it's needed, which is a common mistake, e.g.

{
  "type": "object",
  "properties": {
      "x": { "type": "number" },
      "y": { "type": "number" }
  }
}

If the intent is to not have any properties required, explicitly stating required: [] in the schema will silence this check.

ChALkeR commented 2 years ago

In the additionalProperties: false + properties + minProperties case, this behavior does seem to be intentional (for the schema) though, so perhaps it's ok to skip required check.

I wonder if this could introduce schema errors though vs the benefits of not writing require: [] in such cases.

I'll look closer into that.

andylizi commented 2 years ago

Personally I can't think of any situations where minProperties + additionalProperties: false behavior can be unintentional. The alternative way of doing this would be to use anyOf + required, which is much more verbose, has oblique error messages and presumably compiles to longer code. Numbers larger than one can't be expressed in this way either.

The workaround of require: [] is also not immediately obvious and confusing to future readers. My thoughts after submitting this issue were, "Oh, I guess I just have to disable this check in the mean time then". And since requireValidation can't be trivially overrided in strong mode, I'm sure some people will be lazy enough to disable stronge mode entirely (!).