Enums as separate models #1432

calvinwyoung · 2020-04-25T01:32:08Z

Change Summary

This PR updates the OpenAPI schema generation behavior so that all enums are outputted as separate models, following the recipe for reusable enums. The primary benefit is to improve support for OpenAPI code generation tools — more on this below.

To illustrate the changes, suppose we have the following models:

class Foo(str, enum.Enum):
    A = 'A'
    B = 'B'

class ModelA(pydantic.BaseModel):
    foo: Foo    

class ModelB(pydantic.BaseModel):
    foo: Foo

Prior to this PR, the generated schema would look like the following:

{
    "definitions": {
        "ModelA": {
            "title": "ModelA",
            "type": "object",
            "properties": {
                "foo": {
                    "title": "Foo",
                    "enum": [
                        "A",
                        "B"
                    ],
                    "type": "string"
                }
            },
            "required": [
                "foo"
            ]
        },
        "ModelB": {
            "title": "ModelB",
            "type": "object",
            "properties": {
                "foo": {
                    "title": "Foo",
                    "enum": [
                        "A",
                        "B"
                    ],
                    "type": "string"
                }
            },
            "required": [
                "foo"
            ]
        }
    }
}

Notice how the enum values are duplicated in both the Foo and Bar definitions. As a result, an OpenAPI code generation tool will create 2 definitions of the enum — one for ModelAFoo and one for ModelBFoo. This is obviously not ideal as it makes it harder to reuse the enum definition in the client code.

With this PR, the generated schema will instead look like this:

{
    "definitions": {
        "Foo": {
            "title": "Foo",
            "enum": [
                "A",
                "B"
            ],
            "type": "string"
        },
        "ModelA": {
            "title": "ModelA",
            "type": "object",
            "properties": {
                "foo": {
                    "$ref": "#/definitions/Foo"
                }
            },
            "required": [
                "foo"
            ]
        },
        "ModelB": {
            "title": "ModelB",
            "type": "object",
            "properties": {
                "foo": {
                    "$ref": "#/definitions/Foo"
                }
            },
            "required": [
                "foo"
            ]
        }
    }
}

As a result, an OpenAPI code generation tool will output a single Foo enum definition that's referenced by both ModelA and ModelB.

This PR works seamlessly with fastapi v0.54.1 and with openapi-generator-cli via the openapitools/openapi-generator-cli docker image v4.3.0.

Related issue number

#1173

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
Documentation reflects the changes where applicable
changes/<pull request or issue id>-<github username>.md file added describing change
(see changes/README.md for details)

codecov · 2020-04-25T01:47:37Z

Codecov Report

Merging #1432 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1432   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         3747      3793   +46     
  Branches       742       752   +10     
=========================================
+ Hits          3747      3793   +46

Impacted Files	Coverage Δ
pydantic/schema.py	`100.00% <100.00%> (ø)`
pydantic/types.py	`100.00% <0.00%> (ø)`
pydantic/utils.py	`100.00% <0.00%> (ø)`
pydantic/fields.py	`100.00% <0.00%> (ø)`
pydantic/typing.py	`100.00% <0.00%> (ø)`
pydantic/networks.py	`100.00% <0.00%> (ø)`
pydantic/validators.py	`100.00% <0.00%> (ø)`

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 f96c4dc...7aed1d5. Read the comment docs.

calvinwyoung · 2020-04-25T03:34:01Z

@samuelcolvin One of the fastapi tests is currently failing since it's expecting an OpenAPI schema for the old enum behavior:
https://github.com/tiangolo/fastapi/blob/master/tests/test_tutorial/test_path_params/test_tutorial005.py#L37

What's the best way to handle this? Should I open another PR updating the fastapi test now, or should I do it once this PR gets merged in?

samuelcolvin

generally looks good, but I wonder whether this needs to wait until v2.

@tiangolo do you have an opinion on whether this would be considered a breaking change?

samuelcolvin · 2020-04-30T17:29:02Z

pydantic/schema.py

