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 appending "operation" to some operationIds #789

Closed
jstrunk opened this issue May 30, 2024 · 4 comments · Fixed by #831
Closed

[BUG] type-safe-api appending "operation" to some operationIds #789

jstrunk opened this issue May 30, 2024 · 4 comments · Fixed by #831
Labels
bug Something isn't working needs-triage stale

Comments

@jstrunk
Copy link
Contributor

jstrunk commented May 30, 2024

Describe the bug

For some of the operations with Python Lambda handlers defined in my OpenAPI spec, the operationId gets changed and has "operation" appended. When this happens, a handler file is created without the extra string, but the CDK infra has it in the handler path string.

Expected Behavior

All of the operations defined in the spec should be named consistently, and the infrastructure should be consistent with module names.

Current Behavior

Given a main.yaml including the following:

  /generateSpec:
    post:
      operationId: generateSpec
      x-handler:
        language: python
      requestBody:
...
  /listSessions:
    get:
      description: List all sessions started in a time range.
      operationId: listSessions
      x-handler:
        language: python
      requestBody:
...

generates packages/api/generated/infrastructure/typescript/src/functions.ts

/**
 * Options for the GenerateSpecFunction construct
 */
export interface GenerateSpecFunctionProps extends Omit<FunctionProps, 'code' | 'handler' | 'runtime'> {}

/**
 * Lambda function construct which points to the python implementation of GenerateSpec
 */
export class GenerateSpecFunction extends Function {
  constructor(scope: Construct, id: string, props?: GenerateSpecFunctionProps) {
    super(scope, id, {
      runtime: Runtime.PYTHON_3_12,
      handler: "api_detective_api_python_handlers.generate_spec.handler",
      code: Code.fromAsset(path.resolve(__dirname, "..",
        "../../../handlers/python/dist/lambda",
      )),
      tracing: Tracing.ACTIVE,
      timeout: Duration.seconds(30),
      ...props,
    });
  }
}
...
/**
 * Options for the ListSessionsOperationFunction construct
 */
export interface ListSessionsOperationFunctionProps extends Omit<FunctionProps, 'code' | 'handler' | 'runtime'> {}

/**
 * Lambda function construct which points to the python implementation of ListSessionsOperation
 */
export class ListSessionsOperationFunction extends Function {
  constructor(scope: Construct, id: string, props?: ListSessionsOperationFunctionProps) {
    super(scope, id, {
      runtime: Runtime.PYTHON_3_12,
      handler: "api_detective_api_python_handlers.list_sessions_operation.handler",
      code: Code.fromAsset(path.resolve(__dirname, "..",
        "../../../handlers/python/dist/lambda",
      )),
      tracing: Tracing.ACTIVE,
      timeout: Duration.seconds(30),
      ...props,
    });
  }
}

The handler file created for listSessions is inconsistent with the handler property.
packages/api/handlers/python/api_detective_api_python_handlers/generate_spec.py
packages/api/handlers/python/api_detective_api_python_handlers/list_sessions.py

Reproduction Steps

I have no idea what's triggering this.

Possible Solution

No response

Additional Information/Context

I've created two new resources on this API in the last week. Both had this weird behavior. The first was named commentSpec. It was able to generate everything without "operation" appended when I changed the name to iterateSpec, but only after I deleted the nx cache, the packages/api directory, ran pdk, and restored the non-generated files.

PDK version used

0.23.38 and 0.23.40

What languages are you seeing this issue on?

Typescript, Python

Environment details (OS name and version, etc.)

MacOS 14.4.1 intel

@jstrunk jstrunk added bug Something isn't working needs-triage labels May 30, 2024
@jstrunk
Copy link
Contributor Author

jstrunk commented May 30, 2024

After renaming the operationId to getSessionList, it generated correct output. There was no need to delete the nx cache, the packages/api directory, ran pdk, and restore the non-generated files, after all.

Perhaps it's because I named the schemas for the request and response ListSessionsRequest and ListSessionsResponse?

Here are snippets from the two operations that caused me problems:

  /generateSpec/{session_id}/comment:
    post:
      operationId: iterateSpec
      x-handler:
        language: python
      parameters:
        - name: session_id
          in: path
          required: true
          schema:
            type: string
      requestBody:
        content:
          'application/json':
            schema:
              $ref: '#/components/schemas/CommentSpecRequest'
      responses:
        200:
          description: Successful response
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/InitGenSpecResponse'
...
  /listSessions:
    get:
      description: List all sessions started in a time range.
      operationId: getSessionList
      x-handler:
        language: python
      requestBody:
        content:
          'application/json':
            schema:
              $ref: '#/components/schemas/ListSessionsRequest'
      responses:
        200:
          description: Successful response
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/ListSessionsResponse'
...

Copy link

github-actions bot commented Aug 5, 2024

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon. If you wish to exclude this issue from being marked as stale, add the "backlog" label.

@github-actions github-actions bot added the stale label Aug 5, 2024
Copy link

Closing this issue as it hasn't seen activity for a while. Please add a comment @mentioning a maintainer to reopen. If you wish to exclude this issue from being marked as stale, add the "backlog" label.

@cogwirrel
Copy link
Member

Hi Jeff,

Sorry this got auto-closed, I'm just back from leave!

So I think what's causing this is indeed naming your input schema {{operationId}}Request! You can also reproduce it by defining an inline requestBody in your OpenAPI specification which will implicitly create a structure named as such, eg:

paths:
  /hello:
    post:
      operationId: sayHello
      x-handler:
        language: typescript
      requestBody:
        content:
          application/json:
            schema:
              type: object
              properties:
                message:
                  type: string
              required:
                - message

This causes us to hit this special case built into the TypeScript OpenAPI generator, which appends Operation to the operation ID:

https://github.com/OpenAPITools/openapi-generator/blob/ff1fe256d8a98aad3a1104d11716996dcc2e8788/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptFetchClientCodegen.java#L858-L863

I think the special case is there because the TypeScript OpenAPI generator creates an {{operationId}}Request structure itself for the whole HTTP request including the headers, url etc.

Unfortunately we don't have control over that part of the generator, but we can work around it by not using inline requestBody schemas, or not naming structures {{operationId}}Request.

Note that we always avoid this when using Smithy as the model language as it names the request body: {{operationId}}RequestContent.

I'm going to leave this closed as I don't think there's much we can do here unfortunately, but I hope this clarifies things at least! :)

Cheers,
Jack

cogwirrel added a commit that referenced this issue Sep 30, 2024
…i code generation

This change replaces the java openapi-generator CLI tool with our own code generation tool, which
produces equivalent code. This removes the dependency on Java for the TypeScript REST API projects,
as well as improving the speed of code generation by avoiding package installation steps and the
multiple passes of openapi codegen used to work around limitations of the tool.

Note that this change fixes #789, which means that those hitting the edge case described in the
issue may experience a breaking change.
cogwirrel added a commit that referenced this issue Sep 30, 2024
…i code generation

This change replaces the java openapi-generator CLI tool with our own code generation tool, which
produces equivalent code. This removes the dependency on Java for the TypeScript REST API projects,
as well as improving the speed of code generation by avoiding package installation steps and the
multiple passes of openapi codegen used to work around limitations of the tool.

Note that this change fixes #789, which means that those hitting the edge case described in the
issue may experience a breaking change.

Generated code packages now covered by the new codegen:
- typescript runtime
- typescript infrastructure
- typescript handlers
- typescript hooks
cogwirrel added a commit that referenced this issue Sep 30, 2024
…i code generation (#831)

This change replaces the java openapi-generator CLI tool with our own code generation tool, which
produces equivalent code. This removes the dependency on Java for the TypeScript REST API projects,
as well as improving the speed of code generation by avoiding package installation steps and the
multiple passes of openapi codegen used to work around limitations of the tool.

Note that this change fixes #789, which means that those hitting the edge case described in the
issue may experience a breaking change.

Generated code packages now covered by the new codegen:
- typescript runtime
- typescript infrastructure
- typescript handlers
- typescript hooks
cogwirrel added a commit that referenced this issue Oct 24, 2024
Inline request body schemas were typed as "any" and no model was generated for them. Address this by
hoisting inline request schemas in the same way we do for responses. Note that OpenAPI generator
named these hoisted inline request models "<OperationID>Request" which also clashes with the overall
operation request model - and resolved it by appending "Operation" to the Operation ID (see #789).
We deviate from OpenAPI generator behaviour to avoid this issue recurring by naming the type
<OperationIDRequestBody>.
cogwirrel added a commit that referenced this issue Oct 24, 2024
Inline request body schemas were typed as `any` and no model was generated for them.

Address this by hoisting inline request schemas in the same way we do for responses.

Note that OpenAPI generator named these hoisted inline request models `<OperationID>Request` which
also clashes with the overall operation request model - and resolved it by appending `Operation` to
the Operation ID (see #789).

We deviate from OpenAPI generator behaviour to avoid this issue recurring by naming the type
`<OperationID>RequestContent`.
cogwirrel added a commit that referenced this issue Oct 25, 2024
…873)

Inline request body schemas were typed as `any` and no model was generated for them.

Address this by hoisting inline request schemas in the same way we do for responses.

Note that OpenAPI generator named these hoisted inline request models `<OperationID>Request` which
also clashes with the overall operation request model - and resolved it by appending `Operation` to
the Operation ID (see #789).

We deviate from OpenAPI generator behaviour to avoid this issue recurring by naming the type
`<OperationID>RequestContent`.
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 stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants