Refs #35224 -- Removed special-case GenericForeignKey system check. #17886
Conversation
fa6f82a to
7587505
Compare
|
Ah, this has made To fix the failures, I think we’ll need to either modify the test models or the checks that are enabled in |
7587505 to
b6f07e2
Compare
| class GfkModel(models.Model): | ||
| object_id = models.PositiveIntegerField() | ||
| content_type = models.ForeignKey(ContentType, models.CASCADE) | ||
| gfk = GenericForeignKey() |
There was a problem hiding this comment.
Needed for the below GenericRelation to have something to refer to, otherwise the contenttypes.E004 check fails.
| content_type = models.ForeignKey(ContentType, models.CASCADE, related_name="+") | ||
| object_id = models.PositiveIntegerField() | ||
| content_object = GenericForeignKey() |
There was a problem hiding this comment.
As above, fields required to fill in for GenericRelations below, preventing contenttypes.E004 errors.
| for field in cls._meta.private_fields: | ||
| errors.extend(field.check(**kwargs)) |
There was a problem hiding this comment.
One outstanding wrinkle: private_fields are copied into subclasses, so checks for one field get repeated in subclasses.
I don’t think there’s a way with the current attributes to tell if a field is local and private right now. Maybe more attributes or reorganization could work.
| @@ -357,7 +357,7 @@ def __init__( | |||
|
|
|||
| def check(self, **kwargs): | |||
| return [ | |||
| *super().check(**kwargs), | |||
| *self._check_field_name(), | |||
There was a problem hiding this comment.
After the fix to make the GenericRelation checks run, the test suite showed many errors: fields.E304, fields.E305, and fields.E311. For example:
aggregation_regress.HardbackBook.tags: (fields.E311) 'ItemTag.book_ptr' must be unique because it is referenced by a foreign key.
HINT: Add unique=True to this field or add a UniqueConstraint (without condition) in the model Meta.constraints.
generic_relations_regress.Cafe.links: (fields.E304) Reverse accessor 'Link.places' for 'generic_relations_regress.Cafe.links' clashes with reverse accessor for 'generic_relations_regress.Place.links'.
HINT: Add or change a related_name argument to the definition for 'generic_relations_regress.Cafe.links' or 'generic_relations_regress.Place.links'.
I think these are false positives. The easiest way to prevent them was to modify GenericRelation.check() to not call super(), like GenericForeignKey.check(). I tried to look through all existing checks for what’s relevant, and the field name check is the only one that really stood out.
b6f07e2 to
40cf958
Compare
|
@adamchainz I'm reviewing code around ticket-35224 to do a full review of ticket-36151. Is this PR still relevant? Could we close it if not? |
ticket-35224
Profiling checks after 6002df0 was merged, I found that
check_generic_foreign_keys()was taking ~2% of the total time. It seems that iterating over all fields of all models is fairly expensive.The special check function can be eliminated now that
GenericForeignKeyif we makeModel._check_fieldscover private fields as well. That seems like a change we’d want anyway.