tortoise-orm icon indicating copy to clipboard operation
tortoise-orm copied to clipboard

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

Open madnesspie opened this issue 5 years ago • 6 comments

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!

madnesspie avatar May 05 '20 10:05 madnesspie

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

hqsz avatar May 05 '20 10:05 hqsz

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

hqsz avatar May 05 '20 11:05 hqsz

@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.

madnesspie avatar May 05 '20 11:05 madnesspie

@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?

hqsz avatar May 05 '20 11:05 hqsz

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.

long2ice avatar May 05 '20 14:05 long2ice

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.

grigi avatar May 05 '20 17:05 grigi