Project

Profile

Help

Issue #4185

closed

Content summary returns un-qualified plugin type names and counts

Added by dalley over 5 years ago. Updated over 4 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Platform Release:
OS:
Triaged:
Yes
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Sprint:
Sprint 46
Quarter:

Description

The detail view of a repository version provides a "content summary". This looks like the following:

"content_summary": {
    "package": 50,
    "update": 2,
    "file": 5
}

The problem is that these names come from the TYPE field on the models, and aren't namespaced by plugin. "package" comes from the RPM plugin, but that name is super generic. If another plugin (hypothetically, let's say the Python plugin", defined a content type named "package", the RPM "package"s and the Python "package"s would be counted together as if they were the same content type.

A solution might be to namespace "type" by plugin somehow like so:

"content_summary": {
    "pulp_rpm.package": 50,
    "pulp_rpm.update": 2,
    "pulp_file.file": 5
}

I would discourage a convention-based solution to this problem since with enough plugins, this will eventually become a problem. I would say it's already a problem for understandability if not correctness.


Related issues

Blocks Pulp - Task #4067: Commit migrations to source controlCLOSED - CURRENTRELEASEdaviddavis

Actions
Actions #1

Updated by dalley over 5 years ago

  • Description updated (diff)
Actions #2

Updated by CodeHeeler over 5 years ago

  • Triaged changed from No to Yes
Actions #3

Updated by CodeHeeler over 5 years ago

  • Groomed changed from No to Yes
Actions #4

Updated by dalley over 5 years ago

  • Sprint Candidate changed from No to Yes
  • Tags Pulp 3 RC Blocker added
Actions #5

Updated by jortel@redhat.com over 5 years ago

  • Sprint set to Sprint 46
Actions #6

Updated by daviddavis over 5 years ago

  • Blocks Task #4067: Commit migrations to source control added
Actions #7

Updated by ttereshc over 5 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to ttereshc
Actions #8

Updated by dalley over 5 years ago

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

Actions #9

Updated by bmbouter over 5 years ago

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'?

Actions #10

Updated by dalley over 5 years ago

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.

Actions #11

Updated by ttereshc over 5 years ago

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.

Actions #12

Updated by bmbouter over 5 years ago

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/

Actions #13

Updated by ttereshc over 5 years ago

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.

Actions #14

Updated by daviddavis over 5 years ago

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

Actions #15

Updated by ttereshc over 5 years ago

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) :\.

Actions #16

Updated by bmbouter over 5 years ago

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.

Actions #17

Updated by ttereshc over 5 years ago

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.

Actions #18

Updated by ttereshc over 5 years ago

  • Status changed from ASSIGNED to POST
Actions #19

Updated by ttereshc over 5 years ago

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.

Actions #20

Updated by ttereshc over 5 years ago

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

Added by ttereshc over 5 years ago

Revision adf83ff4 | View on GitHub

Prepend type for master/detail models with a plugin app label

closes #4185 https://pulp.plan.io/issues/4185

Required PR: https://github.com/pulp/pulp_file/pull/150

Added by ttereshc over 5 years ago

Revision adf83ff4 | View on GitHub

Prepend type for master/detail models with a plugin app label

closes #4185 https://pulp.plan.io/issues/4185

Required PR: https://github.com/pulp/pulp_file/pull/150

Actions #21

Updated by ttereshc over 5 years ago

  • Status changed from POST to MODIFIED

Added by dalley about 5 years ago

Revision eb0a1e0f | View on GitHub

Change expected names of content types

re: #4185 https://pulp.plan.io/issues/4185

Actions #22

Updated by daviddavis almost 5 years ago

  • Sprint/Milestone set to 3.0.0
Actions #23

Updated by bmbouter almost 5 years ago

  • Tags deleted (Pulp 3, Pulp 3 RC Blocker)
Actions #24

Updated by bmbouter over 4 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF