WaterButler is a Python web application for interacting with various file storage services via a single RESTful API, developed at Center for Open Science.
Apache License 2.0
62
stars
76
forks
source link
[ENG-332] [OATHPIT] Throw GitHub into the Oath Pit #389
Enable and update the GitHub provider for aiohttp3.
Changes
Code
Fix a response release issue for GitHub rate-limiting. In aiohttp3, release() not only releases the response but also closes the connection. This causes exception_from_response() to encounter a connection already closed failure when trying to release or consume the response. The fix: 1) Don't release the response before calling _rl_handle_forbidden_error() and 2) Let _rl_handle_forbidden_error() and exception_from_response() take care of it.
Tests
Fixed mock response header value type in tests: int -> str. aiohttpretty requires the str type on both header name and value, which calls .encode() on both when building raw headers. Exceptions would be thrown if int header values were provided.
Fixed missing Content-Type header when registering multiple URLs in tests. aiohttp3 is more strict on response content type, where .json() would fail if the content type were not provided or were incorrect. However, the updated aiohttpretty still has the unpleasant limitation that .register_uri() ignores headers (and the body) provided by .register_json_uri() when multiple URL registrations are used. The fix is to explicitly add the required header to the responses param when calling .register_json_uri().
Side effects
No
CR / QA Notes
Here is a list of actions that I tested locally multiple times. Consider a PASS if no extra comment is provided.
Getting metadata for a file / folder
Downloading
DAZ
Uploading
Upload one file works as expected
Upload multiple files at the same time fails. This is an existing bug on staging. Uploading multiple files should be disabled since each upload is independent. The first one gets uploaded successfully but the rest is guaranteed to fail, mostly with waterbutler.core.exceptions.ProviderError: 422, {"documentation_url": "https://developer.github.com/v3", "message": "Update is not a fast forward"} and sometimes with waterbutler.core.exceptions.ProviderError: 422, {"documentation_url": "https://developer.github.com/v3", "message": "Reference cannot be updated"}.
Chunked upload: N / A
Delete
Delete one file fails (an existing bug on staging) with
waterbutler.core.exceptions.MetadataError: 404, {"documentation_url": "https://developer.github.com/v3/git/commits/#get-a-commit", "message": "Not Found"}
Delete one folder works as expected
Delete multiple files / folders not allowed by the frontend
Folder creation / Upload to folder / Folder deletion
Rename files and folders
Verifying non-root folder access for id-based folders: N / A
Intra move/copy:
Nothing happens when moving / copying within the GitHub repo (both OATHPIT and Staging)
Inter move/copy (light testing only)
Copying (1 file and 1 folder containing 128 files) from GitHub to OSFStorage works as expected.
For CR: with the 1 folder of 128 files, I am able to see our rate-limiting handler in effect when there are only about 500 limit left.
For QA: please test rate limiting by copying 1 folder containing 32 subfolders, each of which contains 128 files.
Copying multiple files or folders is not allowed by the frontend (same with staging)
Moving from GitHub to OSFStorage is not allowed by the frontend (same with staging)
Copying / moving from OSFStorage to GitHub is not allowed by the frontend (same with staging)
Comments persist with moves (light testing only)
If enabled, test revisions
Project root is storage root vs. a subfolder: N / A
Coverage increased (+7.8%) to 69.341% when pulling 8f6104d9130e74190d059add2173d27f86822563 on cslzchen:feature/oathpit-github into 37063731cff7fe1fae424c567def9f6627bcc9c0 on CenterForOpenScience:feature/oathpit.
Ticket
https://openscience.atlassian.net/browse/ENG-332
Purpose
Enable and update the GitHub provider for
aiohttp3
.Changes
Code
aiohttp3
,release()
not only releases the response but also closes the connection. This causesexception_from_response()
to encounter a connection already closed failure when trying to release or consume the response. The fix: 1) Don't release the response beforecalling _rl_handle_forbidden_error()
and 2) Let_rl_handle_forbidden_error()
andexception_from_response()
take care of it.Tests
Fixed mock response header value type in tests:
int
->str
.aiohttpretty
requires thestr
type on both header name and value, which calls.encode()
on both when building raw headers. Exceptions would be thrown if int header values were provided.Fixed missing Content-Type header when registering multiple URLs in tests.
aiohttp3
is more strict on response content type, where.json()
would fail if the content type were not provided or were incorrect. However, the updatedaiohttpretty
still has the unpleasant limitation that.register_uri()
ignores headers (and the body) provided by.register_json_uri()
when multiple URL registrations are used. The fix is to explicitly add the required header to theresponses
param when calling.register_json_uri()
.Side effects
No
CR / QA Notes
Here is a list of actions that I tested locally multiple times. Consider a PASS if no extra comment is provided.
Getting metadata for a file / folder
Downloading
DAZ
Uploading
waterbutler.core.exceptions.ProviderError: 422, {"documentation_url": "https://developer.github.com/v3", "message": "Update is not a fast forward"}
and sometimes withwaterbutler.core.exceptions.ProviderError: 422, {"documentation_url": "https://developer.github.com/v3", "message": "Reference cannot be updated"}
.Delete
waterbutler.core.exceptions.MetadataError: 404, {"documentation_url": "https://developer.github.com/v3/git/commits/#get-a-commit", "message": "Not Found"}
Folder creation / Upload to folder / Folder deletion
Rename files and folders
Verifying non-root folder access for id-based folders: N / A
Intra move/copy:
Inter move/copy (light testing only)
Comments persist with moves (light testing only)
If enabled, test revisions
Project root is storage root vs. a subfolder: N / A
Updating a file
Deployment Notes
TBD