[Typescript-Node] Incorrect serialization of subclasses in generated code

[gbrown-ce]

Description

When using a generated client, serialization only looks at the referenced type when serializing properties of the object. If you are using inheritance, you will lose properties of the subclasses when you serialize.

Openapi-codegen version

3.0.0

Swagger declaration file content or url

swagger: "2.0"
info:
  version: "1.0.0"
  title: Price Lists Service
# during dev, should point to your local machine
host: localhost:10010
# basePath prefixes all resource paths 
basePath: /
# 
schemes:
  # tip: remove http to make production-grade
  - http
# format of bodies a client can send (Content-Type)
consumes:
  - application/json
# format of the responses to the client (Accepts)
produces:
  - application/json
paths:
  /pricelists/{priceListId}/pricepoints/{pricePointId}:
    x-swagger-router-controller: pricePoints
    put:
      description: Updates a price point
      operationId: updatePricePoint
      parameters:
        - name: priceListId
          in: path
          description: Unique id of the price list to retrieve
          required: true
          type: integer
          format: int64
          minimum: 0
        - name: pricePointId
          in: path
          description: Unique id of the price point to update
          required: true
          type: integer
          format: int64
          minimum: 0
        - name: pricePoint
          in: body
          description: Price point to update
          required: true
          schema:
            $ref: "#/definitions/UpdatePricePointRequest"
      responses:
        200:
          description: Success
          schema:
            $ref: "#/definitions/PricePoint"
        400:
          description: Bad Request
          schema:
            $ref: "#/definitions/BadRequestResponse"
        404:
          description: Not Found
          schema:
            $ref: "#/definitions/NotFoundResponse"
        409:
          description: Conflict
          schema:
            $ref: "#/definitions/ConflictResponse"
        default:
          description: Error
          schema:
            $ref: "#/definitions/ErrorResponse"
      description: Deletes a price point
      operationId: deletePricePoint
      parameters:
      - name: priceListId
        in: path
        description: Unique id of the price list
        required: true
        type: integer
        format: int64
        minimum: 0
      - name: pricePointId
        in: path
        description: Unique id of the price point to delete
        required: true
        type: integer
        format: int64
        minimum: 0
      responses:
        204:
          description: Success
          schema:
            type: string
        400:
          description: Bad Request
          schema:
            $ref: "#/definitions/BadRequestResponse"
        404:
          description: Not Found
          schema:
            $ref: "#/definitions/NotFoundResponse"
        default:
          description: Error
          schema:
            $ref: "#/definitions/ErrorResponse"
  /swagger:
    x-swagger-pipe: swagger_raw
# complex objects have schema definitions
definitions:
    type: object
    properties:
      restrictions:
        $ref: "#/definitions/PricePointRestrictions"
  PricePointRestrictions:
    type: array
    items:
      $ref: "#/definitions/PricePointRestriction"
  PricePointRestriction:
    type: object
    discriminator: restrictionType
    required:
      - restrictionType
    properties:
      restrictionType:
        type: string
        enum:
        - EntityRestriction
  EntityRestriction:
    type: object
    allOf:
      - $ref: "#/definitions/PricePointRestriction"
      - required:
          - entityId
          - entityType
        properties:
          entityId:
            type: string
          entityType:
            type: string
            format: string
            enum:
            - sku
      type: object
    required:
    - entityId
    - entityType
    - price
    properties:
      entityId:
        type: string
      entityType:
        type: string
        format: string
        enum:
        - product
      price:
        type: number
        format: double
        minimum: 0.00
  PricePoint:
    type: object
    required:
    - id
    - entryId
    - pricePointType
    - restrictions
    - price
    properties:
      id:
        type: integer
        format: int64
        minimum: 0
      entryId:
        type: integer
        format: int64
        minimum: 0
      pricePointType:
        type: string
        format: string
        enum:
        - addition
        - override
      restrictions:
        type: array
        items:
          $ref: "#/definitions/PricePointRestriction"
      price:
        type: number
        format: double
        minimum: 0.00
    type: object
    required:
      - pricePointType
      - restrictions
      - price
    properties:
      pricePointType:
        type: string
        format: string
        enum:
        - addition
        - override
      restrictions:
        type: array
        minItems: 1
        items:
          $ref: "#/definitions/PricePointRestriction"
      price:
        type: number
        format: double
        minimum: 0.00
  UpdatePricePointRequest:
    type: object
    required:
      - pricePointType
      - restrictions
      - price
    properties:
      pricePointType:
        type: string
        format: string
        enum:
        - addition
        - override
      restrictions:
        type: array
        minItems: 1
        items:
          $ref: "#/definitions/PricePointRestriction"
      price:
        type: number
        format: double
        minimum: 0.00
  BadRequestResponse:
    type: object
    required:
    - errorCode
    - messages
    properties:
      errorCode:
        type: string
        example: "400.5.2.0"
      messages:
        type: array
        items:
          type: string
        example:
        - Message here about why the request is invalid
        - Another message here about why the request is invalid
  ConflictResponse:
    type: object
    required:
    - errorCode
    - messages
    properties:
      errorCode:
        type: string
        example: "409.5.2.0"
      messages:
        type: array
        items:
          type: string
        example:
        - Name 'EntryType' is already in use
  ErrorResponse:
    type: object
    required:
    - errorCode
    - messages
    properties:
      errorCode:
        type: string
        example: "500.5.2.0"
      messages:
        type: array
        items:
          type: string
        example:
        - "Something went wrong"
  NotFoundResponse:
    type: object
    required:
    - errorCode
    - messages
    properties:
      errorCode:
        type: string
        example: "404.5.2.0"
      messages:
        type: array
        items:
          type: string
        example:
        - Not Found
  ServiceUnavailableResponse:
    type: object
    required:
      - errorCode
      - messages
    properties:
      errorCode:
        type: string
        example: "503.5.2.0"
      messages:
        type: array
        items:
          type: string
        example:
        - Service is currently unavailable

