"Normalized" Data type for __month and __day lookups?

[Leo Soto M.]

While resuming the Django/Jython work, I've been hit by a small
inconsistency on the types of lookup arguments, as received by DB
backends.

Basically, __year lookup arguments are converted to integers before
being passed to the backend, but for __month and __day it's unicode.
That's weird. And for JDBC backends which are picky about types (like
on PostgreSQL where operators may have specialized meanings for
different types) string arguments won't work.

Now, the blame is on my side here: I didn't saw this situation while
working on #7560[1] and just assumed that lookup arguments were always
integers. Later, when it was clear than unicode arguments should also
be supported (because the admin uses that), changesets 8424[2] and
8526[3] fixed the existing problems. Unfortunately, always passing
unicode to the backend was (part of) the solution. But looks like
normalizing to int should also work.

So I've uploaded a small patch on #10071[4] which does the
normalization to int instead of unicode. I tested it with sqlite
(which was the problematic backend) and it doesn't break any test.

Now, my questions are:

- Is this change OK or would it be considered as backwards
incompatible for slightly changing the interface with DB backends? On
the pragmatic side I'd like to point that this doesn't break anything
on the built-in backends but it would be good get confirmation from
other external backend developers.

- If the change is OK, could it make it into the 1.0.X branch? That'd
be very good, as we are targeting Django 1.0 on the django-jython
project.

- Anyone else with other version of sqlite (I'm using 3.5.9) who
would like to test if the patch works?

- Am I missing something here?

Regards,

[1] http://code.djangoproject.com/ticket/7560
[2] http://code.djangoproject.com/changeset/8494
[3] http://code.djangoproject.com/changeset/8526
[4] http://code.djangoproject.com/ticket/10071
--
Leo Soto M.
http://blog.leosoto.com

[Malcolm Tredinnick]

On Tue, 2009-01-20 at 01:54 -0300, Leo Soto M. wrote:
[...]

> So I've uploaded a small patch on #10071[4] which does the
> normalization to int instead of unicode. I tested it with sqlite
> (which was the problematic backend) and it doesn't break any test.
>
> Now, my questions are:
>
> - Is this change OK or would it be considered as backwards
> incompatible for slightly changing the interface with DB backends? On
> the pragmatic side I'd like to point that this doesn't break anything
> on the built-in backends but it would be good get confirmation from
> other external backend developers.

That area of the code was really problematic just before 1.0. I'll dig
through my memory and read the changes around that time to try and
remember what the tricks were, but I have a recollection we tried to
initially go directly to ints and something or other didn't work
(something form or admin related). That might not have been the same as
the change you're making here, but I'll have a hard think. It might just
have been that the change we ended up making was the least intrusive for
that point in time and it didn't seem to break anything.

There were some people working on this (in person) at the Portland
sprint just before 1.0 and my recollection is that the patch they came
up with didn't quite work and I had to spend a while tweaking it. Don't
know if any of them are on this list, though.

> - If the change is OK, could it make it into the 1.0.X branch? That'd
> be very good, as we are targeting Django 1.0 on the django-jython
> project.

I'd call it a bug fix for that reason. Jython support on 1.0 is/was one
of our goals.

Has something changed that made this only show up now? Or has it always
been there and you just didn't notice?

Regards,
Malcolm

[Leo Soto M.]

On Tue, Jan 20, 2009 at 2:02 AM, Malcolm Tredinnick
<mal...@pointy-stick.com> wrote:
>
> On Tue, 2009-01-20 at 01:54 -0300, Leo Soto M. wrote:
> [...]
>> So I've uploaded a small patch on #10071[4] which does the
>> normalization to int instead of unicode. I tested it with sqlite
>> (which was the problematic backend) and it doesn't break any test.
>>
>> Now, my questions are:
>>
>> - Is this change OK or would it be considered as backwards
>> incompatible for slightly changing the interface with DB backends? On
>> the pragmatic side I'd like to point that this doesn't break anything
>> on the built-in backends but it would be good get confirmation from
>> other external backend developers.
>
> That area of the code was really problematic just before 1.0. I'll dig
> through my memory and read the changes around that time to try and
> remember what the tricks were, but I have a recollection we tried to
> initially go directly to ints and something or other didn't work
> (something form or admin related).

I spent around an hour digging in the history of that code, and come
to the following reasoning: the problems were caused by two mismatches
between what the backend expected and what it got:

- After the get_db_prep_* refactoring, backend expected int, but the
admin passed unicode. This caused #8424.
- The fix for #8424 was to expect unicode on the backend. But most
usages passed an int. Caused #8510.
- The fix for #8510 was to normalize everything to unicode. Both sides
talk happily now :)

So, this little change is based on keep both sides synchronized but
with a better contract. However, people who can remember the actual
facts may be able to confirm or correct my reasoning.

>> - If the change is OK, could it make it into the 1.0.X branch? That'd
>> be very good, as we are targeting Django 1.0 on the django-jython
>> project.
>
> I'd call it a bug fix for that reason. Jython support on 1.0 is/was one
> of our goals.
>
> Has something changed that made this only show up now? Or has it always
> been there and you just didn't notice?

It has always been there.

Regards,

[Malcolm Tredinnick]

On Tue, 2009-01-20 at 02:16 -0300, Leo Soto M. wrote:
[...]

> I spent around an hour digging in the history of that code, and come
> to the following reasoning: the problems were caused by two mismatches
> between what the backend expected and what it got:
>
> - After the get_db_prep_* refactoring, backend expected int, but the
> admin passed unicode. This caused #8424.
> - The fix for #8424 was to expect unicode on the backend. But most
> usages passed an int. Caused #8510.
> - The fix for #8510 was to normalize everything to unicode. Both sides
> talk happily now :)

Okay, that sounds very familiar. I remember lots of bouncing back and
forth and going a bit mental trying to keep everything happy. I think
that's the correct history.

> So, this little change is based on keep both sides synchronized but
> with a better contract.

If your change works, it's a little better, since we are really are
talking about integers there. Should use the right types if we can.

Regards,
Malcolm