fix(error-handling): improve error messages for missing S3 objects and instruction files#149
Conversation
…d instruction files Match Java S3EC behavior when S3 objects or instruction files do not exist: - Add early return in event handler when Body is None (failed S3 response) - Catch ClientError separately in get_object with "Unable to retrieve object" message - Catch ClientError in fetch_instruction_file with instruction-file-specific message - Check for None body in process_instruction_file before reading Tests: - Unit: NoSuchKey, AccessDenied, and missing instruction file error wrapping - Integration: non-existent object and plain object with missing instruction file
| plain_s3.put_object(Bucket=bucket, Key="plain-object-no-instruction-file", Body=b"hello") | ||
|
|
||
| try: | ||
| with pytest.raises(S3EncryptionClientError, match="fetching Instruction File"): | ||
| s3ec.get_object(Bucket=bucket, Key="plain-object-no-instruction-file") | ||
| finally: | ||
| plain_s3.delete_object(Bucket=bucket, Key="plain-object-no-instruction-file") |
There was a problem hiding this comment.
Done — test key now uses uuid.uuid4() to avoid collisions. (028de0b)
| raise S3EncryptionClientError( | ||
| "Exception encountered while fetching Instruction File." | ||
| " Ensure the object you are attempting to decrypt has been encrypted" | ||
| " using the S3 Encryption Client and instruction files are enabled." |
There was a problem hiding this comment.
I don't know if I agree with this change;
if I'm reading the code correctly,
an instruction file which exists but has no body
would be considered invalid JSON during the parsing stage.
That is more specific and feels more descriptive than this.
There was a problem hiding this comment.
I looked at this again and decided it was best to handle the none case here, while the request handler is dealing with the instruction file as the context, as compared to in the JSON parsing, where the context has moved back to the actual object.
| raise | ||
| except ClientError as e: | ||
| # Wrap S3 service errors (e.g., NoSuchKey) with context | ||
| raise S3EncryptionClientError(f"Unable to retrieve object: {str(e)}") from e |
There was a problem hiding this comment.
nit/suggestion: on line 300 below, maybe we should say:
raise S3EncryptionClientError(f"Failed to retrieve and/or decrypt object: {str(e)}") from e
texastony
force-pushed
the
tonyknap/feat-handle-no-object
branch
from
028de0b to
99a707f
Compare
kessplas
left a comment
kessplas
left a comment
There was a problem hiding this comment.
Error text is blocking.
| # If Body is None, the S3 request failed (e.g., NoSuchKey). | ||
| # Return early and let boto3 raise the original error. | ||
| if parsed.get("Body") is None: | ||
| if parsed.get("Body", None) is None: |
There was a problem hiding this comment.
I don't care to enough to quibble about this more than in this comment, so leave it if you want but, it's redundant given get() will return None by default anyway. Or is this a linter thing?
| instruction_key = getattr(self._context, _CTX_KEY, None) | ||
|
|
||
| body = parsed.get("Body") | ||
| body = parsed.get("Body", None) |
| " Ensure the object you are attempting to decrypt has been encrypted" | ||
| " using the S3 Encryption Client and instruction files are enabled." |
There was a problem hiding this comment.
Ah, sorry, we absolutely must keep this part:
Ensure the object you are attempting to decrypt has been encrypted using the S3 Encryption Client.
kessplas
left a comment
kessplas
left a comment
There was a problem hiding this comment.
LGTM but there are merge conflicts
2 participants
Match Java S3EC behavior when S3 objects or instruction files do not exist:
Tests: