[SVCS-233] Fix KeyError: "Key not found: 'X-WATERBUTLER-METADATA'" #260
Conversation
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
A few questions for the purpose of better understanding. A few minor style changes. 🎆
mfr/providers/osf/provider.py
Outdated
| # TODO Remove this when API v0 is officially deprecated | ||
| self.metrics.add('metadata.wb_api', 'v0') | ||
| metadata_url = download_url.replace('/file?', '/data?', 1) | ||
| metadata_request = await self._make_request('GET', metadata_url) |
There was a problem hiding this comment.
Not related to your PR but should the variable be named metadata_response. Not worth to change it though.
There was a problem hiding this comment.
I see what you mean. I could see it either way. It returns a request object, but it also represents the response to an HTTP request.
There was a problem hiding this comment.
._make_request() calls aiohttp.request() which returns a response object: link. I am ok calling it request. If we were to change it, it would be everywhere I think.
mfr/providers/osf/provider.py
Outdated
| metadata_url = download_url.replace('/file?', '/data?', 1) | ||
| metadata_request = await self._make_request('GET', metadata_url) | ||
| metadata = await metadata_request.json() | ||
| await metadata_request.release() |
There was a problem hiding this comment.
Why not having this line before didn't break anything? Is this the answer?
There was a problem hiding this comment.
It's been a while. I think I'm going to be saying that to you a lot now that you've taken on these ancient PRs. Anyway, I believe this is simply because it has become our standard to manually close all requests. I believe this is because we were getting a ton of "unclosed" type errors in sentry. @felliott correct me if I'm wrong here.
There was a problem hiding this comment.
Np, I understand. That's why I try to ask as many questions to help us recall the decision made long time ago, double check a change and document it somewhere. Hope it is constructive rather than causing overhead for you guys.
I agree that it is better we explicitly close/release the connection.
mfr/providers/osf/provider.py
Outdated
| raise exceptions.MetadataError('Failed to fetch file metadata from WaterButler. Received response: code {} {}'.format(str(request_code), str(request_reason)), | ||
| metadata_url=download_url, | ||
| response=await metadata_request.text(), | ||
| response=request_reason, |
There was a problem hiding this comment.
It's expecting text. I'm afraid I can't recall why I changed it, and I don't see where the exception actually does anything with it.
mfr/providers/osf/provider.py
Outdated
| request_headers = metadata_request.headers | ||
| await metadata_request.release() | ||
| if request_code != 200: | ||
| raise exceptions.MetadataError('Failed to fetch file metadata from WaterButler. Received response: code {} {}'.format(str(request_code), str(request_reason)), |
There was a problem hiding this comment.
Similar as the comment below the one below.
mfr/providers/osf/provider.py
Outdated
| request_headers = metadata_request.headers | ||
| await metadata_request.release() | ||
| if request_code != 200: | ||
| raise exceptions.MetadataError('Failed to fetch file metadata from WaterButler. Received response: code {} {}'.format(str(request_code), str(request_reason)), |
There was a problem hiding this comment.
Do we want this exception caught by sentry? Will it be caught automatically? (I came from OSF where we need to explicitly call sentry to log exceptions ...)
There was a problem hiding this comment.
| provider=self.NAME, | ||
| code=400, | ||
| ) | ||
| code=400) |
There was a problem hiding this comment.
One for 200 and one for 400. It will be great if we can gradually replace number literals with status constants in our code base (link). For example:
from http import HTTPStatus
httpStatus.OK
httpStatus.BAD_REQUESTThere was a problem hiding this comment.
Seems like a good idea.
There was a problem hiding this comment.
There are too many status in the file. No need to change for now. Maybe in the future we can have a clean ticket for this but very low priority for now.
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Two non-blocker issues. PR looks good! 🔥 🔥 Move to PCR.
| response_reason = metadata_response.reason | ||
| response_headers = metadata_response.headers | ||
| await metadata_response.release() | ||
| if response_code != 200: |
There was a problem hiding this comment.
Line 79 is too long, how about his? (Not a blocker)
if response_code != 200:
raise exceptions.MetadataError(
'Failed to fetch file metadata from WaterButler. Received response: code {} {}'.format(
str(response_code), str(response_reason)
),
metadata_url=download_url,
response=response_reason,
provider=self.NAME,
code=400
)| raise exceptions.MetadataError('Failed to fetch file metadata from WaterButler. Received response: code {} {}'.format(str(response_code), str(response_reason)), | ||
| metadata_url=download_url, | ||
| response=await metadata_request.text(), | ||
| response=response_reason, |
There was a problem hiding this comment.
In case you had wanted to change this to .text() but forgot. (I am good with either .text() or .reason. (Not a blocker)
cslzchen
changed the title
|
Moving to RTM, will be in v0.23 |
3 participants
Purpose:
Better error handling in mfr/providers/osf/provider.py
Changes:
Catch error in metadata request earlier to avoid KeyError
Side effects
None
[#SVCS-233]