Getting rid of get_db_prep_lookup on Field subclasses?
Leo Soto M.
too easy to write a wrong get_db_prep_lookup method (example:
#7448[2]), I have reached the conclusion that there is no reason to
have *two* methods for doing the python -> db conversion
(``get_db_prep_save`` and ``get_db_prep_lookup``). Not for most of the
fields, at least.
What I'm talking about is that, to ensure the proper type conversion
before querying the database[3], all of django core Fields should
currently do this:
def get_db_prep_save(self, value):
return some_transformation(value)
def get_db_prep_lookup(self, value, lookup_type):
if hasattr(value, 'as_sql'):
sql, params = value.as_sql()
return QueryWrapper(('(%s)' % sql), params)
if lookup_type in ('range', 'in'):
value = [some_transformation(v) for v in value]
elif lookup_type in ('exact', 'gt', 'gte', 'lt', 'lte'):
value = some_transformation(v)
return Field.get_db_prep_lookup(self, value)
Which blatantly violates DRY.
Also note that the documentation for custom fields, suggest to reuse
get_db_prep_save on get_db_prep_lookup[4]. Following that advice, I
propose to add a ``get_db_prep_value`` method to the Field API, to
make the common case much simpler. Then, almost all field can do:
def get_db_prep_value(self, value):
return some_transformation(value)
[Or just implement the transformation in the body, if it is not
trivial. I didn't want to reuse get_db_prep_save as the generic
transformation method, because the 'save' part seems to imply more
than just value transformation].
Then, on the base Field we would use this method to implement
``get_db_prep_save`` and ``get_db_prep_lookup``, and stop repeating
ourselves.
Does that make sense?
Should I add this change to the patch on #7560?
[1] http://code.djangoproject.com/attachment/ticket/7560
[2] http://code.djangoproject.com/attachment/ticket/7448
[3] This is needed by some backends which are not "string-happy", such
as JDBC backends.
[4] http://www.djangoproject.com/documentation/custom_model_fields/#get-db-prep-lookup-self-lookup-type-value
--
Leo Soto M.
http://blog.leosoto.com
Malcolm Tredinnick
On Tue, 2008-07-15 at 11:49 -0400, Leo Soto M. wrote:
> After fixing some corner cases of #7560[1], and realizing that it is
> too easy to write a wrong get_db_prep_lookup method (example:
> #7448[2]), I have reached the conclusion that there is no reason to
> have *two* methods for doing the python -> db conversion
> (``get_db_prep_save`` and ``get_db_prep_lookup``). Not for most of the
> fields, at least.
I think it will take me a few days to think through all the implications
here. However, I suspect the least intrusive change is to keep
db_prep_lookup and db_prep_save, but have them call a common method
(which could be called db_prep_value if you like).
I can think of some slightly twisted cases where lookup and insert
values will be different. For example, if I make a "month" field type, I
would use an underlying date field column and the insert would be a
full-fledged date value, whilst the lookup would be a call to a date
extraction function in SQL. There quite possibly exist some legitimate
cases like this as well (GIS stuff springs to mind as an area to look,
for example).
So, sharing common code would be good. Forcing everything to use the
same code would probably come back and bite us. I think we're too close
to an irreversible boundary for the API to make that type of change now.
It often takes a few months, or longer, for all the corner cases to fall
out. Thus, I'd keep the current API for the boundary between fields and
the backends, but factor out the common bits more explicitly.
Regards,
Malcolm
Leo Soto M.
<mal...@pointy-stick.com> wrote:
>
>
> On Tue, 2008-07-15 at 11:49 -0400, Leo Soto M. wrote:
>> After fixing some corner cases of #7560[1], and realizing that it is
>> too easy to write a wrong get_db_prep_lookup method (example:
>> #7448[2]), I have reached the conclusion that there is no reason to
>> have *two* methods for doing the python -> db conversion
>> (``get_db_prep_save`` and ``get_db_prep_lookup``). Not for most of the
>> fields, at least.
>
> I think it will take me a few days to think through all the implications
> here. However, I suspect the least intrusive change is to keep
> db_prep_lookup and db_prep_save, but have them call a common method
> (which could be called db_prep_value if you like).
Sure, that was what I was proposing.
Seems like I didn't explained myself well, but I don't want to kill
Field.get_db_prep_{save,lookup}. I want to avoid overriding them in
the same repetitive way on the common cases.
So looks like we agree on the general idea, and I'm going to
incorporate it to #7560.