Pulp

Just a quick update that a naming collision would likely cause problems long before it reached the content summary, since the name stored in the 'type' field is used for Master/Detail casting. So the bug described in the issue couldn't happen, as it would blow up before reaching that point.

However, we still have the general problem of possibly having colliding names in the 'type' field, and the general problem of that name not being very descriptive for many plugins (e.g. 'package', 'update'), so namespacing (somehow) is still a good idea.

Maybe we could configure our content viewsets to follow a naming more like this:

/api/v3/content/file/files/ --> /api/v3/content/pulp_file/files/
/api/v3/content/rpm/updates/ --> /api/v3/content/pulp_rpm/updates/
/api/v3/content/rpm/packages/ --> /api/v3/content/pulp_rpm/packages/

This would mean we would want to not define that part of "endpoint_name" manually and inject it in through some mechanism in core somehow?

https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/viewsets.py#L70

dalley wrote:

/api/v3/content/rpm/updates/ --> /api/v3/content/pulp_rpm/updates/

Where does 'rpm', come from today?
Where does 'pulp_rpm' come from when we switch this?
In this example is TYPE='updates'?

Plugins currently write out 'rpm' manually as part of 'endpoint_name' when they write their viewsets. We do some funky things to break that apart inside of "NamedModelViewSet". Although, as I think about this, it then provides an inconsistency when compared against "/rpm/remotes/ and so forth, and I don't know that "/pulp_rpm/remotes/" would make as much sense.

The namespacing, somehow, needs to make it into the 'type' field. So "rpm.update" or "rpm.package" or "pulp_rpm.package" or something instead of just "package", to prevent collisions across plugins. And this would "just work" when it comes down to the content summary because 'type' is what content_summary is using under the hood. We could make this a documented convention, and maybe that's the easiest/best thing to do. I'd like to find an transparent and automatic way to make that happen but it might be too difficult or complex.

Can we use app label for all the plugin specific endpoints? and also for content summary? This is how I read your comment#8, dalley .
I like the idea of doing it for every plugin to maintain unique paths across plugins.

dalley that sounds like a pretty good plan.

What about another option where the namespace for all models comes from the Django AppConfig for that plugin, e.g. https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/__init__.py#L10

So TYPE = 'updates' and label = 'pulp_rpm' and the URL would be /api/v3/content/pulp_rpm/updates/

TL;DR: I think I found a way to automate this namespacing but I believe there is a bug in Django which prevents to do so at the moment. Any suggestions on an acceptable workaround until Django bug(?) is resolved?

My assumptions for this task:
- we want to namespace endpoints for content/ automatically
- we want to namespace content_summary content automatically
- we don't want to namespace endpoints for other master/detail models, like remotes, publishers, etc

There is a new feature in Python 3.6 for customizing subclasses. This can allow us to customize class attributes in plugin subclasses when needed. E.g. TYPE for Content subclasses and endpoints for ContentViewSet subclasses. Something like:

diff --git a/pulpcore/app/models/content.py b/pulpcore/app/models/content.py
index 665ecc47d..3b7bd904f 100644
--- a/pulpcore/app/models/content.py
+++ b/pulpcore/app/models/content.py
@@ -252,6 +252,12 @@ class Content(MasterModel, QueryMixin):
         verbose_name_plural = 'content'
         unique_together = ()

+
+    def __init_subclass__(cls, **kwargs):
+        if hasattr (cls, 'TYPE'):
+            cls.TYPE = app_label + str(cls.TYPE)
+        super().__init_subclass__(**kwargs)
+
     @classmethod
     def natural_key_fields(cls):
         """

It works for viewsets but not for models.
Django tried to fix it but unsuccessfully, in my opinion. The problem is that they defined their own metaclass for models and they didn't handle __init_subclass__ correctly in their __new__ method. That's my current thinking. I'll file an issue when I have a small reproducer for them.

Any thoughts on the approach?
Until the Django fixes the problem, we can generate endpoint_name attributes as described above but for content_summary there will be some nasty hack, probably here.

My worry is that if we leave this for now as a documentation problem and rely on plugin writer to namespace their plugin properly, I'm not sure there is a way to automate namespacing painlessly later, it will break REST API for plugin specific endpoints + will require a DB migration.

+1 from me. I looked at the django fix for __init_subclass__ and there's a test that seems to do what we want basically. I'm surprised it doesn't work.

Anyway, +1 to opening a bug against django if we can come up with a reproducer. Good find.

FWIW, adding __init_subclass__ directly into a model class triggers TypeError in Django model metaclass; using __init_subclass__ in a mixin class (Django test covers only this case) doesn't produce any error (which is tested in Django) but doesn't work either (not tested in Django) :\.

ttereshc thanks for the great investigation. +1 to continuing with the automatic namespacing and doing the workarounds in pulp while we wait for upstream Django to fix it.

Update:
I filed a Django bug https://code.djangoproject.com/ticket/30041
My small reproducer showed that mixin class works (it doesn't help us, we can't use it in pulpcore) but the direct usage of __init_subclass__ doesn't.

The Django bug will probably be fixed with Django 2.2, there is already a patch for it https://github.com/django/django/pull/10763 .
It turned out that the bug I filed is only one of the consequences of the way __new__ is defined for models in Django and one of the issues became a release blocker for Django 2.2.

This issue covers only the type filed in the DB (and content summary as a consequence).
Endpoints will be taken care of in the other issue #4279