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

[Tatiana Tereshchenko]

+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/20191002/e9c11a61/attachment.htm>