@@ -516,6 +516,27 @@ def model_type_schema(
    return out_schema, definitions, nested_models


+def enum_process_schema(enum: EnumMeta) -> Dict[str, Any]:
+    """
+    Take a single ``enum`` and generate its schema.


Suggested change

Take a single ``enum`` and generate its schema.

Take a single `enum` and generate its schema.

docstrings currently aren't rendered as rst or md, but if they ever are, it'll be as markdown, see #1339.

And below.

That sounds awesome. Would it be helpful if I replaced all of the double-tick characters with ` in this file? Or would you prefer that I only update the double-ticks characters that were added by this PR to keep it cleaner?

pydantic/schema.py

samuelcolvin · 2020-04-30T17:57:04Z

@samuelcolvin One of the fastapi tests is currently failing since it's expecting an OpenAPI schema for the old enum behavior:
https://github.com/tiangolo/fastapi/blob/master/tests/test_tutorial/test_path_params/test_tutorial005.py#L37

What's the best way to handle this? Should I open another PR updating the fastapi test now, or should I do it once this PR gets merged in?

Should be fixed if you rebase.

calvinwyoung · 2020-04-30T18:05:08Z

@samuelcolvin One of the fastapi tests is currently failing since it's expecting an OpenAPI schema for the old enum behavior:
https://github.com/tiangolo/fastapi/blob/master/tests/test_tutorial/test_path_params/test_tutorial005.py#L37
What's the best way to handle this? Should I open another PR updating the fastapi test now, or should I do it once this PR gets merged in?

Should be fixed if you rebase.

@samuelcolvin Got it. Also thanks for the comments — I just pushed the suggested fixes.

I'll defer to you and @tiangolo with regard to the release schedule. In theory this shouldn't be a breaking change so long as the end user is following the Open API spec. But I also understand these things don't always work out so cleanly.

samuelcolvin · 2020-05-23T12:06:34Z

@calvinwyoung can you rebase to master.

@tiangolo would love your feedback on the V2 decision.

tiangolo · 2020-05-23T12:19:01Z

Sorry for the delay on this... I think I finally found a way to filter and label GitHub notifications with mentions 😅

I like this! 🚀 🎉

I've been trying to think of a way that this could break something badly enough as to be considered a "breaking change", but I haven't found anything. I don't expect FastAPI users to test their OpenAPI schemas as FastAPI already tests the generation of valid OpenAPI.

The only concern I have is, @calvinwyoung , have you tested how this looks like in the /docs Swagger UI?

But apart from that I think this should be safe to release in a MINOR bump.

I can update the tests in FastAPI to accept the schemas generated by this and release a minor version, that should make tests pass here. ✔️

samuelcolvin · 2020-05-23T12:26:47Z

Oh so the failing FastAPI tests are real - catching an actual change with a test! 😆

I guess you'll have to get the test to pass with both the old and new schema for a while. Let us know when you've updated FastAPI and we'll get tests passing here.

tiangolo · 2020-05-23T16:11:46Z

So, I rushed to add the extra check in the tests and release a new version. But now I realized that it breaks Swagger UI 🤦 .

I'm now checking what needs to be changed in this PR and in FastAPI, I'm getting deeper and deeper into this... 😅

I'll keep reporting here what I find.

Would it be fine if I commit on top of this same branch? Or would it be better if I submit a new PR with extra changes on top of this?

calvinwyoung · 2020-05-23T16:31:05Z

@tiangolo Thanks so much for taking a look at this. I had thought I tested the Swagger UI previously, and it seemed to work okay. Before this PR, the Schemas section rendered something like this:

After this PR, the schemas render like this:

And we also have a new Foo model:

Where else are you seeing breaking changes? And yes, feel free to commit directly on top of the same branch! Thanks again — really appreciate the work you and @samuelcolvin have put into maintaining these two incredible projects 🥇

as they now have independent schemas, they kinda behave like top-level models, and should be taken into account for top level definitions

samuelcolvin · 2020-05-23T16:59:35Z

Would it be fine if I commit on top of this same branch? Or would it be better if I submit a new PR with extra changes on top of this?

Fine by me if you commit directly to the branch for this PR, it might be simpler than a PR into this PR. I hope @calvinwyoung doesn't mind.

tiangolo · 2020-05-23T18:06:08Z

I think that should work 🚀

I updated FastAPI, it ended up touching a bunch of places 😅 The main issue was that enums are supported for parameters in path, cookie, query, etc. And those didn't have a way to collect additional schema definitions. But this is supported now in 0.55.1 (just released).

I updated it here to include enums when collecting flat models and treat them as sort of top-level models for schema generation, as they get their own independent definitions. In short, this works correctly with the current FastAPI (and any other libraries that use this).

I added a couple of alias types to reduce duplication, but that can be changed if you prefer @samuelcolvin .

If you think this is fine, it's also ready from my side 🙂 🚀

samuelcolvin · 2020-05-23T18:11:56Z

Awesome, thank you both.

calvinwyoung · 2020-05-23T18:16:19Z

@tiangolo Ah yes that makes complete sense. Thank you!

see pydantic/pydantic#1432

calvinwyoung added 5 commits April 24, 2020 16:39

Updates schema() to generate enums as separate models

00a0034

Fixes mypy annotations

605721e

Adds changes file

b8a9eac

Fixes comment

6072b77

Removes unused import

5c7a200

calvinwyoung added 2 commits April 24, 2020 18:53

Fixes test case

9c76a43

Fixes missing partial branch in test coverage

38a85a3

samuelcolvin reviewed Apr 30, 2020

View reviewed changes

Resolves PR comments

ab7748e

calvinwyoung mentioned this pull request May 20, 2020

How best to generate OpenAPI schema with reusable enums? tiangolo/fastapi#1303

Closed

3 tasks

tiangolo mentioned this pull request May 23, 2020

✅ Add test to support Enums with their own re-usable schema tiangolo/fastapi#1461

Merged

tiangolo mentioned this pull request May 23, 2020

🐛 Fix Enum handling with their own schema definitions tiangolo/fastapi#1463

Merged

🐛 Include enums in flat model schema handling

f3f0e3a

as they now have independent schemas, they kinda behave like top-level models, and should be taken into account for top level definitions

tiangolo added 2 commits May 23, 2020 19:45

✅ Add test for coverage

450924d

🐛 Use Type[Enum] as type for consistency

7aed1d5

samuelcolvin merged commit 5195e55 into pydantic:master May 23, 2020

calvinwyoung deleted the enums-as-separate-models branch May 23, 2020 18:14

therefromhere mentioned this pull request May 28, 2020

Possible regression of __modify_schema__ for enum fields #1576

Closed

CasperWA mentioned this pull request Jun 12, 2020

Improvements/fixes for openapi.json Materials-Consortia/optimade-python-tools#332

Closed

3 tasks

PrettyWood mentioned this pull request Jul 21, 2020

feat(schema): support custom title, description and default for enums #1749

Merged

4 tasks

mostaphaRoudsari mentioned this pull request Jul 22, 2020

Enum as separate model #1173

Closed

PrettyWood mentioned this pull request Jul 25, 2020

Swagger default Enum missing and Optional field also tiangolo/fastapi#1775

Closed

PrettyWood added a commit to ToucanToco/toucan-connectors that referenced this pull request Jul 27, 2020

fix(test): update expected schemas with pydantic 1.6

4980c97

see pydantic/pydantic#1432

PrettyWood added a commit to ToucanToco/toucan-connectors that referenced this pull request Jul 27, 2020

fix(test): update expected schemas with pydantic 1.6

de1a0e2

see pydantic/pydantic#1432

PrettyWood added a commit to ToucanToco/toucan-connectors that referenced this pull request Jul 27, 2020

fix(test): update expected schemas with pydantic 1.6

ad36202

see pydantic/pydantic#1432

PrettyWood added a commit to ToucanToco/toucan-connectors that referenced this pull request Jul 27, 2020

fix(test): update expected schemas with pydantic 1.6

3632ae4

see pydantic/pydantic#1432

pboehm mentioned this pull request Aug 20, 2020

Regression in OpenAPI schema generation with multiple Enums having the same name but different enum members #1857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enums as separate models #1432

Enums as separate models #1432

calvinwyoung commented Apr 25, 2020

codecov bot commented Apr 25, 2020 •

edited

calvinwyoung commented Apr 25, 2020

samuelcolvin left a comment •

edited

samuelcolvin Apr 30, 2020

calvinwyoung Apr 30, 2020 •

edited

samuelcolvin commented Apr 30, 2020

calvinwyoung commented Apr 30, 2020

samuelcolvin commented May 23, 2020

tiangolo commented May 23, 2020

samuelcolvin commented May 23, 2020

tiangolo commented May 23, 2020 •

edited

calvinwyoung commented May 23, 2020

samuelcolvin commented May 23, 2020

tiangolo commented May 23, 2020

samuelcolvin commented May 23, 2020

calvinwyoung commented May 23, 2020

	Take a single ``enum`` and generate its schema.
	Take a single `enum` and generate its schema.

Enums as separate models #1432

Enums as separate models #1432

Conversation

calvinwyoung commented Apr 25, 2020

Change Summary

Related issue number

Checklist

codecov bot commented Apr 25, 2020 • edited

Codecov Report

calvinwyoung commented Apr 25, 2020

samuelcolvin left a comment • edited

Choose a reason for hiding this comment

samuelcolvin Apr 30, 2020

Choose a reason for hiding this comment

calvinwyoung Apr 30, 2020 • edited

Choose a reason for hiding this comment

samuelcolvin commented Apr 30, 2020

calvinwyoung commented Apr 30, 2020

samuelcolvin commented May 23, 2020

tiangolo commented May 23, 2020

samuelcolvin commented May 23, 2020

tiangolo commented May 23, 2020 • edited

calvinwyoung commented May 23, 2020

samuelcolvin commented May 23, 2020

tiangolo commented May 23, 2020

samuelcolvin commented May 23, 2020

calvinwyoung commented May 23, 2020

codecov bot commented Apr 25, 2020 •

edited

samuelcolvin left a comment •

edited

calvinwyoung Apr 30, 2020 •

edited

tiangolo commented May 23, 2020 •

edited