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: Enum OAS generation (#3518) #3525

Merged
merged 13 commits into from
Nov 29, 2024

Conversation

wallseat
Copy link
Contributor

Changes described here #3518

Closes

Closes #3518

@wallseat wallseat requested review from a team as code owners May 25, 2024 11:32
@github-actions github-actions bot added area/private-api This PR involves changes to the privatized API size: small type/bug pr/external Triage Required 🏥 This requires triage labels May 25, 2024
Copy link

codecov bot commented May 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.36%. Comparing base (3510cab) to head (3cba89c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3525      +/-   ##
==========================================
+ Coverage   98.35%   98.36%   +0.01%     
==========================================
  Files         344      344              
  Lines       15485    15489       +4     
  Branches     1707     1708       +1     
==========================================
+ Hits        15230    15236       +6     
+ Misses        122      121       -1     
+ Partials      133      132       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Alc-Alc
Copy link
Contributor

Alc-Alc commented May 26, 2024

Thanks for the PR, but I believe your work kind of invalidates the work done here #2550?

Can you check the MCVE from the issue and confirm if this affects anything?

@JacobCoffee and @peterschutt ping for awareness

@peterschutt
Copy link
Contributor

@JacobCoffee and @peterschutt ping for awareness

I feel like there's been some lengthy discussion around this recently, in discord I think?? Is this that same issue?

@Alc-Alc
Copy link
Contributor

Alc-Alc commented May 26, 2024

@JacobCoffee and @peterschutt ping for awareness

I feel like there's been some lengthy discussion around this recently, in discord I think?? Is this that same issue?

This is the only thread I am aware of, raised by the same person

@peterschutt
Copy link
Contributor

@JacobCoffee and @peterschutt ping for awareness

I feel like there's been some lengthy discussion around this recently, in discord I think?? Is this that same issue?

This is the only thread I am aware of, raised by the same person

The one I'm thinking of starts here - it was a long discussion and not put into a thread unfortunately. Feels related.

@wallseat
Copy link
Contributor Author

wallseat commented May 26, 2024

Hi, @Alc-Alc and @peterschutt, thx for your comments. I read the #2546 issue and i think it is related to my issue, but it is more about the work of enum with optional in general
My fixes are:

  1. Remove null from enum values when it is marked as optional. Now null will passed as choose in oneOf. So enum still can be used with None, but null will not be the part of enum.
  2. Make enum referable like other scheme objects. Because it quite useful for those consuming specification.

I fix just two test - test_optional_enum and part with enum params in test_create_parameters. So other things with optional fields work as before

CC: @provinzkraut

@Alc-Alc
Copy link
Contributor

Alc-Alc commented May 26, 2024

Hi, @Alc-Alc and @peterschutt, thx for your comments. I read the #2546 issue and i think it is related to my issue, but it is more about the work of enum with optional in general My fixes are:

  1. Remove null from enum values when it is marked as optional. Now null will passed as choose in oneOf. So enum still can be used with None, but null will not be the part of enum.
  2. Make enum referable like other scheme objects. Because it quite useful for those consuming specification.

I fix just two test - test_optional_enum and part with enum params in test_create_parameters. So other things with optional fields work as before

CC: @provinzkraut

Have you confirmed if your changes are not affecting that?

from enum import StrEnum
from litestar import Litestar, get
from litestar.params import Parameter

class Environment(StrEnum):
    DEVELOPMENT = "dev"
    TEST = "test"
    QA = "qa"

@get(path="/")
async def test(
    environment: Environment | None
    = Parameter(
        title="Environment",
        description="The environment to filter by.",
        default=None,
        required=False,
    ),
) -> Environment | None:
    return environment

app = Litestar([test])

Current main

image

Your branch

image

@wallseat
Copy link
Contributor Author

wallseat commented May 26, 2024

Hi, @Alc-Alc and @peterschutt, thx for your comments. I read the #2546 issue and i think it is related to my issue, but it is more about the work of enum with optional in general My fixes are:

  1. Remove null from enum values when it is marked as optional. Now null will passed as choose in oneOf. So enum still can be used with None, but null will not be the part of enum.
  2. Make enum referable like other scheme objects. Because it quite useful for those consuming specification.

I fix just two test - test_optional_enum and part with enum params in test_create_parameters. So other things with optional fields work as before
CC: @provinzkraut

Have you confirmed if your changes are not affecting that?

from enum import StrEnum
from litestar import Litestar, get
from litestar.params import Parameter

class Environment(StrEnum):
    DEVELOPMENT = "dev"
    TEST = "test"
    QA = "qa"

@get(path="/")
async def test(
    environment: Environment | None
    = Parameter(
        title="Environment",
        description="The environment to filter by.",
        default=None,
        required=False,
    ),
) -> Environment | None:
    return environment

app = Litestar([test])

Current main

image Your branch image

Yes, my changes affected that, because Enum now putted in components
image
So it will acting like any other object schema with any other value allowed (in this case Enum | None).

And it is really strange, that swagger register Enum object here
image
but not display that this field is ref to this Enum or null
image

example without None
image

I think we should ask the swagger team about this)

@wallseat
Copy link
Contributor Author

Polar render this as expected)
image

