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

Discriminator + Mapping not working properly #2345

Closed
LasneF opened this issue Aug 1, 2023 · 8 comments
Closed

Discriminator + Mapping not working properly #2345

LasneF opened this issue Aug 1, 2023 · 8 comments

Comments

@LasneF
Copy link

LasneF commented Aug 1, 2023

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
Copy link
Contributor

chohmann commented Aug 4, 2023

@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
Copy link
Contributor

aleung commented Dec 21, 2023

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
Copy link
Contributor

aleung commented Jan 10, 2024

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.

const baseAjvOptions: Partial<Options> = {

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 chohmann reopened this Jan 12, 2024
@chohmann
Copy link
Contributor

@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
Copy link
Contributor

aleung commented Jan 13, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor

aleung commented Feb 1, 2024

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
Copy link
Contributor

chohmann commented Feb 2, 2024

@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.

@chohmann chohmann closed this as completed Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants