aws / aws-sdk-ruby

The official AWS SDK for Ruby.
https://aws.amazon.com/sdk-for-ruby/
Apache License 2.0
3.57k stars 1.22k forks source link

AWS::S3::Object#version_id returns the string 'null' instead of nil on unversioned bucket #2004

Closed kreynolds closed 4 years ago

kreynolds commented 5 years ago

Issue description

When a bucket is unversioned and the version_id of an object is requested, it returns the string 'null' instead of the Ruby object nil. This violates idiomatic expectation of the Ruby language as a legitimate version_id is also a string, just a different one.

Gem name

aws-sdk-s3 (1.30.1)

Version of Ruby, OS environment

ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-darwin18]

Code snippets / steps to reproduce

# In a bucket with versioning disabled, do the following
object = resource.bucket('bucket-name').object('test-key')
object.put(body: 'test body')

# will return 'null' instead of nil
puts object.version_id
cjyclaire commented 5 years ago

Thanks for the feedback!

A bit explain here, this is the raw data from S3 API call, can be considered as an enhancement on our resource model to apply simple validation/converting on the raw response

mullermp commented 4 years ago

As much as it sucks, this is a breaking change to make, for anyone checking object.version_id == 'null'.

kreynolds commented 4 years ago

Is there no way to add a deprecation warning or a notice or some path to fixing this? I understand that it would be a breaking change to spring on users but surely there must be a path to fixing it in a future release?

mullermp commented 4 years ago

Oddly enough, after doing some testing, it will return nil for buckets that have never enabled versioning. Once enabled (or disabled after) it will return "null".

r = Aws::S3::Resource.new

# here I deleted my bucket and recreated it in the console

# test data
r.bucket('mamuller-us-west-2').object('foo').put(body: 'test')

# this returns nil !!
r.bucket('mamuller-us-west-2').object('foo').version_id
=> nil

# here i went into s3's console and enabled bucket versioning
# now it returns null
r.bucket('mamuller-us-west-2').object('foo').version_id
=> "null"

# after disabling bucket versioning, this object still returns null
r.bucket('mamuller-us-west-2').object('foo').version_id
=> "null"

# try a new object after this fiasco but still returns null
r.bucket('mamuller-us-west-2').object('bar').put(body: 'test')
r.bucket('mamuller-us-west-2').object('bar').version_id
=> "null"

The version method takes a string parameter, and will accept "null" returned by version_id. This seems to be by design on S3's part. A deprecation warning would only be useful for cases when a new method is introduced and we want to direct customers to use it instead of the deprecated one. After doing some of this testing, I think it makes more sense not to alter this behavior - it seems intentional.

kreynolds commented 4 years ago

I didn't realize it changed from nil to 'null', for the same unversioned bucket for an unversioned object. That seems like an oversight much more so than intentional behavior. As for deprecation, I would have it emit a notice when it returned null on an unversioned object in an unversioned bucket that had once been versioned that said "in the future, this will be nil instead of null". Its ok to deprecate behavior, not just methods imo. At any rate, thanks for investigating further, I appreciate you taking the time.

mullermp commented 4 years ago

The changing of nil to null after versioning is turned on for the first time is because of s3's response header. For buckets that have never been versioned, this header doesn't exist, and the SDK returns nil.

-> "x-amz-version-id: null\r\n"

Providing customization to change this to nil would mean that customers who want to retrieve first versions of files (unversioned) would need to convert back the nil -> "null" and pass "null" into version()

Edit - Again, as much as I agree with using Ruby idioms, I'm cautious to change this behavior.

kreynolds commented 4 years ago

That seems like the sort of thing the lib could do for them. At the very least, maybe a comment could be added to the API that users will need to check for both nil and 'null' if they want to see if an object is versioned.