#7560: Delegate (most) type conversion to backends

[Leo Soto M.]

Hi all!,

I've posted a patch[1] with a small refactor to some Field's
get_db_prep_* methods. Basically, for non-basic field types, the
database-prepared values is not computed by the Fields anymore, but
delegated to DatabaseOperations functions. As the current default
logic was copied to the default methods implementations on
DatabaseOperations, this should not break anything (I fully tested it
a week ago, with the four official backends and indeed it didn't broke
anything).

This patch is part of my work of running Django on Jython, and in
general helps anybody who maintains an external django database
backend.

I would appreciate thoughts on the general approach, and some guidance
if I missed something.

[1] http://code.djangoproject.com/ticket/7560

--
Leo Soto M.
http://blog.leosoto.com

[Russell Keith-Magee]

On Fri, Jul 4, 2008 at 4:57 AM, Leo Soto M. <leo....@gmail.com> wrote:
>
> Hi all!,
>
> I've posted a patch[1] with a small refactor to some Field's
> get_db_prep_* methods. Basically, for non-basic field types, the
> database-prepared values is not computed by the Fields anymore, but

Hi Leo,

I've had a quick look at this patch. On the surface, it looks like a
good idea and a pretty good implementation, but I'll need to spend a
bit more time with it before I pass final judgement.

> This patch is part of my work of running Django on Jython, and in
> general helps anybody who maintains an external django database
> backend.

Completely aside from that, it may actually fix a few other problems
we've been having - for example, MySQL's habit of returning 1/0 from
boolean fields. It is also a nice cleanup of the handling of time
fields.

Thanks for your work on this.

Yours,
Russ Magee %-)

[Leo Soto M.]

On Thu, Jul 3, 2008 at 8:27 PM, Russell Keith-Magee
<freakb...@gmail.com> wrote:
>
> On Fri, Jul 4, 2008 at 4:57 AM, Leo Soto M. <leo....@gmail.com> wrote:
>>
>> Hi all!,
>>
>> I've posted a patch[1] with a small refactor to some Field's
>> get_db_prep_* methods. Basically, for non-basic field types, the
>> database-prepared values is not computed by the Fields anymore, but
>
> Hi Leo,
>
> I've had a quick look at this patch. On the surface, it looks like a
> good idea and a pretty good implementation, but I'll need to spend a
> bit more time with it before I pass final judgement.

Sure, and thanks for setting a few time aside to do this first review!

>> This patch is part of my work of running Django on Jython, and in
>> general helps anybody who maintains an external django database
>> backend.
>
> Completely aside from that, it may actually fix a few other problems
> we've been having - for example, MySQL's habit of returning 1/0 from
> boolean fields.

I'm not sure about it. The patch improves the Python -> DB conversion,
not the other way around.

[Malcolm Tredinnick]

On Thu, 2008-07-03 at 16:57 -0400, Leo Soto M. wrote:
> Hi all!,
>
> I've posted a patch[1] with a small refactor to some Field's
> get_db_prep_* methods. Basically, for non-basic field types, the
> database-prepared values is not computed by the Fields anymore, but
> delegated to DatabaseOperations functions. As the current default
> logic was copied to the default methods implementations on
> DatabaseOperations, this should not break anything (I fully tested it
> a week ago, with the four official backends and indeed it didn't broke
> anything).

I've also read through the patch already and it seems like the right
track. I like the approach, since it helps in a few areas, including
field extensions and extra database backends.

I haven't given it a complete fine-detail review yet to pick things out
of individual lines, but only one thing jumped out on the initial read.
I've been trying very hard to eliminate lines like:

if settings.DATABASE_ENGINE.endswith('zxjdbc'):

from places outside the database backend-specific directories. This is
because it locks in particular backends to the main code, leading to
extensibility problems for other backends. So rather than introduce a
line like that, I'm always going to prefer to remove things like the
section following it that gives SQLite3 special handling.

Plus, if somebody wants to have a real database, the name needs to look
like they didn't just fall on their keyboard and "zxjdbc" doesn't pass
on those grounds. Looking like we threw up in the code is uncool. :-)

Mentally, I've queued it up behind Honza's validation and normalisation
changes, because I want to see how it interacts with that. I've got a
couple of more important bugs to fix immediately, but I'd like to get to
Honza's stuff as soon as possible and this weekend might be when I have
time. So, if Russell gets there first, no skin off my nose, otherwise I
might deal with it in the next few days.

Regards,
Malcolm

[Leo Soto M.]

On Thu, Jul 3, 2008 at 10:36 PM, Malcolm Tredinnick
<mal...@pointy-stick.com> wrote:
> I've also read through the patch already and it seems like the right
> track. I like the approach, since it helps in a few areas, including
> field extensions and extra database backends.
>
> I haven't given it a complete fine-detail review yet to pick things out
> of individual lines, but only one thing jumped out on the initial read.
> I've been trying very hard to eliminate lines like:
>
> if settings.DATABASE_ENGINE.endswith('zxjdbc'):
>
> from places outside the database backend-specific directories. This is
> because it locks in particular backends to the main code, leading to
> extensibility problems for other backends. So rather than introduce a
> line like that, I'm always going to prefer to remove things like the
> section following it that gives SQLite3 special handling.

Yes, as I noted on the ticket description, I didn't really understood
why all that different formats were needed by each backend, so I was a
bit scared to touch that portion of code.

But now that you make me think again, that was just a lame excuse :-).
I just pushed that logic to DatabaseOperations and named it
"year_lookup_bounds".

So the new posted patch[1] is updated to svn trunk, and also fixes a
test that would broke because get_db_prep* now may return different
values for different backends.

[1] http://code.djangoproject.com/attachment/ticket/7560/get_db_prep_refactor-3.patch

[Leo Soto M.]

On Fri, Jul 4, 2008 at 12:27 AM, Leo Soto M. <leo....@gmail.com> wrote:
> So the new [...]

A quick update: After more testing with real django applications I
discovered that I was breaking get_next_by_FIELD. This fourth patch[1]
fixes this issue and includes tests for this use case (which wasn't
exercised on the rest of the suite)

[1] http://code.djangoproject.com/attachment/ticket/7560/get_db_prep_refactor-4.patch