Project

Profile

Help

Task #4719

refactor handler.py code so that some of it is available in pulpcore-plugin

Added by dkliban@redhat.com 3 months ago. Updated 2 months ago.

Status:
MODIFIED
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
-
Start date:
Due date:
% Done:

100%

Platform Release:
Blocks Release:
Backwards Incompatible:
No
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:

Description

Docker plugin implements it's own content-app handler code. When I added lazy sync support0, I had to copy and paste 3 methods and a constant from handler.py. It would be better if plugin writers could get such functionality by inheriting it from an object provided by the plugin API.

As a result of this task the plugin API will provide a StreamingHandler that will provide the following:

HOP_BY_HOP_HEADERS constant

async def _stream_content_artifact(self, request, response, content_artifact)

async def _stream_remote_artifact(self, request, response, remote_artifact):

def _save_artifact(self, download_result, remote_artifact)

[0] https://github.com/pulp/pulp_docker/pull/344

Associated revisions

Revision a42ef95f View on GitHub
Added by bmbouter 2 months ago

Make `Handler` subclassable

The `Handler` object was not very subclassable, so this improves it in a
few ways.

- hardcoded `Handler` calls are no longer used
- classmethods replace staticmethods
- the HOP_BY_HOP constant is now available on the object
- add a DISTRIBUTION_MODEL constant that subclasses can declare their
corresponding distribution model type. If declared only that model
type will be matched. If unspecified, `BaseDistribution` will be used.

Unrelated to the subclassing an error case was removed which would
silence all exceptions to the log which is not a good behavior.

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

Revision 079d8d27 View on GitHub
Added by bmbouter 2 months ago

Small docs indention fix

This fix is needed because otherwise inclusion of this model's docstring
in Sphinx will error.

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

Revision 069002ec View on GitHub
Added by bmbouter 2 months ago

Adds Handler docs to plugin docs

This adds a new section of documentation on making customized Content
app URLs.

Required PR: https://github.com/pulp/pulpcore/pull/137

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

History

#1 Updated by bmbouter 3 months ago

Would it be possible to name the methods you want to move here and from/to where?

#2 Updated by dkliban@redhat.com 3 months ago

  • Description updated (diff)

#3 Updated by bmbouter 3 months ago

  • Groomed changed from No to Yes
  • Sprint Candidate changed from No to Yes

+1 to doing this story before rc2 is released

#4 Updated by bmbouter 2 months ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to bmbouter

#5 Updated by gmbnomis 2 months ago

If not already planned anyway, I suggest to include the distribution_model attribute from the Docker content app. I think it is useful to be able to select the distribution class to use. I would prefer giving the default on class level though (like e.g. serializer_class in the viewsets)

#7 Updated by bmbouter 2 months ago

gmbnomis wrote:

If not already planned anyway, I suggest to include the distribution_model attribute from the Docker content app. I think it is useful to be able to select the distribution class to use. I would prefer giving the default on class level though (like e.g. serializer_class in the viewsets)

I agreed with this and went to add it in to the new `BaseHandler` but I realized that since master/detail is now here maybe we don't need distribution_model at all since the `BaseDistribution(....).cast()` will automatically give you the correct detail type. Or is the idea that we want to query on the detail type directly to restrict what that subclass handler will match?

Also should we move the _match_distribution and it's supporting function _base_path to `BaseHandler`?

So far I've posted my changed, but I think we probably additional reworking of what is offered from `BaseHandler`.

#8 Updated by gmbnomis 2 months ago

bmbouter wrote:

gmbnomis wrote:
I agreed with this and went to add it in to the new `BaseHandler` but I realized that since master/detail is now here maybe we don't need distribution_model at all since the `BaseDistribution(....).cast()` will automatically give you the correct detail type. Or is the idea that we want to query on the detail type directly to restrict what that subclass handler will match?

Yes, that's the idea. I see it as the 'other half' of #4809. A plugin may want to serve it's distributions at a non-default path (pulp_cookbook is an example of that). Of course we could search all distributions first, cast the result and return a 404 if it is not the right type. But all that is not needed as we can restrict the query to the correct type from the beginning.

