Skip to content

[BUG][typescript] oneOf + allOf breaks serialization #19868

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

Open
5 of 6 tasks
simon-abbott opened this issue Oct 14, 2024 · 7 comments
Open
5 of 6 tasks

[BUG][typescript] oneOf + allOf breaks serialization #19868

simon-abbott opened this issue Oct 14, 2024 · 7 comments

Comments

@simon-abbott
Copy link
Contributor

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

When using an API that uses a oneOf union containing several allOf elements, trying to send/receive data will fail with the error typeMap[type].getAttributeTypeMap is not a function. The underlying reason is that no discriminator value or mapping is generated for this value.

Prior to #19494 this worked fine, but as of 7.9.0 trying to run this schema will crash.

openapi-generator version

7.9.0, this is a regression.

OpenAPI declaration file content or url
openapi: 3.0.1
info:
  title: fruity
  version: 0.0.1
paths:
  /:
    get:
      responses:
        "200":
          description: get a fruit
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Fruit"
components:
  schemas:
    Apple:
      title: Apple
      type: object
      properties:
        variety:
          type: string
          description: The type of apple
      required:
        - variety
    Banana:
      title: Banana
      type: object
      properties:
        ripeness:
          type: number
          description: How ripe the banana is from 0 to 1
    Fruit:
      title: Fruit
      oneOf:
        - type: object
          allOf:
            - type: object
              properties:
                type:
                  type: string
                  enum:
                    - apple
            - $ref: '#/components/schemas/Apple'
          required:
            - type
        - type: object
          allOf:
            - type: object
              properties:
                type:
                  type: string
                  enum:
                    - banana
            - $ref: '#/components/schemas/Banana'
          required:
            - type
Generation Details

Generated with v7.9.0.

Steps to reproduce

Generate the above yaml using the typescript generator, then try to use the API. It will fail with typeMap[type].getAttributeTypeMap is not a function

Related issues/PRs

Not as far as I could tell.

Suggest a fix

I don't have an exact fix in mind since I don't fully understand how this mapping logic is supposed to work.

@ksvirkou-hubspot
Copy link
Contributor

This issue occurs because the schema described doesn't include a discriminator. Without a discriminator, it's challenging to find out which object the API returns.
If you add discriminator, it will work correctly
Example mapping with oneOf objects:

discriminator:
  propertyName: type
  mapping:
    apple: '#/components/schemas/FruitOneOf'
    banana: '#/components/schemas/FruitOneOf1'

Example mapping with Apple and Banana:

discriminator:
  propertyName: type
  mapping:
    apple: '#/components/schemas/Apple'
    banana: '#/components/schemas/Banana'

if type property contains name of classes "Apple" and "Banana" it will work without mapping
Example of response:

{"type":"Apple","variety":"good apples"}

Swagger file:

discriminator:
  propertyName: type

@ksvirkou-hubspot
Copy link
Contributor

ksvirkou-hubspot commented Oct 22, 2024

Example of full Swagger file
openapi: 3.0.1
info:
  title: fruity
  version: 0.0.1
paths:
  /:
    get:
      responses:
        "200":
          description: get a fruit
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Fruit"
components:
  schemas:
    Apple:
      title: Apple
      type: object
      properties:
        variety:
          type: string
          description: The type of apple
      required:
        - variety
    Banana:
      title: Banana
      type: object
      properties:
        ripeness:
          type: number
          description: How ripe the banana is from 0 to 1
    Fruit:
      title: Fruit
      oneOf:
        - type: object
          allOf:
            - type: object
              properties:
                type:
                  type: string
                  enum:
                    - apple
            - $ref: '#/components/schemas/Apple'
          required:
            - type
        - type: object
          allOf:
            - type: object
              properties:
                type:
                  type: string
                  enum:
                    - banana
            - $ref: '#/components/schemas/Banana'
          required:
            - type
      discriminator:
        propertyName: type
        mapping:
          apple: '#/components/schemas/FruitOneOf'
          banana: '#/components/schemas/FruitOneOf1'

@simon-abbott
Copy link
Contributor Author

simon-abbott commented Oct 22, 2024

Yeah, I know that's a workaround, but unfortunately I am dealing with a swagger file in the wild that doesn't specify one. I could patch it but I'd rather not maintain a patch against a complicated and changing 3rd party API if I don't have to. Additionally the OpenAPI v3 spec explicitly allows for discriminator-less schemas (note the use of MAY vs MUST):

In OAS 3.0, a response payload MAY be described to be exactly one of any number of types:

MyResponseType:
  oneOf:
  - $ref: '#/components/schemas/Cat'
  - $ref: '#/components/schemas/Dog'
  - $ref: '#/components/schemas/Lizard'

which means the payload MUST, by validation, match exactly one of the schemas described by Cat, Dog, or Lizard. In this case, a discriminator MAY act as a "hint" to shortcut validation and selection of the matching schema which may be a costly operation, depending on the complexity of the schema.

As seen here, if the discriminator is provided, it can (and probably should) be used to shortcut validation. But if it is missing, like in the example shown above, it should still run validation against the provided options.

@ksvirkou-hubspot
Copy link
Contributor

If the discriminator is not provided, the generator should create a "Franken-class" that combines the properties of each constituent part.
Is this the intended behavior?
@simon-abbott @joscha

@simon-abbott
Copy link
Contributor Author

simon-abbott commented Nov 5, 2024

From what I've read in the spec, I think the way to be fully spec-compliant if there is no discriminator is to list all the child types on the parent and whether it's an allOf or anyOf relationship (either some array/type pair or two arrays one for anyOf and one for allOf), and iterate over them to see which one (if any) matches.

Basically do the following pseudo-code:

func deserialize(data, type):
  if type has discriminator:
    return deserialize(data, type.discriminator)
  else if type has anyOf:
    for subtype of type.anyOf:
      if data matches subtype:
        return deserialize(data, subtype)
    throw error "no match found"
  else if type has allOf:
    match := null
    for subtype of type.allOf:
      if data matches subtype:
        if match != null:
          throw error "multiple matches found"
        else:
          match = subtype
    if match == null
      throw error "no match found"
    return deserialize(data, match)
  else:
    perform normal deserialization logic

Generating a franken-class is a good-enough workaround for my particular use-case though, so I would be happy enough to settle for that in the meantime.

@ksvirkou-hubspot
Copy link
Contributor

@joscha
What do you think?

@joscha
Copy link
Contributor

joscha commented Nov 19, 2024

What do you think?

I'd probably do the following:

  1. go back in version control to before the change, write a test case or fixture that shows the problem described by @simon-abbott
  2. Generate the fixture code and possibly add a consumer to a test in the repo if it's tricky to ensure correctness
  3. go back to HEAD
  4. Implement a fix that satisfies 1. and 2. Bonus points if that fix does not only remove the regression but also implements the spec as expected.

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

3 participants