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

adds LastModifed to object as last_modifed #47

Conversation

samueleaton
Copy link
Contributor

@samueleaton samueleaton commented Mar 5, 2019

exposes the LastModifed time string to the user

example:

client.list_objects(bucket: BUCKET, max_keys: 10).each do |response|
  keys = response.contents.sort_by do |o|
    Time.parse!(o.last_modified, "%FT%T")
  end.map(&.key)

  p keys
end

@samueleaton
Copy link
Contributor Author

samueleaton commented Mar 6, 2019

S3 and DigitalOcean spaces both seem to use ISO string where the date and time are separated by a T.

I could parse the times so @last_modifed is a Time instead of a String.
I'm just not aware of any edge cases, like if times are ever stored without milliseconds, or if they are always in UTC time.

@taylorfinnell
Copy link
Owner

taylorfinnell commented Mar 6, 2019

Thanks! I think parsing it as a Time would be solid. Doing that in other places like https://github.com/taylorfinnell/awscr-s3/blob/master/src/awscr-s3/responses/list_all_my_buckets.cr#L23 albeit it needs to be fixed there :)

Object.new("key2", 1337,
"\"fba9dede5f27731c9771645a39863329\""),
"\"fba9dede5f27731c9771645a39863329\"", "2009-10-12T17:50:30.000Z"),
Copy link
Owner

Choose a reason for hiding this comment

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

It's slightly confusing both of these have the same last_modifed, despite being different in the xml response. Took me a while to figure out why the tests passed until I remembered how the Object equality was implemented. I wonder if last_modified should be added to the def_equals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh awesome i didn't know def_equals was a thing. I'll replace that helper i added with this

object.etag.should eq("etag")
end
it "has etag" do
Copy link
Owner

Choose a reason for hiding this comment

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

Needs a space, above it (or just run crystal tool format) and test description is repeated :).

@samueleaton
Copy link
Contributor Author

I tried to use the existing Time Format string you showed me above but the time returned by DigitalOcean uses a decimal precision (e.g. 2019-03-06T01:52:47.037Z) so it breaks when it sees the period. Should we do it without decimal precision or a timezone and just parse it as UTC?

Example:

# "%Y-%M-%dT%H:%M:%S" is the same as "%FT%T"
last_modified = Time.parse(object.string("LastModified"), "%FT%T", Time::Location::UTC)

(When you don't specify a timezone in the time format string it required a location)

@taylorfinnell
Copy link
Owner

It looks like if a timestamp format is not specified the Ruby gem parses UTC and iso 8601. So I guess let's do that

https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-core/lib/aws-sdk-core/xml/builder.rb#L108

@taylorfinnell taylorfinnell merged commit 1db7165 into taylorfinnell:master Mar 6, 2019
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