Discussion:
Should Model.save() fix incorrect types that happen to save correctly?
Tim Graham
2017-02-13 14:57:49 UTC
Permalink
Once in a while, there's a ticket about this behavior:

m = Model(decimal='12.9')
m.save()
self.assertEqual(m.decimal, '12.9')
m.refresh_from_db()
self.assertEqual(m.decimal, Decimal('12.9'))

That is, you can create a model with an incorrect type and it won't be
fixed until you refresh the object from the database.

Most recent complaint, "it is only a basic expectation that the DB layer
and the Application layer will correspond to each-other after performing
save, which is in other words, syncing your state with the DB. Personally,
this bug (one way binding between application and db on save) broke many of
my tests and took a lot of my time." [0] See [1] for another manifestation
of this.

I think calling to_python() in Model.save() would have unacceptable
performance consequences for little benefit considering that it's a
reasonable expectation for developers to provide the correct type. Further,
you can use full_clean() to coerce to the correct types:

m = Model(decimal='12.9')
m.full_clean()
self.assertEqual(m.decimal, Decimal('12.9'))

What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.

[0] https://code.djangoproject.com/ticket/27825
[1] https://code.djangoproject.com/ticket/24028
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/68e9d568-c627-4734-847a-1d7ac934281f%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Florian Apolloner
2017-02-13 15:21:50 UTC
Permalink
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
Seems sensible.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/b3d5ad3a-7824-4e19-b61e-1199b2038ce5%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Aymeric Augustin
2017-02-13 15:25:36 UTC
Permalink
Hello,

I think this falls in the realm of duck typing. In Python, the programmer is allowed to use one data type instead of another — str instead of decimal.Decimal in this example — as long as they’re “sufficiently compatible” (where “sufficiently” depends on chance and taste, among others).

It’s definitely a gotcha that should be documented. But the general fix for this issue would be to check types on assignment to model fields. Even if Django did an `is instance` check, you could still have problems with subclasses, especially with custom fields.

On a personal note, I encountered an interesting variant of this recently. I have a model with a JSONField. Some values in the JSONField are dates. After saving to the database and reloading, they come back as ISO-formatted strings.

When I create instances of this mode, I leave the values as dates (because that works) and I call refresh_from_db() right after create()/save() to make sure my code always gets consistent type.

Would `full_clean` or `to_python` handle that? I’d be impressed if they did. And if Django’s JSONField doesn’t do it correctly, it’s preposterous to assume that existing custom fields do. I’m not in favour of making a promise that will inevitably be broken by many existing custom fields.

Best regards,
--
Aymeric.
Post by Tim Graham
m = Model(decimal='12.9')
m.save()
self.assertEqual(m.decimal, '12.9')
m.refresh_from_db()
self.assertEqual(m.decimal, Decimal('12.9'))
That is, you can create a model with an incorrect type and it won't be fixed until you refresh the object from the database.
Most recent complaint, "it is only a basic expectation that the DB layer and the Application layer will correspond to each-other after performing save, which is in other words, syncing your state with the DB. Personally, this bug (one way binding between application and db on save) broke many of my tests and took a lot of my time." [0] See [1] for another manifestation of this.
m = Model(decimal='12.9')
m.full_clean()
self.assertEqual(m.decimal, Decimal('12.9'))
What do you think? Absent a better suggestion, we could document this pitfall so that there's something to point to when related tickets come in.
[0] https://code.djangoproject.com/ticket/27825
[1] https://code.djangoproject.com/ticket/24028
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
Visit this group at https://groups.google.com/group/django-developers <https://groups.google.com/group/django-developers>.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/68e9d568-c627-4734-847a-1d7ac934281f%40googlegroups.com <https://groups.google.com/d/msgid/django-developers/68e9d568-c627-4734-847a-1d7ac934281f%40googlegroups.com?utm_medium=email&utm_source=footer>.
For more options, visit https://groups.google.com/d/optout <https://groups.google.com/d/optout>.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/794F91BF-3783-4CC6-9AD4-D2EB32196EC5%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.
charettes
2017-02-13 15:27:32 UTC
Permalink
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.

I also think this is the most appropriate solution.
Post by Tim Graham
m = Model(decimal='12.9')
m.save()
self.assertEqual(m.decimal, '12.9')
m.refresh_from_db()
self.assertEqual(m.decimal, Decimal('12.9'))
That is, you can create a model with an incorrect type and it won't be
fixed until you refresh the object from the database.
Most recent complaint, "it is only a basic expectation that the DB layer
and the Application layer will correspond to each-other after performing
save, which is in other words, syncing your state with the DB.
Personally, this bug (one way binding between application and db on save)
broke many of my tests and took a lot of my time." [0] See [1] for another
manifestation of this.
I think calling to_python() in Model.save() would have unacceptable
performance consequences for little benefit considering that it's a
reasonable expectation for developers to provide the correct type. Further,
m = Model(decimal='12.9')
m.full_clean()
self.assertEqual(m.decimal, Decimal('12.9'))
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
[0] https://code.djangoproject.com/ticket/27825
[1] https://code.djangoproject.com/ticket/24028
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Adam Johnson
2017-02-13 15:51:22 UTC
Permalink
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
+1, as Aymeric points out the more complex fields probably won't work with
coercion-on-set.
Post by Tim Graham
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
I also think this is the most appropriate solution.
Post by Tim Graham
m = Model(decimal='12.9')
m.save()
self.assertEqual(m.decimal, '12.9')
m.refresh_from_db()
self.assertEqual(m.decimal, Decimal('12.9'))
That is, you can create a model with an incorrect type and it won't be
fixed until you refresh the object from the database.
Most recent complaint, "it is only a basic expectation that the DB layer
and the Application layer will correspond to each-other after performing
save, which is in other words, syncing your state with the DB.
Personally, this bug (one way binding between application and db on save)
broke many of my tests and took a lot of my time." [0] See [1] for another
manifestation of this.
I think calling to_python() in Model.save() would have unacceptable
performance consequences for little benefit considering that it's a
reasonable expectation for developers to provide the correct type. Further,
m = Model(decimal='12.9')
m.full_clean()
self.assertEqual(m.decimal, Decimal('12.9'))
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
[0] https://code.djangoproject.com/ticket/27825
[1] https://code.djangoproject.com/ticket/24028
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/
msgid/django-developers/4e59974e-9916-434e-8840-
4f42996e5052%40googlegroups.com
<https://groups.google.com/d/msgid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%40googlegroups.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
--
Adam
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM1B0RJa928Yvk0uXenU009tsU%3DFDtPW_u2gmpgXTVpdFQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Marc Tamlyn
2017-02-14 09:39:33 UTC
Permalink
Agreed - ensuring that this works in the general case is not reliable.
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
Post by Tim Graham
pitfall so that there's something to point to when related tickets come in.
+1, as Aymeric points out the more complex fields probably won't work with
coercion-on-set.
Post by Tim Graham
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
I also think this is the most appropriate solution.
Post by Tim Graham
m = Model(decimal='12.9')
m.save()
self.assertEqual(m.decimal, '12.9')
m.refresh_from_db()
self.assertEqual(m.decimal, Decimal('12.9'))
That is, you can create a model with an incorrect type and it won't be
fixed until you refresh the object from the database.
Most recent complaint, "it is only a basic expectation that the DB layer
and the Application layer will correspond to each-other after performing
save, which is in other words, syncing your state with the DB.
Personally, this bug (one way binding between application and db on save)
broke many of my tests and took a lot of my time." [0] See [1] for another
manifestation of this.
I think calling to_python() in Model.save() would have unacceptable
performance consequences for little benefit considering that it's a
reasonable expectation for developers to provide the correct type. Further,
m = Model(decimal='12.9')
m.full_clean()
self.assertEqual(m.decimal, Decimal('12.9'))
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
[0] https://code.djangoproject.com/ticket/27825
[1] https://code.djangoproject.com/ticket/24028
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/ms
gid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%
40googlegroups.com
<https://groups.google.com/d/msgid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%40googlegroups.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
--
Adam
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/
msgid/django-developers/CAMyDDM1B0RJa928Yvk0uXenU009ts
U%3DFDtPW_u2gmpgXTVpdFQ%40mail.gmail.com
<https://groups.google.com/d/msgid/django-developers/CAMyDDM1B0RJa928Yvk0uXenU009tsU%3DFDtPW_u2gmpgXTVpdFQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMwjO1GtZwarHJ7joqTHo%3DR3euM40gbOqMc%3DyXeCO8g8AS3Rig%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Josh Smeaton
2017-02-14 12:11:58 UTC
Permalink
I posted on the ticket before reading this thread. Whoops. As mentioned
above it's impossible to always convert user input into the output provided
by the database without doing a round trip, because there are driver data
adapters, and django converters that both get a chance at modifying the
data as it flows through the system. We need to do a better job of
highlighting this limitation in some way.
Post by Marc Tamlyn
Agreed - ensuring that this works in the general case is not reliable.
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
Post by Tim Graham
pitfall so that there's something to point to when related tickets come in.
+1, as Aymeric points out the more complex fields probably won't work
with coercion-on-set.
Post by Tim Graham
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
I also think this is the most appropriate solution.
Post by Tim Graham
m = Model(decimal='12.9')
m.save()
self.assertEqual(m.decimal, '12.9')
m.refresh_from_db()
self.assertEqual(m.decimal, Decimal('12.9'))
That is, you can create a model with an incorrect type and it won't be
fixed until you refresh the object from the database.
Most recent complaint, "it is only a basic expectation that the DB
layer and the Application layer will correspond to each-other after
performing save, which is in other words, syncing your state with the
DB. Personally, this bug (one way binding between application and db on
save) broke many of my tests and took a lot of my time." [0] See [1] for
another manifestation of this.
I think calling to_python() in Model.save() would have unacceptable
performance consequences for little benefit considering that it's a
reasonable expectation for developers to provide the correct type. Further,
m = Model(decimal='12.9')
m.full_clean()
self.assertEqual(m.decimal, Decimal('12.9'))
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
[0] https://code.djangoproject.com/ticket/27825
[1] https://code.djangoproject.com/ticket/24028
--
You received this message because you are subscribed to the Google
Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send
<javascript:>.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%40googlegroups.com
<https://groups.google.com/d/msgid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%40googlegroups.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
--
Adam
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an
<javascript:>.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/CAMyDDM1B0RJa928Yvk0uXenU009tsU%3DFDtPW_u2gmpgXTVpdFQ%40mail.gmail.com
<https://groups.google.com/d/msgid/django-developers/CAMyDDM1B0RJa928Yvk0uXenU009tsU%3DFDtPW_u2gmpgXTVpdFQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/471dde73-6dea-49b9-87d2-44a943e102b5%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Robert Roskam
2017-02-16 02:29:57 UTC
Permalink
+1, documenting in this case seems to be the most appropriate.

