Skip to content
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

🐛 Fix bug preventing to use OpenAPI when using tuples #3874

Merged
merged 2 commits into from Jan 23, 2022

Conversation

victorbenichoux
Copy link
Contributor

This PR is meant to fix issues (#3665, #3782) encountered generating openapi.json with nested models that have iterable-typed attributes (e.g. Tuple or List).

Briefly, this behavior did not occur with 0.67.0, and was introduced in 0.68.0. Bisecting the issue led me to the following commit 97fa743.

As per the error raised when using nested schemas of iterables:

pydantic.error_wrappers.ValidationError: 2 validation errors for OpenAPI
components -> schemas -> BuggyItem -> properties -> items -> items -> items
  value is not a valid dict (type=type_error.dict)
components -> schemas -> BuggyItem -> $ref
  field required (type=value_error.missing)

I realized that there must be an issue when initializing the OpenAPI model, and specifically the items field of the Schema. This makes sense, since the commit introduced changes to the specification of the OpenAPI model.

And indeed, consider that we have the following Model to add to the openAPI specification:

class BuggyItem(pydantic.BaseModel):
    items: List[Tuple[str, str]]

We have to instantiate a Schema with

{
  "items": {
    "title": "Items",
    "type": "array",
    "items": {
      "type": "array",
      "items": [{ "type": "string" }, { "type": "string" }]
    }
  }
}

In other words, the definition of the Schema.items attribute cannot be

    items: Optional["Schema"] = None

But must be

    items: Optional[Union["Schema", List["Schema"]]] = None

Which is the change introduced in this commit.

Hopefully this is helpful, let me know if there is anything that I can add to this PR!

@mrtolkien
Copy link

Had the issue and am waiting for a patch right now, would love to see this merged

@RileyMShea
Copy link

RileyMShea commented Sep 13, 2021

While this PR at least allows generation of openapi.json and the /docs endpoint to load, the nested fields themselves are unable to render. That is not unique to this PR though and the same behavior is present in FastAPI 0.67.0. You need to provide min_items and max_items and you (sort of ) have to use a List or a conlist for a completely valid openapi.json to be created.

A item:Tuple[int,int] = Field(...,min_items=2,max_items=2) annotation can't be used because Pydantic thinks your type annotation takes precedence over the Field args and will throw an error about unenforced constraints. You can silence it but then you lose on type validation.

See this example from https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints

from pydantic import BaseModel, Field, PositiveInt

try:
    # this won't work since PositiveInt takes precedence over the
    # constraints defined in Field meaning they're ignored
    class Model(BaseModel):
        foo: PositiveInt = Field(..., lt=10)

I had to change my Model hints to use conlist or List[int] with the min/max in Field(...)

from pydantic import BaseModel, Field,conlist

voltage: conlist(int,min_items=4,max_items=4) 
# OR
voltage: List[int] = Field(..., min_items=4,max_items=4)

Only then would the models properly render in the /docs endpoint.

@victorbenichoux
Copy link
Contributor Author

Thanks for the comments @RileyMShea!

Indeed, the generated openapi.json fails to render the items correctly in this case, and the /docs does not mention that Tuple is expected to be of fixed length (although it remains correctly validated when calling the endpoint, raising 422s appropriately).

It does appear that this would in addition require a fix on the pydantic side which confuses fixed-length Tuple and variable length List in fields.

That is, the following models have the same pydantic.BaseModel.schema() (up to the name):

class ListModel(pydantic.BaseModel):
    items: List[str]

class TupleModel(pydantic.BaseModel):
    items: Tuple[str]

When one would expect that the latter has a schema with minLength and maxLength set. I will raise an issue with them as I could not find anything related to this, and suggest a fix there.

@Mause
Copy link
Contributor

Mause commented Sep 14, 2021

@victorbenichoux that actually isn't correct - openapi has a special syntax for tuples as you correctly detailed in your original post

@victorbenichoux
Copy link
Contributor Author

My bad then, I had not correctly read the initial issue (which I did not write though 😄 ).

Then the solution would be to implement the tuple validation for openapi with prefixItems.

I still feel, however, that it would be best to be implemented on the pydantic side, since fastapi does not generate this aspect of the schema.

@victorbenichoux
Copy link
Contributor Author

Something worth noting, as I mention in pydantic/pydantic#3210, is that the specification for Tuples in the openAPI spec is only available for versions >3.1.0, which, in turn, is not yet supported by swagger yet. As a result, I do not believe that we can really treat the Tuple case until swagger supports the new version of the openAPI spec.

This PR at least reverts the behavior to what it was in 0.67.0: /openapi.json is generated correctly, although Tuple fields don't render correctly in the swagger.

@HansBrende
Copy link

@RileyMShea @Mause :
I agree with @victorbenichoux: even though the behavior in 0.67.0 was not optimal, it was better than it is now. The behavior now is that the Swagger page CRASHES... making it completely useless and we (anyone using tuples) are all stuck on 0.67.0 until this PR gets merged & released.

@tiangolo tiangolo changed the title Fix bug with tuples and list in OpenAPI models 🐛 Fix bug preventing to use OpenAPI when using tuples Jan 23, 2022
@codecov
Copy link

codecov bot commented Jan 23, 2022

Codecov Report

Merging #3874 (969f7d3) into master (1760da0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master     #3874     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files          410       522    +112     
  Lines        10281     13136   +2855     
===========================================
+ Hits         10281     13136   +2855     
Impacted Files Coverage Δ
fastapi/openapi/models.py 100.00% <100.00%> (ø)
tests/test_tuples.py 100.00% <100.00%> (ø)
fastapi/utils.py 100.00% <0.00%> (ø)
fastapi/params.py 100.00% <0.00%> (ø)
fastapi/encoders.py 100.00% <0.00%> (ø)
fastapi/concurrency.py 100.00% <0.00%> (ø)
fastapi/applications.py 100.00% <0.00%> (ø)
fastapi/openapi/docs.py 100.00% <0.00%> (ø)
fastapi/openapi/utils.py 100.00% <0.00%> (ø)
fastapi/datastructures.py 100.00% <0.00%> (ø)
... and 124 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1760da0...969f7d3. Read the comment docs.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 969f7d3 at: https://61edd15b3a3b71a28401e87b--fastapi.netlify.app

@tiangolo
Copy link
Owner

Thanks @victorbenichoux! 🍰

Good detective work! 🚀

And thanks everyone for the comments.

I added a lot of tests to check that using tuples doesn't break the rest of OpenAPI.


For anyone else paying attention to this, have in mind that Swagger UI doesn't support OpenAPI 3.1.0 yet. That is the first version that supports using tuples (also the most recent one), because it's based on JSON Schema draft 07.

They recently changed how JSON Schema defines tuples in the latest drafts as @victorbenichoux pointed out (with prefixItems). But the first thing that Swagger UI will support will be OpenAPI 3.1.0, using the way to define tuples from JSON Schema draft 07 (what this PR enables).

So, whenever Swagger UI supports tuples at some level it will be first with the current results. But again, have in mind it's not really supported by the docs UI yet and they won't be properly rendered.

That aside, because the internal models were defining the OpenAPI models so tightly, they didn't allow using tuples with OpenAPI and Swagger UI (because, technically it's not supported by the spec 😅 ). But with this more relaxed version now you can continue using tuples, even if they are not properly rendered in Swagger UI, but you can still benefit from the rest of OpenAPI and Swagger UI.


This is available in FastAPI 0.73.0, released in the next hours. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants