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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.28%. Comparing base (b0c8f02) to head (6ad671f).
Report is 44 commits behind head on main.

Files Patch % Lines
litestar/_openapi/schema_generation/schema.py 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3525   +/-   ##
=======================================
  Coverage   98.28%   98.28%           
=======================================
  Files         328      328           
  Lines       14886    14890    +4     
  Branches     2367     2367           
=======================================
+ Hits        14631    14635    +4     
  Misses        116      116           
  Partials      139      139           

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

Copy link

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

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/private-api This PR involves changes to the privatized API pr/external size: small Triage Required 🏥 This requires triage type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Enum OAS issues
5 participants