@wallseat
Copy link
Contributor Author

@Alc-Alc Just a ping:)
Is there anything else needed on my part?

@Alc-Alc
Copy link
Contributor

Alc-Alc commented May 31, 2024

apologies for the delay here and thanks for being responsive, I am not much of an authority when it comes to OpenAPI, but if you claim it's a swagger issue, then I have nothing against this being merged, however I would appreciate if @peterschutt and other @litestar-org/maintainers take a look.

@wallseat
Copy link
Contributor Author

@peterschutt @Alc-Alc
Any updates on this?

@peterschutt
Copy link
Contributor

@peterschutt @Alc-Alc Any updates on this?

Not from me at this point @wallseat - apologies, but I've got a bit going on at work at the moment and haven't had a chance to look at anything litestar related for the last few weeks.

@wallseat
Copy link
Contributor Author

wallseat commented Jun 28, 2024

@provinzkraut
Can you take a look? I think it's not a big change and you also worked on OAS

@provinzkraut
Copy link
Member

provinzkraut commented Jun 30, 2024

@wallseat The fix seem reasonable, I'm just concerned about breaking swagger. I'd like to confirm this is an issue on their end, not ours (and potentially just avoid breaking this for our users, since a lot of them use this as their primary interactive docs 👀)

IMO we can move forward if we have either confirmation that it's a swagger bug, or a way to not break this for our users.

@evgene-sh
Copy link

Hello everyone! I have the same problem with enum and null in OpenAPI in my project.

What will happen with this fix?

@findstar
Copy link
Contributor

Hi, @Alc-Alc and @peterschutt, thx for your comments. I read the #2546 issue and i think it is related to my issue, but it is more about the work of enum with optional in general My fixes are:

  1. Remove null from enum values when it is marked as optional. Now null will passed as choose in oneOf. So enum still can be used with None, but null will not be the part of enum.
  2. Make enum referable like other scheme objects. Because it quite useful for those consuming specification.

I fix just two test - test_optional_enum and part with enum params in test_create_parameters. So other things with optional fields work as before
CC: @provinzkraut

Have you confirmed if your changes are not affecting that?

from enum import StrEnum
from litestar import Litestar, get
from litestar.params import Parameter

class Environment(StrEnum):
    DEVELOPMENT = "dev"
    TEST = "test"
    QA = "qa"

@get(path="/")
async def test(
    environment: Environment | None
    = Parameter(
        title="Environment",
        description="The environment to filter by.",
        default=None,
        required=False,
    ),
) -> Environment | None:
    return environment

app = Litestar([test])

Current main

image Your branch image

@wallseat @Alc-Alc
This issue seems to originate from the OAS (OpenAPI Specification).

