Merge pull request #359 from collerek/fix_nullable_pk

Add extra to ignore extra fields, fix around handling None and nullable

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`

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
+[#73]: https://github.com/collerek/ormar/issues/73

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",
 ]

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(

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"]

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"):

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
+    :param value: current values of the model to verify
-    :type values: Dict
+    :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}"
@@ -232,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(

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:

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,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)
 
+        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(
@@ -275,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__()

ormar/models/utils.py

@@ -0,0 +1,6 @@
+from enum import Enum
+
+
+class Extra(str, Enum):
+    ignore = "ignore"
+    forbid = "forbid"

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"

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")

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:

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