improving error message if location header missing #35
Conversation
currently if the location header is missing for any reason (typically because auth is missing or wrong scope) a later error will trigger about an invalid url scheme, and this is misleading. We should check that the location is defined earlier on and provide a more meaningful message Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
|
LGTM. But I suggest it would be better to have another reviewer approve it. |
| @@ -290,6 +294,8 @@ def _put_upload( | |||
|
|
|||
| # Location should be in the header | |||
| session_url = self._get_location(r, container) | |||
| if not session_url: | |||
There was a problem hiding this comment.
Would it make more sense here to use an identity check for None? Actually the _get_location method could use the same treatment, I think.
There was a problem hiding this comment.
It may also make sense on line 320 to set the return type hint as Optional[str] in this case.
There was a problem hiding this comment.
It could be that the location header is None or an empty string (servers can vary in how they handle it), so I think this check is appropriate!
| @@ -352,6 +358,8 @@ def _chunked_upload( | |||
|
|
|||
| # Location should be in the header | |||
| session_url = self._get_location(r, container) | |||
| if not session_url: | |||
There was a problem hiding this comment.
The same identity/equality consideration as above.
|
okay I think this should be good to merge given approval from an admin? We probably should discuss this later - if we can have some number of community reviews good enough to warrant merge (or if we should have a reliably way to ping an admin). At the moment I always feel badly bothering folks on the slack to do this task! |
|
I'm happy to poke my head in and question assumptions from time to time, however valuable you may find that. I looked through your open issues when I was pinged on Slack and might be able to help with some of your TODO at some point as well (like maybe the retry decorator as a way to ease into the codebase). |
|
okay realizing we have two approvals! I think this is good to merge, we have to find a balance between reviews / asking managers that are already busy to trivially approve to merge. I'm thinking we should remove the latter requirement in favor of some number of community reviews. |
currently if the location header is missing for any reason on a blob push (typically because auth is missing or wrong scope) a later error will trigger about an invalid url scheme, and this is misleading. We should check that the location is defined earlier on and provide a more meaningful message.
The test case I used to produce this was providing a token locally with the wrong scope - the request went through the auth flow (but was unsuccessful) and ultimately returned 403, and there was a meaningful message about the error in r.json().
Signed-off-by: vsoch [email protected]