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

Thu Oct 3 13:07:11 UTC 2019

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