Robert Roskam
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
Post by Tim Graham
pitfall so that there's something to point to when related tickets come in.
+1, as Aymeric points out the more complex fields probably won't work with
coercion-on-set.
Post by Tim Graham
Post by Tim Graham
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
I also think this is the most appropriate solution.
Post by Tim Graham
m = Model(decimal='12.9')
m.save()
self.assertEqual(m.decimal, '12.9')
m.refresh_from_db()
self.assertEqual(m.decimal, Decimal('12.9'))
That is, you can create a model with an incorrect type and it won't be
fixed until you refresh the object from the database.
Most recent complaint, "it is only a basic expectation that the DB layer
and the Application layer will correspond to each-other after performing
save, which is in other words, syncing your state with the DB.
Personally, this bug (one way binding between application and db on save)
broke many of my tests and took a lot of my time." [0] See [1] for another
manifestation of this.
I think calling to_python() in Model.save() would have unacceptable
performance consequences for little benefit considering that it's a
reasonable expectation for developers to provide the correct type. Further,
m = Model(decimal='12.9')
m.full_clean()
self.assertEqual(m.decimal, Decimal('12.9'))
What do you think? Absent a better suggestion, we could document this
pitfall so that there's something to point to when related tickets come in.
[0] https://code.djangoproject.com/ticket/27825
[1] https://code.djangoproject.com/ticket/24028
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an
<javascript:>.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%40googlegroups.com
<https://groups.google.com/d/msgid/django-developers/4e59974e-9916-434e-8840-4f42996e5052%40googlegroups.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
--
Adam
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/4728e4e7-2810-441a-a729-109f84253215%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Continue reading on narkive:
Loading...