Pulp

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.

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

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.

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.

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?

@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.

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.

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

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.