New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #28393 -- Added a helpful exception for invalid AutoField/IntegerField values. #8760

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@vdboor
Contributor

vdboor commented Jul 13, 2017

When a large model is updated and saved with invalid values, it produces a traceback deep within the ORM, with no clue which field assignment caused the error. Developers are faced with tracebacks like:

  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/base.py", line 953, in _do_update
    return filtered._update(values) > 0
  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/query.py", line 661, in _update
    return query.get_compiler(self.db).execute_sql(CURSOR)
  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1191, in execute_sql
    cursor = super(SQLUpdateCompiler, self).execute_sql(result_type)
  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 863, in execute_sql
    sql, params = self.as_sql()
  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1157, in as_sql
    val = field.get_db_prep_save(val, connection=self.connection)
  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 766, in get_db_prep_save
    prepared=False)
  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 758, in get_db_prep_value
    value = self.get_prep_value(value)
  File "/Users/diederik/Sites/virtualenvs/test/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 1853, in get_prep_value
    self.name, repr(value)
   TypeError: int() argument must be a string, a bytes-like object or a number, not 'tuple'

This change displays the field name which makes spotting errors a lot easier.

For example, it shows:

TypeError: remote_id: expected string or a number, not (39468135737,)

Trac issue: https://code.djangoproject.com/ticket/28393

@timgraham timgraham changed the title from Make invalid AutoField/IntegerField values easier to debug to Fixed #28393 -- Added a helpful exception for invalid AutoField/IntegerField values. Jul 13, 2017

@vdboor

This comment has been minimized.

Show comment
Hide comment
@vdboor

vdboor Jul 17, 2017

Contributor

My apologies for not noticing the test issues; I ran a plain tox session which apparently skips database tests.

I've fixed the test issues now by reraising the original exception type, and using the raise .. from .. syntax.

I'm a bit in a doubt whether the TypeError/ValueError should be combined to a single type. It's tempting, yet I don't have a clear picture of the consequences that might hold for other projects. It could also introduce new inconsistencies with other field types that don't have such try..catch check. My main aim here is to ease debugging, not to complicate matters so I've left that alone.

Looking into the difference:

  • TypeError occurs when the int(..) doesn't know how to cast a type (e.g. dict, tuple)
  • ValueError occurs when the int(..) fails to cast a known supported type (e.g. str)
Contributor

vdboor commented Jul 17, 2017

My apologies for not noticing the test issues; I ran a plain tox session which apparently skips database tests.

I've fixed the test issues now by reraising the original exception type, and using the raise .. from .. syntax.

I'm a bit in a doubt whether the TypeError/ValueError should be combined to a single type. It's tempting, yet I don't have a clear picture of the consequences that might hold for other projects. It could also introduce new inconsistencies with other field types that don't have such try..catch check. My main aim here is to ease debugging, not to complicate matters so I've left that alone.

Looking into the difference:

  • TypeError occurs when the int(..) doesn't know how to cast a type (e.g. dict, tuple)
  • ValueError occurs when the int(..) fails to cast a known supported type (e.g. str)
@pope1ni

This comment has been minimized.

Show comment
Hide comment
@pope1ni

pope1ni Jul 17, 2017

Contributor

Would it not be possible to do the following to keep it DRY?

try:
   ...
except (TypeError, ValueError) as e:
    raise e.__class__(...) from e

Perhaps a little magic, but saves on the copy-paste...

Contributor

pope1ni commented Jul 17, 2017

Would it not be possible to do the following to keep it DRY?

try:
   ...
except (TypeError, ValueError) as e:
    raise e.__class__(...) from e

Perhaps a little magic, but saves on the copy-paste...

@vdboor

This comment has been minimized.

Show comment
Hide comment
@vdboor

vdboor Jul 17, 2017

Contributor

Good point @pope1ni, I've updated the code. The magic can be counter-balanced with a small comment here I believe.

Contributor

vdboor commented Jul 17, 2017

Good point @pope1ni, I've updated the code. The magic can be counter-balanced with a small comment here I believe.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 17, 2017

Member

Looks okay, but new tests are also required. Uncheck "Patch needs improvement" on the ticket when completed.

Member

timgraham commented Jul 17, 2017

Looks okay, but new tests are also required. Uncheck "Patch needs improvement" on the ticket when completed.

@pope1ni

This comment has been minimized.

Show comment
Hide comment
@pope1ni

pope1ni Jul 17, 2017

Contributor

While you're at it, can you do the same for floatin FloatField.get_prep_value()?

I also noticed that, while there are no conflicts with the base branch, that this pull request branch is "1204 commits behind django:master". That is very far behind and the repository has had many significant changes including removal of support for Python 2. Perhaps it is worth rebasing against upstream master?

Contributor

pope1ni commented Jul 17, 2017

While you're at it, can you do the same for floatin FloatField.get_prep_value()?

I also noticed that, while there are no conflicts with the base branch, that this pull request branch is "1204 commits behind django:master". That is very far behind and the repository has had many significant changes including removal of support for Python 2. Perhaps it is worth rebasing against upstream master?

Make invalid AutoField/IntegerField/FloatField values easier to debug
When a large model is updated and saved with invalid values,
it produces a traceback deep within the ORM, with no clue
which field assignment caused the error. Developers are faced with:
"TypeError: int() argument must be a string, a bytes-like object or a number, not 'tuple'"
This change displays the field name which makes spotting errors a lot easier.

Trac issue 28393
@vdboor

This comment has been minimized.

Show comment
Hide comment
@vdboor

vdboor Jul 20, 2017

Contributor

@pope1ni, I've updated the FloatField too and rebased on master.

@timgraham, before I continue writing extra tests, I wonder about one thing: The other field types (DateField, BooleanField, DecimalField etc..) all use to_python() from get_prep_value(). Their code is suspiciously similar to my proposed change, except that they raise a ValidationError instead of a TypeError/ValueError.

Would it make sense to port the IntegerField, FloatField and AutoField to this construct instead? It would introduce a backwards incompatibility change however, which my current solution does not.

Contributor

vdboor commented Jul 20, 2017

@pope1ni, I've updated the FloatField too and rebased on master.

@timgraham, before I continue writing extra tests, I wonder about one thing: The other field types (DateField, BooleanField, DecimalField etc..) all use to_python() from get_prep_value(). Their code is suspiciously similar to my proposed change, except that they raise a ValidationError instead of a TypeError/ValueError.

Would it make sense to port the IntegerField, FloatField and AutoField to this construct instead? It would introduce a backwards incompatibility change however, which my current solution does not.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 20, 2017

Member

I think backwards compatibility is important. I know I've written code that catches these exceptions.

Member

timgraham commented Jul 20, 2017

I think backwards compatibility is important. I know I've written code that catches these exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment