Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ArgumentError when S3 responds with invalid Expires Header #1216

Merged
merged 3 commits into from
Jun 16, 2016

Conversation

trevorrowe
Copy link
Member

Resolved an issue with Aws::S3::Client#head_object and #get_object where an ArgumentError was raised if Amazon S3 responded with an Expires header that contained an unparsable string.

The #head_object and #get_object response now return nil when the Expires header contains an invalid value. You can now access the raw string value of the Expires header with #expires_string.

# If Amazon S3 responds with `Expires: abc` as a header
resp = s3.head_object(bucket:'bucket', key:'key')
resp.expires #=> nil
resp.expires_string #=> "abc"

See related GitHub issue #1184.

%w(HeadObjectOutput GetObjectOutput).each do |shape|
members = api['shapes'][shape]['members']
# inject this into sort order so this appears directly
# after #expires
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this will make it show up BEFORE 'Expires' in my testing, not after.

@awood45
Copy link
Member

awood45 commented Jun 16, 2016

There's a mismatch in the comments about where ExpiresString goes in the hash order and where it is actually put. I'm not really sure why it matters though, at least in the context of the diff.

If we are deciding to care about this, we should probably add an assertion in the unit test that this order is ensured.

Added tests that ensure you can stub the Expires header with
a valid Time object or an invalid string.
@trevorrowe
Copy link
Member Author

I've confirmed that the order does not matter, but I've pushed a commit that makes sure they line up.

@awood45
Copy link
Member

awood45 commented Jun 16, 2016

LGTM then

@trevorrowe trevorrowe merged commit faa9056 into master Jun 16, 2016
awood45 added a commit that referenced this pull request Jun 21, 2016
@cjyclaire cjyclaire deleted the expires-fix branch May 5, 2017 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants