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

[David Davis]

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/87980532/attachment.htm>