-
-
Notifications
You must be signed in to change notification settings - Fork 752
support optional annotated field with default value #862
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
base: main
Are you sure you want to change the base?
Conversation
looks like some (many) tests are failing, I'll try to fix them! Is this feature something that should work with Pydantic v1 too? I'm not too familiar with it. |
I've just hit exactly this problem; is there any chance you could update your MR please @proever? |
Would argue this PR fixes a bug and is not a FR pydantic validators is a core reason for the existence of both pydantic and SQLModel. SQLModel currently only supports validators using decorator-pattern Which is neither DRY nor reusable SQLModel does not support reusable Validators using annotated-pattern This PR would fix this. Example pydantic validator
Apply to Field
or
Equivalent to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it, this PR is basically about fixing the following scenario:
MyType: TypeAlias = Annotated[str, Field(max_length=5)]
class PriceModel(SQLModel, table=True):
id: int | None = Field(primary_key=True, default=None)
working: MyType # VARCHAR(5)
not_working: Optional[MyType] = None # VARCHAR(255)
You are going to open a Pandora's box :)
I think this PR only handles one specific case, but in general we need to merge all nested Field
annotations and handle conflicts. This seems to be a lot of work and quite error-prone.
Although, it would be nice to support this one day.
I suggest we skip it for now and get back to this feature later.
Seems to be related: #1281
As for initial code example, you can make it working by moving | None
inside Annotated
:
price: Annotated[Decimal | None, Field(decimal_places=2, max_digits=9)] = None
@proever, thanks for your interest and efforts!
@@ -653,13 +659,24 @@ def get_sqlalchemy_type(field: Any) -> Any: | |||
return sa_type | |||
|
|||
type_ = get_sa_type_from_field(field) | |||
metadata = get_field_metadata(field) | |||
if isinstance(type_, _AnnotatedAlias): | |||
class_to_compare = type_.__origin__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to use_type
?
Currently, it is not possible to specify a
SQLModel
with anOptional
complex type, such as aDecimal
, as follows:Doing so results in the following error (see #67, #312) in
get_sqlalchemy_type
:TypeError: issubclass() arg 1 must be a class
This PR attempts to fix this issue by adding a check in
get_sqlalchemy_type
for whether the type of theField
is atyping_extensions.AnnotatedAlias
. If it is, instead of using the class of theAnnotatedAlias
for the following comparisons (which results in the above error), it usesAnnotatedAlias.__origin__
. Similarly, it infers the metadata fromAnnotatedAlias.__metadata__
instead of theAnnotatedAlias
itself.In my testing, this approach seems to work well. I also added a simple test here that checks whether restrictions on an optional annotated field are enforced on the database side. It could probably be improved or extended.