Project

Profile

Help

Issue #3054

closed

Task cancelation via REST API does not work

Added by amacdona@redhat.com over 6 years ago. Updated over 4 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Category:
-
Sprint/Milestone:
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Platform Release:
OS:
Triaged:
No
Groomed:
No
Sprint Candidate:
No
Tags:
Sprint:
Quarter:

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

Related to Pulp - Issue #4883: task cancel api is incorrectCLOSED - CURRENTRELEASEActions
Actions #1

Updated by mhrivnak over 6 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."

Actions #2

Updated by amacdona@redhat.com over 6 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.

Actions #3

Updated by amacdona@redhat.com over 6 years ago

  • Status changed from NEW to POST
  • Assignee set to amacdona@redhat.com
Actions #4

Updated by amacdona@redhat.com over 6 years ago

  • Status changed from POST to MODIFIED
Actions #5

Updated by pcreech over 6 years ago

  • Tags Pulp 3 added
Actions #6

Updated by daviddavis almost 5 years ago

  • Sprint/Milestone set to 3.0.0
Actions #7

Updated by bmbouter almost 5 years ago

  • Tags deleted (Pulp 3)
Actions #8

Updated by kersom almost 5 years ago

  • Related to Issue #4883: task cancel api is incorrect added
Actions #9

Updated by bmbouter over 4 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF