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):  
    118118
    119119                self.assertSequenceEqual(ctx.exception.messages, messages)
    120120
     121    def test_clean_fields_exclude_pk(self):
     122        post = Token()
     123        post.clean_fields(exclude=["pk"])
     124
    121125    def test_field_conflicts(self):
    122126        test_cases = (
    123127            ({"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 Sarah Boyce, 23 hours ago

Resolution: needsinfo
Status: newclosed

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?

comment:2 by Jacob Walls, 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 Sarah Boyce, 5 hours ago

Resolution: needsinfo
Severity: NormalRelease blocker
Status: closednew
Triage Stage: UnreviewedAccepted

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

Last edited 5 hours ago by Sarah Boyce (previous) (diff)

comment:4 by Jacob Walls, 78 minutes ago

Owner: set to Jacob Walls
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top