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

Thu Oct 3 13:44:45 UTC 2019

FYI, I merged https://github.com/pulp/pulpcore/pull/319. It was sort of a
breaking change but I hope no one was relying on the broken behavior[0].

[0] https://imgs.xkcd.com/comics/workflow.png

David

On Thu, Oct 3, 2019 at 9:07 AM Tatiana Tereshchenko <ttereshc at redhat.com>
wrote:

> Thanks everyone for discussion!
>
> PRs are up
> https://github.com/pulp/pulpcore/pull/322
> https://github.com/pulp/pulpcore-plugin/pull/140
> https://github.com/pulp/pulp_rpm/pull/1467
>
> On Thu, Oct 3, 2019 at 10:27 AM Tatiana Tereshchenko <ttereshc at redhat.com>
> wrote:
>
>> 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/ea4a564e/attachment.htm>