From 8376fb023abdc163a776f9b6100cd27c75b9dbf8 Mon Sep 17 00:00:00 2001 From: collerek Date: Sun, 26 Sep 2021 12:07:08 +0200 Subject: [PATCH 1/4] fix for #348 --- ormar/fields/model_fields.py | 4 +- .../test_pk_field_is_always_not_null.py | 39 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 tests/test_model_definition/test_pk_field_is_always_not_null.py diff --git a/ormar/fields/model_fields.py b/ormar/fields/model_fields.py index ac1d534..9455f8d 100644 --- a/ormar/fields/model_fields.py +++ b/ormar/fields/model_fields.py @@ -91,7 +91,9 @@ class ModelFieldFactory: nullable, default, server_default, pydantic_only ) or is_auto_primary_key(primary_key, autoincrement) sql_nullable = ( - nullable if sql_nullable is None else (sql_nullable and not primary_key) + False + if primary_key + else (nullable if sql_nullable is None else sql_nullable) ) namespace = dict( diff --git a/tests/test_model_definition/test_pk_field_is_always_not_null.py b/tests/test_model_definition/test_pk_field_is_always_not_null.py new file mode 100644 index 0000000..22b03a3 --- /dev/null +++ b/tests/test_model_definition/test_pk_field_is_always_not_null.py @@ -0,0 +1,39 @@ +import databases +import sqlalchemy + +import ormar +from tests.settings import DATABASE_URL + +database = databases.Database(DATABASE_URL) +metadata = sqlalchemy.MetaData() + + +class BaseMeta(ormar.ModelMeta): + metadata = metadata + database = database + + +class AutoincrementModel(ormar.Model): + class Meta(BaseMeta): + pass + + id: int = ormar.Integer(primary_key=True) + + +class NonAutoincrementModel(ormar.Model): + class Meta(BaseMeta): + pass + + id: int = ormar.Integer(primary_key=True, autoincrement=False) + + +class ExplicitNullableModel(ormar.Model): + class Meta(BaseMeta): + pass + + id: int = ormar.Integer(primary_key=True, nullable=True) + + +def test_pk_field_is_not_null(): + for model in [AutoincrementModel, NonAutoincrementModel, ExplicitNullableModel]: + assert not model.Meta.table.c.get("id").nullable From 67487ebf9cfc18c7a4ec756b851171f6f069eb1c Mon Sep 17 00:00:00 2001 From: collerek Date: Sun, 26 Sep 2021 12:43:09 +0200 Subject: [PATCH 2/4] fix for #354 - allow None for nullable field with choices --- ormar/models/helpers/validation.py | 6 +++-- tests/test_model_definition/test_models.py | 31 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/ormar/models/helpers/validation.py b/ormar/models/helpers/validation.py index 617381f..e7d66fe 100644 --- a/ormar/models/helpers/validation.py +++ b/ormar/models/helpers/validation.py @@ -52,8 +52,8 @@ def convert_choices_if_needed( # noqa: CCR001 :param field: ormar field to check with choices :type field: BaseField - :param values: current values of the model to verify - :type values: Dict + :param value: current values of the model to verify + :type value: Dict :return: value, choices list :rtype: Tuple[Any, List] """ @@ -97,6 +97,8 @@ def validate_choices(field: "BaseField", value: Any) -> None: :type value: Any """ value, choices = convert_choices_if_needed(field=field, value=value) + if field.nullable: + choices.append(None) if value is not ormar.Undefined and value not in choices: raise ValueError( f"{field.name}: '{value}' " f"not in allowed choices set:" f" {choices}" diff --git a/tests/test_model_definition/test_models.py b/tests/test_model_definition/test_models.py index 71d12aa..583ba9d 100644 --- a/tests/test_model_definition/test_models.py +++ b/tests/test_model_definition/test_models.py @@ -134,6 +134,26 @@ class Country(ormar.Model): ) +class NullableCountry(ormar.Model): + class Meta: + tablename = "country2" + metadata = metadata + database = database + + id: int = ormar.Integer(primary_key=True) + name: str = ormar.String(max_length=9, choices=country_name_choices, nullable=True) + + +class NotNullableCountry(ormar.Model): + class Meta: + tablename = "country3" + metadata = metadata + database = database + + id: int = ormar.Integer(primary_key=True) + name: str = ormar.String(max_length=9, choices=country_name_choices, nullable=False) + + @pytest.fixture(autouse=True, scope="module") def create_test_database(): engine = sqlalchemy.create_engine(DATABASE_URL) @@ -538,6 +558,17 @@ async def test_model_choices(): await Country.objects.filter(name="Belize").update(name="Vietnam") +@pytest.mark.asyncio +async def test_nullable_field_model_choices(): + """Test that choices work properly for according to nullable setting""" + async with database: + c1 = await NullableCountry(name=None).save() + assert c1.name is None + + with pytest.raises(ValueError): + await NotNullableCountry(name=None).save() + + @pytest.mark.asyncio async def test_start_and_end_filters(): async with database: From 1da9111dbdc5594b2a0aaa26fba5c174e74cd70d Mon Sep 17 00:00:00 2001 From: collerek Date: Sun, 26 Sep 2021 14:07:47 +0200 Subject: [PATCH 3/4] add extra to Model.Meta, update docs and bump version --- docs/models/index.md | 31 +++++++++ docs/releases.md | 13 +++- ormar/__init__.py | 5 +- ormar/models/__init__.py | 3 +- ormar/models/helpers/models.py | 3 + ormar/models/metaclass.py | 2 + ormar/models/newbasemodel.py | 8 +++ ormar/models/utils.py | 6 ++ .../test_extra_ignore_parameter.py | 63 +++++++++++++++++++ .../test_extra_ignore_parameter.py | 28 +++++++++ 10 files changed, 158 insertions(+), 4 deletions(-) create mode 100644 ormar/models/utils.py create mode 100644 tests/test_fastapi/test_extra_ignore_parameter.py create mode 100644 tests/test_model_definition/test_extra_ignore_parameter.py diff --git a/docs/models/index.md b/docs/models/index.md index bba6966..428b389 100644 --- a/docs/models/index.md +++ b/docs/models/index.md @@ -417,6 +417,37 @@ So to overwrite setting or provide your own a sample model can look like followi --8<-- "../docs_src/models/docs016.py" ``` +### Extra fields in models + +By default `ormar` forbids you to pass extra fields to Model. + +If you try to do so the `ModelError` will be raised. + +Since the extra fields cannot be saved in the database the default to disallow such fields seems a feasible option. + +On the contrary in `pydantic` the default option is to ignore such extra fields, therefore `ormar` provides an `Meta.extra` setting to behave in the same way. + +To ignore extra fields passed to `ormar` set this setting to `Extra.ignore` instead of default `Extra.forbid`. + +Note that `ormar` does not allow accepting extra fields, you can only ignore them or forbid them (raise exception if present) + +```python +from ormar import Extra + +class Child(ormar.Model): + class Meta(ormar.ModelMeta): + tablename = "children" + metadata = metadata + database = database + extra = Extra.ignore # set extra setting to prevent exceptions on extra fields presence + + id: int = ormar.Integer(name="child_id", primary_key=True) + first_name: str = ormar.String(name="fname", max_length=100) + last_name: str = ormar.String(name="lname", max_length=100) +``` + +To set the same setting on all model check the [best practices]("../models/index/#best-practice") and `BaseMeta` concept. + ## Model sort order When querying the database with given model by default the Model is ordered by the `primary_key` diff --git a/docs/releases.md b/docs/releases.md index 651023e..babf6e6 100644 --- a/docs/releases.md +++ b/docs/releases.md @@ -1,3 +1,14 @@ +# 0.10.20 + +## ✨ Features + +* Add `extra` parameter in `Model.Meta` that accepts `Extra.ignore` and `Extra.forbid` (default) and either ignores the extra fields passed to `ormar` model or raises an exception if one is encountered [#358](https://github.com/collerek/ormar/issues/358) + +## 🐛 Fixes + +* Allow `None` if field is nullable and have choices set [#354](https://github.com/collerek/ormar/issues/354) +* Always set `primary_key` to `not null` regardless of `autoincrement` and explicit `nullable` setting to avoid problems with migrations [#348](https://github.com/collerek/ormar/issues/348) + # 0.10.19 ## ✨ Features @@ -960,4 +971,4 @@ Add queryset level methods [#68]: https://github.com/collerek/ormar/issues/68 [#70]: https://github.com/collerek/ormar/issues/70 [#71]: https://github.com/collerek/ormar/issues/71 -[#73]: https://github.com/collerek/ormar/issues/73 \ No newline at end of file +[#73]: https://github.com/collerek/ormar/issues/73 diff --git a/ormar/__init__.py b/ormar/__init__.py index 859159e..1d49395 100644 --- a/ormar/__init__.py +++ b/ormar/__init__.py @@ -64,7 +64,7 @@ from ormar.fields import ( UUID, UniqueColumns, ) # noqa: I100 -from ormar.models import ExcludableItems, Model +from ormar.models import ExcludableItems, Model, Extra from ormar.models.metaclass import ModelMeta from ormar.queryset import OrderAction, QuerySet, and_, or_ from ormar.relations import RelationType @@ -78,7 +78,7 @@ class UndefinedType: # pragma no cover Undefined = UndefinedType() -__version__ = "0.10.19" +__version__ = "0.10.20" __all__ = [ "Integer", "BigInteger", @@ -130,4 +130,5 @@ __all__ = [ "ENCODERS_MAP", "DECODERS_MAP", "LargeBinary", + "Extra", ] diff --git a/ormar/models/__init__.py b/ormar/models/__init__.py index 694043e..1bac952 100644 --- a/ormar/models/__init__.py +++ b/ormar/models/__init__.py @@ -8,5 +8,6 @@ from ormar.models.newbasemodel import NewBaseModel # noqa I100 from ormar.models.model_row import ModelRow # noqa I100 from ormar.models.model import Model, T # noqa I100 from ormar.models.excludable import ExcludableItems # noqa I100 +from ormar.models.utils import Extra # noqa I100 -__all__ = ["NewBaseModel", "Model", "ModelRow", "ExcludableItems", "T"] +__all__ = ["NewBaseModel", "Model", "ModelRow", "ExcludableItems", "T", "Extra"] diff --git a/ormar/models/helpers/models.py b/ormar/models/helpers/models.py index 73d5b4e..e7e4473 100644 --- a/ormar/models/helpers/models.py +++ b/ormar/models/helpers/models.py @@ -7,6 +7,7 @@ import pydantic from pydantic.typing import ForwardRef import ormar # noqa: I100 from ormar.models.helpers.pydantic import populate_pydantic_default_values +from ormar.models.utils import Extra if TYPE_CHECKING: # pragma no cover from ormar import Model @@ -52,6 +53,8 @@ def populate_default_options_values( new_model.Meta.model_fields = model_fields if not hasattr(new_model.Meta, "abstract"): new_model.Meta.abstract = False + if not hasattr(new_model.Meta, "extra"): + new_model.Meta.extra = Extra.forbid if not hasattr(new_model.Meta, "orders_by"): new_model.Meta.orders_by = [] if not hasattr(new_model.Meta, "exclude_parent_fields"): diff --git a/ormar/models/metaclass.py b/ormar/models/metaclass.py index e3c67e8..1eb84ec 100644 --- a/ormar/models/metaclass.py +++ b/ormar/models/metaclass.py @@ -49,6 +49,7 @@ from ormar.models.helpers import ( sqlalchemy_columns_from_model_fields, ) from ormar.models.quick_access_views import quick_access_set +from ormar.models.utils import Extra from ormar.queryset import FieldAccessor, QuerySet from ormar.relations.alias_manager import AliasManager from ormar.signals import Signal, SignalEmitter @@ -83,6 +84,7 @@ class ModelMeta: requires_ref_update: bool orders_by: List[str] exclude_parent_fields: List[str] + extra: Extra def add_cached_properties(new_model: Type["Model"]) -> None: diff --git a/ormar/models/newbasemodel.py b/ormar/models/newbasemodel.py index 04968d4..b6add0c 100644 --- a/ormar/models/newbasemodel.py +++ b/ormar/models/newbasemodel.py @@ -18,6 +18,8 @@ from typing import ( cast, ) +from ormar.models.utils import Extra + try: import orjson as json except ImportError: # pragma: no cover @@ -250,6 +252,12 @@ class NewBaseModel(pydantic.BaseModel, ModelTableProxy, metaclass=ModelMetaclass for field_name in self.extract_through_names(): through_tmp_dict[field_name] = kwargs.pop(field_name, None) + if self.Meta.extra == Extra.ignore: + kwargs = { + k: v + for k, v in kwargs.items() + if k in model_fields or k in pydantic_fields + } try: new_kwargs: Dict[str, Any] = { k: self._convert_to_bytes( diff --git a/ormar/models/utils.py b/ormar/models/utils.py new file mode 100644 index 0000000..bd04974 --- /dev/null +++ b/ormar/models/utils.py @@ -0,0 +1,6 @@ +from enum import Enum + + +class Extra(str, Enum): + ignore = "ignore" + forbid = "forbid" diff --git a/tests/test_fastapi/test_extra_ignore_parameter.py b/tests/test_fastapi/test_extra_ignore_parameter.py new file mode 100644 index 0000000..87309c8 --- /dev/null +++ b/tests/test_fastapi/test_extra_ignore_parameter.py @@ -0,0 +1,63 @@ +import json + +import databases +import pytest +import sqlalchemy +from fastapi import FastAPI +from starlette.testclient import TestClient + +import ormar +from ormar import Extra +from tests.settings import DATABASE_URL + +app = FastAPI() +metadata = sqlalchemy.MetaData() +database = databases.Database(DATABASE_URL, force_rollback=True) +app.state.database = database + + +@app.on_event("startup") +async def startup() -> None: + database_ = app.state.database + if not database_.is_connected: + await database_.connect() + + +@app.on_event("shutdown") +async def shutdown() -> None: + database_ = app.state.database + if database_.is_connected: + await database_.disconnect() + + +class Item(ormar.Model): + class Meta: + database = database + metadata = metadata + extra = Extra.ignore + + id: int = ormar.Integer(primary_key=True) + name: str = ormar.String(max_length=100) + + +@pytest.fixture(autouse=True, scope="module") +def create_test_database(): + engine = sqlalchemy.create_engine(DATABASE_URL) + metadata.create_all(engine) + yield + metadata.drop_all(engine) + + +@app.post("/item/", response_model=Item) +async def create_item(item: Item): + return await item.save() + + +def test_extra_parameters_in_request(): + client = TestClient(app) + with client as client: + data = {"name": "Name", "extraname": "to ignore"} + resp = client.post("item/", data=json.dumps(data)) + assert resp.status_code == 200 + assert "name" in resp.json() + assert resp.json().get("name") == "Name" diff --git a/tests/test_model_definition/test_extra_ignore_parameter.py b/tests/test_model_definition/test_extra_ignore_parameter.py new file mode 100644 index 0000000..cd3960c --- /dev/null +++ b/tests/test_model_definition/test_extra_ignore_parameter.py @@ -0,0 +1,28 @@ +import databases +import sqlalchemy + +import ormar +from ormar import Extra +from tests.settings import DATABASE_URL + +database = databases.Database(DATABASE_URL, force_rollback=True) +metadata = sqlalchemy.MetaData() + + +class Child(ormar.Model): + class Meta(ormar.ModelMeta): + tablename = "children" + metadata = metadata + database = database + extra = Extra.ignore + + id: int = ormar.Integer(name="child_id", primary_key=True) + first_name: str = ormar.String(name="fname", max_length=100) + last_name: str = ormar.String(name="lname", max_length=100) + + +def test_allow_extra_parameter(): + child = Child(first_name="Test", last_name="Name", extra_param="Unexpected") + assert child.first_name == "Test" + assert child.last_name == "Name" + assert not hasattr(child, "extra_param") From ca87e74b2ec59ec2fbe4207abdfcca9171cf1944 Mon Sep 17 00:00:00 2001 From: collerek Date: Sun, 26 Sep 2021 14:20:47 +0200 Subject: [PATCH 4/4] fix some code smells --- ormar/models/helpers/validation.py | 5 ++--- ormar/models/newbasemodel.py | 32 ++++++++++++++++++++++++------ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/ormar/models/helpers/validation.py b/ormar/models/helpers/validation.py index e7d66fe..01ebb9e 100644 --- a/ormar/models/helpers/validation.py +++ b/ormar/models/helpers/validation.py @@ -234,10 +234,9 @@ def get_pydantic_example_repr(type_: Any) -> Any: """ if issubclass(type_, (numbers.Number, decimal.Decimal)): return 0 - elif issubclass(type_, pydantic.BaseModel): + if issubclass(type_, pydantic.BaseModel): return generate_pydantic_example(pydantic_model=type_) - else: - return "string" + return "string" def overwrite_example_and_description( diff --git a/ormar/models/newbasemodel.py b/ormar/models/newbasemodel.py index b6add0c..6b4eccf 100644 --- a/ormar/models/newbasemodel.py +++ b/ormar/models/newbasemodel.py @@ -252,12 +252,9 @@ class NewBaseModel(pydantic.BaseModel, ModelTableProxy, metaclass=ModelMetaclass for field_name in self.extract_through_names(): through_tmp_dict[field_name] = kwargs.pop(field_name, None) - if self.Meta.extra == Extra.ignore: - kwargs = { - k: v - for k, v in kwargs.items() - if k in model_fields or k in pydantic_fields - } + kwargs = self._remove_extra_parameters_if_they_should_be_ignored( + kwargs=kwargs, model_fields=model_fields, pydantic_fields=pydantic_fields + ) try: new_kwargs: Dict[str, Any] = { k: self._convert_to_bytes( @@ -283,6 +280,29 @@ class NewBaseModel(pydantic.BaseModel, ModelTableProxy, metaclass=ModelMetaclass return new_kwargs, through_tmp_dict + def _remove_extra_parameters_if_they_should_be_ignored( + self, kwargs: Dict, model_fields: Dict, pydantic_fields: Set + ) -> Dict: + """ + Removes the extra fields from kwargs if they should be ignored. + + :param kwargs: passed arguments + :type kwargs: Dict + :param model_fields: dictionary of model fields + :type model_fields: Dict + :param pydantic_fields: set of pydantic fields names + :type pydantic_fields: Set + :return: dict without extra fields + :rtype: Dict + """ + if self.Meta.extra == Extra.ignore: + kwargs = { + k: v + for k, v in kwargs.items() + if k in model_fields or k in pydantic_fields + } + return kwargs + def _initialize_internal_attributes(self) -> None: """ Initializes internal attributes during __init__()