Do not recreate RelatedObject instances

[Krzysztof Jurewicz]

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.

[Krzysztof Jurewicz]

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

[Anssi Kääriäinen]

Seems valid - I assume all tests pass?

[Krzysztof Jurewicz]

Yes:

Ran 4915 tests in 431.403s

OK (skipped=135, expected failures=7)

[Krzysztof Jurewicz]

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.

[Krzysztof Jurewicz]

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

[Nick Sandford]

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

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

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.

[Krzysztof Jurewicz]

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.

[Anssi Kääriäinen <akaariai@…>]

In 3647c0a49a2f4535b8a9aba40e662743e4d53e76:

Avoided unnecessary recreation of RelatedObjects

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

[Anssi Kääriäinen]

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.

[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

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

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.

[Alex Gaynor]

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

[Tim Graham]

As there hasn't been any followup from the reporter regarding the "AlternateBook issue" described in comment 12, I'm going to close this for now. Please open a new ticket if any issues remain.