Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#22210 closed Bug (fixed)

Saving Model instance to Float Field

Reported by: hphilip456@… Owned by: anonymous
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I came across what I believe to be an undesired behavior.

If we have a model such as:

class Person(models.Model):
    first_name = models.CharField(max_length=30)
    last_name = models.CharField(max_length=30)
    age = models.FloatField()

Then try to do this:

obj = Person.objects.get(pk=1)
obj.age = obj
obj.save()

It will succeed with out throwing any errors. If we then get that same instance back from the database the age of that instance will be one which is the primary key of the object.

This is clearly a corner case but I think it would be worth hardening django to not allow this behavior to happen. I thought that the get_prep_value() method would have been called which would have thrown a TypeError since you can not cast a model instance to a float. It turns out that this does not happen because django.db.models.sql.compiler.SQLUpdateCompiler.as_sql checks if the value of a field has the attribute 'prepare_database_save' which a model instance does it uses that instead of field.get_db_prep_save.

Change History (7)

comment:1 Changed 16 months ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Agreed that validation here could be tightened; it's an edge case, but a potentially confusing one if you do it by accident.

comment:2 Changed 16 months ago by PirosB3

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

Hi,

Thanks for the information.
I have managed to replicate the bug on a unit test and I have written a patch for it. The patch will be linked here soon. As it's my first patch, please let me know if anything is incorrect.

Regards,
Dan

comment:3 Changed 16 months ago by anonymous

Hi,

This is my initial pull request, please let me know if there is anything I need to adjust.
https://github.com/django/django/pull/2404

Regards,
Dan

comment:4 Changed 16 months ago by smeatonj

  • Has patch set
  • Patch needs improvement set

comment:5 Changed 16 months ago by smeatonj

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 16 months ago by Marc Tamlyn <marc.tamlyn@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 819e09b848e4f1cc45165a99ffbef1307b215a08:

Fixed #22210 -- Saving model instances to non-related fields.

Previously, saving a model instance to a non-related field (in
particular a FloatField) would silently convert the model to an Integer
(the pk) and save it. This is undesirable behaviour, and likely to cause
confusion so the validatio has been hardened.

Thanks to @PirosB3 for the patch and @jarshwah for the review.

comment:7 Changed 16 months ago by Shai Berger <shai@…>

In d181384e5f01e9d23fd9872b2cb532e0d70249b6:

Fixed test failure on Oracle: model_fields.tests.test_float_validates_object

Failing test introduced in fix for refs #22210.

Note: See TracTickets for help on using tickets.
Back to Top