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] (type-safe-api) (regression) Path param on path level not recognize properly #841

Closed
valebedu opened this issue Oct 6, 2024 · 6 comments · Fixed by #845
Closed
Labels
bug Something isn't working needs-triage

Comments

@valebedu
Copy link
Contributor

valebedu commented Oct 6, 2024

Describe the bug

With a shared parameters directly under the path instead of the method (to share path ID for example for get, put, delete methods) the ID model is generated but not imported in the runtime Api.ts file.

For example with this OpenAPI file:

openapi: 3.0.3
info:
  version: 1.0.0
  title: My API
paths:
  /hello:
    get:
      operationId: sayHello
      x-handler:
        language: typescript
      parameters:
        - in: query
          name: name
          schema:
            type: string
          required: true
      responses:
        200:
          description: Successful response
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/SayHelloResponseContent'
        500:
          description: An internal failure at the fault of the server
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/InternalFailureErrorResponseContent'
        400:
          description: An error at the fault of the client sending invalid input
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/BadRequestErrorResponseContent'
        403:
          description: An error due to the client not being authorized to access the resource
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/NotAuthorizedErrorResponseContent'
        # default:
        #   description: An error due to the client not being authorized to access the resource
        #   content:
        #     'application/json':
        #       schema:
        #         $ref: '#/components/schemas/ServiceUnavailableErrorResponseContent'
  /hello/{id}:
    parameters:
      - in: path
        name: id
        schema:
          $ref: '#/components/schemas/HelloId'
        required: true
    get:
      operationId: sayHelloId
      x-handler:
        language: typescript
      responses:
        200:
          description: Successful response
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/SayHelloResponseContent'
        500:
          description: An internal failure at the fault of the server
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/InternalFailureErrorResponseContent'
        400:
          description: An error at the fault of the client sending invalid input
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/BadRequestErrorResponseContent'
        403:
          description: An error due to the client not being authorized to access the resource
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/NotAuthorizedErrorResponseContent'
        # default:
        #   description: An error due to the client not being authorized to access the resource
        #   content:
        #     'application/json':
        #       schema:
        #         $ref: '#/components/schemas/ServiceUnavailableErrorResponseContent'
components:
  schemas:
    SayHelloResponseContent:
      type: object
      properties:
        id:
          $ref: '#/components/schemas/HelloId'
        message:
          type: string
      required:
        - message
    HelloId:
      type: string
      format: uuid
    InternalFailureErrorResponseContent:
      type: object
      properties:
        message:
          type: string
      required:
        - message
    ServiceUnavailableErrorResponseContent:
      type: object
      properties:
        message:
          type: string
      required:
        - message
    BadRequestErrorResponseContent:
      type: object
      properties:
        message:
          type: string
      required:
        - message
    NotAuthorizedErrorResponseContent:
      type: object
      properties:
        message:
          type: string
      required:
        - message

Expected Behavior

Path parameters should be handled same as method parameter

Current Behavior

HelloId is correctly generated in models/HelloId.ts but not imported in DefaultApi.ts so build fail.

Reproduction Steps

  1. Follow getting started with OpenAPI configuration
  2. Replace OpenAPI spec with the one specified previously as an example
  3. Build and observe error
  4. Move path param under get method
  5. Build and observe success

If you also try to uncomment default responses you'll also see another error, I use default in all my APIs for 503 responses and it worked before but now it is interpreted as 200 and cause a duplicate in OperationConfig.ts

It is not the same issue so if needed i can report another issue for it

Possible Solution

No time to look at a solution sorry but I'm pretty sure something change in a recent version because I didn't have any issue in v0.23.49

Additional Information/Context

No response

PDK version used

v0.23.55

What languages are you seeing this issue on?

Typescript

Environment details (OS name and version, etc.)

macOS 14.6.1, apple m1 pro, node 20.18.0, pnpm 8.15.9

@valebedu valebedu added bug Something isn't working needs-triage labels Oct 6, 2024
@valebedu valebedu changed the title [BUG] [REGRESSION] (type-safe-api) Path param on path level not recognize properly [BUG] (type-safe-api) (regression) Path param on path level not recognize properly Oct 7, 2024
@valebedu
Copy link
Contributor Author

valebedu commented Oct 7, 2024

Furthermore in a case of a schema using allOf the model fail to import components too in the model

Try with:

components:
  Template:
    allOf:
      - $ref: '#/components/schemas/TemplateBase'
      - $ref: '#/components/schemas/TemplateBody'
      - title: Template
        description: |
          Represents a complete template in the system.
  TemplateBase:
    type: object
    title: TemplateBase
    description: |
      Represents the base properties of a template.
    additionalProperties: false
    required:
      - id
    properties:
      id:
        $ref: '#/components/schemas/TemplateID'
  TemplateBody:
    type: object
    title: TemplateBody
    description: |
      Represents the body of a template.
    additionalProperties: false
    properties:
      parent_id:
        $ref: '#/components/schemas/TemplateID'
      boolean:
        type: boolean
        description: A boolean value.
  TemplateID:
    type: string
    format: uuid
    title: TemplateID
    description: The unique identifier for a template.
    maxLength: 36

And i got:

api-typescript-runtime: src/models/Template.ts(41,9): error TS2552: Cannot find name 'TemplateID'. Did you mean 'TemplateBody'?
api-typescript-runtime: src/models/Template.ts(65,15): error TS2552: Cannot find name 'TemplateIDFromJSON'. Did you mean 'TemplateFromJSON'?
api-typescript-runtime: src/models/Template.ts(78,15): error TS2552: Cannot find name 'TemplateIDToJSON'. Did you mean 'TemplateToJSON'?
api-typescript-runtime: 👾 Task "build » compile" failed when executing "tsc --build" (cwd: /Users/vale/Workspace/tsylana/api/template-api/packages/api/generated/runtime/typescript)

Again it's not the exact same error like the default error but i think it's related, so if needed i could create another issue

@cogwirrel
Copy link
Member

Hey Vale! Thanks for raising this!

Since 0.23.50 we've replaced openapi-generator with our own code generator for most of the typescript projects. While it's much faster and doesn't have the dependency on Java, it seems I've missed some edge cases here! I'll investigate the three problems you've raised in this issue, don't worry about separating them :)

For now, I'd recommend pinning at 0.23.49 so you're not blocked :)

cogwirrel added a commit that referenced this issue Oct 8, 2024
Parameter models were not being imported in the API client in the typescript runtime.

References #841
cogwirrel added a commit that referenced this issue Oct 8, 2024
…fault responses

OpenAPI generator treated "default" responses as responses with code 0, however parseOpenapi treats
default as 200. Adjust code generation to follow OpenAPI for backwards compatibility and to avoid
duplicate 200 responses. Note that this should be revisited such that code generation allows for
any response code other than those defined to be returned for the default response, rather than 0.

References #841
cogwirrel added a commit that referenced this issue Oct 8, 2024
Composed models (using all-of, one-of or any-of) are generated as "mixins" rather than using
inheritance, to match openapi generator's behaviour. We therefore need to ensure models import all
types from the composed properties.

References #841
@cogwirrel
Copy link
Member

I've raised #845 to fix these 3 issues!

Note that the default response is an interesting case. While the previous code generation "worked" with a default response, OpenAPI Generator treated the response code as 0, so we generated eg SayHello0ResponseContent types.

Are you returning any default responses from your API in your use case?

I'm curious to know whether you're actually using the generated code for the default response, as it doesn't seem to have ever matched the expected behaviour from the specification: https://swagger.io/docs/specification/v3_0/describing-responses/#default-response

cogwirrel added a commit that referenced this issue Oct 8, 2024
#845)

* fix(type-safe-api): add missing imports for parameter models

Parameter models were not being imported in the API client in the typescript runtime.

References #841

* fix(type-safe-api): fix duplicate 200 response for operations with default responses

OpenAPI generator treated "default" responses as responses with code 0, however parseOpenapi treats
default as 200. Adjust code generation to follow OpenAPI for backwards compatibility and to avoid
duplicate 200 responses. Note that this should be revisited such that code generation allows for
any response code other than those defined to be returned for the default response, rather than 0.

References #841

* fix(type-safe-api): add missing imports for composed models

Composed models (using all-of, one-of or any-of) are generated as "mixins" rather than using
inheritance, to match openapi generator's behaviour. We therefore need to ensure models import all
types from the composed properties.

References #841
@valebedu
Copy link
Contributor Author

valebedu commented Oct 8, 2024

Hi @cogwirrel,

I never used the default 503 responses in my API handlers, but implemented it in api model as a best practice (by looking at recommendations and OpenAPI audit tools)

@valebedu
Copy link
Contributor Author

valebedu commented Oct 8, 2024

Fixed in 0.23.60 thx a lot :)

@cogwirrel
Copy link
Member

Hi @cogwirrel,

I never used the default 503 responses in my API handlers, but implemented it in api model as a best practice (by looking at recommendations and OpenAPI audit tools)

Cool that makes sense! Definitely better that the generated code compiles now but we should revisit what it looks like for default responses. I've raised #847 to track this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants