Issue #3054
closedTask cancelation via REST API does not work
Description
This detail route should allow a task to be canceled with a POST to the endpoint `/v3/tasks/<task_id>/cancel/`
Expected: (204 if successful, 404 if task doesnt exist)
Actual: (405).
Suspecting that this was because POST was disallowed for the whole viewset, I added 'post' to this list:
https://github.com/pulp/pulp/blob/253ae59c9d80a4698c47a0608f96e5d81a62f0d7/platform/pulpcore/app/viewsets/task.py#L27
Sure enough, this allowed me to pass POST the cancel endpoint (but it also incorrectly allowed POST at v3/tasks/)
I found another small error that will interfere with fixing this issue. The constant for a 204 is `HTTP_204_NO_CONTENT`. https://github.com/pulp/pulp/blob/253ae59c9d80a4698c47a0608f96e5d81a62f0d7/platform/pulpcore/app/viewsets/task.py#L33
I think that this proves that the pattern of "Use NamedModelViewset and disallow extraneous methods" will not work. I think each viewset should inherit directly from all of the mixins that it needs, directly.
Related issues
Updated by mhrivnak about 7 years ago
That's a shame. I like the attribute approach, but this comment supports what you're suggesting that we shouldn't depend on the attribute "http_method_names".
https://github.com/encode/django-rest-framework/issues/4691#issuecomment-261915377
"http_method_names is an internal implementation detail and is not intended a documented way to restrict which methods may be applied."
Updated by amacdona@redhat.com about 7 years ago
Using mixins the mixins directly will make our implementation a little bit more verbose, but it will also be more explicit. In particular, our abstraction inherits from mixins that we don't need and then use that variable to ignore those functions. It's easy on the eyes, but it has some code smell.
There is another reason to inherit directly from the mixins, which is the need for asynchronous calls. I wrote it up in this refactor:
https://pulp.plan.io/issues/3038. With the addition of async mixins, we will need more "inheritance abstracting classes" like the NamedModelViewSet. Rather than choosing from a long list of classes to get the right mixins, lets just pick the right mixins where we use them. This will be more flexible and probably less likely to surprise us.
Updated by amacdona@redhat.com about 7 years ago
- Status changed from NEW to POST
- Assignee set to amacdona@redhat.com
Added by amacdona@redhat.com about 7 years ago
Added by amacdona@redhat.com about 7 years ago
Revision 31142da2 | View on GitHub
fix task cancel
closes #3054
Updated by amacdona@redhat.com about 7 years ago
- Status changed from POST to MODIFIED
Applied in changeset pulp|31142da2d70a18c56ecf1b646bb7c7314f031dc8.
Updated by kersom over 5 years ago
- Related to Issue #4883: task cancel api is incorrect added
Updated by bmbouter about 5 years ago
- Status changed from MODIFIED to CLOSED - CURRENTRELEASE
fix task cancel
closes #3054