I observed that different Swagger UI results are displayed depending on whether null appears first or $ref is listed first as an option in oneOf.

Case 1 - When type: null comes first

"parameters": [
  {
    "name": "environment",
    "in": "query",
    "schema": {
      "oneOf": [
        {
          "type": "null"
        },
        {
          "$ref": "#/components/schemas/Environment"
        }
      ],
      "title": "Environment",
      "description": "The environment to filter by."
    },
    "description": "The environment to filter by.",
    "required": false,
    "deprecated": false,
    "allowEmptyValue": false,
    "allowReserved": false
  }
]
image

Case 2 - When $ref comes first

"parameters": [
  {
    "name": "environment",
    "in": "query",
    "schema": {
      "oneOf": [
        {
          "$ref": "#/components/schemas/Environment"
        },
        {
          "type": "null"
        }
      ],
      "title": "Environment",
      "description": "The environment to filter by."
    },
    "description": "The environment to filter by.",
    "required": false,
    "deprecated": false,
    "allowEmptyValue": false,
    "allowReserved": false
  }
]
image

@wallseat How about changing it to generate the ref schema first?

Additionally, in the case of FastAPI, it generates the ref schema first when handling the same enum | None parameter.

{
    "openapi": "3.1.0",
    "info": {
      "title": "FastAPI",
      "version": "0.1.0"
    },
    "paths": {
      "/": {
        "get": {
          "summary": "Test",
          "operationId": "test__get",
          "parameters": [
            {
              "name": "environment",
              "in": "query",
              "required": false,
              "schema": {
                "anyOf": [
                  {
                    "$ref": "#/components/schemas/Environment"
                  },
                  {
                    "type": "null"
                  }
                ],
                "title": "Environment",
                "description": "The environment to filter by.",
                "required": false
              },
              "description": "The environment to filter by."
            }
          ],
          "responses": {
            "200": {
              "description": "Successful Response",
              "content": {
                "application/json": {
                  "schema": {
                    "anyOf": [
                      {
                        "$ref": "#/components/schemas/Environment"
                      },
                      {
                        "type": "null"
                      }
                    ],
                    "title": "Response Test  Get"
                  }
                }
              }
            },
            "422": {
              "description": "Validation Error",
              "content": {
                "application/json": {
                  "schema": {
                    "$ref": "#/components/schemas/HTTPValidationError"
                  }
                }
              }
            }
          }
        }
      }
    },
    "components": {
      "schemas": {
        "Environment": {
          "type": "string",
          "enum": [
            "dev",
            "test",
            "qa"
          ],
          "title": "Environment"
        },
        "HTTPValidationError": {
          "properties": {
            "detail": {
              "items": {
                "$ref": "#/components/schemas/ValidationError"
              },
              "type": "array",
              "title": "Detail"
            }
          },
          "type": "object",
          "title": "HTTPValidationError"
        },
        "ValidationError": {
          "properties": {
            "loc": {
              "items": {
                "anyOf": [
                  {
                    "type": "string"
                  },
                  {
                    "type": "integer"
                  }
                ]
              },
              "type": "array",
              "title": "Location"
            },
            "msg": {
              "type": "string",
              "title": "Message"
            },
            "type": {
              "type": "string",
              "title": "Error Type"
            }
          },
          "type": "object",
          "required": [
            "loc",
            "msg",
            "type"
          ],
          "title": "ValidationError"
        }
      }
    }
  }

@provinzkraut
Copy link
Member

@findstar This is very interesting! Since I can't imagine the order making a difference being a deliberate design choice, maybe this actually is a bug in swagger and an issue should be filed there?

@isyangban
Copy link

isyangban commented Nov 25, 2024

@provinzkraut @findstar Hi. I’ve recently encountered this issue as well, and I’d like to share my opinion. Additionally, I’m happy to provide up to date patch of this PR that can be applied to the main branch, especially since this PR has been open for quite a while.

