eng: fix forward type reference in Pydantic schemas BNCH-112697

[eli-bl]

This fixes what I think is arguably a bug in the upstream code, but one that I've never reproduced until now and still can't reproduce in any simple automated test. I was only able to reproduce it by testing with one particular large API spec file—but not all the time. I don't understand the reason for this indeterminacy, but I do think I understand the error.

The code generator is using Pydantic as a validation framework for the OpenAPI schema (that is, the schema of OpenAPI documents in general, not of our API). There is a class derived from Pydantic's BaseModel for every standard OpenAPI component.

In one area of OpenAPI, there is a circular reference between the types Encoding, Header, and Parameter. So in the corresponding Python code, to avoid an import cycle, there is a forward reference—a type annotation that's just a string—from Encoding to "Header". That should be totally legal, and has been working fine till now.

However, according to the Pydantic docs, the logic it uses to lazily resolve such references is extremely complicated and it seems that in some hard-to-define cases, you may need to give it an assist by calling .model_rebuild() on the class that had the forward reference. So that's what I've added here. It's what the docs advise you to do if all else fails, and as far as I can tell it has no negative effects on anything.

To be clear, I had never changed any of the code in these Pydantic classes in our fork. I can only imagine that some subtle change in the order that other modules are importing each other in, or something like that, was enough to upset Pydantic's delicate logic so the reference would no longer just automatically work.

I've also submitted this change upstream, although I don't know if they'll accept it since the issue is so weird and unreproducible outside of my local machine (update: turns out others have reproduced it).

[edwin-benchling]

Can you share the upstream PR as well? I'd like to follow the discussion there as well. I must admit I read the link in the PR description, but I don't see the applicability, but this isn't a blocker. Disclaimer: I did not read too much into the backwards compatibility bit. I trust that it fixes the issue you're seeing. My question marks are:

• Parameter does not define any members that depend on Header
• I'm not sure why Parameter would need to be rebuilt, as that is done to help resolve forward annotations. Does Parameter.__annotations__ include anything that references 'Header'?
• Is it because both Header and Parameter depend on ParameterLocation? The latter also does not have a forward reference to either Header or Parameter but maybe this is where the backwards compatibility bit is important to understand, which I admittedly skipped 😅

[edwin-benchling]

Very nit: it looks like you could identify whether model rebuilding is necessary by inspecting the class's __pydantic_core_schema__. This could be used to conditionally calling Parameter.model_rebuild()

[eli-bl]

I guess. I thought of that as a pydantic implementation detail, but I see that they did document it. However, I don't see a downside to just unconditionally calling model_rebuild, when we know for sure that there is a potential issue (due to the circular reference). This is a one-time action.

[eli-bl]

Can you share the upstream PR as well?

openapi-generators#1171

Parameter does not define any members that depend on Header

• Parameter depends on MediaType: https://github.com/benchling/openapi-python-client/blob/prod/2.x/openapi_python_client/schema/openapi_schema_pydantic/parameter.py#L36
• MediaType depends on Encoding: https://github.com/benchling/openapi-python-client/blob/prod/2.x/openapi_python_client/schema/openapi_schema_pydantic/media_type.py#L22
• Encoding depends on Header: https://github.com/benchling/openapi-python-client/blob/prod/2.x/openapi_python_client/schema/openapi_schema_pydantic/encoding.py#L22
• Header is a subclass of Parameter: https://github.com/benchling/openapi-python-client/blob/prod/2.x/openapi_python_client/schema/openapi_schema_pydantic/header.py#L7

This took some investigation, but the original error message did basically spell out both the underlying issue and the remedy: PydanticUserError: Parameter is not fully defined; you should define Header, then call Parameter.model_rebuild().

(Btw, in case you might think, as I did, "What if Header wasn't declared as a subclass, but just declared the same fields as Parameter: that doesn't help, there would still be a cycle from the reference to MediaType.)

Discussion on that PR suggests that this behavior only started due to a patch release of pydantic. I'm not 100% convinced because that pydantic update has been present for about 2 months and I've done plenty of work on the fork during that time. But in any case, based on what the pydantic docs at those links say (and online discussions, earlier than that patch release), the behavior was always vaguely-defined enough and unreliable enough that there was always the chance you'd have to call model_rebuild in the case of a circular reference.