#11863: Add a method to the orm to create Model instances from raw sql queries
---------------------------------------------------+------------------------
Reporter: seanoc | Owner: seanoc
Status: assigned | Milestone: 1.2
Component: Database layer (models, ORM) | Version: SVN
Resolution: | Keywords:
Stage: Unreviewed | Has_patch: 1
Needs_docs: 1 | Needs_tests: 0
Needs_better_patch: 0 |
---------------------------------------------------+------------------------
Comment (by seanoc):
Thanks for the feedback Jacob! I'm going to be swamped with work for the
next week or two but getting these worked into the patch are the first
thing on my queue once work lets up a bit. In the mean time here's some
quick responses:
* Making kwargs and annotations available to the test suite is kinda
gross. I don't think the tests really need to know that info: they just
need to validate that the raw query works.
I tend to agree, the main reason I did this was during initial development
there were situations where real fields were being added as annotations
instead of being passed into the {{{__init__}}} of the model. Since this
could have some significant side effects I wanted to make sure that I
caught it happening going forward and this was the easiest way I thought
of to test it. I'll rework it so that those are no longer unneccesarrily
exposed and develop a test with a model which will error if it doesn't
receive all of it's fields in it's {{{__init__}}}.
* The signature of raw() will, I think, promote bad SQL practice and
could lead to SQL injection. Specifically, the standard DB-API interface
is execute(query, params), but for raw()it'd be raw(query, params=params).
That's a minor difference, but raw(query % params) is now shorter, so
people'll use it. Please change the API of raw() to be raw(query, params,
**kwargs).
Good call. Part of the goal here was to keep things relatively non-SQL
specific so that the .raw() method could be implemented for non-relational
back-ends. That being said I think the query+params best practice is
common enough across systems which would allow something resembling a raw
query that this makes sense.
* Please spell-check your comments.
* Naming nit: no need for the Exception part of
InsufficiantFieldsException; InsufficiantFields will suffice. Ditto for
InvalidQueryException.
* When you check for a SELECT query, make sure to strip() the query
string (query.py line 57).
* Please use fixtures in tests instead of that big setUp.
Will do on all of these
* Do you think RawQuerySet should cache results the same way QuerySet
does? I think not, but we'll need to explain why that'd be.
I've gone a bit back and forth on this one. To be honest the main reason
it doesn't right now is I've just been focusing on getting things working
and getting the API hammered out. At this point I'm inclined to try and
be consistent with QuerySet's behavior and to cache results. Then if
somebody wants to hit the database again, they would need to be explicit
and call raw() again to get a new RawQuerySet. If anybody has a strong
feeling/argument either way I'd be very happy to hear it. This will
probably be one of the last bits I implement in the patch.
* Have you thought about integrating this with the lazy field support
(QuerySet.defer()/QS.only())? Seems like a logical thing to support.
Yes I have but so far I've really focused on just getting things to work
and to get the API worked out. I've basically been treating this as a
"nice to have" aspect to the patch. Once I get your other recommendations
worked in and a first pass at the documentation complete I'll dig into
what work would be required to implement this.
* Is cursor.description supported across all Django's supported
database backends (I think it's not). If not, the same API can still work
-- probably by supplying a complete translations dict -- but we need to
avoid erroring out if description isn't available.
That's a very good question. I'll try to do some testing/research to find
out. Either way I'll look at ways to handle cursor.description not being
supported in a particular backend.
* This has some implications w/r/t Alex Gaynor's multi-db work. You
might want to try to get his thoughts on how you're handling connection
and how that'll need to change.
Alex, Russel, and I talked about this a bit during the Djangocon sprints.
Their advice at the time was to go forward with developing against Django
trunk. Once Alex's multi-db work hits trunk we'll look at what will be
needed to make sure that .raw() uses the right connection to get a cursor.
In generally tho it should be a relatively small change. To be safe I'll
be sure that when I go to work in these recommendations, I'll do a review
of how I'm handling the cursor and start to plan for the change.
--
Ticket URL: <http://code.djangoproject.com/ticket/11863#comment:12>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en
-~----------~----~----~----~------~----~------~--~---
