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

Issues with Component Responses #4

Open
aphex opened this issue Jan 11, 2022 · 1 comment
Open

Issues with Component Responses #4

aphex opened this issue Jan 11, 2022 · 1 comment

Comments

@aphex
Copy link

aphex commented Jan 11, 2022

First off, I wish I know FP better so I could contribute more here, but this stuff reads like stereo instructions to me sadly. So I figure I'll do my best to provide you with the info and hopefully this is easy stuff for you.

1) Response Name Typeo
First off I think there is a typo here https://github.com/Fredx87/openapi-io-ts/blob/main/packages/cli/src/parser/components.ts#L202

I believe it should be const generatedName = toValidVariableName(name, "pascal"); without the quotes around name. I also think responses shouild eb pascal case, as you will see later. I keep getting a name.ts in my responses folder unless I fix this.

2) Import type when not used as value
Secondly, another minor one, is could we change operations.ts here https://github.com/Fredx87/openapi-io-ts/blob/main/packages/cli/src/codegen/operations.ts#L119

to this

import type { TaskEither } from "fp-ts/TaskEither";

Since this import is only used as a type in the operation my TS config keeps yelling at me because I am using importsNotUsedAsValues.

3) Issues with reusable Responses
Alright now for the big stuff, I am seeing issues when trying to share responses across my spec. This is mainly for error response as I have a common error handler that all my api will use, so every endpoint will get the same API error schema. I would like to just write that one and share it. I see 2 ways this could go down.

a shared response with a custom schema inline.
a shared response with a ref to a schema object

To test this I have added the following errors to addPet in `petstore.yaml

        "400":
          description: Invalid input
          content:
            application/json:
              schema:
                  type: object
                  properties:
                    custommessage:
                      type: string
        "401":
          description: Invalid input
          $ref: "#/components/responses/Error401"
        "405":
          description: Invalid input
          $ref: "#/components/responses/Error405"

The inline 400 error works as expected. I then added the following items to components (line 623)

components:
  responses:
    Error401:
      content:
        application/json:
          schema:
            type: object
            properties:
              noref: 
                type: string
    Error405:
      content:
        application/json:
          schema:
            $ref: "#/components/schemas/Error"
  schemas:
    Error:
      properties:
        message:
          type: string

As a result, after updating that name issue above. I am seeing the following contents

Error401.ts

export const Error401 = { _tag: "JsonResponse", decoder: Error401Schema};

Here the Error401Schema does not exist anywhere and obviously is not important in anyway

Error405.ts

export const Error405 = { _tag: "JsonResponse", decoder: schemas.Error};

Better but there is no import for schemas in this file so this also would break if used.

Finally operations/allPets.ts

        "200": { _tag: "JsonResponse", decoder: schemas.Pet },
        "400": { _tag: "JsonResponse", decoder: AddPetResponse400Schema },
        "401": { _tag: "JsonResponse", decoder: Error401Schema },
        "405": { _tag: "JsonResponse", decoder: schemas.Error }

Again Error401Schema does not exist anywhere and is never imported. Also 405 refers to schemas.Error. Though I guess this is technicall ok, I am thinking this probably shoudl refer to Error405Schema which then is simple

export type Error405Schema = schemas.Error

This is minor I guess, but seems like what I would have expected. Maybe i am off though.

I am thinking overall Error401.ts and Error405.ts should look more like schema files. I am not seeing much purpose in having the { _tag: "JsonResponse", decoder: Error401Schema } exported from there. It would be nice if there was an exported Error401 io-ts type and a Error401 TS Type. Then in the addPetOperation if it could import and reference those schemas.

I hope this is all making since, I have been banging my head against this for a day or so trying to clean it up myself, but the fp mentality is just killing me. If i can provide any other info or testing please let me know.

Goes without saying but thanks for this library, I am hoping to use it as a foundational piece to my architecture.

@Fredx87
Copy link
Owner

Fredx87 commented Jan 19, 2022

Hi @aphex, thank you for helping test the library. As mentioned in the README there are still some bugs and missing things around. I am working on some refactorings and I am planning to do a new release soon.

Regarding your issues:

  1. This is probably a typo, I will check it

  2. It is possible to use import type, but I am refactoring the generated code for operations, and probably TaskEither will not be imported in these files anymore

  3. The handling of error responses is not very well implemented and tested in this version, for the first iteration I looked more at the successful responses. For this reason, it is possible that in the generated code there are some references to missing things.
    The reason for having things like { _tag: "JsonResponse", decoder: Error401Schema } for handling errors is that is possible that an error is not a JSON but a string, or an empty response (or maybe also a binary file... I guess nobody is doing that but it is possible). So the runtime will need to know the response type to decode it.
    At the moment the decoding of errors in the runtime is not implemented (it will just return an ApiError with the Response inside). The idea is that you will be able to have an ApiError<T>, where <T> in your case will be schemas.Error.

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

No branches or pull requests

2 participants