Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  duplicate
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by timgraham):

 * status:  new => closed
 * resolution:   => duplicate


Comment:

 Yes, duplicate of #14334.

--
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/064.8274d759c6292d153c07a3291eb882f5%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by collinanderson):

 I believe this is fixed now.

--
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/064.b20630f63cabe79e6d1d6da9009969d5%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by akaariai):

 The patch to #19385 did extensive changes to how foreign key lookup value
 checks are done. This will very likely need a revisit. It is now
 impossible to use wrong model class in lookups, but the current way is
 very likely too restrictive. Lets find out problematic cases and fix
 them...

-- 
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.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by akaariai):

 I am sure it is possible to do the validity checks in a way that doesn't
 break duck typing, and preserves the OneToOneField behaviour. The
 _pk_trace code could do validity checks only if the value is subclass of
 model. But doing the OneToOneField preservation will likely be ugly.

 If a nice patch surfaces I do think it is a good idea to add the check. It
 is not a good idea to do a backwards compatibility break just for the
 check.

-- 
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 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 https://groups.google.com/groups/opt_out.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by lukeplant):

 There is an argument for not fixing this: any time you add issubclass or
 isinstance checks you are breaking duck-typing.

 I have seen Django code where there were 'business' objects that wrapped
 model objects, and sometimes, IIRC, were used interchangeably. Fixing this
 bug might potentially break such code.

 I'm also not entirely convinced that the removed/changed tests ought to be
 invalid use cases. This is certainly a backwards incompatible change.

-- 
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 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 https://groups.google.com/groups/opt_out.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by akaariai):

 This will be horrible to fix. At least the following work currently, also
 with multistep chains:
 {{{
 parent__in=list_of_childs
 child__in=list_of_parents

 class A:
 pass

 class B:
 a = models.OneToOneField(A, primary_key=True)

 a__in=list_of_B
 b__in=list_of_A
 }}}

 This means that we must check issubclass in two directions + primary key
 chain from value to the related model, and from related model to value.

 The sanest fixes would be to precalculate a set of allowed models in
 Model._meta. The calculation of the set of allowed models will still be
 ugly, but at least checking would be somewhat fast... Patches welcome.

 Another way is to disallow the OneToOneField related lookups. The
 issubclass checks are easy to do.

-- 
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 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 https://groups.google.com/groups/opt_out.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by claudep):

 #19669 was a duplicate.

-- 
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 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 https://groups.google.com/groups/opt_out.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by akaariai):

 Maybe a model instance method `_get_field_value(field)` which checks that
 the field exists in the instance and then gets its value is the way to go.
 This way because we need to allow also using parent classes:
 {{{
 p = Place.objects.get(somecond)
 Chef.objects.filter(restaurant=p)
 }}}
 is a valid query if Place and Restaurant have the same primary key field.

-- 
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 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 https://groups.google.com/groups/opt_out.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by akaariai):

 * needs_better_patch:  0 => 1
 * stage:  Ready for checkin => Accepted


Comment:

 While looking how related field does its get_prep_lookup() I come up with
 this comment (models/fields/related.py:176-180):
 {{{
 def pk_trace:
 # Value may be a primary key, or an object held in a relation.
 # If it is an object, then we need to get the primary key value
 for
 # that object. In certain conditions (especially one-to-one
 relations),
 # the primary key may itself be an object - so we need to keep
 drilling
 # down until we hit a value that can be used for a comparison.
 }}}
 So, it seems to me it is intentional that you can use o2o fields like in
 the one_to_one tests. The changes to o2o handling need further
 investigation.

 My main concern is that It should be the field's responsibility to do the
 validation of the values. It does not belong into `QuerySet`.add_filter.
 It would be better to do "lookup, values =
 field.check_lookup_values(lookup, values)" at the point the check is in
 the patch. By default the method just returns lookup, values, but it could
 raise `TypeError` or do something to the values or lookup if needed. If
 this was done, the check could use the code of _pk_trace directly, and
 thus the check and actual implementation would be guaranteed to match.

