Gi60s / openapi-enforcer

Apache License 2.0
94 stars 22 forks source link

Object's properties not deserialized correctly when using allOf inside of allOf #111

Closed kristof-mattei closed 3 years ago

kristof-mattei commented 3 years ago

Here I am again! This time with an issue where I do have a solution for, but I am trying to understand whether it should work without my solution.

Let's get started with our schema:

openapi: 3.0.3
info:
  title: Test
  version: 0.0.1
  description: description
  termsOfService: "https://terms"
  contact:
    name: test
    url: "http://url"
    email: test@test.com
servers:
  - url: "https://server"
paths: {}
components:
  schemas:
    Object1: 
      allOf: 
        - $ref: "#/components/schemas/Object2" 
        - type: object
          required:
            - date
          properties:
            date:
              type: string
              format: date-time

    Object2: 
      # type: object # THIS LINE
      allOf: 
       - $ref: "#/components/schemas/Object3" 
       - $ref: "#/components/schemas/Object4" 

    Object3: 
      properties:
        foo: 
          type: string

    Object4: 
      properties:
        bar: 
          type: string

And our test code:

const Enforcer = require('openapi-enforcer')

Enforcer("./reproduce.yaml", { fullResult: true }).then(openapi => {
    const deserialized = openapi.value.components.schemas.Object1.deserialize({
        foo: "test",
        bar: "test",
        date: "2000-01-01T00:00:00.000Z"
    })

    const validated = openapi.value.components.schemas.Object1.validate(deserialized.value);

    console.log(validated);
});

When we run this we get

$ node ./index.js
[ EnforcerException: Invalid value
    Did not validate against all schemas
      at: 1 > date
        Expected a valid date object. Received: "2000-01-01T00:00:00.000Z" ]

However, in our schema, in Object2 you'll notice that I've commented out the type: object. That was on purpose. I originally missed that, because when I create a schema called Foo with properties openapi-enforcer deduces that it is of type: object.

With Object2 this is not the case. I explicitely need to specify type: object otherwise the deserializers of Object1.allOf[1] don't run, because Object1.allOf[0] wasn't of type: object: https://github.com/byu-oit/openapi-enforcer/blob/e29d5b35bd8fd55f065d7476907234ad51cba21f/src/schema/deserialize.js#L50

Now, I see 3 solutions, in order of what I think is more correct to less correct.:

  1. When deserializing the schema and the code encounters an allOf, it checks for the presense of type: object as a sibling. If not present, it adds it, somewhere around here: https://github.com/byu-oit/openapi-enforcer/blob/e29d5b35bd8fd55f065d7476907234ad51cba21f/src/enforcers/Schema.js#L256
  2. We add type: object to our Object2 and be explicit.
  3. When deserializing the data and we hit the code specified above we update the if clause as follows: if (schema.allOf[0].type === 'object' || !!schema.allOf[0].allOf)

I've spent some time thinking if it's possible to create a schema with allOf and its type not to be object... and I cannot come up with anything.

E.g.:

# ...
components:
  schemas:
    Object1: 
      # type: object # What else?
      allOf: 
       - $ref: "#/components/schemas/Object2" 
       - $ref: "#/components/schemas/Object3" 

    Object2: 
        type: string

    Object3: 
      type: string

Like what would Object1 really be?

Edit: I've also created a repl.it with everything inline to easier validation: https://repl.it/@KristofMattei/ViciousCyberKeygens#index.js

Gi60s commented 3 years ago

Hey @Kristof-Mattei. This is a brain scratcher. I've spent some time looking at this and am still trying to figure it out.

The problem of determining the type can become increasingly difficult when we run into allOf with nested oneOf or anyOf.

Also, you mentioned that it may not be possible to have an allOf with the type not object. When I originally built the library that was my impression too, but as I've continued to grow the library I've found that is not the case. Here is an example:

components:
  schemas:
    AllNumbers:
      allOf:
        - $ref: '#/components/schemas/Number1'
        - $ref: '#/components/schemas/Number2'
    Number1:
      type: number
      minimum: 0
    Number2:
      type: number
      maximum: 10
Gi60s commented 3 years ago

I've spent a lot of time trying to solve this and the conclusion I've come to is that this might need to wait for Enforcer version 2 to fix. For now the best solution is probably to create a schema that includes all 3 allOf schemas.

I created an ObjectX schema below as an example.

components:
  schemas:
    ObjectX:
      allOf: 
        - $ref: "#/components/schemas/Object2" 
       - $ref: "#/components/schemas/Object3" 
       - $ref: "#/components/schemas/Object4"
    Object1: 
      allOf: 
        - $ref: "#/components/schemas/Object2" 
        - type: object
          required:
            - date
          properties:
            date:
              type: string
              format: date-time

    Object2: 
      # type: object # THIS LINE
      allOf: 
       - $ref: "#/components/schemas/Object3" 
       - $ref: "#/components/schemas/Object4"