using additionalProperties: false does not considers merged schemas

[rolfisub]

OpenAPI version
3

Describe the bug
When you use additionalProperties: false on a merged schema, it considers only the object where this statement is placed, not the resulting merged schema.

To Reproduce
given the following example spec

components:
  schemas:
    Model:
      type: object
      properties:
        id: integer
paths:
  /test:
    get:
      responses:
        '200': 
          description: OK
          content:
            application/json:
              schema:
                allOf:
                  - $ref: "#/components/schemas/Model"
                  - type: object
                    additionalProperties: false
                    properties:
                      name:
                        type: string     

when you call the endpoint /test and you receive the following data:

{
  "id": 1,
  "name":"test"
}

validating the response:

test("test", async ()=>{
  const res = await axios.get("/test");
  expect(res).to.satisfyApiSpec;
});

Throws the following error:

{ message: 'The response was not valid.',
  errors:
   [ { path: 'root',
       errorCode: 'additionalProperties.openapi.responseValidation',
       message: 'root should NOT have additional properties' } ],
  actualResponse: .....

I'm using the root path because this is all pseudo code, I dont know what path it uses when we are talking about objects at the root level

Expected behavior
It should consider the resolved merged schema when validating for additional properties

Screenshots

Additional context

[rolfisub]

It would also be useful if the error would say which property is being detected as additional, but I guess that would be more of a feature request

[rolfisub]

happy new year! :-)

[rwalle61]

Happy new year! Thanks for raising this.

I don't have much time this week but here are my initial thoughts:

Presumably the bug is in the code that validates schemas merging allOf several schemas. (And somehow the bug is triggered when one of the schemas has additionalProperties, which prevents the merging logic from merging the schemas together).

The validation code is in this dependency (openapi-response-validator). We can check if the offending code is in this dependency by recreating this bug in one of their tests.

If we can't recreate it there, then presumably the bug is in our code somewhere

And re your feature request, that would probably be best raised in the dependency repo, because its their code :)

[rolfisub]

Ok make sense that the issue is in that dependency. I don't know when I will have the time to submit a test case but I'll try not to keep it linger for too long. Thanks for the feedback :-)

[rwalle61]

Hey @rolfisub I recreated the bug in openapi-response-validator, and raised a bug there with some thoughts in kogosoftwarellc/open-api#609. It looks like the bug is 2 dependency levels down (in AJV), so might take more time than I have available right now.

[rwalle61]

fyi I also recreated this bug in our repo, then made a template to simplify reporting bugs in future :) #47

[mvegter]

Directly from swagger.io

Note: When validating the data, servers and clients will validate the combined model against each model it consists of. It is recommended to avoid using conflicting properties (like properties that have the same names, but different data types).

This implies that behavior is actually correctly. The two schemas are evaluated individually for the whole object, thus the additionalProperties will fail because it expects nothing or the name property but actually receives the combined model.

[rwalle61]

I think you're right @mvegter !

As you say,

{
  "id": 1,
  "name":"test"
}

does not satisfy the schema

               - type: object
                    additionalProperties: false
                    properties:
                      name:
                        type: string     

because of the additional id property. Therefore it doesn't satisfy allOf these 2 schemas:

                allOf:
                  - $ref: "#/components/schemas/Model"
                  - type: object
                    additionalProperties: false
                    properties:
                      name:
                        type: string  

Indeed, swagger and JSON schema evaluate schemas individually:

• https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/
• https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#composition-and-inheritance-polymorphism

So allOf is more restricting than I'd initially understood. I'd thought that allOf would merge the schemas together, then validate the response body against that big schema. But actually allOf validates the response body against each schema, and the body has to pass all of those schemas!

Seems this is a common misconception: https://stackoverflow.com/a/23001194/9864252

@rolfisub if you concur I'll let you close this issue :)

[rolfisub]

Well we can close this issue for sure, I just think that the most common use case is validating combined schemas not that all of the schemas validate individually to the merged object. It seems to me that the merged object validation is missing then. How can we validate a merged schema that has no additional properties then?

[mvegter]

@rolfisub , I fully agree with you but it is unfortunately conform the specification. The "solution" we are using is to parse and resolve all the allOf references to a single schema. So you are going to end up with a generated spec file either in version control or ignored.

[rwalle61]

@rolfisub, yes as @mvegter says the OpenAPI spec does not seem to allow merging: OAI/OpenAPI-Specification#674. That page suggests there may be alternative ways to achieve what you want, though I'm not familiar with them

I'll close this then, but feel free to re-open if necessary.

Thanks all for contributing your discussion!