Summary

Add support for prefixing query expressions

Review Request #7774 — Created Nov. 17, 2015 and submitted Nov. 23, 2015, 5:25 p.m.

Information

Owner

brennie

Repository

Djblets

Branch

master

Bugs

Depends On

Commit

850d88a...

Reviewers

Groups

djblets

People

Description

A new utility method, prefix_q, has been added to facilitate
prefixing of query expressions. Query expressions, or (Q()
expressions) are Django's way of building the WHERE (and related)
clauses when querying for models.

If a foreign key fk exists from a model A to B (that is, A.fk
points at an instance of B), then a query expression that is used
to filter instances of B can be used with A via prefix_q:

q_a = prefix_q('fk', q_b)

This allows complex queries that select one model to be reused to
select related models very easily.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
Col: 13 E122 continuation line missing indentation or outdented	reviewbot	Nov. 17, 2015, 11:20 p.m.
Col: 13 E122 continuation line missing indentation or outdented	reviewbot	Nov. 17, 2015, 11:20 p.m.
I think I'd like more explanation about what "Prefix a Q expression" means. Can you give an example?	david	Nov. 18, 2015, 8:03 p.m.
Just calling six.binary_type will explode if there's any non-ascii characters in there. Maybe that's what you want, but otherwise, perhaps …	david	Nov. 18, 2015, 4:01 p.m.
It's pretty weird to see the u'' in there. Is that just something funky with Q.__str__?	david	Nov. 18, 2015, 4:01 p.m.
How about explaining that: B.objects.filter(prefix_q(Q(field='value'), 'a')) is the same as: B.objects.filter(Q(a__field='value'))	david	Nov. 19, 2015, 2:14 p.m.
"Return," not "Get." I don't think it's worth using Sphinx references for things like "None" in a summary.	chipx86	Nov. 21, 2015, 12:36 a.m.
For these, maybe: :py;meth:`Manager.get <django.db.models.manager.Manager.get>	chipx86	Nov. 21, 2015, 12:37 a.m.
""" is indented too far.	chipx86	Nov. 21, 2015, 12:37 a.m.
I feel like this doesn't describe this well enough. The example does, but I'd like to see a bit more …	chipx86	Nov. 21, 2015, 12:57 a.m.
This is backwards. It's title <reference>.	chipx86	Nov. 21, 2015, 12:38 a.m.
This should be: Example: .. code-block:: python <example here> In other files, this is also after Args and Returns.	chipx86	Nov. 21, 2015, 12:38 a.m.
Sentence casing.	chipx86	Nov. 21, 2015, 12:38 a.m.
Sentence casing.	chipx86	Nov. 21, 2015, 12:38 a.m.
Are you sure about this reference? The Django docs are showing it under django.db.models.Q.	chipx86	Nov. 21, 2015, 12:39 a.m.
I don't know that :py:data: is really appropriate for things like True, False, or None. It's really intended for module-level …	chipx86	Nov. 21, 2015, 12:39 a.m.
It's not clear to me, reading the unit tests and the code, whether all these cases are covered. Probably because …	chipx86	Nov. 21, 2015, 12:39 a.m.
Any reason we don't just use isinstance? Generally, it's nice not to restrict to a specific base type, so long …	chipx86	Nov. 21, 2015, 12:41 a.m.
Is this needed? Are we aware of cases where it must be a byte string? I thought Unicode strings were …	chipx86	Nov. 21, 2015, 12:46 a.m.
Unit test suite docstrings are in the form of: "Unit tests for module.path.to.thing." Also, trailing period.	chipx86	Nov. 21, 2015, 12:43 a.m.
The docstrings should all mention prefix_q.	chipx86	Nov. 21, 2015, 12:43 a.m.
Should be "Return a model instance"	david	Nov. 23, 2015, 5:24 p.m.

Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py

Diff:

Revision 2 (+89 -6)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_query.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py

djblets/db/tests/test_query.py (Diff revision 2)

Show all issues

Col: 13
 E122 continuation line missing indentation or outdented

djblets/db/tests/test_query.py (Diff revision 2)

Show all issues

Col: 13
 E122 continuation line missing indentation or outdented

Diff:

Revision 3 (+89 -6)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_query.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py

djblets/db/query.py (Diff revision 3)

Show all issues

I think I'd like more explanation about what "Prefix a Q expression" means. Can you give an example?

djblets/db/query.py (Diff revision 3)

Show all issues

Just calling six.binary_type will explode if there's any non-ascii characters in there. Maybe that's what you want, but otherwise, perhaps use .encode('utf-8')?

djblets/db/tests/test_query.py (Diff revision 3)

Show all issues

It's pretty weird to see the u'' in there. Is that just something funky with Q.__str__?

brennie Nov. 18, 2015, 3:44 p.m.

It calls repr on its elements, and so it renders as a unicode string.

Diff:

Revision 4 (+105 -6)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_query.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py

Diff:

Revision 5 (+104 -6)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_query.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py

djblets/db/query.py (Diff revision 5)

Show all issues

How about explaining that:

B.objects.filter(prefix_q(Q(field='value'), 'a'))


is the same as:

B.objects.filter(Q(a__field='value'))

Change Summary:

Address David's issues.

Diff:

Revision 6 (+122 -6)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_query.py

Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py

Summary:

-	Add support for prefixing querysets
+	Add support for prefixing query expressions

Description:

~		`prefix_q` allows a generated queryset to be prefixed. This is useful if
~		there is a queryset describing restrictions on a object, but we want to
~		select items related to that object.
	~	`prefix_q` allows a generated query expression to be prefixed. This is
	~	useful if there is a queryset describing restrictions on a object, but
	~	we want to select items related to that object.

		This also updates the documenation of `get_object_or_none`.

chipx86 Nov. 19, 2015, 3:35 p.m.

The review request description should go into more detail on this. It feels to me to be descriptive only if you already know what this change is about.

djblets/db/query.py (Diff revision 6)

Show all issues

"Return," not "Get."

I don't think it's worth using Sphinx references for things like "None" in a summary.

djblets/db/query.py (Diff revision 6)

Show all issues

For these, maybe:
:py;meth:`Manager.get <django.db.models.manager.Manager.get>

djblets/db/query.py (Diff revision 6)
Show all issues
```
""" is indented too far.
```

djblets/db/query.py (Diff revision 6)

Show all issues

I feel like this doesn't describe this well enough. The example does, but I'd like to see a bit more about how this is useful.

It should also go into the cloning behavior and defaults.

djblets/db/query.py (Diff revision 6)

Show all issues

This is backwards. It's title <reference>.

djblets/db/query.py (Diff revision 6)

Show all issues

This should be:

Example:
    .. code-block:: python

       <example here>

In other files, this is also after Args and Returns.

djblets/db/query.py (Diff revision 6)
Show all issues
```
Sentence casing.
```
djblets/db/query.py (Diff revision 6)
Show all issues
```
Sentence casing.
```

djblets/db/query.py (Diff revision 6)

Show all issues

Are you sure about this reference? The Django docs are showing it under django.db.models.Q.

brennie Nov. 23, 2015, 5:26 p.m.

https://github.com/django/django/blob/master/django/db/models/query_utils.py#L45

djblets/db/query.py (Diff revision 6)

Show all issues

I don't know that :py:data: is really appropriate for things like True, False, or None. It's really intended for module-level data, with the implication being the current module. Does it result in actual links to the Python docs?

djblets/db/query.py (Diff revision 6)

Show all issues

It's not clear to me, reading the unit tests and the code, whether all these cases are covered. Probably because parts involve internal Q knowledge, but I want to make sure we have 100% coverage of this.

djblets/db/query.py (Diff revision 6)

Show all issues

Any reason we don't just use isinstance? Generally, it's nice not to restrict to a specific base type, so long as the thing coming in behaves like the expected type.
Also, this shouldn't be an assertion, it should raise an exception if it's not a valid type. Assertions are for internal code that should always be correct, and are just to verify something isn't screwed up (like an internal function calling another internal function with some certain state).

brennie Nov. 23, 2015, 5:26 p.m.

This should never ever be anything other than a tuple due to the internals of Q.

djblets/db/query.py (Diff revision 6)

Show all issues

Is this needed? Are we aware of cases where it must be a byte string? I thought Unicode strings were fine here.

If this is indeed required, we need to have a unit test for it.

brennie Nov. 23, 2015, 5:26 p.m.

Q() objects created as Q(key=val) will have key as a bytestring.

djblets/db/tests/test_query.py (Diff revision 6)

Show all issues

Unit test suite docstrings are in the form of: "Unit tests for module.path.to.thing." Also, trailing period.

djblets/db/tests/test_query.py (Diff revision 6)
Show all issues
```
The docstrings should all mention prefix_q.
```

Change Summary:

Address Christian's issues. Rewrite description

Description:

~		`prefix_q` allows a generated query expression to be prefixed. This is
~		useful if there is a queryset describing restrictions on a object, but
~		we want to select items related to that object.
	~	A new utility method, `prefix_q`, has been added to facilitate
	~	prefixing of query expressions. Query expressions, or (`Q()`
	~	expressions) are Django's way of building the `WHERE` (and related)
	+	clauses when querying for models.

~		This also updates the documenation of `get_object_or_none`.
	~	If a foreign key `fk` exists from a model `A` to `B` (that is, `A.fk`
	+	points at an instance of `B`), then a query expression that is used
	+	to filter instances of `B` can be used with `A` via prefix_q:
	+
	+	`q_a = prefix_q('fk', q_b)`
	+
	+
	+	This allows complex queries that select one model to be reused to
	+	select related models very easily.

Diff:

Revision 7 (+139 -6)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_query.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_query.py

Ship it!

djblets/db/query.py (Diff revision 7)
Show all issues
```
Should be "Return a model instance"
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (86a4b74)