-- 
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 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.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Ready for
 Keywords:   |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by nate_b):

 Ignore my previous patch - those imports were in-function to avoid a
 circular dependency (which is what I get for not looking before leaping).
 This one just corrects the issues in {{{test_foreign_key}}}.

-- 
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 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.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Ready for
 Keywords:   |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by nate_b):

 * needs_better_patch:  1 => 0
 * stage:  Accepted => Ready for checkin


-- 
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 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.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by nate_b):

 * needs_better_patch:  0 => 1


Comment:

 Your patch still applies cleanly, and there are no unexpected test
 failures.  After careful consideration toying with alternatives, I agree
 with your solution.  Two things though:

 For the sake of PEP8ness, could you put your imports at the top?  Or did
 you have a good reason to do your imports immediately preceeding your
 check?

 Also, in the {{{OneToOneTests.test_foreign_key}}} edit, why delete that
 line, instead of change it from {{{p}}} to {{{r}}}?

 In fact, that whole section looks like it always ought to have been closer
 to this:

 {{{
 assert_filter_waiters(restaurant__exact=self.r.pk)
 assert_filter_waiters(restaurant__exact=self.r)
 assert_filter_waiters(restaurant__pk=self.r.pk)
 assert_filter_waiters(restaurant=self.r.pk)
 assert_filter_waiters(restaurant=self.r)
 assert_filter_waiters(id__exact=self.r.pk)
 assert_filter_waiters(pk=self.r.pk)
 # Delete the restaurant; the waiter should also be removed
 r = Restaurant.objects.get(pk=self.r.pk)
 }}}

 It was only passing by the fortuitous accident that {{{self.p1.pk ==
 self.r.pk}}}.  Fix that up, and this patch looks good to me.

-- 
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 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.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by dgouldin):

 The test which was removed is no longer a valid use case.  Passing the
 wrong model type to a queryset filter kwarg should raise a TypeError, even
 in the instance where the PKs happen to always be the same.  The proper
 way to perform such a test would be to use concrete model inheritance, in
 which case isinstance would return True and the check I've added would
 pass.

-- 
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 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.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by dgouldin):

 * has_patch:  0 => 1


-- 
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 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.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  dgouldin
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  0|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by dgouldin):

 * owner:  nobody => dgouldin
 * cc: dgouldin (added)


-- 
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 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.

Re: [Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
 Reporter:  jdunck   |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.3
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  0|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by bpeschier):

 * stage:  Unreviewed => Accepted


Comment:

 Indeed, when using a Foo with an id also available in Author:
 {{{
 >>> foo2 = Foo.objects.create(pk=1)
 >>> Author.objects.filter(books=foo2)
 []
 }}}

-- 
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 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.

[Django] #16955: Querying on the reverse of a FK with the wrong class silently returns bad data

[Django]

#16955: Querying on the reverse of a FK with the wrong class silently returns 
bad
data
-+-
   Reporter:  jdunck |  Owner:  nobody
   Type:  Bug| Status:  new
  Component:  Database   |Version:  1.3
  layer (models, ORM)|   Keywords:
   Severity:  Normal |  Has patch:  0
   Triage Stage: |Needs tests:  0
  Unreviewed |  Easy pickings:  0
Needs documentation:  0  |
Patch needs improvement:  0  |
  UI/UX:  0  |
-+-
 {{{
 class Author(Model):
 pass

 class Book(Model):
 author = ForeignKey(Author, related_name='books')

 class Foo(Model):
 pass

 >>> a = Author.objects.create()
 >>> b  = Book.objects.create(author=a)
 >>> foo = Foo.objects.create(pk=999)
 >>> Author.objects.filter(books=foo)
 []
 }}}

 I would expect this to throw an exception because a Foo is not a Book.
 Instead, the Foo is coerced to its ID, which is then used as the PK for
 the reverse query.

 This also affects ManyToManyField with through (effectively a reverse FK).

-- 
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 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.