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

[BUG] [typescript-node] Models not generated properly using oneOf #10145

Open
5 of 6 tasks
sarumont opened this issue Aug 12, 2021 · 1 comment
Open
5 of 6 tasks

[BUG] [typescript-node] Models not generated properly using oneOf #10145

sarumont opened this issue Aug 12, 2021 · 1 comment

Comments

@sarumont
Copy link
Contributor

sarumont commented Aug 12, 2021

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

First off - there are many issues dancing around this issue (I've spent half a day trying to identify the rhyme/reason for the current behavior). None, however, specifically target the typescript-node generator. The typescript-* generators share no templates, though it appears the new typescript generator has some copypasta from typescript-node.

There are two issues I'm addressing here:

  1. Models specified using oneOf are not generated properly
  2. ObjectSerializer does not properly handle discriminator

Point 2 is fairly self-explanatory, but I'll elaborate: the discriminator property allows for a mapping object. The current implementation assumes the discriminator property is the name of a class:

var discriminatorType = data[discriminatorProperty];
if(typeMap[discriminatorType]){
    return discriminatorType; // use the type given in the discriminator
} else {
    return expectedType; // discriminator did not map to a type
}

Point 1 is a bit gnarlier. In the attached example, the class Message is generated as a superset of ALL subclass properties. This makes it a big damn mess and could lead to unexpected results in the event that subclasses define properties using the same names. If you attempt to call ObjectSerializer.deserialize(data, 'Message'), it deserializes what should be a FooMessage as a Message. If you don't care about the typing, however, the properties are all there, at least.

openapi-generator version

5.1.1, main (5.2.1-SNAPSHOT as of this writing)

OpenAPI declaration file content or url
openapi: 3.0.3
info:
  description: Example
  version: '1.2.3'
  title: 'example'
paths:
  /foo:
    get:
      tags:
        - foo
      operationId: getFoo
      responses:
        '204':
          description: Foo
components:
  schemas:
    MessageOperation:
      description: Message operation
      type: string
      enum:
        - foo_op
        - bar_op

    Message:
      type: object
      description: Any message 
      oneOf:
        - $ref: '#/components/schemas/FooMessage'
        - $ref: '#/components/schemas/BarMessage'
      discriminator:
        propertyName: operation
        mapping:
          foo_op: '#/components/schemas/FooMessage'
          bar_op: '#/components/schemas/BarMessage'

    MessageBase:
      type: object
      description: Base class for a message
      required:
        - id
        - operation
        - error
        - timestamp
      properties:
        id:
          type: string
          format: uuid
          description: A unique ID for this message
        operation:
          description: The operation to be performed
          $ref: '#/components/schemas/MessageOperation'
        error:
          type: boolean
          default: false
          description: Indicates whether or not this is an error message
        timestamp:
          type: string
          format: date-time
          description: The time when this message was sent

    FooMessage:
      description: A Foo message
      type: object
      allOf:
        - $ref: '#/components/schemas/MessageBase'
        - type: object
          required:
            - foo
          properties:
            foo: 
              type: string

    BarMessage:
      description: A Bar message
      type: object
      allOf:
        - $ref: '#/components/schemas/MessageBase'
        - type: object
          required:
            - bar
          properties:
            bar: 
              type: string
Generation Details

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g typescript-node -i ./poly.yaml -o tmp/

Steps to reproduce

Generate code

Related issues/PRs

#6376 #9305 #4374 #15 #10017 #6513

Suggest a fix

My proposed solution here is two-fold:

  1. Generate a typedef for so-called tagged union types. The typescript-axios generator does this currently, and I believe it results in much cleaner generated code. Depending on how people use the generated code, this will be a breaking change (if anyone is using the base class).
  2. Encapsulate the contents of the discriminator.mapping into ObjectSerializer to allow deserializing an e.g. Message into FooMessage or BarMessage, as determined by the value of discriminator. The current behavior can be kept as a fallback.

I'm currently working on a PR for these but would like some feedback from the TS team. :)

CC @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)

@sarumont
Copy link
Contributor Author

sarumont commented Aug 12, 2021

As I'm working through this, I've found a corner case here...

If a model uses oneOf AND specifies properties, it is not a true tagged union. I'm not sure how to treat this case - I think it is valid, as per the OAPI spec, but it doesn't make any sense to me to do this. Could handle it in a couple ways:

  • generate a typedef and duplicate the common properties to each of the sub models
  • generate the base with just its properties and also generate a typedef i.e. BaseType which would then resemble: const type BaseType = (Foo & Base) | (Bar & Base);

sarumont added a commit to sarumont/openapi-generator that referenced this issue Oct 5, 2021
This allows (de)serializing models using `oneOf` via the union type
name, returning the proper sub-type. Part of OpenAPITools#10145.
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

1 participant