Opened 8 years ago

Last modified 2 weeks ago

#27587 assigned Cleanup/optimization

Document str(QuerySet.query)

Reported by: Peter Inglesby Owned by: JosiahDub
Component: Documentation Version: 1.10
Severity: Normal Keywords:
Cc: Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

As far as I can tell, the only place where the behaviour of str(QuerySet.query) is mentioned in the docs is as a throwaway comment in ​the aggregation topic, and in a code example in ​the GeoDjango tutorial.

This is very useful behaviour but it is not widely known. Additionally, its limitations (see ticket:12611, "Incorrect quoting in QuerySet.query.str()") are even less widely known.

I think it would be suitable to add this to ​section 2 of the tutorial, and to the ​Making queries topic guide.

Change History (13)

comment:1 by Aymeric Augustin, 8 years ago

Regarding the limitations, I don't think we should accept the current status quo as the best we can do:

  1. if it's reasonably easy to perform the escaping correctly, typically just by escaping arguments with a function provided by the database adapter prior to interpolation, then Django should do it.
  2. if there's no function for escaping arguments, but a well documented and not too complicated process do to so (replace " by "" then wrap in "...") then we should consider doing it as well
  3. if the escaping rules are unclear and there's no way to ask the database to do it, then we should return something that is *obviously* invalid, like QUERY = ... ; PARAMS = ...

I did that some time ago for last_executed_query. SQLite stayed at 3 for half a decade before moving to 2.

I believe the same logic should apply to QuerySet.query and similar methods.

comment:2 by Tim Graham, 8 years ago

Easy pickings: unset
Summary: str(QuerySet.query) should be documented → Document str(QuerySet.query)
Triage Stage: Unreviewed → Accepted
Type: Uncategorized → Cleanup/optimization

Another place to document it is docs/ref/models/querysets.txt.

comment:3 by Mads Jensen, 8 years ago

​PR

comment:4 by Mads Jensen, 8 years ago

Has patch: set

comment:5 by Tim Graham, 8 years ago

Patch needs improvement: set

The current limitations described in this ticket and in #18631 should also be mentioned.

comment:6 by Mads Jensen, 8 years ago

Patch needs improvement: unset

comment:7 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:8 by JosiahDub, 2 years ago

Has patch: unset
Owner: changed from nobody to JosiahDub
Patch needs improvement: unset
Status: new → assigned

comment:9 by Carlton Gibson, 2 years ago

Patch needs improvement: set

Docs patch looks good. Small comments on PR.

I recall related work to improve the output of str(qs.query) — by having the backend do the quoting — but I'm not 100% sure how far that got. This matters for the it's not great disclaimer, and whether we want to close this as completed (or not) on merge? 🤔

comment:10 by Jacob Walls, 2 years ago

Has patch: set

​PR

comment:11 by Michael Howitz, 18 months ago

Patch needs improvement: unset

The ​PR is an improvement over what is currently there, so why not merge it?

comment:12 by Mariusz Felisiak, 18 months ago

Patch needs improvement: set

Have you checked comments on the latest PR? We cannot merge this as .query is not something that we can recommend to users without fixing #25705.

comment:13 by Ülgen Sarıkavak, 2 weeks ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top