Skip to content

Don't prepend title based property names, use Titles for full model type names #559

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

Closed
rtaycher opened this issue Dec 27, 2021 · 3 comments · Fixed by #560
Closed

Don't prepend title based property names, use Titles for full model type names #559

rtaycher opened this issue Dec 27, 2021 · 3 comments · Fixed by #560
Labels
✨ enhancement New feature or improvement

Comments

@rtaycher
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Ideally an openapi description would get us nice not too long types without a lot of futzing with overriding type names via class_overrides.

If you add a title/subtitle to an objects body it seems to append it to the path based name making names very long and often redundant
and you have to fix it via class_overrides (or if you control the openapi description by moving everything into components which dont cause long names)

An example to clarify

paths:
  /bob:
    get:
      operationId: get_bob_method
      requestBody:
        content: 
          application/json:
            schema:
              title: Get Bob Body
              type: object
              properties:
                ArrayProp:
                  type: array
                  items:
                    title: Bob Body Item
                    type: object
                    properties:
                      MyProp:
                        type: string
      responses:
        200:
          description: "Response"
          content:
            application/json:
              schema:
                title: The Robert Report
                type: object
                properties:
                  ArrayProp:
                    type: array
                    items:
                      title: Robert Response Item
                      type: object
                      properties:
                        MyPropR:
                          type: string       

results in GetBobMethodGetBobBody / GetBobMethodGetBobBodyBobBodyItem for the json_body and
GetBobMethodTheRobertReport / GetBobMethodTheRobertReportRobertResponseItem for the response type instead of GetBobBody, BobBodyItem, and TheRobertReport/RobertResponseItem

Trying out the alternative openapi-generator-cli it returns GetBobBody, BobBodyItem, and TheRobertReport/RobertResponseItem.

While this is not mandated anywhere I think its much more sensible.

Usually with a title you will have named the full type/subtype and not need to prepend it with the method name/outer type.

Describe the solution you'd like
We should keep appending to a name when using default property names/item but start from scratch every time we hit a title.

Currently the only way to fix things is to rename via class_overrides or if you have control of the openapi description move everything into components(since types from component only use the component name and not the full path name)

@rtaycher rtaycher added the ✨ enhancement New feature or improvement label Dec 27, 2021
@rtaycher
Copy link
Contributor Author

I forgot to add that this could cause a lot of type name changes/churn for anyone generating their library with an older version then regenerating with a newer version of openapi-python-client.

I think the names would be nicer for most people but it would be a significant change(for who's openapi files use titles).
I don't think this project grantees that sort of stability but if this is an issue maybe add a config flag in the config file for those who want to preserve old behavior?

@dbanty
Copy link
Collaborator

dbanty commented Dec 27, 2021

We used to just use the title and not prepend a bunch of stuff, but folks ended up getting a bunch of duplicate class names. Detecting those duplicate class names and renaming them in a deterministic manner turned out to be a really hard problem to solve, so we just made the names as unique as possible.

I'm all for generating better names, but we'd have to come up with a very good system for determining those names consistently (so adding a new route/body or reordering properties doesn't end up being a breaking change to the generated code).

If we come up with such a system, we also then have to be very sure it's worth the breaking change or worth the additional maintenance of two different naming systems (to your point about this being jarring after regen). Most users are still on 0.9, I assume because of breaking changes in 0.10 😬.

Maybe an option we can consider is using modules to guard against duplicate class names. We only really need reusable classes (e.g., things in #/components) in models, we could put route-specific classes in their route's module.

@rtaycher
Copy link
Contributor Author

rtaycher commented Dec 28, 2021

I did think about namespacing models (although I imagined they would be layered in models instead of in api routes
IE get_bob_method.get_bob_body.BobBodyItem instead of GetBobMethodGetBobBodyBobBodyItem (that way you don't have to worry about duplicates model names in the same route).

But that seemed more complicated/might need a lot of discussion. Plus I've been using the model re-export

import api_lib.model as m
from api_lib.api. import get_stuff
#...
get_stuff.sync(client=client, json_body=m.StuffGetter(...)

and it seemed like it might not work as nicely with that pattern.


Would it be possible to make fullpaths the default/and have short title names via an override in the config file?
That way people who might have duplicate titles/components can avoid it while others can take advantage.

I tried it out in #560 and it doesn't seem like a lot of code.

dbanty added a commit that referenced this issue Dec 17, 2022
… simpler model names [#559, #560]. Thanks @rtaycher!

Co-authored-by: Dylan Anthony <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or improvement
Projects
None yet
2 participants