oapi-codegen icon indicating copy to clipboard operation
oapi-codegen copied to clipboard

fails to generate valid code for this api specification

Open jxsl13 opened this issue 4 years ago • 8 comments

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

jxsl13 avatar May 23 '21 18:05 jxsl13

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.

deepmap-marcinr avatar May 25 '21 15:05 deepmap-marcinr

giant thanks (Y)

jxsl13 avatar May 25 '21 17:05 jxsl13

Oh wow, this one is really hard.

This bug hits in the following case:

  • ClientWithResponses is generated
  • Response contains field with additional properties, no other members
  • AdditionalProperty key names are constrained by enum

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 avatar May 25 '21 22:05 deepmap-marcinr

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 avatar May 25 '21 22:05 deepmap-marcinr

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.

deepmap-marcinr avatar May 25 '21 23:05 deepmap-marcinr

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.

  1. 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.

zzh8829 avatar Sep 21 '21 03:09 zzh8829

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?

m1schka-bdr avatar Apr 23 '24 11:04 m1schka-bdr