Prefer property name instead of title for class name

[jrversteegh]

When the title of a schema object doesn't match its property name, reference resolution breaks. Simplest demonstration:

#!/usr/bin/env python3
from typing import Generic, TypeVar
from pydantic import BaseModel
from pydantic.generics import GenericModel
from fastapi import FastAPI

app = FastAPI()

T = TypeVar('T')

class GenericItem(GenericModel, Generic[T]):
    prop: T

@app.get("/int_item", response_model=GenericItem[int])
def get_int_item():
    return GenericItem[int](prop=88)

This will cause the client to produce a warning:

Warning(s) encountered while generating. Client was generated, but some pieces may be missing

WARNING parsing GET /int_item within default.

Cannot parse response for status code 200, response will be ommitted from generated client

Reference(ref='#/components/schemas/GenericItem_int_')

This pull request fixes this by prefering the property name as the class name for a model over its title.

[jrversteegh]

The 3 failing tests and 8 warnings were already present before my modification

[dbanty]

Odd, main looks like it passed all checks on last build. I'll figure out what's going on there.

@bowenwr (and team) do you want to take a look at this to see if you disagree? I don't have a strong opinion either way, but it would be a breaking change to existing clients (meaning we'd do this in a 0.8.0).

[bowenwr]

I'm still playing catch-up on our other fixes to restore our integration tests. If I can get there, I'll take a look at this over the weekend to see if it has any implications for us. Thanks!

[jrversteegh]

@dbanty note that I also have no opinion on how the class name is determined. It's just that I think reference resolution is currently broken. I guess it would also be fine if the class_name is set to title, but then the schema.models dictionary should at least also have an entry with the attribute name of the model in #/components/schemas, so reference resolution works. Simplest json to demonstrate broken reference resolution:

{
  "openapi": "3.0.2",
  "info": {
    "title": "test",
    "version": "0.1.0"
  },
  "paths": {
    "/": {
      "get": {
        "responses": {
          "200": {
            "description": "Successful Response",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/Ham"
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "Ham": {
        "title": "Eggs",
        "type": "object",
      }
    }
  }
}

[emann]

I assume that a mismatch between the schema name and title is fairly uncommon, so I don't foresee this being too bad of a breaking change to go around and fix. We need to support it regardless, so worst come to worst we could make it a config option but that feels a smidge icky - config shouldn't be required to generate a client from a valid openapi document.

[bowenwr]

I wasn't able to get to this, so don't block on my account. Though I appreciate the opportunity!

[dbanty]

@dbanty note that I also have no opinion on how the class name is determined. It's just that I think reference resolution is currently broken. I guess it would also be fine if the class_name is set to title, but then the schema.models dictionary should at least also have an entry with the attribute name of the model in #/components/schemas, so reference resolution works.

I see, when we loop through components to register models prior to property parsing, we register whatever the "class_name" is of the model rather than the defined reference name. This is a bug for sure.

I think the right move then is to change parser.properties.build_model_property to register the name instead of the title. I believe this is dict key is only used for looking up references, so it really should match the component definition. There's no reason it has to match to class name as far as I can tell.

That should fix the resolution bug without changing anyone's existing generated code. @jrversteegh do you want to take a stab at this? If not then I can do it probably later this week.

[jrversteegh]

@dbanty OK, I'll have a look at it and update this pull request if I can find out how to do it.

[jrversteegh]

@dbanty I've updated the pull request to only modify the key in the schema dictionary, not the class name. I hope that is what you had in mind.
After looking at this some more, I feel using the "title" for the class_name is not appropriate however. According to json schema, the title is a "short description", which does not sound like it'd be generally suitable for a class name imho. Also it's just confusing, as this issue demonstrates ;)

[dbanty]

I checked out this code and poked around to figure out why the e2e tests are failing. There are a few other places in the code that we assume the generated class name to be the key of the model intentionally because we can't have two classes with the same name. So the schemas.models and schemas.enums dictionaries must continue to be named after the generated class name.

This is an issue because the class name will not always be equal to the ref name... so clearly more work is required to decouple the deduplication of classes from the registration & lookup of references. We could just loop through all models/enums when doing a deduplication check instead of looking up by key this just feels slow. Then again, having two separate dictionaries (lookup by Ref and lookup by class_name) feels wasteful.

If we went back to your idea of always naming on name and never title, that would solve the issue because class_name would always be derived from reference name and therefore it's still a single lookup. However, that means that someone with a deeply nested object somewhere would have to live with the autogenerated GrandParentParentPropertyName class instead of being able to provide a title with a more sensible name.

I don't have a recommended solution, just pointing out what the problem is at the moment.

[dbanty]

Just removing the milestone since I don't think this will end up being a breaking change and so it can make it into a lesser release later down the line. I do periodically still think about this issue and my only solution is to have two dictionaries, this time keyed with types that indicate what the string is, so we don't make this mistake again.

[dbanty]

I'm going to close this PR just because the requirement here has changed a lot since inception. I think when we're ready to fix we can do a new PR closing #342.