stoplightio / prism

Turn any OpenAPI2/3 and Postman Collection file into an API server with mocking, transformations and validations.
https://stoplight.io/open-source/prism
Apache License 2.0
4.35k stars 349 forks source link

Discriminator + Mapping not working properly #2345

Closed LasneF closed 10 months ago

LasneF commented 1 year ago

When combining Discriminator and mapping , validation looks not working (mapping not taken into account)

Context

given the above API spec defining

Pet (Id , type) and 2 kind of cat both beeing pet NiceCat (having having miaou field) hunterCat (having hunter field)

Here is the spec

Notice that the PetResult has a discriminator , if petType match NICE schema have to be niceCatResult , if petType match HUNT schema is hunterCatResult

openapi: 3.1.0
info:
  title: Curves
  version: "1.0"

paths:
  /pet:
    get:
      responses:
        '200':
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/PetResult'
              example:
                petType: NICE
                miaou: true
components:
  schemas:
    PetResult:
      oneOf:
        - $ref: '#/components/schemas/niceCatResult'
        - $ref: '#/components/schemas/hunterCatResult'    
      discriminator:
        propertyName: petType
        mapping:
          NICE: '#/components/schemas/niceCatResult'
          HUNT: '#/components/schemas/hunterCatResult'

    PetCommon:
      type: object
      required: 
        - petType
      properties:
        petType:
          type: string

    niceCatResult:
      allOf:
        - $ref: '#/components/schemas/PetCommon'
        - type: object
          required:
            - miaou
          properties:
            miaou:
              type: boolean
    hunterCatResult:
      allOf:
        - $ref: '#/components/schemas/PetCommon'
        - type: object
          properties:
            hunt:
              type: string

Current Behavior

the NiceCat looks mapping hunter cat even if it is set as Nice

[CLI] ► start Prism is listening on http://127.0.0.1:4010 [17:05:38] » [HTTP SERVER] get /pet info Request received

Prism raise [17:01:24] » [VALIDATOR] × error Violation: response.body Request body must match exactly one schema in oneOf

Looks Prism does not leverage on the petType is set to NICE, and so matching the NICE cat model

Expected Behavior

during payload validation the mapping should be taken into account to pickup the right schema

Possible Workaround/Solution

Not sure it is a bug or a limitation, if it is a limitation , this could be mentionned in the documentation that the mapping keyword has no effect upon validation, and so that models in th oneOf must be imcompatible each others here nice cat could be also hunter cat .

Steps to Reproduce

take the above Spec,

prism mock spec.yml curl --location 'http://127.0.0.1:4010/pet'

an error trace occurs [17:05:38] » [VALIDATOR] × error Violation: response.body Request body must match exactly one schema in oneOf

Notice same appears in proxy validation mode

Environment

Prism 5.2.0

chohmann commented 1 year ago

@LasneF Prism does not support discriminators at this time, and there's no plan to support them in the foreseeable future. We will turn this into a documentation task and get this limitation listed in Prism documentation.

aleung commented 11 months ago

Ajv has already partially supported the discriminator feature since version 8.9.0, although there are some limitations.

I have tested that Prism also supports the discriminator feature as long as the schemas adhere to the requirements documented by Ajv.

aleung commented 10 months ago

Correct my previous message: discriminator can be supported with some limitations, and it have to use option discriminator: true with Ajv constructor - it is not enabled by default.

I had modified prism code to add this option when I did the test. https://github.com/stoplightio/prism/blob/a1360b32160f1b27c1ff08894e7ce992cccf8d43/packages/http/src/validator/validators/utils.ts#L28

const baseAjvOptions = {
    allErrors: true,
    allowUnionTypes: true,
    allowMatchingProperties: true,
    strict: false,
    discriminator: true,             // added
    logger: unknownFormatSilencerLogger,
};

@chohmann Would you like to accept a PR to add the discriminator option?

chohmann commented 10 months ago

@aleung we will gladly accept a PR for this change!

Can you be sure to include the following in the proposed PR:

  1. tests: unit and harness tests
  2. take a pass at the docs and update with the change in support for discriminator where appropriate
  3. ensure that it fits the entire above use case outlined in this ticket (discriminator and mapping)
aleung commented 10 months ago

@chohmann

Unfortunately, Ajv does not support all discriminator features, and mapping is included in the list of unsupported features. The limitations are documented here. However, it is possible to modify the API definition as a workaround by not using mapping, but instead using the const keyword to set the value of the discriminator property in each subschema.

Without the discriminator: true option, the discriminator keyword is completely ignored.

I believe the Prism document can refer to the Ajv documentation for information on the constraints of using the discriminator keyword.

I'll propose a PR if that's acceptable.

chohmann commented 10 months ago

@aleung Thanks for explaining. Yes, we'd be ok with the approach you outlined. We just ask that the docs gets updated to clearly show how to get around the lack of mapping support. Thank you!

aleung commented 10 months ago

After further reading documents and testing, I have found that AJV's discriminator support is not as useful as initially thought.

AJV just make the discriminator keyword a no-op from the validation point of view. The actual validation occurs as defined in the oneOf keyword. The discriminator keyword simply modifies the error message.

There have been discussions (OAI/OpenAPI-Specification#2143, OAI/OpenAPI-Specification#2141) regarding the potential deprecation of the discriminator feature in the OpenAPI Specification.

Now, I am leaning towards not using the discriminator + mapping at all. Alternatively, I could create a converter that transforms it into a pure JSON schema equivalent (single-valued enum or OAS 3.1 const) for the validator.

chohmann commented 10 months ago

@aleung thanks for all your research into this topic! We'll close the issue again until a new proposal for how to support this almost deprecated keyword is found.