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 type checks ordering in get_sqlalchemy_type #442

Closed
wants to merge 1 commit into from

Conversation

pierrecdn
Copy link
Contributor

When enum subclassing/mix-in[1] is used, especially to ensure compatibility and produce correct OpenAPI specifications with FastAPI[2], SQLModel on its side doesn't map to the correct SQLAlchemy type.

e.g.:

  • class Foo(Enum) will work but won't allow a proper OpenAPI spec. to be generated.
  • class Foo(str, Enum) will fix the previous situation, but prevents SQLModel to read and map the object from the DB (due to validation errors).

Changing ordering of checks to ensure that Enum subclasses are tested first.

[1] https://docs.python.org/3/library/enum.html#restricted-enum-subclassing
[2] https://fastapi.tiangolo.com/sq/tutorial/path-params/#create-an-enum-class

When enum subclassing/mix-in[1] is used, especially to ensure compatibility
and produce correct OpenAPI specifications with FastAPI[2], SQLModel on its
side doesn't map to the correct SQLAlchemy type.

e.g.:
* `class Foo(Enum)` will work but won't allow a proper OpenAPI spec. to be
generated.
* `class Foo(str, Enum)` will fix the previous situation, but prevents
SQLModel to read and map the object from the DB (due to validation errors).

Changing ordering of checks to ensure that Enum subclasses are tested first.

[1] https://docs.python.org/3/library/enum.html#restricted-enum-subclassing
[2] https://fastapi.tiangolo.com/sq/tutorial/path-params/#create-an-enum-class
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

📝 Docs preview for commit a6d9568 at: https://631a68e284fe1d20404a6714--sqlmodel.netlify.app

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #442 (a6d9568) into main (75ce455) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #442   +/-   ##
=======================================
  Coverage   97.76%   97.76%           
=======================================
  Files         187      187           
  Lines        6268     6268           
=======================================
  Hits         6128     6128           
  Misses        140      140           
Impacted Files Coverage Δ
sqlmodel/main.py 84.92% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pierrecdn
Copy link
Contributor Author

Hmm, seeing that I'm not the only one to observe this:

@tiangolo any thoughts?

@daniil-berg
Copy link
Contributor

This makes sense. It may need to be rebased with PR #425.

If PR #436 is merged, you'll be able to specify the SQL type explicitly whenever you want. Then PR #366 will become obsolete. It's still worth maintaining/optimizing get_sqlachemy_type though to cover the simple and straightforward cases, so that you don't need to specify the SQL type.

@pierrecdn
Copy link
Contributor Author

If PR #436 is merged, you'll be able to specify the SQL type explicitly whenever you want. Then PR #366 will become obsolete. It's still worth maintaining/optimizing get_sqlachemy_type though to cover the simple and straightforward cases, so that you don't need to specify the SQL type.

Indeed, just seen you proposal (#436), sounds really nice.
The PR is quite large though, so it might take longer to get merged.

@tiangolo
Copy link
Owner

Thank you!

As I can't push to this branch, I included your commits in a new branch in a new PR here: #669

Given that, I'll close this one. Thanks for the work! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants