Code

Opened 17 months ago

Last modified 15 months ago

#19399 new Cleanup/optimization

Do not recreate RelatedObject instances

Reported by: KJ Owned by: KJ
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: KJ, sun.void@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When generating accessor names for fields in django/core/management/validation.py , new RelatedObject instances are created. Using existing ones retrieved from fields will make the code cleaner and more flexible, allowing some modifications of RelatedObject instances used by third party apps.

Attachments (0)

Change History (17)

comment:1 Changed 17 months ago by KJ

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I've created a pull request: https://github.com/django/django/pull/563

comment:2 Changed 17 months ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

Seems valid - I assume all tests pass?

comment:3 Changed 17 months ago by KJ

Yes:

Ran 4915 tests in 431.403s

OK (skipped=135, expected failures=7)

comment:4 Changed 16 months ago by ramiro

  • Needs tests set

comment:5 Changed 16 months ago by KJ

  • Component changed from Core (Management commands) to Database layer (models, ORM)
  • Summary changed from Do not recreate RelatedObject instances in validation to Do not recreate RelatedObject instances

I do agree that some tests are worth adding for this ticket. Meanwhile, I've located other places in code where RelatedObject was being recreated and made a similar change to them. This caused test_inline_formsets_with_multi_table_inheritance from model_tests.model_formset.ModelFormSetTest to fail, but since the reason was only the change of generated formset's prefix (which shouldn't be harmful by itself), I've resolved the problem by changing the test.

comment:6 Changed 16 months ago by KJ

  • Needs tests unset

I've added tests and updated the pull request (commit hash is now c2028f6bfcde2db5681b).

Last edited 16 months ago by KJ (previous) (diff)

comment:7 Changed 15 months ago by slurms

  • Triage Stage changed from Accepted to Ready for checkin

All tests pass on sqlite. Patch looks good, marking RFC.

comment:8 Changed 15 months ago by akaariai

I took a look at the patch. It looks otherwise good, but I can't see why the change in the model_formsets is correct. Not saying it isn't, I just can't see why it is. On a quick look it seems we are really changing semantics here - the form uses the plain fk to Book, while the user asked for AlternateBook.

It seems this change has (small) potential for breaking existing code, so there should at least be a good reason to do so. My initial feeling is to not commit this part of the code.

comment:9 Changed 15 months ago by akaariai

Actually, if you have two subclasses of a Book and you try to use them in inlineformset_factory in the same view, wouldn't the ID names conflict when they didn't before? On this ground I think we need to not do the form changes.

comment:10 Changed 15 months ago by KJ

The user asks for AlternateBook when creating inline formset, but foreign key is created on Book. Author class doesn't know anything about AlternateBook class being tied to it. So if we ask what will happen if we have two subclasses of Book with inline formsets, then this situation is roughly equivalent to creating two inline formsets for Book class, and similar problems will occur. If there is really separate logic tied to AlternateBook, maybe separate foreign key should be created, but these are just my loose thoughts, not strict reasoning.

If we however decide to preserve AlternateBook formset prefixes, I will argue to change the method of generating them. Current implementation doesn't use RelatedObject from AlternateBook, which was created for Book class and returns 'book-' prefixes. Instead of it another RelatedObject is created to obtain different prefix, but the original one remains inside AlternateBook class. This is tricky and may be misleading — if we already have one RelatedObject inside AlternateBook class, then we should stick to it.

I've updated the pull request to fix a typo reported in comment. Commit hash is now 6cfb8a8a506b2c.

comment:11 Changed 15 months ago by Anssi Kääriäinen <akaariai@…>

In 3647c0a49a2f4535b8a9aba40e662743e4d53e76:

Avoided unnecessary recreation of RelatedObjects

Refs #19399. Thanks to Track alias KJ for the patch.

comment:12 Changed 15 months ago by akaariai

  • Easy pickings unset
  • Has patch unset
  • Triage Stage changed from Ready for checkin to Accepted

I committed the validation and caching changes, without the tests.

Personally, I don't think the tests add that much value. They test 3 specific places for not creating RelatedObject instances, but we have still the rest of codebase uncovered. It is impossible to test that RelatedObjects aren't recreated anywhere in the codebase by testing specific places for that. In addition, if we ever decide to alter the underlying implementations, the tests would be noise errors. (I am currently wondering why we need both field.rel and field.related. Seems strange...).

I also removed the self.rel_name line from the forms/models.py. It is dead-code.

I don't feel comfortable doing the alternatebook name change. It *will* break existing public-API using code, at least tests, as we change what values can be passed in the request.POST. Those using public APIs are more important than those who want to hack with the internals.

So, leaving this open if you want to tackle the AlternateBook issue. It doesn't seem to be an easy one to tackle.

comment:13 Changed 15 months ago by anonymous

Commit 3647c0a49a2f4535b8a9aba40e662743e4d53e76 broke django-taggit with the following traceback:

  File ".../django/django/core/management/base.py", line 277, in validate
    num_errors = get_validation_errors(s, app)
  File ".../django/django/core/management/validation.py", line 169, in get_validation_errors
    for r in rel_opts.get_all_related_many_to_many_objects():
  File ".../django/django/db/models/options.py", line 437, in get_all_related_many_to_many_objects
    cache = self._fill_related_many_to_many_cache()
  File ".../django/django/db/models/options.py", line 470, in _fill_related_many_to_many_cache
    cache[f.related] = None
AttributeError: 'TaggableManager' object has no attribute 'related'

So, it seems that it was possible for 3rd-party fields which emulate ManyToManyField to have .rel property, but don't have .related property

comment:14 Changed 15 months ago by akaariai

I will still try to investigate the related fields API and see if it was just accidental that django-taggit worked before. Still, my gut feeling is to revert the commit - the idea for the commit was to make life easier for 3rd party apps, yet we break other 3rd party apps.

comment:15 Changed 15 months ago by akaariai

And, after some digging, I think the right resolution is to leave the commit in, and django-taggit should include this fix into TaggableManager:

     def post_through_setup(self, cls):
+        self.related = RelatedObject(cls, self.model, self)

I am very confused about the whole related field setup. We have these three things:

   - RelatedField (which isn't a Field subclass)
   - RelatedObject (telling us what the relation is) - found from field.related
   - ManyToOneRel (as far as I can see, telling us what the relation is) - found from field.rel

At least the last two seem like they could be merged. Maybe we could get totally rid of the last two. OTOH this is a change that will break 3rd party apps for no other reason than code clarity, so I don't feel like doing anything about this. Still, if somebody knows why the setup is like this a comment wouldn't hurt...

If it seems the commit for this ticket breaks more stuff, I am willing to consider reverting it. For now, it seems a relation field should have both field.rel and field.related.

comment:16 Changed 15 months ago by Alex

For what it's worth django-taggit is definitely using non-public APIs, so don't feel obligated to fix it.

comment:17 Changed 15 months ago by void

  • Cc sun.void@… added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from KJ to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.