Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  Shai
 |  Berger
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  2.2
  (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
-+-
Changes (by Carlton Gibson):

 * needs_better_patch:  0 => 1


Comment:

 OK, after
 [https://github.com/django/django/pull/14260#pullrequestreview-742687431
 looking at the PR], I'm going to mark this as Patch needs improvement
 again. (It may be we uncheck as the discussion on the PR continues.)

 In summary, two main points from the review:

 * The proposal to `add_to_class()` effects all passes through that, rather
 than just those where an attribute is added late, and may override an
 existing attribute. Making the change means we end with different error
 paths for the case on the concrete instance vs inheritance, and the
 existing system checks for that seem more tightly targeted for that.
 * A specific error raised at the point where the default objects manager
 is added would be able to offer a much better error message, whilst
 avoiding the above. (A subsidiary System Check could alert to
 `extra_manager` cases.)

-- 
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/063.6d4e775dab335982d4b8445e3570b69c%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  Shai
 |  Berger
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Shai Berger):

 * needs_better_patch:  1 => 0
 * needs_tests:  1 => 0


Comment:

 The flags "needs improvement" and "needs tests" were set on this ticket
 with vague comments on the PR which basically say the same as the flags,
 with no more details. A request for clarification has been left unanswered
 for two months. Hence, I'm putting this back on the review queue.

-- 
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/063.c206902e195dbfe9cfcb755a20bbd383%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  Shai
 |  Berger
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Johannes Maron):

 * needs_better_patch:  0 => 1
 * needs_tests:  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 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/063.330a98d4d25d5b9092283a3ea4183d8d%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  Shai
 |  Berger
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Shai Berger):

 * needs_tests:  1 => 0


-- 
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/063.5d9f533530d965e15382ad6434658dd0%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  Shai
 |  Berger
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Shai Berger):

 * needs_better_patch:  1 => 0


-- 
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/063.7f3f36dbd3b9bb2f0ba312ae447b91c4%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  Shai
 |  Berger
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Shai Berger):

 For the record, CI agrees with my machine. It's the same two tests that
 fail.

-- 
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/063.36031572881aeb031f4dc21b6c54785c%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  Shai
 |  Berger
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Shai Berger):

 * owner:  nobody => Shai Berger
 * needs_better_patch:  0 => 1
 * has_patch:  0 => 1
 * status:  new => assigned


Comment:

 We'll see what CI says, but on my machine, the
 [https://github.com/django/django/pull/14260 WIP PR] only seems to break
 tests for invalid models, by making them "more invalid" (fail to create
 the class instead of failing a check).

 I'm not sure if that's a bad thing.

-- 
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/063.19cbdb406a0bf9eb54091fe0502e1928%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Shai Berger):

 Hmmm. That makes me wonder if the real change we need to make is in
 {{{add_to_class()}}} -- make it not override existing attributes so
 carelessly. That will fix this issue, and maybe some others like it; I
 only wonder what it will break.

 I'll try to make a PR for that later.

-- 
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/063.0fb734937fb92c7f193d807b6254ebe3%40djangoproject.com.

Re: [Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
 Reporter:  Shai Berger  |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  2.2
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * stage:  Unreviewed => Accepted


Comment:

 Hi Shai.

 I think we might be able to do **something** here…

 If you get into
 
[https://github.com/django/django/blob/3b8527e32b665df91622649550813bb1ec9a9251/django/db/models/base.py#L357-L365
 this block where the `objects` manager is automatically added]:


 {{{
 if not opts.managers:
 if any(f.name == 'objects' for f in opts.fields):
 raise ValueError(
 "Model %s must specify a custom Manager, because it
 has a "
 "field named 'objects'." % cls.__name__
 )
 manager = Manager()
 manager.auto_created = True
 cls.add_to_class('objects', manager)
 }}}

 …then it seems that any value of `objects` is an error 🤔 — but
 particularly if `objects` is a `Manager` **class**.

 {{{
 >>> from django.db import models
 >>> class MyModel(models.Model):
 ... objects = "not-a-manager"
 ... class Meta:
 ... app_label = "testing"
 ...
 >>> MyModel.objects
 
 }}}

 I'd think we could at least warn there, but arguably even raise.

 Not sure how far down this road it would be worth going.

-- 
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/063.928d867be6b7525282bc305fee9789fc%40djangoproject.com.

[Django] #32640: Non-manager instance assigned to model class' objects is silently ignored

[Django]

#32640: Non-manager instance assigned to model class' objects is silently  
ignored
-+-
   Reporter:  Shai   |  Owner:  nobody
  Berger |
   Type:  Bug| Status:  new
  Component:  Database   |Version:  2.2
  layer (models, ORM)|
   Severity:  Normal |   Keywords:
   Triage Stage: |  Has patch:  0
  Unreviewed |
Needs documentation:  0  |Needs tests:  1
Patch needs improvement:  0  |  Easy pickings:  0
  UI/UX:  0  |
-+-
 Consider this models file:

 {{{#!python
 from django.db import models


 class MyManager(models.Manager):
 def get_queryset(self):
 return self.none()


 class MyModel(models.Model):
 one = models.IntegerField()

 objects = MyManager  # Note: Missing parentheses
 }}}

 The assignment of a manager class, rather than manager instance, is an
 easy mistake to make. I know, because I've made it. In a private
 discussion, Carlton Gibson suggested another variant of this:

 {{{#!python
 class MyOtherModel(models.Model):
 ...
 objects = models.Manager.from_queryset(SomeQueryset())
 # The above, too, is missing a pair of parentheses
 }}}

 But what should Django do?

 There's two possible behaviors I would consider reasonable:
 - Trust the user, and rely on Duck Typing. This would blow up very fast in
 this case, as any method invocation would be missing the {{{self}}}
 argument.
 - Raise an error on model creation, or at least in {{{check}}}.

 What Django does instead, is ignore the assignment and use a
 {{{models.Manager}}} instance.

 Two points to clarify that this is indeed a bug:
 - Assigning any object which isn't a {{{models.Manager}}} instance -- say,
 the number 17 -- gets the same result.
 - This only happens if only one manager is defined. If another manager is
 defined, the assignment stands as written (the first "reasonable" behavior
 described above).

 I found this on 2.2, it is still this way on master.

-- 
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/048.0ae4e734f527ffe2b79b509bab3547fe%40djangoproject.com.