Project

Profile

Help

Issue #3054

Task cancelation via REST API does not work

Added by amacdona@redhat.com about 2 years ago. Updated 6 months ago.

Status:
MODIFIED
Priority:
Normal
Category:
-
Sprint/Milestone:
Start date:
Due date:
Severity:
2. Medium
Version:
Platform Release:
Blocks Release:
OS:
Backwards Incompatible:
No
Triaged:
No
Groomed:
No
Sprint Candidate:
No
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:

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 incorrect MODIFIED Actions

Associated revisions

History

#1 Updated by mhrivnak about 2 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."

#2 Updated by amacdona@redhat.com about 2 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.

#3 Updated by amacdona@redhat.com about 2 years ago

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

#4 Updated by amacdona@redhat.com about 2 years ago

  • Status changed from POST to MODIFIED

#5 Updated by pcreech about 2 years ago

  • Tags Pulp 3 added

#6 Updated by daviddavis 6 months ago

  • Sprint/Milestone set to 3.0

#7 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3)

#8 Updated by kersom 5 months ago

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

Please register to edit this issue

Also available in: Atom PDF