Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
-+-
 Reporter:  Jon Dufresne |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  model form   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Iván Brizuela):

 This change results in a breaking change or a “gotcha” for validations
 that depended on proxy model classes.

 See this pseudocode setup:

 {{{
 class AccountDocument(models.Model):
 INVOICE_TYPE = 'I'
 PAYMENT_TYPE = 'P'
 DOCUMENT_TYPES = [
 (INVOICE_TYPE, 'Invoice'),
 (PAYMENT_TYPE, 'Payment'),
 ]
 document_type = models.CharField(
 max_length=1,
 choices=DOCUMENT_TYPES,
 default=INVOICE_TYPE)
 account: str
 total: Decimal

 class Invoice(AccountDocument):
 # On insert / update guarantee that document_type == INVOICE_TYPE

 objects: Custom manager filters AccountDocument by document_type ==
 INVOICE_TYPE

 class Meta:
 proxy = True

 class Payment(AccountDocument):
 # On insert / update guarantee that document_type == PAYMENT_TYPE

 objects: Custom manager filters AccountDocument by document_type ==
 PAYMENT_TYPE

 class Meta:
 proxy = True

 class Receipt(models.Model):
 invoice: Invoice
 payment: Payment
 amount: Decimal
 }}}

 When using this setup:

 {{{
 invoice:Invoice = Invoice(document_type=INVOICE_TYPE, account='xyz',
 total=Decimal('100'))
 payment:Payment = Payment(document_type=PAYMENT_TYPE, account='xyz',
 total=Decimal('50'))

 invoice.save()
 payment.save()

 # Setting error condition to be tested: invalid account documents are
 assigned as receipt properties
 receipt = Receipt(
 invoice=payment, # invalid!
 payment=invoice, # invalid!
 amount=Decimal('50'))

 receipt.full_clean()
 }}}

 **Expected (Before 3.2):**
 ValidationError is raised, as neither invoice:Payment nor payment:Invoice
 exist when looked up by their default manager.

 **Actual (Since 3.2):**
 No exception is raised, which is unexpected given the field definitions.

 Both the example given to request this change and the one I am providing,
 stretch the base idea behind proxy models: “only change the Python
 behavior of a model”.

 Semantically, I don't think an ArticleManager should filter by 'archived'
 status. It would be preferred to have three different managers or proxy
 models:
 * Article with ArticleManager (not filtered by archived status): Use this
 when any article can be picked from a list.
 * ActiveArticle(proxy) with ActiveArticleManager (filter archived==False):
 Enforce custom rules, like editing an article.
 * ArchivedArticle(proxy) with ArchivedArticleManager (filter
 archived==True): Enforce other rules, like read-only status.

 **Keeping a proxy model self-contained by using its default manager to
 validate a ForeignKey seems a more valuable use case to me.**

 Consider adding the following note to the release notes in 3.2:
 https://docs.djangoproject.com/en/4.0/releases/3.2/

 > (existent) ForeignKey.validate() now uses _base_manager rather than
 _default_manager to check that related instances exist.
 > (addition) This might break validations enforced by using proxy model
 classes in ForeignKey field definitions.

-- 
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/0107018196cc683b-a65c3e61-c690-4588-b2c5-ca1bd1aea350-00%40eu-central-1.amazonses.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
-+-
 Reporter:  Jon Dufresne |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  model form   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak):

 Replying to [comment:8 Damian Nardelli]:
 > Hello. The documentation for v2.2
 (https://docs.djangoproject.com/en/2.2/topics/db/managers/#using-managers-
 for-related-object-access) is stating that the base manager will be used
 in that version too instead of the default manager. We just saw that this
 fix has been released 12 days ago for v3.2.5, we were wondering when it'll
 be released for v2.2? Thanks!

 It is not a regression, per our backporting policy this means it doesn't
 qualify for a backport to 2.2.x (see
 [https://docs.djangoproject.com/en/stable/internals/release-process/
 Django’s release process] for more details). Moreover Django 2.2 is in
 extended support so it doesn't get bugfixes (except security issues)
 anymore.

-- 
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/067.26396cb819db8f816810dc8c32c277c3%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
-+-
 Reporter:  Jon Dufresne |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  dev
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  model form   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Damiox):

 Hello. The documentation for v2.2
 (https://docs.djangoproject.com/en/2.2/topics/db/managers/#using-managers-
 for-related-object-access) is stating that the base manager will be used
 in that version too instead of the default manager. We just saw that this
 fix has been released 12 days ago for v3.2.5, we were wondering when it'll
 be released for v2.2? Thanks!

-- 
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/067.0e0efe9a353b508d178e53872a10ae3c%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
-+-
 Reporter:  Jon Dufresne |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  model form   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson ):

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


