tortoise / tortoise-orm

Familiar asyncio ORM for python, built with relations in mind
https://tortoise.github.io
Apache License 2.0
4.55k stars 374 forks source link

Model.create method doesn't raise an error if it gets unexpected arg #381

Open madnesspie opened 4 years ago

madnesspie commented 4 years ago

Hi, there! First of all, thanks for your amazing work!

The problem that cause feature request.

class SomeModel(models.Model):
    id = fields.IntField(pk=True)  # pylint: disable=invalid-name

obj = SomeModel.create(
    nonexistent_field="lol"
)

The code above is executed without errors. It surprised me because I expect to see an error due to I specified the field that doesn't exist in the model.

Describe the solution you'd like Maybe it will be good to add fields validation in the method and related methods as well?

I ready to contribute this feature if you consider it as useful. Thank for attention!

hqsz commented 4 years ago

Your example is wrong. because tortoise is based on async. instead of obj = SomeModel.create(nonexistent_field="lol") use obj = await SomeModel.create(...)

hqsz commented 4 years ago

After fix example, below logs will come

  File "test.py", line 14, in <module>
    run_async(run())
  File "/storage/tortoise-orm/tortoise/__init__.py", line 636, in run_async
    loop.run_until_complete(coro)
  File "/root/.pyenv/versions/3.7.6/lib/python3.7/asyncio/base_events.py", line 583, in run_until_complete
    return future.result()
  File "test.py", line 11, in run
    obj = await SomeModel.create(non='lol')
  File "/storage/tortoise-orm/tortoise/models.py", line 925, in create
    await instance.save(using_db=db)
  File "/storage/tortoise-orm/tortoise/models.py", line 819, in save
    executor = db.executor_class(model=self.__class__, db=db)
  File "/storage/tortoise-orm/tortoise/backends/base/executor.py", line 53, in __init__
    self.insert_query = self._prepare_insert_statement(columns)
  File "/storage/tortoise-orm/tortoise/backends/base/executor.py", line 144, in _prepare_insert_statement
    .columns(*columns)
  File "/root/.pyenv/versions/py37/lib/python3.7/site-packages/pypika/utils.py", line 50, in _copy
    result = func(self_copy, *args, **kwargs)
  File "/root/.pyenv/versions/py37/lib/python3.7/site-packages/pypika/queries.py", line 764, in columns
    if isinstance(terms[0], (list, tuple)):
IndexError: tuple index out of range

I found current preparing insert SQL with Model that have only pk with IntField and BigIntField is error (Maybe related to pypika ... ). I'll look into it and make hotfix

ps) I found that tortoise is working normally with hotfix of https://github.com/kayak/pypika/pull/431. I think we don't need to make hotfix to tortoise. But if need we can use below patch

diff --git a/tortoise/backends/base/executor.py b/tortoise/backends/base/executor.py
index b0ebf9b..d041db0 100644
--- a/tortoise/backends/base/executor.py
+++ b/tortoise/backends/base/executor.py
@@ -141,7 +141,7 @@ class BaseExecutor:
         # go to descendant executors
         return str(
             self.db.query_class.into(self.model._meta.basetable)
-            .columns(*columns)
+            .columns(columns)
             .insert(*[self.parameter(i) for i in range(len(columns))])
         )
madnesspie commented 4 years ago

@lntuition Oh. I fix it as well, but the issue still exists. You can try following code:

class SomeModel(models.Model):
    id = fields.IntField(pk=True)  # pylint: disable=invalid-name
    field = fields.IntField()

async def example():
    obj = await models.SomeModel.create(
        nonexistent_field="lol",
        field=1,
    )

It will work without any validation error.

hqsz commented 4 years ago

@madnesspie Ah you are right, With your fixed example i found same issue too. Seems like that we are now just ignoring the unexpected fields... @grigi I think raise Exception is valid here. Could you look this plz?

long2ice commented 4 years ago

According to the source code, tortoise call setattr() in Model.__init__() when call create(),and generate query with exists field which call fields_db_projection,so it don't raise some exception and the final result is right, so the question is whether we should raise exception when get some invalid fields.

grigi commented 4 years ago

I'll have to confirm if annotations only use _from_db(), then we can consider adding some kind of validation to the constructor.

We could probably do this by adding an else here: https://github.com/tortoise/tortoise-orm/blob/develop/tortoise/models.py#L668 But would need to test if it breaks anything.