Re: [Django] #25425: Enforce calling resolve_expression before as_sql on all expressions

[Django]

#25425: Enforce calling resolve_expression before as_sql on all expressions
-+-
 Reporter:  Josh Smeaton |Owner:  nobody
 Type:   |   Status:  new
  Cleanup/optimization   |
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"f783a990725fbfce17ef827d3b55ef702535ae96" f783a990]:
 {{{
 #!CommitTicketReference repository=""
 revision="f783a990725fbfce17ef827d3b55ef702535ae96"
 Refs #25425 -- Allowed unresolved Value() instances to be compiled.

 Previously unresolved Value() instances were only allowed to be
 compiled if they weren't initialized with an output_field.

 Given the usage of unresolved Value() instances is relatively common in
 as_sql() overrides it's less controversial to add explicit support for
 this previously undefined behavior now and revisit whether or not it
 should be deprecated in the future.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.7643818d4a90047b50e9aca434f47e14%40djangoproject.com.

Re: [Django] #25425: Enforce calling resolve_expression before as_sql on all expressions

[Django]

#25425: Enforce calling resolve_expression before as_sql on all expressions
-+-
 Reporter:  jarshwah |Owner:  nobody
 Type:   |   Status:  new
  Cleanup/optimization   |
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by timgraham):

 * type:  Bug => Cleanup/optimization
 * stage:  Unreviewed => Accepted


--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.29c0eff5c9320bd6e922843d1affa98e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #25425: Enforce calling resolve_expression before as_sql on all expressions

[Django]

#25425: Enforce calling resolve_expression before as_sql on all expressions
-+-
 Reporter:  jarshwah |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by akaariai):

 The way expressions in filter() calls work is that if you do
 `.filter(Greater(F('field'), 3))`, then Greater is resolved to an
 expression, and that expression is added to WhereNode.

 Similarly, if you do `.filter(Q(Greater(F('field'), 3)) &
 Q(Greater(F('other_field'), 4)))`, then the Q is resolved, and this also
 resolves the nested expressions. Finally the resolved expression is added
 to the query's WhereNode.

 Notably you can't do `.filter(WhereNode(Greater(F('field', 3`. But
 this is alright, the user facing API is Q-objects, not WhereNode objects.
 Also, you are free to generate an expression that resolves to a WhereNode,
 but WhereNode itself isn't resolvable.

 What we should do is enforce that any expression added to a WhereNode is
 already resolved.

 Now, we could also add a required attribute `resolved` to all compilables.
 Then we could check if that attribute is set in compiler.compile(). Those
 objects that aren't resolvable could set it always to True. I'm not
 convinced this is needed, but I won't object to doing this.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.159a490a17b9acb4c99232e2878e6af6%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #25425: Enforce calling resolve_expression before as_sql on all expressions

[Django]

#25425: Enforce calling resolve_expression before as_sql on all expressions
-+-
 Reporter:  jarshwah |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by jarshwah):

 Yes, but what about a resolvable nested within a non-resolvable. The docs
 also (perhaps incorrectly) point out that resolve_expression is a place to
 do validation
 
https://docs.djangoproject.com/en/1.8/ref/models/expressions/#django.db.models.Expression.resolve_expression

 > Provides the chance to do any pre-processing or validation of the
 expression before it’s added to the query. resolve_expression() must also
 be called on any nested expressions. A copy() of self should be returned
 with any necessary transformations.

 If we want to support expressions as filters, then we're probably going to
 have to support F() within a WhereNode():

 {{{
 Model.objects.filter(Greater(F('field'), 3)) -> WhereNode()
 }}}

 That WhereNode will have to be resolved at some point so that the nested
 F() is also resolved. This was the major source of problems when
 converting transforms into expressions:

 
https://github.com/django/django/blob/f2975c021d247bf8c6a5fc23988639c636da86f5/django/db/models/lookups.py#L76

 I'm resolving the transform during the as_sql phase above which is wrong.
 But it was way too hard trying to track down all of the places that
 WhereNodes (and Lookups within the where node) were being rendered without
 first being resolved. Which is the very reason for this ticket.

 A WhereNode does not have to resolve to anything itself, but it has to
 give nested expressions the chance to resolve.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.0d3b4105dabfcbb229c3a004ee5d1a8f%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #25425: Enforce calling resolve_expression before as_sql on all expressions

[Django]

#25425: Enforce calling resolve_expression before as_sql on all expressions
-+-
 Reporter:  jarshwah |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by akaariai):

 We have two interfaces:
   - Resolvable: resolved to an expression with resolve_expression
   - Compilable: can be turned into SQL

 Not all compilables are resolvables, nor the other way around. So, it
 doesn't make sense to enforce that all compilables have been resolved.

 I'm not even sure what WhereNode.resolve_expression() would do? We have
 other similar types that are not resolved ever, for example BaseTable and
 Join.

 We could do the check for those cases where we have the resolve + compile
 contract.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.dace88662c872641aa84d54ed10f0d66%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

[Django] #25425: Enforce calling resolve_expression before as_sql on all expressions

[Django]

#25425: Enforce calling resolve_expression before as_sql on all expressions
-+-
   Reporter:  jarshwah   |  Owner:  nobody
   Type:  Bug| Status:  new
  Component:  Database   |Version:  master
  layer (models, ORM)|
   Severity:  Normal |   Keywords:
   Triage Stage: |  Has patch:  0
  Unreviewed |
Needs documentation:  0  |Needs tests:  0
Patch needs improvement:  0  |  Easy pickings:  0
  UI/UX:  0  |
-+-
 There are many places in the ORM that will try to compile an expression
 without first having ensured it was resolved.  The contract of expressions
 is as follows:

 {{{
 resolved = expression.resolve_expression(*kwargs)
 sql, params = compiler.compile(resolved)
 }}}

 Resolving the expression does two major things. First, it creates a copy
 so that expressions can be shared without clashing with each other.
 Secondly, the resolve_expression method may do necessary validation or
 extra query work.

 There are a few known places where expression like objects are used
 without first being resolved.

 - Most `Where` nodes are not resolved correctly (anywhere where self.where
 or self.query.where is compiled)
 - SQLUpdateCompiler
 - SQLDeleteCompiler
 - django.db.models.lookups.Lookup
 - django.db.models.related_lookups.RelatedIn

 I wonder if we should consider enforcing this at the code level. Refuse to
 compile the expression unless it has been resolved first. A naive way of
 doing this would be to maintain a `resolved` boolean flag, and switch it
 to `True` when resolved. The `as_sql` method could then error if the
 expression has not been resolved.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/051.f4adfacf12ffd334baa507a41a334282%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.