Make it harder to hit collisions with json schema defrefs

[dmontagu]

Addresses an issue where, if you had a model called Model and another called ModelInput, it would detect a potential naming conflict and replace ModelInput with ModelInputInput.

I spent a lot of effort trying to rework this function to only respect actual conflicts and not potential conflicts, but the challenge is that remapping the defs refs requires modifications to the referenced definitions, and these references may be present in nested schemas which might become "unified" after replacing the input/output refs with a common one. This basically means that it requires iteration to determine what simplifications can be made, and doing so is somewhat complex. I don't see a good way around this, so for now, I have just tried to make it harder to run into this scenario by introducing double underscores.

It is still possible to achieve an unintentional conflict based on the mode, but with the changes in this PR, the only way to do that would be to have my_package.MyModule.Input and my_package.MyModule__Input both be classes. Considering that nesting basemodel definitions is rare, and having modules named in PascalCase is rare, and even if you did one of those you'd still need to have a conflict on the specific name, this seems safe to me.

The biggest concern I have is that having one model called OrganizationInput and another called Organization which has a difference between its input and output schemas (and therefore gets the ref Organization__Input) might result in problems if trying to generate a client based on an OpenAPI spec or whatever. (I'm thinking both might be given the class name OrganizationInput, but I'm not totally sure.)

I'll note that there are various complex cases with defs-ref generation that are easy to forget about (and which I wasted time trying other things before discovering they would cause problems), so some approaches that might seem obvious and work in the "normal" case may still have problems. But if anyone has a problem with this approach, or especially has other suggestions, I'm happy to consider them.

Selected Reviewer: @Kludex

[dmontagu]

Also I suspect we'll eventually want to support a way to specify explicitly what the name of a model/dataclass/enum/typeddict/etc. should be, and error if there is a conflict (though we don't support that today). I think it will make sense for that to be introduced if/when we replace use of GenerateJsonSchema.generate_definitions with a higher-level api (perhaps a classmethod of TypeAdapter, or similar). Given that, I don't think it's worth adding much complexity. (Again, open to an alternative implementation though if it is simple and better.)

[cloudflare-pages]

Deploying with    Cloudflare Pages

Latest commit:	a88ecf3
Status:	✅  Deploy successful!
Preview URL:	https://63e584cb.pydantic-docs2.pages.dev
Branch Preview URL:	https://reduce-json-schema-name-conf.pydantic-docs2.pages.dev

View logs

[dmontagu]

please review

[Kludex]

Is it too crazy to create a test suite with an openapi-client-generator?

[dmontagu]

Is it too crazy to create a test suite with an openapi-client-generator?

I don't necessarily think it's crazy, but I'm not very excited about the amount of effort it would entail, and I also think it might better belong in the FastAPI repo since it will be dependent on the OpenAPI spec more than the JSON schema spec. I think it could make sense to add some tests checking the JSON schema though, using the jsonschema package or whatever.

(Existing issue, for reference: #5164)

Admittedly, working with OpenAPI/swagger is ultimately more important than just that the JSON schema is valid, just I think it would be a lot more work/reliance on FastAPI implementation details.

[Kludex]

I've confirmed that it solves tiangolo/fastapi#9709 (comment). 👍

A lot of tests failing in FastAPI - as you may have already predicted? 👀 Do you want to check them first?

I wanted to just comment...

[dmontagu]

@Kludex I just pushed a commit that uses a - instead of __ for separating the class name from its Input/Output-ness; with this latest commit I think no fastapi tests start failing.

[dmontagu]

Let's not merge this until after #6883 is merged and we run with fastapi to make sure it doesn't break anything, but I think it should be fine.