Opened 5 weeks ago

Closed 4 weeks ago

Last modified 4 weeks ago

#36075 closed Cleanup/optimization (fixed)

Field.primary_key documentation should details its interaction with CompositePrimaryKey

Reported by: Simon Charette Owned by: Simon Charette
Component: Documentation Version: dev
Severity: Release blocker Keywords:
Cc: Csirmaz Bendegúz Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Simon Charette)

A particular focus should be put on the fact that the flag will and should only be set to True if the field is the single member of the primary key. When a composite primary is defined on the model this flag will be False for all the fields defined on the model and _meta.pk_fields (which should be documented) should be used instead to build composite primary key ready third-party application.

In other words, it should be made clear that code that relies solely on Field.primary_key is not composite primary key ready and that _meta.pk_fields should be used instead. I feel like this is something that should be clearly pointed out in the composite primary key documentation on how to make reusable app code composite primary key ready.

Change History (13)

comment:1 by Simon Charette, 5 weeks ago

Description: modified (diff)

comment:2 by Simon Charette, 5 weeks ago

Description: modified (diff)

comment:3 by Simon Charette, 5 weeks ago

Description: modified (diff)

comment:4 by Sarah Boyce, 5 weeks ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:5 by Sarah Boyce, 5 weeks ago

Cc: Csirmaz Bendegúz added

From #36074

If you grep for .primary_key you'll notice that most of the checks against this field need to be adjusted to use field in opts.pk_fields instead which makes me wonder if we took the right decision by not setting this flag to True when included as a member of CompositePrimaryKey.

I am not sure how best to reword the title but I think this piece of work (checking the existing checks against the primary_key attribute and whether they should be using field in opts.pk_fields, as well as checking the decision to not setting this flag to True when included as a member of CompositePrimaryKey) could be part of this ticket, as this documentation should accompany those changes.

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

comment:6 by Simon Charette, 5 weeks ago

Owner: set to Simon Charette
Status: newassigned

Sleeping on it I think it's fine that we keep things as they are code wise on the assignment of primary_key = False when used with CompositePrimaryKey as long as we clearly document that _meta.pk_fields should be used. The reason is that many of the field.primary_key usages immediately stop when they first their first match so they are likely broken anyway.

Forcing third-party applications that wish to support composite primary keys to migrate to _meta.pk_fields and abandon .primary_key seem like a better option that having code implicitly break because they only expect one field to have primary_key = True which is decade old assumption.

comment:7 by Simon Charette, 4 weeks ago

Has patch: set

comment:8 by Sarah Boyce <42296566+sarahboyce@…>, 4 weeks ago

In bf7b17d1:

Refs #36075 -- Used field in pk_fields over field.primary_key.

comment:9 by Sarah Boyce, 4 weeks ago

Triage Stage: AcceptedReady for checkin

comment:10 by Sarah Boyce <42296566+sarahboyce@…>, 4 weeks ago

Resolution: fixed
Status: assignedclosed

In e580926d:

Fixed #36075 -- Documented how to introspect composite primary keys.

Document _meta.pk_fields and interactions between Field.primary_key and
CompositePrimaryKey.

Thanks Mariusz for the review.

comment:11 by Sarah Boyce <42296566+sarahboyce@…>, 4 weeks ago

In 4bfec242:

Fixed #36093 -- Adjusted unique checks to account for inherited primary keys.

Regression in bf7b17d16d3978b2e1cee4a0f7ce8840bd1a8dc4 refs #36075.

Thanks Sage Abdullah for the report and tests.

comment:12 by Sarah Boyce <42296566+sarahboyce@…>, 4 weeks ago

In 161e79d2:

Refs #36075 -- Adjusted pk_fields usage in bulk_update eligibility checks.

Regression in bf7b17d16d3978b2e1cee4a0f7ce8840bd1a8dc4.

Thanks Sage Abdullah for the report.

comment:13 by Sarah Boyce <42296566+sarahboyce@…>, 4 weeks ago

In f07360e8:

Refs #36075 -- Adjusted MTI handling of _non_pk_concrete_field_names.

Regression in bf7b17d16d3978b2e1cee4a0f7ce8840bd1a8dc4.

Thanks Sage Abdullah for the report.

Note: See TracTickets for help on using tickets.
Back to Top