Opened 3 years ago
Last modified 19 months ago
#32847 new Cleanup/optimization
Adjust models.E025 system check for updated field descriptor setting.
Reported by: | Carlton Gibson | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Natalia Bidart | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Fix for #30427 and #16176 adjusted the way field descriptors are set when subclassing abstract models (the descriptor is now always set of child class)
This left check models.E025
as perhaps unnecessary, or in need of replacement/adjustment.
models.E025: The property
<property name>
clashes with a related field accessor.
The original check made sure you didn't have a clash with a @property
named for a foreign key _id
attribute, such as fk_id
.
In this case the field descriptor would not have been set, and you'd have had the property but not the FK.
This is no longer possible, the descriptor will be applied. (See the adjustment to make the test pass here.)
- Can we still though detect that such a configuration error occurred, i.e. that I named a property
*_id
matching an FK accessor? - Can we then update the system check (or replace it) and adjust/remove the
test_property_and_related_field_accessor_clash
test intests/invalid_models_tests/test_models.py
?
Accepting based on the discussion on the PR
Change History (4)
comment:1 by , 3 years ago
comment:2 by , 3 years ago
Hi Hasan, thanks for looking.
I couldn't find a way to detect.
Yes, that's likely. I'll have one more pass at it and see if I can come up with anything, but if not we can close. (It's a pretty niche config error...)
comment:3 by , 20 months ago
Cc: | added |
---|
comment:4 by , 19 months ago
I spent some time reading this ticket and following the referenced PRs and associated tickets. I think I understand the overall history of the issue, though I'm somehow lost when it comes to small/specific details. Having said that, I did try to create a scenario to answer the question:
Can we still though detect that such a configuration error occurred, i.e. that I named a property *_id matching an FK accessor
and while I don't yet have an answer about detect such configuration, it's definitely something that in some situations yield to a surprising result. For example:
class ModelA(models.Model): title = models.CharField(max_length=50) class ModelB(models.Model): modela = models.ForeignKey(ModelA, on_delete=models.CASCADE) foo = models.CharField(max_length=3, default='foo') @property def modela_id(self): return 'ERROR'
Produces models such as any instance of ModelB
has modela_id
effectively being the id of the related ModelA
instance, instead of returning the string ERROR
. As a user, I would appreciate if this produces a check error or a migration failure or some sort of warning. The same happens if one would add this field to ModelB
:
modela_id = models.IntegerField(default=-100)
When running makemigrations
, No changes detected
is reported and modela_id
keeps returning the id of the related ModelA
instance.
So I would not close this issue, I think at some point we'd want to be able to produce some check/warning about this clashing.
Hey @Carlton,
I've checked it a little bit and it seems the property replaced by this change.
I couldn't find a way to detect.
I think we can remove the check and test but I am not sure!