Gi60s / openapi-enforcer

Apache License 2.0
94 stars 22 forks source link

required ... is not required ? #137

Closed LasneF closed 2 years ago

LasneF commented 2 years ago

given the definition there about required statement of json schema

https://datatracker.ietf.org/doc/html/draft-fge-json-schema-validation-00#section-5.4.3.2 ie 5.4.3. required

5.4.3.1. Valid values

The value of this keyword MUST be an array. This array MUST have at least one element. Elements of this array MUST be strings, and MUST be unique.

5.4.3.2. Conditions for successful validation

An object instance is valid against this keyword if its property set contains all elements in this keyword's array value.

given this object

  MyData:
    type: object
    required:
      - "a"
      - "b"
    properties:
      a:
        type: string
        description: field a
      c:
        type: string
        description: field c  

this object pass the enforcer validation dispite the fact that the object has no b field ,

what about raising a warning if not even an error ?

Gi60s commented 2 years ago

Thank you for the issue. I was a bit perplexed by this one initially because I thought this was implemented, and it is, although not in the way you might expect at first.

You can see here that a test is written for this (https://github.com/Gi60s/openapi-enforcer/blob/master/test/definition.schema.test.js#L1870-L1882) and I've pasted the current code below:

it('must be included in defined property or allowed through additional properties', () => {
    const def = {
        type: 'object',
        required: ['a', 'd'],
        additionalProperties: false,
        properties: {
            a: { type: 'number' },
        }
    };
    const [ , err ] = Enforcer.v3_0.Schema(def);
    expect(err).to.match(/at: required > d\s+Property is listed as required but is not defined in the schema properties and additional properties are not allowed/);
    expect(err.count).to.equal(1);
});

The name for the test is what helped me to remember what is going on here. Your example will only fail validation if additionalProperties is set to false. You've said that a property b is required, but you haven't assigned a schema to it, so it falls into the additionalProperties category. A valid object that matches your schema could look like this:

{
  "a": "Field A",
  "b": 5
}

One thing I could do to help when you have a schema like the one you've shared is to produce a warning. You'll have to check for warnings, but it could help.

Would that be helpful? Any additional feedback?

Thanks for your help.

LasneF commented 2 years ago

again this nasty additionnalproperties

it s a opinion only but i think that additional properties should not be taken into account here because additionnal properties is something "not specified at all", meaning something else

versus having a required field means it must be there, can this must be applied to the additional data that is undefined ? looks not 100 % fair , it s a bit like defining the undefined :)

not sure when the spec says An object instance is valid against this keyword if every item in the array is the name of a property in the instance.

if property include or not additionnalproperties

to me it deserves a warning

then it s an opinion ,and you are the boss :)

Gi60s commented 2 years ago

I'll work on making a warning appear.

Also, when version 2 of the OpenAPI Enforcer is released you'll have the ability to escalate or deescalate some warning/error messages. Essentially what I'm saying is that for this warning in v2 it will be possible to tell it to produce an error if that is what you want even though the default would be a warning.

Once I've added this warning I'll let you know and then I'll close this issue.

Gi60s commented 2 years ago

Sorry for the slowness but I've finally gotten around to writing this into the code for you. Now, by default, you will receive a warning when you list a property as required but don't include it in the list of properties (assuming that additional properties are allowed).

You can escalate or skip that warning using the exception code WSCH007. If you're not familiar with how to use the codes, please check out these docs: https://openapi-enforcer.com/api/#enforcer

I've published this change to NPM with version 1.21.0.

Thanks for your patience and have a great day!