Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7560 closed (fixed)

Delegate (most) type conversion to backends

Reported by: leosoto Owned by: mtredinnick
Component: Database layer (models, ORM) Version: master
Severity: Keywords: jython db-be-api
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

As discussed on the Mailing list and on IRC, some backends would be greatly simplified if they stop receiving string values for non-basic types, such as dates and decimal.

This patch delegates most of the get_db_prep_* logic of these fields to backend methods (named value_to_db_something where something is date, time, decimal, etc) It also implements get_db_prep_* for basic field types to avoid leaking string values.

For Jython/zxJDBC compatibility purposes, an special check was made on Field.get_db_prep_lookup and 'year' lookup. I was not able to understand it, so I choose to introduce as few modifications as possible there. It could be a good idea to delegate that logic to the backend too, but I think a second opinion is needed.

Finally, PhoneNumberField doesn't inherits from IntegerField anymore. There was no point on such inheritance, as phones are mapped to varchar fields on every backend. Not to mention that every method declared by IntegerField was overriden by PhoneNumberField.

This was tested against mysql, postgresql, oracle and sqlite3, and didn't broke anything (that wasn't already broken on trunk, at least).

Attachments (8)

get_db_prep_refactor.patch (18.7 KB) - added by leosoto 6 years ago.
get_db_prep_refactor2.patch (18.1 KB) - added by adamv 6 years ago.
Patch against revision 7818
get_db_prep_refactor-3.patch (23.7 KB) - added by leosoto 6 years ago.
Updated to current trunk. Also refactors year lookup code.
get_db_prep_refactor-4.patch (28.9 KB) - added by leosoto 6 years ago.
Also fixes to_python on Time and DateTime fields. Includes tests for this addition.
get_db_prep_refactor-5.patch (28.8 KB) - added by leosoto 6 years ago.
Avoid failing test if the backend doesn't support usecs
7560_get_db_prep_refactor.diff (30.7 KB) - added by leosoto 6 years ago.
7560_get_db_prep_refactor.2.diff (29.3 KB) - added by leosoto 6 years ago.
Updated to svn r8011
7560_get_db_prep_refactor.3.diff (31.1 KB) - added by seanl 6 years ago.
Fixes for TimeField.to_python and extra tests

Download all attachments as: .zip

Change History (18)

Changed 6 years ago by leosoto

Changed 6 years ago by adamv

Patch against revision 7818

comment:1 Changed 6 years ago by anonymous

  • milestone set to 1.0 beta
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Setting 1.0beta milestone, as Jython (and others Python VM such as PyPy) compatibility is on the 1.0 maybe list.

comment:2 follow-up: Changed 6 years ago by jacob

This looks good at first glance. There's some commented-out code and # XXX comments that need to be removed/resolved; I'll read the patch in more detail a bit later and leave more feedback then.

In the meantime, Leo: can you post a summary of this ticket and the change to django-dev and make sure nobody sees problems with the general approach? Now that you have a working patch a final discussion of weather we should be doing this needs to happen real quick.

comment:3 in reply to: ↑ 2 Changed 6 years ago by leosoto

Replying to jacob:

This looks good at first glance. There's some commented-out code and # XXX comments that need to be removed/resolved;

Oh. Well, the commented out code was accidental. But the #XXXs where left on purpose, to make sure that the question doesn't remain unanswered when/if the commit occurs.

Changed 6 years ago by leosoto

Updated to current trunk. Also refactors year lookup code.

comment:4 Changed 6 years ago by leosoto

Hmm. I discovered that this refactor breaks _get_next_or_previous_by_FIELD on DateTime and Time fields. That's because Model._get_next_or_previous_by_FIELD always coerces the "current" value as string, then datetimes (and times) are converted to a string with usecs that can't be parsed later by DateTimeField.to_python.

So I'm going to improve the to_python method on both DateTimeField and TimeField.

Changed 6 years ago by leosoto

Also fixes to_python on Time and DateTime fields. Includes tests for this addition.

comment:5 Changed 6 years ago by ramiro

  • Keywords db-be-api added

Changed 6 years ago by leosoto

Avoid failing test if the backend doesn't support usecs

Changed 6 years ago by leosoto

comment:6 Changed 6 years ago by leosoto

Yet another patch update. This time, I've also removed some code duplication of almost all get_db_prep_save/get_db_prep_lookup methods, as discussed on http://groups.google.com/group/django-developers/browse_thread/thread/29f95436ed433d85.

Changed 6 years ago by leosoto

Updated to svn r8011

comment:7 Changed 6 years ago by leosoto

BTW, after the get_db_prep_value addition, I included changes to the documentation on the patch. I'm sure that it needs many changes as my English is not on par with the quality of Django docs.

Changed 6 years ago by seanl

Fixes for TimeField.to_python and extra tests

comment:8 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

I started reviewing this last night. Will get it committed today or tomorrow.

comment:9 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [8131]) Fixed #7560 -- Moved a lot of the value conversion preparation for
loading/saving interactions with the databases into django.db.backend. This
helps external db backend writers and removes a bunch of database-specific
if-tests in django.db.models.fields.

Great work from Leo Soto.

comment:10 Changed 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.