Project

Profile

Help

Issue #4681

Error caused by non-unique Master-Detail model names

Added by tustvold 6 months ago. Updated about 1 month ago.

Status:
MODIFIED
Priority:
High
Category:
-
Sprint/Milestone:
Start date:
Due date:
Severity:
3. High
Version:
Platform Release:
Blocks Release:
OS:
Backwards Incompatible:
No
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:
Sprint 57

Description

When both pulp_deb and pulp_rpm are installed one gets the following error running django-admin makemigrations.

deb.Package.content_ptr: (fields.E304) Reverse accessor for 'Package.content_ptr' clashes with reverse accessor for 'Package.content_ptr'.
        HINT: Add or change a related_name argument to the definition for 'Package.content_ptr' or 'Package.content_ptr'.
deb.Package.content_ptr: (fields.E305) Reverse query name for 'Package.content_ptr' clashes with reverse query name for 'Package.content_ptr'.
        HINT: Add or change a related_name argument to the definition for 'Package.content_ptr' or 'Package.content_ptr'.
rpm.Package.content_ptr: (fields.E304) Reverse accessor for 'Package.content_ptr' clashes with reverse accessor for 'Package.content_ptr'.
        HINT: Add or change a related_name argument to the definition for 'Package.content_ptr' or 'Package.content_ptr'.
rpm.Package.content_ptr: (fields.E305) Reverse query name for 'Package.content_ptr' clashes with reverse query name for 'Package.content_ptr'.
        HINT: Add or change a related_name argument to the definition for 'Package.content_ptr' or 'Package.content_ptr'.
rpm.Package.release: (models.E006) The field 'release' clashes with the field 'release' from model 'core.content'.
rpm.UpdateRecord.release: (models.E006) The field 'release' clashes with the field 'release' from model 'core.content'.

My hypothesis, although I am new to django and so could be mistaken, is this is the result of pulpcore.app.models.Content, which is the base pulp_deb.app.models.Package and pulp_rpm.app.models.Package, not being abstract.

Django tries to create a reverse accessor on pulpcore.app.models.Content for each of the derived types but as they have the same name we get the above error.

One way of addressing this would be to make plugin authors either name their models more uniquely, or use parent_link and related_name to work around the issue. Alternatively one could make the Content model abstract, although I suspect there is some reason that it isn't. Either way this is probably a bug that should be fixed.


Checklist


Related issues

Blocks Ansible Plugin - Task #4710: Rename AnsibleRole to just Role MODIFIED Actions

Associated revisions

Revision 4f7a9f3f View on GitHub
Added by Fabricio Aguiar 3 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681

Revision 6efc3c37 View on GitHub
Added by Fabricio Aguiar 3 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681

Revision 31f4937e View on GitHub
Added by Fabricio Aguiar 2 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681

Revision a9b33e9b View on GitHub
Added by Fabricio Aguiar 2 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681

Revision af1e844c View on GitHub
Added by Fabricio Aguiar 2 months ago

Updating docs for subclassing models

ref #4681

Revision 206e3d00 View on GitHub
Added by Fabricio Aguiar 2 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681

Revision 206e3d00 View on GitHub
Added by Fabricio Aguiar 2 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681

Revision 9f48b9b7 View on GitHub
Added by Fabricio Aguiar 2 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681

Revision 3c62889e View on GitHub
Added by Fabricio Aguiar 2 months ago

Avoid collisions between plugin models

Making mandatory declaring default_related_name
Required PR: https://github.com/pulp/pulp_file/pull/269
Required PR: https://github.com/pulp/pulp-certguard/pull/27
closes #4681

Revision 996b1ccb View on GitHub
Added by Fabricio Aguiar 2 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681
https://pulp.plan.io/issues/4681

Revision 99ac6642 View on GitHub
Added by Fabricio Aguiar 2 months ago

dealing with pulpcore default_related_name

Declaring default_related_name
ref #4681
https://pulp.plan.io/issues/4681

History

#1 Updated by daviddavis 6 months ago

  • Tags Pulp 3 added

#2 Updated by daviddavis 6 months ago

  • Priority changed from Normal to High
  • Severity changed from 2. Medium to 3. High
  • Triaged changed from No to Yes
  • Sprint set to Sprint 51

#3 Updated by daviddavis 6 months ago

  • Blocks Task #4710: Rename AnsibleRole to just Role added

#4 Updated by rchan 6 months ago

  • Sprint changed from Sprint 51 to Sprint 52

#5 Updated by dalley 6 months ago

tustvold, thank you for this report! I suspect the best course of action is to have plugins define their parent link, or have clearer names, like you said.

The reason Content isn't abstract is that Pulp needs to be able to run queries across all types of content, generically, and the only two options we've seen for doing that are Django model inheritance and Generic Foreign Keys, unfortunately. Between those two, model inheritance is probably the least-bad option. We did attempt to use GFKs but it was much more difficult to deal with and also had worse performance.

#6 Updated by daviddavis 6 months ago

  • Subject changed from pulpcore.app.models.Content is not abstract to Error caused by non-unique Master-Detail model names

#7 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3)

#8 Updated by daviddavis 6 months ago

Emailing pulp-dev0, it seems like most people are in favor of setting the OneToOneField or default_related_name on the detail model. If it's possible and not too complex, it might be nice to do this automatically. I think the next step would be to investigate doing it automatically and failing that, adding docs for plugin writers on how to set the parent_link name via OneToOneField or default_related_name.

[0] https://www.redhat.com/archives/pulp-dev/2019-April/msg00096.html

#9 Updated by bmbouter 6 months ago

If we can get the automatic one working that would be ideal. Documenting it in addition explicitly would be cool tool. Maybe as a note in the plugin writer's guide.

#10 Updated by rchan 5 months ago

  • Sprint changed from Sprint 52 to Sprint 53

#11 Updated by dalley 5 months ago

I'd like to put forwards a suggestion. If we're going to manually implement this link from the detail model back to the base Content model anyways, we could just ditch model inheritance for the content models entirely. It would be only mildly more inconvenient to use (because you can't transparently access the _created fields and so forth from the detail model, we wouldn't get proxy model behavior), but we would gain much more flexibility. This would enable us to easily manually implement bulk_create() for detail content by first bulk saving a bunch of Content objects, and then adding them to the detail models, and then bulk saving all the detail models. The Django proxy model magic really gets in the way of doing this in any straightforward way with multi-table model inheritance.

Positives:

  • dramatically simplifies a feature (bulk_create on content) that would enable massive speedups (at least 2x, probably more like 3x, possibly more than that)
  • conceptually basically the same but more explicit. the only thing we're missing is the proxy behavior

Negatives:

  • It would be a breaking plugin API change, but probably just a minor one, since PK would still be usable from detail models (the FK to base Content is the PK of the detail model, so they will always be the same).
  • Since _id, _href and so forth would not be transparently available as flat fields without the proxy model behavior, so we would either need to reimplement that bit (with properties?), or we would need to nest those fields under a _metadata field or something.
  • The latter option might not be desirable for the sake of filtering by those properties and possibly the bindings, and it would be a breaking API change, and since the structure is flat everywhere else it might introduce some inconsistency.
  • If we decided to expand this idea to encompass all models, we might have a complication with the "_last_updated" field because it would no longer be auto-saved when the detail model is. But content is immutable so it is not a concern here.
  • We would also need to reimplement save() so that it dealt with creating and saving the base model before saving the detail model, but that's not necessarily difficult.

Basically we'd be giving up a bit of code cleanliness and a bit of duplication to gain a whole lot more explicitness about what was going on and the opportunity to get a lot better sync performance.

#12 Updated by daviddavis 5 months ago

@dalley I like your proposal and I think it's worth exploring. A couple questions:

This would enable us to easily manually implement bulk_create() for detail content by first bulk saving a bunch of Content objects, and then adding them to the detail models, and then bulk saving all the detail models.

How would we handle the case where the detail models failed to save? Maybe have some way to cleanup the Content object?

Also, what do we do for other master/detail models?

#13 Updated by amacdona@redhat.com 5 months ago

  • Sprint changed from Sprint 53 to Sprint 54

#14 Updated by dalley 4 months ago

@david

1. We can fall back to saving them one by one in the error case, and then once we know which one(s) are erroring, we decide what to do in those circumstances. We can clean up the Content objects, yes.

2. Do we need to do anything for other models? Hopefully nobody is creating a PackageRemote, that would be less justifiable than a content model named Package. In theory we could do the same thing, but like I said we would then need to be careful about _last_updated.

#15 Updated by ttereshc 4 months ago

  • Sprint changed from Sprint 54 to Sprint 55

#16 Updated by daviddavis 4 months ago

  • Sprint/Milestone set to 3.0

#17 Updated by dkliban@redhat.com 3 months ago

  • Sprint changed from Sprint 55 to Sprint 56

#18 Updated by bmbouter 3 months ago

At open floor today the discussion was to try to see if the suggestion from @daviddavis in Comment 8 resolves the issue or not and post info on this issue.

#19 Updated by daviddavis 3 months ago

I looked into setting default_related_name for detail models and doing so seems to solve the problem. We have to update default_related_name to include the app_label but django provides a way to use variables in the default_related_name string.

When it comes to actually setting default_related_name, there are two options. The first is to manually set it:

 class FileContent(Content):
    TYPE = "file" 

    class Meta:
        default_related_name = "%(app_label)s_%(model_name)s"

The other option is to set it automatically but it involves extending django's Model metaclass. It's not pretty but I've posted a patch here0.

It should be noted that this change will require a migration in plugins as django names the FK based on the relation name, which we're changing.

[0] https://github.com/daviddavis/pulpcore/commit/2b9ff6f4f3484bd1cf9bac09451b3484397796b2

#20 Updated by bmbouter 3 months ago

I read the patch and it makes sense to me. I think we should go with the manual approach for few reasons. It's better to have this be something explicit I think instead of an implicit side effect for some class types (zen of python). Also having the implicit behavior isn't easy to opt out of versus having plugin writers easily opt-in.

Also we could perhaps have a unit test that checks for this in the plugin_template so that all plugins will define this in the Meta of any Master/Detail subclasses for that plugin.

#21 Updated by rchan 3 months ago

  • Sprint changed from Sprint 56 to Sprint 57

#22 Updated by daviddavis 3 months ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to daviddavis

#23 Updated by daviddavis 3 months ago

  • Checklist item Add docs that plugin writers should set default_related_name (preferably to "%(app_label)s_%(model_name)s") for detail models added
  • Checklist item Add a check to the master model that default_related_name is defined added
  • Checklist item Notify the pulp-dev mailing list that in a week default_related_name must be defined added

#24 Updated by daviddavis 3 months ago

  • Assignee changed from daviddavis to fabricio.aguiar

#26 Updated by Anonymous 2 months ago

  • Status changed from POST to MODIFIED

#27 Updated by dkliban@redhat.com about 1 month ago

  • Status changed from MODIFIED to POST

#28 Updated by daviddavis about 1 month ago

  • Status changed from POST to MODIFIED

Please register to edit this issue

Also available in: Atom PDF