Comment:

 In [changeset:"e13cfc6dfd4212ef7a40db1a41d3ae6ac4b97de0" e13cfc6d]:
 {{{
 #!CommitTicketReference repository=""
 revision="e13cfc6dfd4212ef7a40db1a41d3ae6ac4b97de0"
 Fixed #31596 -- Changed ForeignKey.validate() to use the base manager.
 }}}

-- 
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/067.e3a31997145aa3f93a36d547ec85411b%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
-+-
 Reporter:  Jon Dufresne |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  model form   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * 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 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/067.643478a671143246ed95e4a18d07e699%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
-+-
 Reporter:  Jon Dufresne |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  model form   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Jon Dufresne):

 > I take it the change here is ORM rather than forms per se.

 Makes sense. This is a change to model validation to allow the form
 validation to work.

-- 
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/067.e2203d9d0d8499fbf530b76d7859de66%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
-+-
 Reporter:  Jon Dufresne |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  model form   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * keywords:   => model form
 * component:  Forms => Database layer (models, ORM)


Comment:

 I take it the change here is ORM rather than forms per se.

-- 
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/067.97e69afe0190510297a750bc0bdd7651%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
--+
 Reporter:  Jon Dufresne  |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Forms |  Version:  master
 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 Carlton Gibson):

 * stage:  Unreviewed => Accepted


Comment:

 OK, yes, let's provisionally Accept this: I think it's probably correct.
 (At this level we're avoiding DB errors, not business logic errors, like
 "was this archived", which properly belong to the form layer...)

-- 
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/067.7a9df28c0d478f89db49e93f4851c804%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
--+--
 Reporter:  Jon Dufresne  |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Forms |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Unreviewed
Has patch:  1 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+--
Changes (by Carlton Gibson):

 * cc: Carlton Gibson (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 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/067.29769c30728064d27355bf979d4fee9b%40djangoproject.com.

Re: [Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
--+--
 Reporter:  Jon Dufresne  |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Forms |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Unreviewed
Has patch:  1 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+--
Changes (by Jon Dufresne):

 * has_patch:  0 => 1


Comment:

 https://github.com/django/django/pull/12923

-- 
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/067.1508c850b6ed71eb2f6ae000ff1a92b7%40djangoproject.com.

[Django] #31596: ForeignKey.validate() should validate using the base manager.

[Django]

#31596: ForeignKey.validate() should validate using the base manager.
+
   Reporter:  Jon Dufresne  |  Owner:  nobody
   Type:  Bug   | Status:  new
  Component:  Forms |Version:  master
   Severity:  Normal|   Keywords:
   Triage Stage:  Unreviewed|  Has patch:  0
Needs documentation:  0 |Needs tests:  0
Patch needs improvement:  0 |  Easy pickings:  0
  UI/UX:  0 |
+
 `ForeignKey.validate()` should validate using the base manager instead of
 the default manager.

 Consider the models:

 {{{
 class ArticleManager(models.Manage):
 def get_queryset(self):
 qs = super().get_queryset()
 return qs.filter(archived=False)

 class Article(models.Model):
 title = models.CharField(max_length=100)
 archived = models.BooleanField(default=False)

 # Don't include archived articles by default.
 objects = ArticleManager()

 class FavoriteAricles(models.Model):
 article = models.ForeignKey(Article, on_delete=models.CASCADE)
 }}}

 In the example, now consider a form that allows users to pick a favorite
 article including archived articles.

 {{{
 class FavoriteAriclesForm(forms.ModelForm):
 class Meta:
 model = FavoriteArticle
 fields = '__all__'

 def __init__(self, *args, **kwargs):
 super().__init__(*args, **kwargs)
 # Use the base manager instead of the default manager to allow
 archived articles.
 self.fields['article'].queryset = Article._base_manager.all()
 }}}

 The above form will never validate as `True` when a user selects an
 archived article. This is because the ForeignKey validation always uses
 `_default_manager` instead of `_base_manager`. The user facing error
 message is "article instance with id 123 does not exist." (quite confusing
 to typical users). The code for this validation is here:

 
https://github.com/django/django/blob/94f63b926fd32d7a7b6e2591ef72aa8f040f25cc/django/db/models/fields/related.py#L917-L919

 The `FavoriteAriclesForm` is specifically designed to use a different
 manager, but the ForeignKey validation makes this difficult.

 In this example scenario, it is not acceptable to change the model's
 default manager as the default should avoid archived articles in other
 typical scenarios.

 Suggested solution: the ForeignKey validation should use the
 `_base_manager` instead which does not include the default filters.

-- 
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/052.492df47c4248cd263cf0642e9e2d303f%40djangoproject.com.