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

[Brian Bouterse]

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