lint: non-existing object properties listed as required are ignored

[davidkuridza]

Describe the bug
The openapi-cli ignores non-existing object properties which are listed as required. Anything can be listed under the required: tag and the lint will not return any errors.

To Reproduce
Steps to reproduce the behavior:

1. Use the example petstore.yaml (the behaviour is the same with 2.0, 3.0 and 3.1)
2. Add a non-existing or rename an existing required property:

diff --git a/petstore.yaml b/petstore.yaml
index 5f41fe0..531cbbf 100644
--- a/petstore.yaml
+++ b/petstore.yaml
@@ -78,6 +78,7 @@ definitions:
     required:
       - id
       - name
+      - non-existing-property
     properties:
       id:
         type: integer

3. Run openapi lint petstore.yaml --format stylish
4. The output is:

No configurations were defined in extends -- using built-in recommended configuration by default.

validating petstore.yaml...
petstore.yaml:
  2:1   warning  info-description        Info object should contain `description` field.
  5:3   warning  info-license-url        License object should contain `url` field.
  29:7  warning  operation-4xx-response  Operation must have at least one `4xx` response.
  47:7  warning  operation-4xx-response  Operation must have at least one `4xx` response.
  66:7  warning  operation-4xx-response  Operation must have at least one `4xx` response.

petstore.yaml: validated in 26ms

Woohoo! Your OpenAPI definition is valid. 🎉
You have 5 warnings.

5. The exit code:

$ echo $?
0

Expected behavior
There should be an error saying the required property is not defined, for example:

"non-existing-property" is present in required but not defined as property in definition "Pet"

openapi-cli Version(s)

$ openapi --version
1.0.0-beta.73

Node.js Version(s)

$ node --version
v17.2.0

Additional context

• The behaviour is the same with all OpenAPI versions.
• There is no .redocly.yaml present, the default out-of-the-box openapi-cli is used.
• The same can be observed using the latest NPM and Docker versions.

[adamaltman]

I don't think this is a bug. This seems like a feature request?

By default the objects allow additional properties. So by example it is technically correct (unless the additionalProperties was set to false -- in which case, it would be a bug).

[RomanHotsiy]

This is indeed not a bug. This is technically a valid openapi definition. It states that the non-exisiting-property should just exists in the payload data, no extra constraints like type are applied.

But I think we need an additional lint rule to enforce all properties defined in required are described in properties.

Not sure how to name this rule... no-undescribed-properties, no-undefined-properties, required-properties-described?

Any other ideas?

[adamaltman]

required-properties-described

[davidkuridza]

The OpenAPI Specification (both 2x and 3x) states the Schema Object is based on the JSON Schema Core and JSON Schema Validation. The JSON Schema Validation states the following at 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.

My understanding is the required should contain only the elements from the properties array, but I am probably wrong since English is not my native language :)

[adamaltman]

Hi David, yes, we believe you have a misunderstanding. Your request is a good feature request, but it is not bug.

Let's look at this example:

type: object
required:
  - adam
properties:
  david:
     type: string
additionalProperties: true

The additionalProperties: true means that it is allowed to include additional properties in the object.

The additionalProperties is true by default value in OpenAPI 2 and 3. So, you need to explicitly set additionalProperties: false if you don't allow any additional properties.

This example object is invalid:

{}

This example object is invalid:

{ "david": "sample"}

This example object is valid:

{ "adam": "sample"}

This example object is valid:

{ 
  "adam": "sample",
  "david": "sample"
}

This example object is valid:

{ 
  "adam": "sample",
  "david": "sample",
  "elvira": "sample"
}

You may be wondering... the valid example objects have properties that are not defined. That is what "additionalProperties" means.

http://json-schema.org/understanding-json-schema/reference/object.html#additional-properties

[davidkuridza]

Hi @adamaltman, thank you for the explanation, I was not aware of the additionalProperties. I'm looking forward to seeing the feature added, let me know if I can help in any way.

[adamaltman]

@davidkuridza finally in progress on this one. 👍

[tatomyr]

Sorry @davidkuridza, we had to put this on hold for some time. I hope we will eventually get back to it.

[davidkuridza]

@tatomyr no problem, good things come to those who wait :)

[tib-c]

We usually set the additionalProperties to false, because we don't want the objects open for extension. In this case any property named in the "required" array must exist as an entry in the properties dictionary. Otherwise an error should be thrown.

type: object
required: 
   - adam
properties:  
  david:    
    type: string 
additionalProperties: false

Should throw an error, that the property adam cannot be a required property, since it is not defined.

And even if I set additionalProperties to true, I might want to check if there are undefined properties in my required array. I would suggest the name schema-object-required-properties-defined to add a little bit more context.

[tatomyr]

Finally we've managed to have this done in v1.9 😅.
@davidkuridza, @tib-c could you check if the rule works as expected? Here are the docs: https://redocly.com/docs/cli/rules/no-required-schema-properties-undefined/#no-required-schema-properties-undefined

[tib-c]

Thank you for the update. We checked the new rule and it throws the expected errors.
It works case-sensitive, that means a property "name" will be invalid in combination with the string "Name" in the required-array. That could be added to the docs?

At some points we use allOf to extend an object without required properties by an object with required properties. I'm not sure, if this is a common practice. We use it, when a schema has properties that are required in some contexts and optional in other contexts. See the following example:

Schema 1: (only props, not required)

• properties: { "myProp": {}}
• required: []

Schema 2: (no props, just required array that includes Property names of Schema 1)

• properties: {}
• required: ["myProp"]

allOf: {

• Schema1
• Schema2
  }
  Combining the schemas with allOf would make the Prop "myProp" required now.

The problem is, that every Schema 2 will throw an error now, because there are no props included. Is there a better way to specify that scenario?

[tatomyr]

Thanks for the feedback @tib-c!

It works case-sensitive, that means a property "name" will be invalid in combination with the string "Name" in the required-array. That could be added to the docs?

Certainly! Will do that.

The problem is, that every Schema 2 will throw an error now, because there are no props included.

The linter visits every schema separately, and it'd be challenging to know the context a schema is used in (and even if so, the schema could be used in different contexts, in some correctly, in some not).

I can see several solutions to this, however.
First, as a workaround, you can add those cases to the ignore file.
Also, we can introduce an option to configure the rule, which will skip validating open schemas (those that don't have additionalProperites explicitly set to false), something like this:

rules:
  no-required-schema-properties-undefined: 
    severity: warn
    allowAdditionalProperties: true

And the last option I can think of is to take into account additionalPropertis set explicitly to true and skip those.

If you think any of the latter two should be implemented, feel free to open a new issue.

NB: The issue is somewhat similar to this one: #1437.

[davidkuridza]

@tatomyr it works, thank you very much!