Opened 3 years ago

Last modified 13 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 in tests/invalid_models_tests/test_models.py?

Accepting based on the discussion on the PR

Change History (4)

comment:1 by Hasan Ramezani, 3 years ago

Hey @Carlton,

I've checked it a little bit and it seems the property replaced by this change.

Can we still though detect that such a configuration error occurred, i.e. that I named a property *_id matching an FK accessor?

I couldn't find a way to detect.

Can we then update the system check (or replace it) and adjust/remove the test_property_and_related_field_accessor_clash test in tests/invalid_models_tests/test_models.py?

I think we can remove the check and test but I am not sure!

comment:2 by Carlton Gibson, 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 Mariusz Felisiak, 13 months ago

Cc: Natalia Bidart added

comment:4 by Natalia Bidart, 13 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.

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