Project

Profile

Help

Issue #4681

closed

Error caused by non-unique Master-Detail model names

Added by tustvold over 5 years ago. Updated almost 5 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
High
Assignee:
Category:
-
Sprint/Milestone:
Start date:
Due date:
Estimated time:
Severity:
3. High
Version:
Platform Release:
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Sprint:
Sprint 57
Quarter:

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.


Related issues

Blocks Ansible Plugin - Task #4710: Rename AnsibleRole to just RoleCLOSED - CURRENTRELEASEbmbouter

Actions
Actions #1

Updated by daviddavis over 5 years ago

  • Tags Pulp 3 added
Actions #2

Updated by daviddavis over 5 years 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
Actions #3

Updated by daviddavis over 5 years ago

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

Updated by rchan over 5 years ago

  • Sprint changed from Sprint 51 to Sprint 52
Actions #5

Updated by dalley over 5 years 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.

Actions #6

Updated by daviddavis over 5 years ago

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

Updated by bmbouter over 5 years ago

  • Tags deleted (Pulp 3)
Actions #8

Updated by daviddavis over 5 years ago

Emailing pulp-dev[0], 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

Actions #9

Updated by bmbouter over 5 years 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.

Actions #10

Updated by rchan over 5 years ago

  • Sprint changed from Sprint 52 to Sprint 53
Actions #11

Updated by dalley over 5 years 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.

Actions #12

Updated by daviddavis over 5 years 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?

Actions #13

Updated by amacdona@redhat.com over 5 years ago

  • Sprint changed from Sprint 53 to Sprint 54
Actions #14

Updated by dalley over 5 years 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.

Actions #15

Updated by ttereshc over 5 years ago

  • Sprint changed from Sprint 54 to Sprint 55
Actions #16

Updated by daviddavis over 5 years ago

  • Sprint/Milestone set to 3.0.0
Actions #17

Updated by dkliban@redhat.com over 5 years ago

  • Sprint changed from Sprint 55 to Sprint 56
Actions #18

Updated by bmbouter over 5 years 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.

Actions #19

Updated by daviddavis over 5 years 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 here[0].

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

Actions #20

Updated by bmbouter over 5 years 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.

Actions #21

Updated by rchan over 5 years ago

  • Sprint changed from Sprint 56 to Sprint 57
Actions #22

Updated by daviddavis over 5 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to daviddavis
Actions #23

Updated by daviddavis over 5 years ago

Actions #24

Updated by daviddavis over 5 years ago

  • Assignee changed from daviddavis to fao89

Added by Fabricio Aguiar over 5 years ago

Revision 4f7a9f3f | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 6efc3c37 | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 31f4937e | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision a9b33e9b | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 206e3d00 | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 206e3d00 | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 206e3d00 | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 206e3d00 | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 9f48b9b7 | View on GitHub

dealing with pulpcore default_related_name

Declaring default_related_name ref #4681

Added by Fabricio Aguiar over 5 years ago

Revision 3c62889e | View on GitHub

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

Actions #26

Updated by Anonymous over 5 years ago

  • Status changed from POST to MODIFIED

Added by Fabricio Aguiar over 5 years ago

Revision 996b1ccb | View on GitHub

dealing with pulpcore default_related_name

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

Added by Fabricio Aguiar over 5 years ago

Revision 99ac6642 | View on GitHub

dealing with pulpcore default_related_name

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

Actions #27

Updated by dkliban@redhat.com over 5 years ago

  • Status changed from MODIFIED to POST
Actions #28

Updated by daviddavis about 5 years ago

  • Status changed from POST to MODIFIED
Actions #29

Updated by bmbouter almost 5 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF