[Pulp-dev] ContentViewset assumes that POST is needed for any content

Tatiana Tereshchenko ttereshc at redhat.com
Thu Oct 3 08:27:49 UTC 2019


Thank you!

I filed a task to provide a read-only content viewset
https://pulp.plan.io/issues/5535.

Tanya

On Wed, Oct 2, 2019 at 8:38 PM David Davis <daviddavis at redhat.com> wrote:

> I filed an issue[0] and opened a couple PRs which demonstrate how to fix
> the problem:
>
> https://github.com/pulp/pulpcore/pull/319
> https://github.com/pulp/pulp_rpm/pull/1465
>
> [0] https://pulp.plan.io/issues/5533
>
> David
>
>
> On Wed, Oct 2, 2019 at 1:32 PM Tatiana Tereshchenko <ttereshc at redhat.com>
> wrote:
>
>> +1 to what David said. Thanks for detailed explanation.
>>
>> +1 to file an issue to make it possible to define a custom endpoint for a
>> Detail model and keep automatic prepending at the same time.
>>
>>
>> On Wed, Oct 2, 2019 at 7:06 PM David Davis <daviddavis at redhat.com> wrote:
>>
>>> No, it's more complicated than that. There's a bit of magic for
>>> Master/Detail model endpoints[0] where the NamedModelViewSet tries to form
>>> the endpoint by looking up the master_endpoint, app_label, and
>>> endpoint_name (and combining them). In the case of distribution_trees,
>>> there is no master viewset so the master_endpoint becomes
>>> 'distribution_trees' and thus the url we end up with is
>>> /pulp/api/v3/distribution_trees/rpm/distribution_trees/. If we used
>>> 'content', it would just be /pulp/api/v3/distribution_trees/rpm/content/.
>>>
>>> This is a problem we should probably fix regardless of how we handle
>>> Tanya's problem. To restate the problem: it's impossible (or very hard) to
>>> create your own endpoint for a Detail model because the code automatically
>>> tries to namespace your endpoint for you. I can file an issue if others
>>> agree.
>>>
>>> [0]
>>> https://github.com/pulp/pulpcore/blob/5bd0f31604de3079c8bb1b710155d7c6e94d7425/pulpcore/app/viewsets/base.py#L185-L216
>>>
>>> David
>>>
>>>
>>> On Wed, Oct 2, 2019 at 12:52 PM Brian Bouterse <bmbouter at redhat.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Wed, Oct 2, 2019 at 12:44 PM David Davis <daviddavis at redhat.com>
>>>> wrote:
>>>>
>>>>>
>>>>> On Wed, Oct 2, 2019 at 12:37 PM Brian Bouterse <bmbouter at redhat.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Oct 2, 2019 at 11:03 AM Tatiana Tereshchenko <
>>>>>> ttereshc at redhat.com> wrote:
>>>>>>
>>>>>>> Thank you! Good idea to have an additional one.
>>>>>>>
>>>>>> I agree
>>>>>>
>>>>>> I'm on the fence between ReadOnly and just Generic one without any
>>>>>>> mixins.
>>>>>>>
>>>>>> I see having the ReadOnlyContentViewset as overall being slightly
>>>>>> better than a Generic option for plugin writers for two reasons.
>>>>>>
>>>>>> 1) the majority of cases (if not all) will want both list and get()
>>>>>> mixins so we're handling the common case with this.
>>>>>> 2) In the event a plugin writer needs to not have list and/or get(),
>>>>>> they could could use NamedModelViewset directly still. Is this accurate?
>>>>>>
>>>>>
>>>>> Per Tanya's original email it sounds like it was hard/impossible to
>>>>> roll a viewset at /pulp/api/v3/content/rpm/distribution_trees/.
>>>>>
>>>>
>>>> Oh I see, but isn't that an outcome of `endpoint_name = '
>>>> distribution_trees'` here
>>>> <https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/viewsets.py#L256>?
>>>> What happens if that becomes `endpoint_name = 'content'`. I believe
>>>> the NamedModelViewset would translate that into the url
>>>> /pulp/api/v3/content/rpm/distribution_trees/ where the `distribution_trees`
>>>> part comes fro the classname. I haven't looking into it in a while though,
>>>> so let me know if I'm still not getting it right.
>>>>
>>>> In the event it's not easy for the plugin writer to be in control of
>>>> that then I would be in favor of GenericContentViewset.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Just to write it out for clarity I imagine it would be:
>>>>>> ReadOnlyContentViewset(NamedModelViewset, mixins.RetrieveModelMixin,
>>>>>> mixins.ListModelMixin)
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>>
>>>>>>> Any other opinions/suggestions?
>>>>>>>
>>>>>>> On Wed, Oct 2, 2019 at 4:35 PM Matthias Dellweg <dellweg at atix.de>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> I would do a variation of 1. :
>>>>>>>> Provide a ReadonlyContentViewSet with only GET mixed in and leave
>>>>>>>> the
>>>>>>>> 'standard' ContentViewset as is.
>>>>>>>> Cheers, Matthias
>>>>>>>>
>>>>>>>> On Wed, 2 Oct 2019 16:16:14 +0200
>>>>>>>> Tatiana Tereshchenko <ttereshc at redhat.com> wrote:
>>>>>>>>
>>>>>>>> > Current implementation of ContentViewset
>>>>>>>> > <
>>>>>>>> https://github.com/pulp/pulpcore/blob/master/pulpcore/app/viewsets/content.py#L98-L102
>>>>>>>> >
>>>>>>>> > includes
>>>>>>>> > mixins for create (POST) and retrieve/list (GET).
>>>>>>>> > In case a plugin doesn't need to support POST for a content
>>>>>>>> endpoint,
>>>>>>>> > a plugin writer compiles a viewset from the mixins they need, e.g.
>>>>>>>> > distribution trees and custom metadata
>>>>>>>> > <
>>>>>>>> https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/viewsets.py#L233-L258
>>>>>>>> >
>>>>>>>> > (same
>>>>>>>> > use case is expected for modularity endpoints)
>>>>>>>> > This leads to the inconsistent REST API.
>>>>>>>> >
>>>>>>>> > # ContentViewset is used
>>>>>>>> > /pulp/api/v3/content/rpm/advisories/
>>>>>>>> >
>>>>>>>> > # custom plugin content viewset
>>>>>>>> > /pulp/api/v3/distribution_trees/rpm/distribution_trees/
>>>>>>>> >
>>>>>>>> > Possible solutions:
>>>>>>>> > 1. Make ContentViewset more generic (no mixins, or only GET ones?)
>>>>>>>> > and let plugins include any mixins they need.
>>>>>>>> > This option might be painful to switch to for plugin writers,
>>>>>>>> because
>>>>>>>> > every plugin will be affected and will need to make this change.
>>>>>>>> > At the same time probably not many plugins support upload for
>>>>>>>> every
>>>>>>>> > content type, so in many cases the POST is broken/not used anyway.
>>>>>>>> >
>>>>>>>> > 2. Disable POST at the plugin level in some other way.
>>>>>>>> > I'm not sure if there is any native option to disable it.
>>>>>>>> > Hacky way is to override `create` method which will return
>>>>>>>> > appropriate HTTP error that POST is not supported.
>>>>>>>> >
>>>>>>>> > 3. Make plugin writers manually define a proper endpoint name.
>>>>>>>> > Apart from not being reliable, I'm not sure how to do it because
>>>>>>>> of
>>>>>>>> > how we tweak endpoint generation.
>>>>>>>> > Notice the distribution trees example ^, "distribution_trees" is
>>>>>>>> used
>>>>>>>> > twice in the endpoint.
>>>>>>>> >
>>>>>>>> > 4. Any other solutions? Easy ones which I missed?
>>>>>>>> >
>>>>>>>> > Thank you,
>>>>>>>> > Tanya
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Pulp-dev mailing list
>>>>>>> Pulp-dev at redhat.com
>>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>>
>>>>>> _______________________________________________
>>>>>> Pulp-dev mailing list
>>>>>> Pulp-dev at redhat.com
>>>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>>>
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20191003/d0996e0e/attachment.htm>


More information about the Pulp-dev mailing list