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

Wed Oct 2 18:38:06 UTC 2019

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