Also should we move the _match_distribution and it's supporting function _base_path to `BaseHandler`?

So far I've posted my changed, but I think we probably additional reworking of what is offered from `BaseHandler`.

For pulp_cookbook, the current `BaseHandler` looks like a huge step backward. pulp_cookbook's content handler basically takes a very slightly adapted default handler and adds a live URL on top (see https://github.com/gmbnomis/pulp_cookbook/blob/345ce3a911905609238c65b8a92bbf4e0f125552/pulp_cookbook/app/content/handler.py#L19).

Now, that that default handler is not part of the plugin API anymore, I will have to copy lots of code.

#9 Updated by bmbouter 2 months ago

@gmnomis w.r.t the querying I'm + 1 to adding DISTRIBUTION_MODEL and having the matching machinery use that versus casting it to it's detail.

I can see how splitting this into two objects is creating problems and I see benefit to keeping it together and having all Handlers subclass the single one provided by core (how it was before my PRs).

That begs the question for pulp_docker, can your Registry build off of Handler and if not, why not? If it could then we would close these PRs.

One thing I can see making subclassing of Handler hard is that the use of staticmethods causes the hardcoded class name of Handler to prevent additional customization of subclasses. We can fix that easily by switching those to classmethods.

#10 Updated by dkliban@redhat.com 2 months ago

I think I should be able to use the whole handler in pulp_docker by overriding '_match_distribution' and not using the _match_and_stream method.

#11 Updated by gmbnomis 2 months ago

bmbouter wrote:

One thing I can see making subclassing of Handler hard is that the use of staticmethods causes the hardcoded class name of Handler to prevent additional customization of subclasses. We can fix that easily by switching those to classmethods.

I am ambivalent on this question (in a broader sense than your question on static methods): The Handler class has considerable size and complexity now and it is not clear which parts of it are implementation details and which parts are open to customization by subclassing.

To make things even more complex, Handler seems to provide a mixture of two things:
- Reusable code that may come handy in content apps ('library API' style, e.g. methods that stream content)
- Hooks to customize behavior (plugin-API style, e.g. a plugin may want to do additional things on _save_content)

I don't know whether we should do this right now, but I would like to see those two types of reuse separated and documented. If we don't do this, I fear that almost any change in Handler might break some plugin. Does this make sense?

#12 Updated by bmbouter 2 months ago

@dkliban cool so it sounds like I can close these PRs and pulp_docker should port onto the existing `Handler` in the API (at least for the immediate plan). I also need to make a new PR that will:

  • make the staticmethods cls methods to allow customization for subclasses
  • move the HOP_BY_HOP to Handler because it's not in the plugin API currently

@gmbnomis, I agree w/ your observation and concerns. I believe what we should do is think about which of these methods are expected to be overridden and which aren't and make the overridable ones public and the non-overridable ones (internals) private.

From @dkliban's post it sounds like '_match_distribution' should become public. What else needs to be public? What should be private that is already public?

#13 Updated by bmbouter 2 months ago

  • Status changed from POST to ASSIGNED

#14 Updated by bmbouter 2 months ago

  • Status changed from ASSIGNED to POST

new PR is available here: https://github.com/pulp/pulpcore/pull/130

This does not resolve the public/private naming issues we talked about, but I think it talked us a lot closer and I'm hoping to handle the public/private issues as a followup ticket and piece of work after giving it some thought. I guess I should file a separate ticket so we can focus on that?

#15 Updated by bmbouter 2 months ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#16 Updated by bmbouter 2 months ago

  • Status changed from MODIFIED to POST

moving back to POST for a small fix

#17 Updated by bmbouter 2 months ago

  • Status changed from POST to MODIFIED

#18 Updated by bmbouter 2 months ago

  • Status changed from MODIFIED to POST

moving back to post for one more docs commit

#19 Updated by bmbouter 2 months ago

  • Status changed from POST to MODIFIED

Please register to edit this issue

Also available in: Atom PDF