From d1fd2e3fab4489b0395614c284f132c6b73b3ba2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Kr=C3=BCger=20Svensson?= Date: Mon, 29 Aug 2022 12:46:25 +0200 Subject: [PATCH 1/5] =?UTF-8?q?=E2=9C=85=20Add=20test=20for=20nullable=20f?= =?UTF-8?q?ields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/test_nullable.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 tests/test_nullable.py diff --git a/tests/test_nullable.py b/tests/test_nullable.py new file mode 100644 index 0000000000..b69a8cdadd --- /dev/null +++ b/tests/test_nullable.py @@ -0,0 +1,36 @@ +from typing import Optional + +import pytest +from sqlalchemy.exc import InterfaceError +from sqlalchemy.orm import Session +from sqlmodel import Field, SQLModel, create_engine + + +def test_nullable_fields_set_properly(clear_sqlmodel, caplog): + class Hero(SQLModel, table=True): + nullable_integer_primary_key: Optional[int] = Field( + default=None, primary_key=True + ) + nullable_optional_string: Optional[str] = Field(default=None, nullable=True) + nullable_integer: Optional[int] = None + not_null_string: str = Field(default=..., nullable=False) + not_null_integer: int + + engine = create_engine("sqlite://", echo=True) + SQLModel.metadata.create_all(engine) + + create_table_log = [ + message for message in caplog.messages if "CREATE TABLE hero" in message + ][0] + assert "\n\tnullable_integer_primary_key INTEGER NOT NULL," in create_table_log + assert "\n\tnot_null_string VARCHAR NOT NULL," in create_table_log + assert "\n\tnullable_optional_string VARCHAR," in create_table_log + assert "\n\tnullable_integer INTEGER," in create_table_log + assert "\n\tnot_null_integer INTEGER NOT NULL," in create_table_log + + # Ensure we cannot create a hero with `None` set for not-nullable-fields: + hero = Hero(not_null_integer=None, not_null_string=None) + with Session(engine) as session: + session.add(hero) + with pytest.raises(InterfaceError): + session.commit() From 4ca8a40a6e2ddf14a392452b1daae5268b4eda38 Mon Sep 17 00:00:00 2001 From: Benjamin Rapaport Date: Mon, 29 Aug 2022 10:54:33 -0400 Subject: [PATCH 2/5] - Update tests/test_nullable.py to exercise the failing condition of setting an `Optional` type but passing `nullable=True` to the `Field` definition - Fix the bug --- sqlmodel/main.py | 4 ++- tests/test_nullable.py | 72 +++++++++++++++++++++++++++++++++--------- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/sqlmodel/main.py b/sqlmodel/main.py index a5ce8faf74..8bf6326592 100644 --- a/sqlmodel/main.py +++ b/sqlmodel/main.py @@ -423,11 +423,13 @@ def get_column_from_field(field: ModelField) -> Column: # type: ignore index = getattr(field.field_info, "index", Undefined) if index is Undefined: index = False + nullable = not primary_key and _is_field_nullable(field) + # Override derived nullability if the nullable property is set explicitly + # on the field if hasattr(field.field_info, "nullable"): field_nullable = getattr(field.field_info, "nullable") if field_nullable != Undefined: nullable = field_nullable - nullable = not primary_key and _is_field_nullable(field) args = [] foreign_key = getattr(field.field_info, "foreign_key", None) unique = getattr(field.field_info, "unique", False) diff --git a/tests/test_nullable.py b/tests/test_nullable.py index b69a8cdadd..7054a66d3d 100644 --- a/tests/test_nullable.py +++ b/tests/test_nullable.py @@ -1,20 +1,63 @@ from typing import Optional import pytest -from sqlalchemy.exc import InterfaceError -from sqlalchemy.orm import Session -from sqlmodel import Field, SQLModel, create_engine +from sqlalchemy.exc import IntegrityError +from sqlmodel import Field, Session, SQLModel, create_engine def test_nullable_fields_set_properly(clear_sqlmodel, caplog): + class Hero(SQLModel, table=True): + primary_key: Optional[int] = Field( + default=None, + primary_key=True, + ) + required_value: str + optional_non_nullable: Optional[str] = Field( + nullable=False, + ) + optional_default_ellipses_non_nullable: Optional[str] = Field( + default=..., + nullable=False, + ) + optional_default_none_non_nullable: Optional[str] = Field( + default=None, + nullable=False, + ) + default_ellipses_non_nullable: str = Field(default=..., nullable=False) + optional_default_none_nullable: Optional[str] = Field( + default=None, + nullable=True, + ) + + engine = create_engine("sqlite://", echo=True) + SQLModel.metadata.create_all(engine) + + create_table_log = [ + message for message in caplog.messages if "CREATE TABLE hero" in message + ][0] + assert "\n\tprimary_key INTEGER NOT NULL," in create_table_log + assert "\n\trequired_value VARCHAR NOT NULL," in create_table_log + assert "\n\toptional_non_nullable VARCHAR NOT NULL," in create_table_log + assert ( + "\n\toptional_default_ellipses_non_nullable VARCHAR NOT NULL," + in create_table_log + ) + assert ( + "\n\toptional_default_none_non_nullable VARCHAR NOT NULL," in create_table_log + ) + assert "\n\tdefault_ellipses_non_nullable VARCHAR NOT NULL," in create_table_log + assert "\n\toptional_default_none_nullable VARCHAR," in create_table_log + + +# Test for regression in https://github.com/tiangolo/sqlmodel/issues/420 +def test_non_nullable_optional_field_with_no_default_set(clear_sqlmodel, caplog): class Hero(SQLModel, table=True): nullable_integer_primary_key: Optional[int] = Field( - default=None, primary_key=True + default=None, + primary_key=True, ) - nullable_optional_string: Optional[str] = Field(default=None, nullable=True) - nullable_integer: Optional[int] = None - not_null_string: str = Field(default=..., nullable=False) - not_null_integer: int + + optional_non_nullable_no_default: Optional[str] = Field(nullable=False) engine = create_engine("sqlite://", echo=True) SQLModel.metadata.create_all(engine) @@ -23,14 +66,13 @@ class Hero(SQLModel, table=True): message for message in caplog.messages if "CREATE TABLE hero" in message ][0] assert "\n\tnullable_integer_primary_key INTEGER NOT NULL," in create_table_log - assert "\n\tnot_null_string VARCHAR NOT NULL," in create_table_log - assert "\n\tnullable_optional_string VARCHAR," in create_table_log - assert "\n\tnullable_integer INTEGER," in create_table_log - assert "\n\tnot_null_integer INTEGER NOT NULL," in create_table_log + assert "\n\toptional_non_nullable_no_default VARCHAR NOT NULL," in create_table_log - # Ensure we cannot create a hero with `None` set for not-nullable-fields: - hero = Hero(not_null_integer=None, not_null_string=None) + # We can create a hero with `None` set for the optional non-nullable field + hero = Hero(nullable_integer_primary_key=123, optional_non_nullable_no_default=None) + # But we cannot commit it. with Session(engine) as session: session.add(hero) - with pytest.raises(InterfaceError): + with pytest.raises(IntegrityError): session.commit() + From 0253e58402cb6e55f4d2b67098371e994b0e9197 Mon Sep 17 00:00:00 2001 From: Benjamin Rapaport Date: Mon, 29 Aug 2022 11:00:10 -0400 Subject: [PATCH 3/5] run linter --- tests/test_nullable.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_nullable.py b/tests/test_nullable.py index 7054a66d3d..172e7cffca 100644 --- a/tests/test_nullable.py +++ b/tests/test_nullable.py @@ -75,4 +75,3 @@ class Hero(SQLModel, table=True): session.add(hero) with pytest.raises(IntegrityError): session.commit() - From d879efaccefc530302d5227afe2a2d94e60d36df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Ram=C3=ADrez?= Date: Tue, 30 Aug 2022 18:12:58 +0200 Subject: [PATCH 4/5] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Update=20checking=20no?= =?UTF-8?q?neability,=20accept=20fields=20marked=20as=20noneable=20with=20?= =?UTF-8?q?a=20non-None=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- sqlmodel/main.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sqlmodel/main.py b/sqlmodel/main.py index 8bf6326592..d343c698e9 100644 --- a/sqlmodel/main.py +++ b/sqlmodel/main.py @@ -423,7 +423,7 @@ def get_column_from_field(field: ModelField) -> Column: # type: ignore index = getattr(field.field_info, "index", Undefined) if index is Undefined: index = False - nullable = not primary_key and _is_field_nullable(field) + nullable = not primary_key and _is_field_noneable(field) # Override derived nullability if the nullable property is set explicitly # on the field if hasattr(field.field_info, "nullable"): @@ -646,11 +646,10 @@ def __tablename__(cls) -> str: return cls.__name__.lower() -def _is_field_nullable(field: ModelField) -> bool: +def _is_field_noneable(field: ModelField) -> bool: if not field.required: # Taken from [Pydantic](https://github.com/samuelcolvin/pydantic/blob/v1.8.2/pydantic/fields.py#L946-L947) - is_optional = field.allow_none and ( + return field.allow_none and ( field.shape != SHAPE_SINGLETON or not field.sub_fields ) - return is_optional and field.default is None and field.default_factory is None return False From fc62e388cd965527ac747d43a4c23496958f9be1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Ram=C3=ADrez?= Date: Tue, 30 Aug 2022 18:14:56 +0200 Subject: [PATCH 5/5] =?UTF-8?q?=E2=9C=85=20Update=20tests,=20add=20additio?= =?UTF-8?q?nal=20checks=20for=20other=20corner=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/test_nullable.py | 80 +++++++++++++++++++++++++++++++++--------- 1 file changed, 64 insertions(+), 16 deletions(-) diff --git a/tests/test_nullable.py b/tests/test_nullable.py index 172e7cffca..1c8b37b218 100644 --- a/tests/test_nullable.py +++ b/tests/test_nullable.py @@ -5,29 +5,50 @@ from sqlmodel import Field, Session, SQLModel, create_engine -def test_nullable_fields_set_properly(clear_sqlmodel, caplog): +def test_nullable_fields(clear_sqlmodel, caplog): class Hero(SQLModel, table=True): primary_key: Optional[int] = Field( default=None, primary_key=True, ) required_value: str + optional_default_ellipsis: Optional[str] = Field(default=...) + optional_default_none: Optional[str] = Field(default=None) optional_non_nullable: Optional[str] = Field( nullable=False, ) + optional_nullable: Optional[str] = Field( + nullable=True, + ) optional_default_ellipses_non_nullable: Optional[str] = Field( default=..., nullable=False, ) + optional_default_ellipses_nullable: Optional[str] = Field( + default=..., + nullable=True, + ) optional_default_none_non_nullable: Optional[str] = Field( default=None, nullable=False, ) - default_ellipses_non_nullable: str = Field(default=..., nullable=False) optional_default_none_nullable: Optional[str] = Field( default=None, nullable=True, ) + default_ellipses_non_nullable: str = Field(default=..., nullable=False) + optional_default_str: Optional[str] = "default" + optional_default_str_non_nullable: Optional[str] = Field( + default="default", nullable=False + ) + optional_default_str_nullable: Optional[str] = Field( + default="default", nullable=True + ) + str_default_str: str = "default" + str_default_str_non_nullable: str = Field(default="default", nullable=False) + str_default_str_nullable: str = Field(default="default", nullable=True) + str_default_ellipsis_non_nullable: str = Field(default=..., nullable=False) + str_default_ellipsis_nullable: str = Field(default=..., nullable=True) engine = create_engine("sqlite://", echo=True) SQLModel.metadata.create_all(engine) @@ -35,24 +56,33 @@ class Hero(SQLModel, table=True): create_table_log = [ message for message in caplog.messages if "CREATE TABLE hero" in message ][0] - assert "\n\tprimary_key INTEGER NOT NULL," in create_table_log - assert "\n\trequired_value VARCHAR NOT NULL," in create_table_log - assert "\n\toptional_non_nullable VARCHAR NOT NULL," in create_table_log - assert ( - "\n\toptional_default_ellipses_non_nullable VARCHAR NOT NULL," - in create_table_log - ) + assert "primary_key INTEGER NOT NULL," in create_table_log + assert "required_value VARCHAR NOT NULL," in create_table_log + assert "optional_default_ellipsis VARCHAR NOT NULL," in create_table_log + assert "optional_default_none VARCHAR," in create_table_log + assert "optional_non_nullable VARCHAR NOT NULL," in create_table_log + assert "optional_nullable VARCHAR," in create_table_log assert ( - "\n\toptional_default_none_non_nullable VARCHAR NOT NULL," in create_table_log + "optional_default_ellipses_non_nullable VARCHAR NOT NULL," in create_table_log ) - assert "\n\tdefault_ellipses_non_nullable VARCHAR NOT NULL," in create_table_log - assert "\n\toptional_default_none_nullable VARCHAR," in create_table_log + assert "optional_default_ellipses_nullable VARCHAR," in create_table_log + assert "optional_default_none_non_nullable VARCHAR NOT NULL," in create_table_log + assert "optional_default_none_nullable VARCHAR," in create_table_log + assert "default_ellipses_non_nullable VARCHAR NOT NULL," in create_table_log + assert "optional_default_str VARCHAR," in create_table_log + assert "optional_default_str_non_nullable VARCHAR NOT NULL," in create_table_log + assert "optional_default_str_nullable VARCHAR," in create_table_log + assert "str_default_str VARCHAR NOT NULL," in create_table_log + assert "str_default_str_non_nullable VARCHAR NOT NULL," in create_table_log + assert "str_default_str_nullable VARCHAR," in create_table_log + assert "str_default_ellipsis_non_nullable VARCHAR NOT NULL," in create_table_log + assert "str_default_ellipsis_nullable VARCHAR," in create_table_log # Test for regression in https://github.com/tiangolo/sqlmodel/issues/420 def test_non_nullable_optional_field_with_no_default_set(clear_sqlmodel, caplog): class Hero(SQLModel, table=True): - nullable_integer_primary_key: Optional[int] = Field( + primary_key: Optional[int] = Field( default=None, primary_key=True, ) @@ -65,13 +95,31 @@ class Hero(SQLModel, table=True): create_table_log = [ message for message in caplog.messages if "CREATE TABLE hero" in message ][0] - assert "\n\tnullable_integer_primary_key INTEGER NOT NULL," in create_table_log - assert "\n\toptional_non_nullable_no_default VARCHAR NOT NULL," in create_table_log + assert "primary_key INTEGER NOT NULL," in create_table_log + assert "optional_non_nullable_no_default VARCHAR NOT NULL," in create_table_log # We can create a hero with `None` set for the optional non-nullable field - hero = Hero(nullable_integer_primary_key=123, optional_non_nullable_no_default=None) + hero = Hero(primary_key=123, optional_non_nullable_no_default=None) # But we cannot commit it. with Session(engine) as session: session.add(hero) with pytest.raises(IntegrityError): session.commit() + + +def test_nullable_primary_key(clear_sqlmodel, caplog): + # Probably the weirdest corner case, it shouldn't happen anywhere, but let's test it + class Hero(SQLModel, table=True): + nullable_integer_primary_key: Optional[int] = Field( + default=None, + primary_key=True, + nullable=True, + ) + + engine = create_engine("sqlite://", echo=True) + SQLModel.metadata.create_all(engine) + + create_table_log = [ + message for message in caplog.messages if "CREATE TABLE hero" in message + ][0] + assert "nullable_integer_primary_key INTEGER," in create_table_log