Should Model.save() fix incorrect types that happen to save correctly?

[Tim Graham]

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

[Florian Apolloner]

On Monday, February 13, 2017 at 3:57:49 PM UTC+1, Tim Graham wrote:

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.

[Aymeric Augustin]

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.

--
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-develop...@googlegroups.com.
To post to this group, send email to django-d...@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.

[charettes]

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

[Adam Johnson]

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.

--

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+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@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

[Marc Tamlyn]

Agreed - ensuring that this works in the general case is not reliable.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM1B0RJa928Yvk0uXenU009tsU%3DFDtPW_u2gmpgXTVpdFQ%40mail.gmail.com.

[Josh Smeaton]

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.

To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@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

--
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-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

Visit this group at https://groups.google.com/group/django-developers.

[Robert Roskam]

+1, documenting in this case seems to be the most appropriate. 

Robert Roskam

On Monday, February 13, 2017 at 10:51:59 AM UTC-5, Adam Johnson wrote:

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.

On 13 February 2017 at 15:27, charettes <chare...@gmail.com> wrote:

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

Le lundi 13 février 2017 09:57:49 UTC-5, Tim Graham a écrit :

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-develop...@googlegroups.com.
To post to this group, send email to django-d...@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