Command line used for generation

java -jar .\openapi-generator-cli.jar generate -i /path/to/swagger.yaml -l typescript-node -o /path/to/target/directory

Steps to reproduce

With the generated code, run the following:

import { 
    DefaultApi, UpdatePricePointRequest, PricePointRestriction, EntityRestriction 
} from "/path/to/target/directory";

var api:DefaultApi = new DefaultApi();
var updatePricePointRequest: UpdatePricePointRequest = new UpdatePricePointRequest();
updatePricePointRequest.price = 0
updatePricePointRequest.pricePointType = UpdatePricePointRequest.PricePointTypeEnum.Override;
updatePricePointRequest.restrictions = [
    {
       restrictionType: PricePointRestriction.RestrictionTypeEnum.EntityRestriction,
       entityId: 'Success',
       entityType: EntityRestriction.EntityTypeEnum.Sku
    }
];
api.updatePricePoint(0, 0, updatePricePointRequest);

If you step into the updatePricePoint call to where it goes to serialize the list of restrictions, you'll notice that entityId and entityType get left off the serialized object, so that when it is sent onto the server, it is invalid.

[TiFu]

Where are entityId and entityType defined in the spec?
IMO if some properties are not part of the API contract defined in the open api spec, they should not be included in the request.

The subclass case is interesting. The spec requires the described behavior (i.e. send the sub class) in the case of a oneOf relationship and the serialization of all valid schemas in the case of anyOf and allOf.

It looks like the serializer just does not take into account oneOf,anyOf and allOf at all.

[gbrown-ce]

@TiFu sorry, my mistake.. I boiled this spec down from a larger one, and I seemed to have left parts out. Let me fix.

[gbrown-ce]

@TiFu fixed.

[TiFu]

Thanks!

Why does EntityRestriction inherit from PricePointRestriction? Shouldn't this be the other way around? i.e. PricePointRestriction has allOf EntityRestriction?
I'd argue that those properties should be stripped as they are not part of PricePointRestriction and it would be invalid to send them to the server.

But this shows another issue: I think this might also happen if the allOf relationship is the other way around (so PricePointRestriction has allOf EntityRestriction).

The root cause for this seems to be the handling of anyOf, allOf and oneOf in the ObjectSerializer. A proper fix for this isn't trivial, because the Serializer does not take those three options into account (unless they are somehow implemented in the object model). Supporting this requires a partial rewrite of the serializer so that it supports requests like matchesType(object, type) so that we can properly check anyOf, oneOf and allOf. This then needs to be combined with the current serialization logic, so that no properties are lost.
For the record: I have also added a comment to #102 about this issue.

[gbrown-ce]

@TiFu PricePointRestriction is the base class.

So this is in the generated client, so at this point oneOf/allOf/anyOf aren't really things. The classes have already been generated outwith their given properties, inheritance tree, and the discriminator values, etc. So I actually think this is correct for handling the subclassed case.

At this point in the process I believe we are looking at regular old polymorphic objects with the discriminator for a little help.

[TiFu]

Thanks for the clarification. Let's ignore the oneOf, anyOf and allOf stuff I said for now. That actually is an unrelated issue.

I'm torn on what the correct behavior in this case is. There are basically two options:

1. The spec above defines that the endpoint accepts an array of PricePointRestriction - which only has one property restrictionType and the server may only rely on receiving that property.
2. Allow passing sub class objects with all properties to the method - the server then also needs to know the reverse mapping from subclass to parent class in order to properly deserialize the data (i.e. would need to use a discriminator to know which type the sent object actually has).

IMO 2. feels messy and is bad design. The endpoint includes knowledge about subclasses which might not yet exist (imagine if you add another type of restriction), so adding a new subclass of PrizePointRestriction might break the endpoint.

I guess there's a reason why EntityRestriction is a sub class of a PrizePointRestriction? This relation feels weird because Entity is more general than PrizePoint.
If you reverse the inheritance hierarchy and make EntityRestriction a parent class of PrizePointRestriction, PrizePointRestriction then has all properties of EntityRestriction and there should be no issue.

[gbrown-ce]

I mean, yes the server would need to know about both types and use the discriminator possibly, but such is inheritance.

In our business domain, EntityRestriction is more specific. Really a better name would be "EntityPricePointRestriction", which is a specific type of PricePointRestriction that adds on an entityId and entityType.

If we went with #1 (the current behavior) that would essentially eliminate the ability to use polymorphism in an Swagger 2.0 spec... which really isn't great.

If my understanding of OpenAPI 3.0 is correct, you could use "anyOf" (provided the serializer supported it) rather than inheritance. I don't really see a good reason to disallow the inheritance though.

[TiFu]

Oh okay. I see your point now. Sorry for being so slow xD.
You're totally right. That's a valid use case. Thanks for having so much patience with me^^.