Skip to content
This repository has been archived by the owner on Nov 11, 2023. It is now read-only.

Invalid interface generated by import when allOf is used with an object type #317

Open
DavidGReid opened this issue Nov 5, 2020 · 5 comments

Comments

@DavidGReid
Copy link

Describe the bug
The code generated by importing an OpenAPI definition that featured a requestBody using allOf to combine multiple types, including an object type, is invalid, and the fixed version of that code does not match the semantics of the OpenAPI definition.

To Reproduce
I imported the following OpenAPI definition

openapi: 3.1.0
info:
    title: Customer API
    version: "1.0"
    description: ""
paths:
    /customers/{CustomerId}:
        patch:
            operationId: api.customers.update
            tags:
                - Customer
            requestBody:
                description: Update customer with properties to be changed
                content:
                    application/json:
                        schema:
                            allOf:
                                - $ref: "#/components/schemas/CustomerProperties"
                                - $ref: "#/components/schemas/CustomerRequiredProperties"
                                - type: object
                                  properties:
                                      Segment:
                                          nullable: true
            parameters:
                - $ref: "#/components/parameters/CustomerId"
            responses:
                204:
                    description: Updated
components:
    schemas:
        CustomerProperties:
            type: object
            properties:
                FirstName:
                    type: string
                LastName:
                    type: string
                DOB:
                    type: string
                    format: date-time
                Segment:
                    type: string
                    enum:
                        - Young
                        - MiddleAged
                        - Old
                        - Moribund
        CustomerRequiredProperties:
            type: object
            required:
                - FirstName
                - LastName
                - DOB
        Id:
            type: integer
    parameters:
        CustomerId:
            name: CustomerId
            in: path
            required: true
            schema:
                $ref: "#/components/schemas/Id"

by running npx restful-react import --file customer-api.yaml --output customer-api.tsx.
The generated TypeScript output code included a broken interface definition:

export interface ApiCustomersUpdateRequestBody CustomerProperties & CustomerRequiredProperties & {
  Segment?: {} | null;
}

This should probably be defined as an intersection type instead to make it valid TypeScript:

export type ApiCustomersUpdateRequestBody = CustomerProperties & CustomerRequiredProperties & {
  Segment?: {} | null;
}

However, this type still does not behave as intended:

  1. Segment in ApiCustomersUpdateRequestBody cannot have null assigned to it.
  2. FirstName, LastName and DOB would still be optional.

Expected behavior
I expected the TypeScript generated by the import of the OpenAPI definition to be valid, and for the generated ApiCustomersUpdateRequestBody to have a nullable Segment property, and required FirstName, LastName and DOB properties.

Desktop (please complete the following information):

  • OS: macOS Catalina 10.15.7

Additional context
I was trying out the code from this guide, which is intended to solve the problem of lots of repetition in schemas for related endpoints, where properties can differ in terms of nullability or optionality across endpoints. The solution involves splitting schemas into reusable parts that are composed together using OpenAPI's allOf.

@fabien0102
Copy link
Collaborator

Nice catch! Thanks for the repro 👌

@fabien0102
Copy link
Collaborator

I need to check for this behavior of allOf and props merging, but the interface instead of the type should be fixed in latest

@fabien0102 fabien0102 reopened this Nov 6, 2020
@DavidGReid
Copy link
Author

I think it's quite a difficult problem to handle merging constraints from different schemas, where the base types for the properties are defined in one schema, but the required/nullable constraints come from another. The current approach of converting allOf to an intersection type doesn't seem workable.

interface Foo {
	a?: string;
}

interface Bar {
	a: any | null;
}

type Baz = Foo & Baz;
let x: Baz; // x.a is of type `any` rather than `string` | `null`

I imagine you could define a mapped type that could make specific properties nullable / required (type ApplyConstraints<BaseType, NullableProps, RequiredProps> = ...), but that probably wouldn't fit the current method of computing intermediate types and then combining them.

@fabien0102
Copy link
Collaborator

I will not call this a difficult problem, we "just" need to combine the specs before computing the type. I didn't payed enought attention to this part of specs. I would also love to keep the composition when possible, because this can add some context about the type so this is always good to have (and this is how this task become a more tricky 😁)

BTW, regarding this allOf feature, and types compositions, I'm not sure about the benefit, yes this will probably reduce your specs size, but not sure this is worth it, this will become a nightmare to review, since you need to merge the specs in your head on github and most of time with a part context due to the openAPI spec structure & github PR UX.
Don't get me wrong, I also hate code duplications in my software, but we are talking about a piece of documentation here. This was just to challenge a bit this nice article you sent, looks like a good idea but carreful of the side effects 😉

This said, I will totally enjoy this little allOf generation challenge 😁 🤘

@chrisahardie
Copy link

I believe anyOf also exhibits this undesired behaviour.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants