TimeField broken in Oracle
Ian Kelly
includes a TimeField. The reason appears to be that save_base calls
TimeField.get_db_prep_save prior to passing the value on to the
queryset, which calls TimeField.to_python -- returning a time instance
-- and then passes the result to the backend, where the time instance
is converted to a datetime. Later, UpdateQuery.add_update_fields
calls TimeField.get_db_prep_save again, passing in the datetime, which
to_python doesn't know how to handle.
Should TimeField.to_python be able to handle datetime values, by
simply returning datetime.time()? I'll note that DateField already
has the equivalent logic, so it would at least be more consistent.
There may also be an argument for removing the seemingly redundant
get_db_prep_save call from save_base. Any thoughts on that?
Thanks,
Ian
Ian Kelly
> There may also be an argument for removing the seemingly redundant
> get_db_prep_save call from save_base. Any thoughts on that?
I should add that the main thing that concerns me about this fix is
that it would make save_base internally inconsistent, calling
get_db_prep_save for inserts but not for updates.
Regards,
Ian
simonb
http://code.djangoproject.com/ticket/10443
Simon
Malcolm Tredinnick
> After r10003, Model.save() raises an error in Oracle if the model
> includes a TimeField. The reason appears to be that save_base calls
> TimeField.get_db_prep_save prior to passing the value on to the
> queryset, which calls TimeField.to_python -- returning a time instance
> -- and then passes the result to the backend, where the time instance
> is converted to a datetime. Later, UpdateQuery.add_update_fields
> calls TimeField.get_db_prep_save again, passing in the datetime, which
> to_python doesn't know how to handle.
>
> Should TimeField.to_python be able to handle datetime values, by
> simply returning datetime.time()? I'll note that DateField already
> has the equivalent logic, so it would at least be more consistent.
Sounds like it, yes. Although it's really a bug to assign a datetime to
a time, since they're quite different objects that just happen to both
include an hour/minute/second portion. Akin to assigning an elephant to
giraffe enclosure on the grounds that they both have legs (I know there
are implementation reasons it happens; doesn't make it clean). No
accounting for taste or code quality and since this seems to have broken
some things, I'll have to make it backwards compatible.
> There may also be an argument for removing the seemingly redundant
> get_db_prep_save call from save_base.
Which would break large amounts of code, so let's not do that. :-)
One side effect is that (unfortunately, in my opinion), Django doesn't
require you to actually assign the right types to attributes like
datetime fields, so some conversion happens. More importantly, many
fields, particularly custom fields, require some preparation to convert
from the Python object to the appropriate serialised database form. And
that's what the get_prep_* methods are for.
I'd completely forgotten about the update path in save_base(), which is
a bug in the original patch. I'll need to fix that. The idea is that
everything should be going through the same data conversion paths, since
calls to create objects and calls to update() tend to be interchanged as
code develops (which is a paraphrase of how this problem was discovered
via the geodjango list).
Regards,
Malcolm
Malcolm Tredinnick
> On Mon, 2009-03-09 at 15:46 -0600, Ian Kelly wrote:
> > There may also be an argument for removing the seemingly redundant
> > get_db_prep_save call from save_base.
>
> Which would break large amounts of code, so let's not do that. :-)
Whoops. My bad. Re-reading, trying to work out the test case, I see
you're talking about the one in the update path, not the "save
initially" path. That seems reasonable, since we have to do it in
UpdateQuery. I'm back on track now.
Malcolm