Programmatic yanking #16912
Programmatic yanking #16912
Conversation
POST and GET to API endpoints works, but POST is unauthenticated.
* HTTP PATCH to provide a REST-ish API for modifying the release resource. Since we're currently only allowing modification of `yank` and `yank_reason`, and not the entire resource representation, I'm not using PUT. I don't particularly like POST actions, so that's why I'm not using that method either. * HTTP GET on the release returns the full release representation. * Create a new route: api.rest.release; `api.` because these are client-accessible API methods. `api.rest.` because I have a dream of a much fuller RESTful API for warehouse. * Added a new APIModify permission. This is given to user principles who are administrators of a project, not to uploaders. Semantics: * The PATCH request body can provide `yanked` and `yanked_reason` attributes. * `yanked` must be a boolean * `yanked_release` must be a string or null * Both attributes are optional * If neither are given, nothing changes * If `yanked` is given then the release's `yanked` status is changed to the given boolean value * If the release ends up unyanked, then the request's `yanked_reason` attribute is ignored and the `yanked_reason` is unconditionally reset to the empty string. * If the release ends up yanked, then the request's `yanked_reason` is assigned to the JSON request attribute, if given. If not given, the `yanked_reason` is not changed. Note that this means if the release is *already* yanked, and either the request provides `"yanked": true` or no `yanked` attribute, *and* `yanked_reason` is given, the request will update the `yanked_reason`. Missing: * Tests * Documentation * Probably some refactoring * openapi/api_version view attributes and integration
|
I'm going to let CI complete before I click the "Update branch" button. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| release.yanked_reason = "" if yanked_reason is None else yanked_reason | ||
|
|
||
| # Return the new JSON body of the release object. | ||
| return json_release(release, request) |
Check warning
Code scanning / CodeQL
Reflected server-side cross-site scripting Medium
There was a problem hiding this comment.
Is this a real issue? We use json_release(release, request) already above, and other views use it too, so it seems like it's a false positive.
|
I should also mention that a fun way to test this locally at the command line is to Now, you can issue I like to pipe the output through Keep an eye on the Now, to yank that release and set a reason do this: Now that the release has been yanked, you can change the reason: You can unyank the release by doing: Feel free to throw bogus data at the |
Closes #12708
This adds "programmatic yanking", essentially an API for yanking and unyanking releases, along with setting the yank reason for yanked releases. It lays the groundwork for a REST-ish API for other programmatic access to warehouse data.
Highlights
Permissions.APIModify. This permission is given to project owners, but not maintainers.api.rest.release. The thinking here is thatapi.is the relevant root route name, and usingapi.rest.*gives us room to expand the REST API in the future.api.rest.releasein that releases can be a root resource in this theoretical REST resource hierarchy. Eventually we may want to provide access to releases through project resources, but for now we only need to manipulate releases so keep the scope narrow. But this also keeps our options open for the future./api/projects/{name}/{version}to this new route. User-facing API calls should go through/api/and because we're essentially accessing project releases through the/api/projectsroot resource, that's the route pattern I've chosen. I don't know that it makes sense to expose an/api/releasesroot resource since how do you reference a release that doesn't go through its project?json_release_get()responds only toGETon the release resource. API calls to this endpoint method do not need to be authenticated. Obviously, it returns the JSON representation of the release resource.json_release_modify()responds only toPATCHmethods, and only authenticated project owners can call this method. Why did I choosePATCH? Again, it's to keep the scope narrow for now. Because I'm trying to implement only yanking, this PR doesn't provide full resource replacement, i.e.PUT. I also do not likePOST-centric "action" methods; they don't document or scale well, and aren't RESTful. So why not do things right, right from the start? The functionality here is a "partial" resource change, where we only support modifying theyankedandyanked_reasonproperties on the release resource, and even those properties are optional in the request. This is exactly whatPATCHis designed for! In the future, we could support partial updates for other release properties using the same route, method, and view.Semantics
yankedproperty to True or False. It must be a boolean.yankedis being set to False, thenyanked_reasonis ignored, and in fact any previousyanked_reasonis deleted (i.e. set to the empty string).yankedis set to True, then you can optionally also set theyanked_reasonto any string. JSONnullis equivalent to setting it to the empty string.yankedis optional; you can set only theyanked_reason. If the release is in the unyanked state, i.e.yankedis already set to False, thenyanked_reasonis ignored.yankedis already set to True, then you can overwrite theyanked_reason.Other changes
Open questions
openapiorapi_versionattributes in the@view_config(); should we do this, and if so, should we do this now or in a follow up PR? My thinking is yeah, maybe! I think we should also explicitly define a RESTful API version, but whetherapi-v1is the right value for that or not is uncertain. I'm not touching theopenapi.yamlfile either for now.TODO) comment, but should we do that now or in a follow up PR?