Strict mode validation: property names in "required" should be present in the sibling "properties" object

[PBug90]

{
    "type" : "object",
    "properties" : {
      "test": {
      	"type":"number"
      }
    },
    "required" : [
        "identifier",
        "features"
    ]
}

What version of Ajv you are you using?
7.0.3

What problem do you want to solve?
Optional strict mode addition to check that keys given in "required" must exist in the sibling "properties" definition.

What do you think is the correct solution to problem?
Pass an additional constructor option to validate that every key name given in "required" exists as an explicitly defined property in "properties" when using strict mode.

Will you be able to implement it?
Yes

[epoberezkin]

Love the idea - very happy if you add it!
strictRequired?

The challenging bit here is that you often have "required" used inside "if" (or allOf etc) when "properties" are defined on the higher levels. So, for example, with this option this schema should be still considered "strict":

{
  type: "object"
  properties: {foo: {}},
  if: {required: ["foo"]}, // it should be seen as compliant with strictRequired
  then: {properties: {bar: {type: "boolean"}}} // it still complies with strictTypes here, as there is type: object above
}

So there should be a place in the SchemaCxt that would keep track of defined properties on the current and upper levels, same as now happens with types to enable strictTypes without requiring to repeat type on all levels.

I don't think it needs to see inside the children schemas though, as unevaluatedProperties does - it would be more complex and there is no real use case for it.

Also see #1392 - in v8 strict: true would just enable all strict options, in v7 it would remain opt-in.

[PBug90]

Added a basic implementation and have to take a look at the special case you mentioned above in the next couple of days. Feel free to provide feedback on the PR already.

[epoberezkin]

Cool. Supporting advanced scenarios with properties defined on higher levels is likely to change this code though.

You need to accumulate defined properties inside schema compilation context (and reset it every time data instance location changes - see subschema.ts) - and use these accumulated properties in strictRequired check.

[epoberezkin]

Also, a question, do we want to allow requiring properties that match patternPropeties patterns? Doesn’t need to be in the first release though - it’s a rare use case..

[PBug90]

I would like to keep it as simple as possible at first and it can be expanded later, so no patternProperties yet.

Your example above seems to be pretty straight forward. Can you provide me with an example case that needs to match more than one level into a parent schema? I dont use the more complex features of JSONSchema in my day to day work and the JSONSchema spec is pretty complex.

[PBug90]

I updated the PR and now some tests fail because they do not satisfy the strictRequired conditions.

The current implementation properly validates your schema above and also one more advanced use case like this:

{
   "type":"object",
   "properties":{
      "foo":{
         "type":"object",
         "required":"foo",
         "properties":{
            "test":{
               "type":"number"
            }
         }
      }
   }
}

Currently i am resetting the keys in the context every time a new schema of type object is encountered. This makes your if/then example above work properly, but does not keep all of the higher level keys in context all the time which would result in false negative checks.

Is there a simple case that is not covered by that validation yet?

[epoberezkin]

it's not sufficient to just check an immediate parent - there might be several layers of schemas for the same data level - with allOf, oneOf, if/then/else etc. - with different properties defined on different levels.

It's more difficult (and slower) to traverse the schema upwards collecting all the properties every time required is encountered, it is easier to collect these properties in SchemaCxt object in some property (e.g. definedProperties) on the way down the schema (but it should be initialised on every new data layer in applySubschema). In this case required would only need to check this property in SchemaCxt and properties in the current schema (because required is validated before properties...). Hope it makes sense.

[PBug90]

I can understand the reasoning perfectly fine. I did not intend to go down the whole schema-tree to check for required keys.

Can you give me some more guidance on where exactly to implement the list that collects the properties in subschemas? I have a hard time following the function calls that are happening and why they are happening because I do not have a lot of background knowledge about all the JSONSchema wording that is used in this project.

Did I miss a general architecture doc that would help me track the ajv logic?

[epoberezkin]

There isn't much of architecture doc, other than components.md - probably worth adding, but it's a separate subject.

You can extend SchemaCxt interface by adding definedProperties property to it (say, a Set of strings), initialise this property in compile/index.ts and add properties to this set inside properties keyword - it is available via cxt.it.definedProperties.

The only caveat is that it would not include properties from sibling "properties" keyword, as it is executed after "required", so they have to be accounted for separately - they are in cxt.parentSchema.properties - that is assuming you do this check inside "required" keyword code. Alternatively the check could stay where it is in your PR in which case you would combine properties in it.schema.properties - you check it already - and also include properties from it.definedProperties set - this is the same it as is inside cxt that is passed to keywords code.
Performing strictRequired check inside "required" keyword seems better though, as it would only be called if "required" keyword is present (one fewer if) and it is also consistent with how other keyword-specific "strict" checks are done - e.g. strictTuples - see here: https://github.com/ajv-validator/ajv/blob/master/lib/vocabularies/applicator/items.ts#L28

Hope it all makes sense - do ask questions please. There are definitely lots of missing bits in the docs, and if this conversation results in architecture.md doc helping other people to figure out the inner workings of Ajv to some extent that would be very valuable too.

Thank you

[epoberezkin]

but it should be initialised on every new data layer in applySubschema

yes - this too - it is here :)

[PBug90]

I updated the PR with your recommendations and hints. For some reason the CICD automation is not run. Can we move further implementation discussions to the PR itself?