[Django] #31596: ForeignKey.validate() should validate using the base manager.
Django
----------------------------------------+------------------------
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:
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: <https://code.djangoproject.com/ticket/31596>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
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: <https://code.djangoproject.com/ticket/31596#comment:1>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
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: <https://code.djangoproject.com/ticket/31596#comment:2>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
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: <https://code.djangoproject.com/ticket/31596#comment:3>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Bug | Status: new
(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: <https://code.djangoproject.com/ticket/31596#comment:4>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: model form | Triage Stage: Accepted
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: <https://code.djangoproject.com/ticket/31596#comment:5>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| 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: <https://code.djangoproject.com/ticket/31596#comment:6>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Keywords: model form | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson <carlton@…>):
* 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: <https://code.djangoproject.com/ticket/31596#comment:7>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: model form | Triage Stage: Ready for
| checkin
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: <https://code.djangoproject.com/ticket/31596#comment:8>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: model form | Triage Stage: Ready for
| checkin
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: <https://code.djangoproject.com/ticket/31596#comment:9>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: model form | Triage Stage: Ready for
| checkin
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: <https://code.djangoproject.com/ticket/31596#comment:10>