Opened 24 hours ago
Last modified 78 minutes ago
#36070 assigned Cleanup/optimization
clean_fields(exclude=["pk"]) not implemented for composite primary keys
Reported by: | Jacob Walls | Owned by: | Jacob Walls |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Passing exclude=["pk"]
to full_clean()
/ clean_fields()
has no effect: the underlying fields have to be provided instead.
This "no effect" might be silent (coercing a value when you didn't expect) or loud (raising ValidationError) given that we don't raise ValueError
for providing an extraneous field name.
I'm suggesting we either implement exclude=["pk"]
to exclude all the composed fields or raise ValueError
. Raising an error might be wiser?
Failing test:
-
tests/composite_pk/test_models.py
diff --git a/tests/composite_pk/test_models.py b/tests/composite_pk/test_models.py index ca6ad8b5dc..ed54d6c352 100644
a b class CompositePKModelsTests(TestCase): 118 118 119 119 self.assertSequenceEqual(ctx.exception.messages, messages) 120 120 121 def test_clean_fields_exclude_pk(self): 122 post = Token() 123 post.clean_fields(exclude=["pk"]) 124 121 125 def test_field_conflicts(self): 122 126 test_cases = ( 123 127 ({"pk": (1, 1), "id": 2}, (1, 1)),
====================================================================== ERROR: test_clean_fields_exclude_pk (composite_pk.test_models.CompositePKModelsTests.test_clean_fields_exclude_pk) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/.../django/tests/composite_pk/test_models.py", line 123, in test_clean_fields_exclude_pk post.clean_fields(exclude=["pk"]) ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^ File "/Users/.../django/django/db/models/base.py", line 1708, in clean_fields raise ValidationError(errors) django.core.exceptions.ValidationError: {'tenant': ['This field cannot be null.'], 'id': ['This field cannot be null.']} ----------------------------------------------------------------------
Change History (4)
comment:1 by , 23 hours ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:2 by , 21 hours ago
This might be a nothing, but the doc explains that a "virtual field" is "excluded from model forms" and "does not have a form field", but it doesn't tell me what impact there is on model validation, forms aside. "Virtual field" isn't described elsewhere in the docs, so for me I didn't follow that it couldn't be passed to exclude=
.
A model with composite pk's looks like it has a model field called pk
, so the idea is a user might think you can pass it to exclude_fields
and get every part of the pk
excluded. From the linked doc I can't really tell that I can't.
(As I'm sure you've gathered I'm just kicking the tires here. Fine to wait to see if this comes up in practice.)
comment:3 by , 5 hours ago
Resolution: | needsinfo |
---|---|
Severity: | Normal → Release blocker |
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
True, a "virtual field" is not really a defined term and in my head things like a GenericForeignKey or a virtual GeneratedField might be in a similar category (category roughly being fields that don't map to a single database column).
I will accept as happy to review a docs update
comment:4 by , 78 minutes ago
Owner: | set to |
---|---|
Status: | new → assigned |
I might be misunderstanding but I feel like the current behavior is correct
I think we tried to hint at this in the docs in the Composite Primary Keys in forms section. Do you think we need to expand on this or have I misunderstood what you're expecting?