I believe this PR attempts to address 1 out of 2 issues:

  1. Generating a single schema object for an enum, using a reference, and reusing the object for other req/res objects.
  2. Excluding null in the OpenAPI schema enum property because it is a type, not a string.
  3. The order of the type "null" and "string" in the generated OpenAPI schema.

Issue 2 is currently circumvented in this PR by using the oneOf keyword instead of the type keyword. (I’m not sure if it should be anyOf instead, because based on the FastAPI-generated output, it uses anyOf rather than oneOf.)

As @findstar mentioned, swagger-ui seems to have a bug where the order of types matters ("string" and "null" works, but "null" and "string" doesn’t). However, according to the OpenAPI spec (OAI/OpenAPI-Specification#2244 (comment)), it’s clear that the order of "string" and "null" shouldn’t matter. So, filing an issue with swagger-ui seems reasonable. Or litestar can maybe change the code to change the generated order if it takes a long time to fix in swagger-ui.

That said, I think problems 1 and 2 still persist in the current version of litestar and should be addressed on litestar’s side. Problem 1 is particularly important because, when using a code generator to create client types from OpenAPI specs, you end up with many duplicated enum types on the client side, even though there’s only one Enum type on the server side. FastAPI fixed this years ago (fastapi/fastapi#1303).
Problem 2 is significant for the sake of correctness. (After further investigation i think it is correct to have null on enum)

@wallseat
Copy link
Contributor Author

wallseat commented Nov 25, 2024

Hi @isyangban, @findstar
I prepared an update for this PR based on your observations

I changed an order, in which elements are generated, however, there are no changes in the swagger

Test code example:

from enum import Enum
from typing import Optional

from litestar import Litestar, get
from litestar.openapi import OpenAPIConfig
from litestar.params import Parameter


class Environment(Enum):
    DEVELOPMENT = "dev"
    TEST = "test"
    QA = "qa"


@get(path="/")
async def test(
    environment: Optional[Environment] = Parameter(  # noqa: B008
        title="Environment",
        description="The environment to filter by.",
        default=None,
        required=False,
    ),
) -> Optional[Environment]:
    return environment


app = Litestar(
    [test],
    openapi_config=OpenAPIConfig(
        title="test",
        version="0.0.1",
        path="/docs",
        root_schema_site="swagger",
    ),
)

Generated OAS:

{
  "info": {
    "title": "test",
    "version": "0.0.1"
  },
  "openapi": "3.1.0",
  "servers": [
    {
      "url": "/"
    }
  ],
  "paths": {
    "/": {
      "get": {
        "summary": "Test",
        "operationId": "Test",
        "parameters": [
          {
            "name": "environment",
            "in": "query",
            "schema": {
              "oneOf": [
                {
                  "$ref": "#/components/schemas/Environment"
                },
                {
                  "type": "null"
                }
              ],
              "title": "Environment",
              "description": "The environment to filter by."
            },
            "description": "The environment to filter by.",
            "required": false,
            "deprecated": false,
            "allowEmptyValue": false,
            "allowReserved": false
          }
        ],
        "responses": {
          "200": {
            "description": "Request fulfilled, document follows",
            "headers": {

            },
            "content": {
              "application/json": {
                "schema": {
                  "oneOf": [
                    {
                      "$ref": "#/components/schemas/Environment"
                    },
                    {
                      "type": "null"
                    }
                  ]
                }
              }
            }
          },
          "400": {
            "description": "Bad request syntax or unsupported method",
            "content": {
              "application/json": {
                "schema": {
                  "properties": {
                    "status_code": {
                      "type": "integer"
                    },
                    "detail": {
                      "type": "string"
                    },
                    "extra": {
                      "additionalProperties": {

                      },
                      "type": [
                        "null",
                        "object",
                        "array"
                      ]
                    }
                  },
                  "type": "object",
                  "required": [
                    "detail",
                    "status_code"
                  ],
                  "description": "Validation Exception",
                  "examples": [
                    {
                      "status_code": 400,
                      "detail": "Bad Request",
                      "extra": {

                      }
                    }
                  ]
                }
              }
            }
          }
        },
        "deprecated": false
      }
    }
  },
  "components": {
    "schemas": {
      "Environment": {
        "type": "string",
        "enum": [
          "dev",
          "test",
          "qa"
        ],
        "title": "Environment",
        "description": "An enumeration."
      }
    }
  }
}

Swagger UI:
image

@wallseat
Copy link
Contributor Author

@wallseat I think i've found the problem with swagger-ui

app = Litestar(
    [test],
    openapi_config=OpenAPIConfig(
        title="Test API",
        version="0.0.1",
        path="/docs",
        render_plugins=[
            # SwaggerRenderPlugin(version="5.18.2"),  # newer version will work like a charm
            SwaggerRenderPlugin(), # Stuck with "5.1.3" version
            JsonRenderPlugin(),
            ScalarRenderPlugin(),
        ],
    ),
)

Should we update default pinned version?
@provinzkraut

@findstar
Copy link
Contributor

@wallseat I think i've found the problem with swagger-ui

app = Litestar(
    [test],
    openapi_config=OpenAPIConfig(
        title="Test API",
        version="0.0.1",
        path="/docs",
        render_plugins=[
            # SwaggerRenderPlugin(version="5.18.2"),  # newer version will work like a charm
            SwaggerRenderPlugin(), # Stuck with "5.1.3" version
            JsonRenderPlugin(),
            ScalarRenderPlugin(),
        ],
    ),
)

Should we update default pinned version? @provinzkraut

I tested the following openapi.json with Swagger UI version 5.18.2

{
  "info": {
    "title": "Test API",
    "version": "0.0.1"
  },
  "openapi": "3.1.0",
  "servers": [
    {
      "url": "/"
    }
  ],
  "paths": {
    "/": {
      "get": {
        "summary": "Test",
        "operationId": "Test",
        "parameters": [
          {
            "name": "environment",
            "in": "query",
            "schema": {
              "anyOf": [
                {
                  "$ref": "#/components/schemas/Environment"
                },
                {
                  "type": "null"
                }
              ],
              "title": "Environment",
              "description": "The environment to filter by."
            },
            "description": "The environment to filter by.",
            "required": false,
            "deprecated": false,
            "allowEmptyValue": false,
            "allowReserved": false
          }
        ],
        "responses": {
          "200": {
            "description": "Request fulfilled, document follows",
            "headers": {
              
            },
            "content": {
              "application/json": {
                "schema": {
                  "anyOf": [
                    {
                      "$ref": "#/components/schemas/Environment"
                    },
                    {
                      "type": "null"
                    }
                  ]
                }
              }
            }
          },
          "400": {
            "description": "Bad request syntax or unsupported method",
            "content": {
              "application/json": {
                "schema": {
                  "properties": {
                    "status_code": {
                      "type": "integer"
                    },
                    "detail": {
                      "type": "string"
                    },
                    "extra": {
                      "additionalProperties": {
                        
                      },
                      "type": [
                        "null",
                        "object",
                        "array"
                      ]
                    }
                  },
                  "type": "object",
                  "required": [
                    "detail",
                    "status_code"
                  ],
                  "description": "Validation Exception",
                  "examples": [
                    {
                      "status_code": 400,
                      "detail": "Bad Request",
                      "extra": {
                        
                      }
                    }
                  ]
                }
              }
            }
          }
        },
        "deprecated": false
      }
    }
  },
  "components": {
    "schemas": {
      "Environment": {
        "type": "string",
        "enum": [
          "dev",
          "test",
          "qa"
        ],
        "title": "Environment"
      }
    }
  }
}

image

Upon checking more precisely, I found that versions up to 5.17.9 display the same results as before, but starting from version 5.17.10, a revised view is displayed.

image

image

OAS issue swagger-api/swagger-ui#7912 and PR swagger-api/swagger-ui#9934

cc. isyangban

@provinzkraut
Copy link
Member

Bumping swagger to the non-broken version seems to address all the open issues here. With that done, this PR would be good to merge I think. @Alc-Alc for a second opinion :)

@wallseat
Copy link
Contributor Author

wallseat commented Nov 25, 2024

@isyangban
So, if we change "oneOf" key to "anyOf" key in case of Enum | None, should we change this for other situations too?
For example, if we have an Union of str and int, then key will be "oneOf", but if we add None to this union then key becomes "anyOf".

Maybe we can keep using oneOf? because according to the RFC that's how it is

https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-00#section-10.2.1.2

* return one_of
@github-actions github-actions bot added area/docs This PR involves changes to the documentation size: medium and removed size: small labels Nov 25, 2024
@isyangban
Copy link

@wallseat
After skimming through other implementations and spec docs, it appears that using oneOf is more appropriate than anyOf. That said, it seems like the OpenAPI spec and JSON Schema spec leave some aspects ambiguous, which allows for varying interpretations by different implementations :(. As a result, there are multiple valid ways to represent the same nullable type. For example, Pydantic uses anyOf instead of oneOf, though it comes with some quirks (see: pydantic/pydantic#7161).

In my view, the most compatible approach would be to use type: [T, "null"] when T is a primitive type, and oneOf: [{type: T}, {type: "null"}] otherwise. However, for simplicity, we could just stick with oneOf: [{type: T}, {type: "null"}] for now.

These are just only my personal thoughts, and I think it’s best for the litestar maintainers @provinzkraut and @Alc-Alc to decide?.

@wallseat
Copy link
Contributor Author

@isyangban
I leave it oneOf, because it is less to change:)
(Less chance to break smth)

@wallseat
Copy link
Contributor Author

wallseat commented Nov 26, 2024

@provinzkraut @Alc-Alc

I think this PR is ready to review and merge.
Summary:

  1. Add $reft schema creation for Enum objects
  2. Update pinned swagger version and change order of "null" and other elements in oneOf (also create an issue on swagger side Order of "null" in anyOf/oneOf array change the behaivor of produced docs swagger-api/swagger-ui#10225)
  3. Fix tests according new "null" elements order
  4. Change tests for new Enum $ref logic

Can you pls restart runners? They failed on pdm setup

cc: @isyangban @findstar

@Alc-Alc
Copy link
Contributor

Alc-Alc commented Nov 26, 2024

Can you pls restart runners? They failed on pdm setup

I am fixing that in this PR :)

#3878

Copy link
Contributor

@Alc-Alc Alc-Alc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this locally and I can see the swagger page I expect.

Thanks @wallseat for keeping up with this PR. My bad for any delays I have caused 🙂

Also, thanks to @findstar and @isyangban for reporting your findings 🔥

@provinzkraut this is a good to go from me

@provinzkraut provinzkraut enabled auto-merge (squash) November 27, 2024 14:04
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3525

@cofin cofin disabled auto-merge November 27, 2024 15:02
@provinzkraut
Copy link
Member

@wallseat can you address the coverage issues by adding the missing test cases?

@wallseat
Copy link
Contributor Author

@provinzkraut Done

@Alc-Alc
Copy link
Contributor

Alc-Alc commented Nov 28, 2024

I added a "no branch" pragma because the method in question is always executed with enums (in the context of its current usage). It did not seem like a benefit to add a new test to check a behavior that would not happen in the actual workflow.

The doc errors are unrelated to this PR. Will update if I find anything wrt that.

@Alc-Alc
Copy link
Contributor

Alc-Alc commented Nov 29, 2024

@litestar-org/maintainers can we merge this considering it failed due to doc issues?

@provinzkraut provinzkraut merged commit 35a9837 into litestar-org:main Nov 29, 2024
23 of 24 checks passed
@wallseat wallseat deleted the change-oprional-enum-schema branch November 29, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs This PR involves changes to the documentation area/private-api This PR involves changes to the privatized API pr/external pr/internal size: medium Triage Required 🏥 This requires triage type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Enum OAS issues
7 participants