oapi-codegen / oapi-codegen

Generate Go client and server boilerplate from OpenAPI 3 specifications
Apache License 2.0
5.61k stars 816 forks source link

fails to generate valid code for this api specification #372

Open jxsl13 opened 3 years ago

jxsl13 commented 3 years ago

Hi, I have been wanting to test to generate a client for this specification, sadly it failed. I think compared to the https://github.com/OpenAPITools/openapi-generator generator this one is way better as it's more Go-like and has a really clean interface, so I'd really love for it to be perfected, hence this issue.

https://api.mangadex.org/docs.html

api-codegen api/api.yaml > api/api.go                                                                                                                     ✔  10041  20:20:38
error generating code: error formatting Go code: Api.go:8995:18: '_' must separate successive digits (and 3 more errors)

actually a .yaml file: api.txt

deepmap-marcinr commented 3 years ago

I see, it's somehow generating a type name that starts with a number, that's not allowed. The ClientWithResponses is my nemesis. I'll have a look.

jxsl13 commented 3 years ago

giant thanks (Y)

deepmap-marcinr commented 3 years ago

Oh wow, this one is really hard.

This bug hits in the following case:

Fixing this is quite a large, painful refactoring. The structure of the function which maps OpenAPI schema to Go types recursively is incorrect and hard to fix without breaking a lot of other things.

Sheesh, you found a good one!

To fix this, all Go schema and additional property generation for ClientWithResponses need to be hooked into the global schema generation. The ClientWithResponses duplicates a lot of code in a way which is sometimes incompatible. It would be best to unify it. I think this will be a very breaking change, and I'll make this issue a test case in v2.0. I'll get to this, but it'll take some time.

deepmap-marcinr commented 3 years ago

Generally, ClientWithResponses is bugged for types which require AdditionalProperties handling.

You can work round this for now by declaring your own type and refactoring this spec a tiny bit, then you can get something which works.

Add this to components/schemas:

components:
  schemas:
    MangaStatus:
      type: object
      properties:
        result:
          type: string
          default: ok
        statuses:
          type: object
          additionalProperties:
            type: string

And change the manga status endpoint to look like this:

  /manga/status:
    get:
      summary: Get all Manga reading status for logged User
      operationId: get-manga-status
      tags:
        - Manga
      parameters:
        - schema:
            type: string
            enum:
              - reading
              - on_hold
              - plan_to_read
              - dropped
              - re_reading
              - completed
          in: query
          name: status
          description: Used to filter the list by given status
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MangaStatus' # <---- this is the change, replace inline definition with reference

What we've done is move the handling of this type outside of ClientWithResponses into code which works, then used it by reference.

I hope this is good enough workaround until I can fix this, but like I said, it's basically a rewrite to fix this bug.

deepmap-marcinr commented 3 years ago

This is also quite an interesting construct that I've never considered before:

                  statuses:
                    type: object
                    additionalProperties:
                      type: string
                      enum:
                        - reading
                        - on_hold
                        - plan_to_read
                        - dropped
                        - re_reading
                        - completed

so, it looks like this should make a map[string]string where keys can be anything, but values are constrained.

zzh8829 commented 2 years ago

Finally found this github issue, I'm also seeing the same error message.

  1. the response code is not a valid golang type. this is super easy to fix

    typeName := PathToTypeName(propertyPath)
    ->
    typeName := SchemaNameToTypeName(PathToTypeName(propertyPath))

    this fixes the '_' must separate successive digits error but it's still broken since the type definition is never generated.

  2. Im seeing the same type gen error with a super basic block without enums

    fieldname:
    type: object
    nullable: false
    additionalProperties: true

So it looks like the underscore is easy to fix. custom type in response without ref is probably difficult cuz it requires the huge refactoring you mentioned. enum in type is like a whole separate issue unrelated to this but def still worth fixing. Anyways looking forward to v2 release, let me know if I can help with any coding work related to this bug.

m1schka-bdr commented 2 months ago

I guess this issue hasn't made it's way into the v2 release. I've also got a quite complex openapi v3 spec. dealing with 60 additionalProperties and a lot of enums, I'm close to 300 errors in that regard:

: client.go:90003:16: '_' must separate successive digits (and 295 more errors)

I tried other generators but all of them are failing or generating a huge amount of code, I just need a small subset of the API as client code.

Is there anything I can do to fix it?