Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Any OpenAPI validator that properly validates discriminator against data? #2141

Closed
mewalig opened this issue Feb 15, 2020 · 16 comments
Closed

Comments

@mewalig
Copy link

mewalig commented Feb 15, 2020

I would like to use an OpenAPI schema with a discriminator, then validate some input againts the schema. Thought this would be really easy-- there are so many validators out there that say they have discriminator support... and none of them I've tried so far worked! Seems like a fundamental task that anyone working with OpenAPI would need to perform, so maybe there's obvious and easy solution and I'm just not seeing it or I'm doing something wrong (maybe the spec I am using is flawed?)

In particular, I would like to do the following:

  1. Given an OpenAPI spec that includes a POST request body using a schema with a discriminator, start a localhost web server on any port
  2. Send a POST request using curl to the above service, and receive an accept/reject response based on whether the POSTed data conformed to the request body schema

I have tried the following, and in each case, I got the same result: validation worked fine in general, but failed to throw an error when the mapped schema and the discriminator value did not match (see specific example below)

The specific test case I used is as follows:

  • the spec describes a polymorphic object called Object, which has two subclasses: "obj1" and "obj2", each with a corresponding "properties" object that contains "property1a" & "property1b" in the case of "obj1", and "property2a" & "property2b" in the case of "obj2"
  • the test cases are i) a valid object, and ii) an invalid object that contains type "obj2" but properties corresponding to the obj1 subclass properties schema

Test Case 1 (valid data):

{
  "id": "hi",
  "type": "obj1",
  "properties": {
    "property1a": 1,
    "property1b": 2
  }
}

curl command:

curl -X POST -k 'http://localhost:3000/v1/object/hi' -H 'Content-type: application/json' --data '{"id":"hi","type":"obj1","properties":{"property1a":1,"property1b":2}}'

Test Case 2 (invalid data, validator properly rejects):

{
  "id": "hi",
  "type": "obj1",
  "properties": {
    "nonexistent_property": 1,
    "property1b": 2
  }
}

Notice that nonexistent_property is invalid. All of the validators I tried correctly reject this

curl -X POST -k 'http://localhost:3000/v1/object/hi' -H 'Content-type: application/json' --data '{"id":"hi","type":"obj2","properties":{"nonexistent_property":1,"property1b":2}}'

Test Case 3 (validator should throw an error, but does not!):

{
  "id": "hi",
  "type": "obj2",
  "properties": {
    "property1a": 1,
    "property1b": 2
  }
}

Notice that the type is obj2 but the properties are those corresponding to obj1. None of the validators reject this-- but they should, right?

curl command:

curl -X POST -k 'http://localhost:3000/v1/object/hi' -H 'Content-type: application/json' --data '{"id":"hi","type":"obj2","properties":{"property1a":1,"property1b":2}}'

YAML:

  openapi: "3.0.0"
  info: 
    version: "1.0.0"
    title: "Discriminator example"
    license: 
      name: "MIT"
  servers: 
    - url: "http://localhost/v1"
  components: 
    schemas: 
      Error: 
        type: "object"
        required: 
          - "code"
          - "message"
        properties: 
          code: 
            type: "integer"
            format: "int32"
          message: 
            type: "string"
      Object1Properties: 
        type: "object"
        required: 
          - "property1a"
          - "property1b"
        properties: 
          property1a: 
            type: "string"
          property1b: 
            type: "string"
      Object2Properties: 
        type: "object"
        required: 
          - "property2a"
          - "property2b"
        properties: 
          property2a: 
            type: "string"
          property2b: 
            type: "string"
      Object: 
        type: "object"
        required: 
          - "id"
          - "type"
          - "properties"
        properties: 
          id: 
            type: "string"
          type: 
            type: "string"
            enum: 
              - "obj1"
              - "obj2"
          properties: 
            oneOf: 
              - 
                $ref: "#/components/schemas/Object1Properties"
              - 
                $ref: "#/components/schemas/Object2Properties"
            discriminator: 
              propertyName: "type"
              mapping: 
                obj1: "#/components/schemas/Object1Properties"
                obj2: "#/components/schemas/Object2Properties"
    requestBodies:
      Object: 
        description: "Object schema in request body"
        content: 
          application/json: 
            schema: 
              $ref: "#/components/schemas/Object"
    parameters: 
      objectId: 
        name: "objectId"
        in: "path"
        required: true
        description: "Id of the object"
        schema: 
          type: "string"
    responses: 
      defaultError: 
        description: "Unexpected error"
        content: 
          application/json: 
            schema: 
              $ref: "#/components/schemas/Error"
  paths: 
    /object/{objectId}:
      post:
        summary: "Create an object"
        operationId: "createObject"
        parameters: 
          - 
            $ref: "#/components/parameters/objectId"
        tags: 
          - "objects"
        requestBody: 
          $ref: "#/components/requestBodies/Object"
        responses: 
          201: 
            description: "Object created"
            content: 
              application/json: 
                schema: 
                  $ref: "#/components/schemas/Object"
          default: 
            $ref: "#/components/responses/defaultError"

Example of validator server using express-openapi-validator is attached.
validation_server.zip

Any help is much appreciated!

@tedepstein
Copy link
Contributor

tedepstein commented Feb 16, 2020

@mewalig, this project only maintains the written OpenAPI Specification and a few other supporting artifacts. No OpenAPI-related tooling lives here.

Issues related to API documentation components, UIs, editors, programming frameworks, or other OpenAPI implementations should be posted in the appropriate project repository or directed the provider. You might find the appropriate project or product on the Implementations page.

That said, I think your issue also contains a relevant question about discriminators, and how to define them in your schemas. If you wouldn't mind changing the title of this issue to change the focus to the specification-related usage question, that will help us manage the issue, and might help get the attention of community members with relevant knowledge.

Looking at your example, I noticed that your discriminator is defined on the properties property of Object, while your examples have the type property value defined on the Object itself. I wouldn't expect your discriminator to recognize this type property on Object:

{
  "id": "hi",
  "type": "obj2",
  "properties": {
    "property1a": 1,
    "property1b": 2
  }
}

... but it might recognize this type property, defined on the properties object:

{
  "id": "hi",
  "properties": {
    "type": "obj2",
    "property1a": 1,
    "property1b": 2
  }
}

Assuming you want to keep type on the root Object, I think this would be a more idiomatic way to define it, using allOf on the subtypes:

# Top-level schema included so you can play with this in a standard JSON Schema validator,
# like http://jsonschmalint.com
$ref: "#/components/schemas/Object"

components:
  schemas:
    # This schema ensures that the object is one of the known subtypes. 
    # We can't put this in ObjectBaseType, because that creates a cycle with allOf, 
    # causing infinite recursion. 
    Object:
      oneOf:
      - $ref: "#/components/schemas/Object1"
      - $ref: "#/components/schemas/Object2"

    # Abstract supertype
    ObjectBaseType: 
      type: "object"
      required: 
        - "id"
        - "type"
      properties: 
        id: 
          type: "string"
        type: 
          type: "string"
      discriminator: 
        propertyName: "type"
        mapping: 
          obj1: "#/components/schemas/Object1"
          obj2: "#/components/schemas/Object2"

    # Concrete Subtypes
    Object1:
      # Inherit all constraints defined in the base type
      allOf:
      - $ref: "#/components/schemas/ObjectBaseType"
      # Additionally require subtype-specific properties.
      required:
      - subtypeProperties
      # Disallow properties that are not defined here.
      additionalProperties: false
      properties:
        # Redeclare inherited properties to allow them with additionalProperties: false
        # No additional constraints on id
        id: {}
        # Require type==obj1, so a standard JSON schema validator can discriminate.
        # This duplicates the discriminator logic, so should not be required with 
        # an OAS-compliant validator.
        type: 
          enum:
          - obj1
        # Define subtype-specific properties
        subtypeProperties:
          type: object
          additionalProperties: false
          required: 
          - "property1a"
          - "property1b"
          properties:
            property1a: 
              type: "string"
            property1b: 
              type: "string"

    Object2:
      # Same idea here...
      allOf:
      - $ref: "#/components/schemas/ObjectBaseType"
      required:
      - subtypeProperties
      additionalProperties: false
      properties:
        id: {}
        type: 
          enum:
          - obj2
        subtypeProperties:
          type: object
          additionalProperties: false
          required: 
          - "property2a"
          - "property2b"
          properties:
            property2a: 
              type: "string"
            property2b: 
              type: "string"

You might find the examples and explanations in the OpenAPI spec helpful too:

@mewalig
Copy link
Author

mewalig commented Feb 16, 2020

Thank you @tedepstein, that's really helpful.

Before I get too far into the nitty gritty, I just want to make sure I'm understanding correctly in general.

I think what you're saying in general is that if I change the structure of the API, I can get OpenAPI to describe a schema that can achieve the same purpose as my use case requires-- albeit through a different data structure-- but that as is OpenAPI does not support the schema as originally described.

In other words, if I am understanding this right, there is no way for OpenAPI to support a schema such that, without change, Test Case 1 passes and Test Case 3 fails, but, I could come up with a different schema, with different corresponding test cases, that could achieve the same purpose of having an object with two subclasses and different related properties.

Does that sound like a correct interpretation in general?

@tedepstein
Copy link
Contributor

@mewalig, you can create a schema that exactly models the example JSON instances you provided. In fact, the schema I posted matches your structure, except I renamed your properties property to subtypeProperties. I only did this to avoid confusion as I was writing and debugging that schema.

@mewalig
Copy link
Author

mewalig commented Feb 16, 2020

ah, I see, thank you for the clarification

@tedepstein
Copy link
Contributor

It was a problem with the indentation level of discriminator. It was indented one level too deep, making it a property. I have corrected the example above.

@mewalig
Copy link
Author

mewalig commented Feb 16, 2020

I see, I thought it was just extraneous because right after I posted, I got it working by deleting the "discriminator" section altogether, seeing as how the rest of the spec enforces it without it.

And the validators work properly!

Given this experience, then, assuming as has been mentioned that, there is nothing that discriminator does that can't already be done otherwise with plan JSON Schema, I would agree with #2143 that discriminator should be removed. It's definitely more confusing with it than without.

If it had never existed, and the OpenAPI documentation's Cat/Dog example used this JSON Schema approach instead, I would have immediately understood how to use it. What I am saying is, "discriminator" adds zero positive value because it's not easier to use, and it adds plenty of negative value because it is confusing and validators don't work with it (perhaps this is just my personal experience, but part of the reason I find it easier to understand using the alternative approach is that it directly parallels the way the same concept would be implemented in a relational database, and the task of applying polymorphism to relational databases is one that has been around and widely discussed / used / etc for a long time).

So, yes, please, by all means, get rid of discriminator. Its absence would have saved me a ton of time and frustration.

@tedepstein
Copy link
Contributor

tedepstein commented Feb 16, 2020

I think we will need to hear from some providers and consumers of code generators. There may be some code generators that recognize inheritance hierarchies based on discriminators, and might not (yet) have an easy path to do this without the discriminator property.

@mewalig
Copy link
Author

mewalig commented Feb 17, 2020

I agree. That said, the fact that no validator enforcing the discriminator seems to exist makes me wonder whether any code generator would have relied on it. It just seems unlikely that anyone going through the trouble of generating code with that feature would not require, as a part of the process, a testing framework that required validation. Perhaps they didn't care about that test coverage, or perhaps they built their own validator and kept it non-public or I just wasn't able to find it-- am certainly not saying you shouldn't first gauge the community opinion, and am certainly willing to place some bets on how that will turn out!

Either way, thank you for your responses, which have been far more helpful than the many hours I have spent trying to find / configure validators to enforce the discriminator.

@tedepstein
Copy link
Contributor

Glad to hear it was helpful!

@tedepstein
Copy link
Contributor

@mewalig, it looks to me like this issue is resolved, so I'll close it for now. If you feel the issue isn't really resolved, please feel free to reopen the issue and provide further comments.

@mewalig
Copy link
Author

mewalig commented Feb 18, 2020

👍

@NathanHazout
Copy link

@mewalig I’m looking into this as well.
Did you find validators that support discrimination?

I’m using NodeJS, if it matters

@mewalig
Copy link
Author

mewalig commented Jun 25, 2020

@nasht00 No, I did not. Not a single one in JS or Python.

Since I needed the scalability of auto-generated validation, but discriminator wasn't supported by any validators, I decided to abandon discriminator and use its pure-JSON-schema equivalent in a form similar to what @tedepstein had suggested above. Once that was done, I believe the first two of two JS validators I tried worked fine (but I can't remember which, as I ended up using python's fastjsonschema instead).

It would not be hard to write a converter as described above to convert schema-with-discriminator to pure-JSON-schema where the latter could be used in standard validators. It would be better to do so with some community agreement on how that would work, which is precisely what I was trying to get to in the discussion at #2143 (comment), but even without that broader acceptance, if we can get at least a few (maybe 3?) people (including me) willing to be involved in such an effort, I would be happy to write and/or contribute to the writing of such a converter.

Alternatively, one could try to find some way to support discriminator validation directly e.g. by modifying an existing validator codebase, but my sense is that the shortest path to implementing that would likely end up just being a one-way converter that is embedded in the validator's internals rather than a standalone codebase.

Let me know if you (or others you know) might be interested in working on a converter together. Otherwise / until others are (or validators support them), I'll just stick to avoiding discriminator.

@NathanHazout
Copy link

Did you see cdimascio/express-openapi-validator#323 ?
I did not try yet as I pushed this problem for later, but maybe you’ll understand the comment better than me

@mewalig
Copy link
Author

mewalig commented Jun 26, 2020

Unless something has recently changed, that validator does not support discriminator. The comment in cdimascio/express-openapi-validator#95 (comment) says "Yes", but then it says "indirectly" and goes on to say it only supports when "removing discriminator in v3 should always result in the same validation outcome". In other words, when it sees "discriminator", it does not break, but it simply ignores it. From a practical standpoint, as far as I'm concerned, "ignoring" is definitely not the same as "supporting"-- i.e. it means "No", it does not support.

@sachinsaxena-okta
Copy link

@mewalig, this project only maintains the written OpenAPI Specification and a few other supporting artifacts. No OpenAPI-related tooling lives here.

Issues related to API documentation components, UIs, editors, programming frameworks, or other OpenAPI implementations should be posted in the appropriate project repository or directed the provider. You might find the appropriate project or product on the Implementations page.

That said, I think your issue also contains a relevant question about discriminators, and how to define them in your schemas. If you wouldn't mind changing the title of this issue to change the focus to the specification-related usage question, that will help us manage the issue, and might help get the attention of community members with relevant knowledge.

Looking at your example, I noticed that your discriminator is defined on the properties property of Object, while your examples have the type property value defined on the Object itself. I wouldn't expect your discriminator to recognize this type property on Object:

{
  "id": "hi",
  "type": "obj2",
  "properties": {
    "property1a": 1,
    "property1b": 2
  }
}

... but it might recognize this type property, defined on the properties object:

{
  "id": "hi",
  "properties": {
    "type": "obj2",
    "property1a": 1,
    "property1b": 2
  }
}

Assuming you want to keep type on the root Object, I think this would be a more idiomatic way to define it, using allOf on the subtypes:

# Top-level schema included so you can play with this in a standard JSON Schema validator,
# like http://jsonschmalint.com
$ref: "#/components/schemas/Object"

components:
  schemas:
    # This schema ensures that the object is one of the known subtypes. 
    # We can't put this in ObjectBaseType, because that creates a cycle with allOf, 
    # causing infinite recursion. 
    Object:
      oneOf:
      - $ref: "#/components/schemas/Object1"
      - $ref: "#/components/schemas/Object2"

    # Abstract supertype
    ObjectBaseType: 
      type: "object"
      required: 
        - "id"
        - "type"
      properties: 
        id: 
          type: "string"
        type: 
          type: "string"
      discriminator: 
        propertyName: "type"
        mapping: 
          obj1: "#/components/schemas/Object1"
          obj2: "#/components/schemas/Object2"

    # Concrete Subtypes
    Object1:
      # Inherit all constraints defined in the base type
      allOf:
      - $ref: "#/components/schemas/ObjectBaseType"
      # Additionally require subtype-specific properties.
      required:
      - subtypeProperties
      # Disallow properties that are not defined here.
      additionalProperties: false
      properties:
        # Redeclare inherited properties to allow them with additionalProperties: false
        # No additional constraints on id
        id: {}
        # Require type==obj1, so a standard JSON schema validator can discriminate.
        # This duplicates the discriminator logic, so should not be required with 
        # an OAS-compliant validator.
        type: 
          enum:
          - obj1
        # Define subtype-specific properties
        subtypeProperties:
          type: object
          additionalProperties: false
          required: 
          - "property1a"
          - "property1b"
          properties:
            property1a: 
              type: "string"
            property1b: 
              type: "string"

    Object2:
      # Same idea here...
      allOf:
      - $ref: "#/components/schemas/ObjectBaseType"
      required:
      - subtypeProperties
      additionalProperties: false
      properties:
        id: {}
        type: 
          enum:
          - obj2
        subtypeProperties:
          type: object
          additionalProperties: false
          required: 
          - "property2a"
          - "property2b"
          properties:
            property2a: 
              type: "string"
            property2b: 
              type: "string"

You might find the examples and explanations in the OpenAPI spec helpful too:

@tedepstein I tried to follow the approach you mentioned here. I have one more special case where discriminator value can be null too and in that case I want to map it to let's say BaseType. Not sure if there is a way to handle that in openapi spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants