Error caused by non-unique Master-Detail model names
When both pulp_deb and pulp_rpm are installed one gets the following error running
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.
#5 Updated by dalley over 2 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.
#8 Updated by daviddavis over 2 years ago
Emailing pulp-dev, 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.
#11 Updated by dalley over 2 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.
- 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
- 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 over 2 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?
#14 Updated by dalley over 2 years ago
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.
#19 Updated by daviddavis over 2 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
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.
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.
#20 Updated by bmbouter over 2 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.
#23 Updated by daviddavis about 2 years 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
#25 Updated by fao89 about 2 years ago
- Status changed from ASSIGNED to POST
Please register to edit this issue