Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Rackspace storage metadata #1251

Merged
merged 5 commits into from

3 participants

@pastorius

I needed support for object level metadata on Rackspace Cloud Storage. I assume you're open to this based on AWS::File#metadata?

I added some tests as well, but only for what I changed. Let me know if there's anything else I can do to get this patch in. Shindo was definitely new to me. Not sure if I'm using it correctly.

pastorius added some commits
@pastorius pastorius added support for Storage::Rackspace::File#metadata 32355ae
@pastorius pastorius added support for Storage::Rackspace::File#metadata
Added rackspace/models/storage/file_tests.
Added tests for Fog::Storage::Rackspace::File#metadata
Added Fog::Rackspace::Storage::File#metadata
449b227
@pastorius pastorius merged with origin 5770589
@geemus
Owner

@pastorius - seems good I think. Shindo is new to everybody (I made the mistake of writing a test framework, oops).

@bradgignac @brianhartsock - thoughts?

@dustacio dustacio referenced this pull request from a commit in dustacio/fog
@dustacio dustacio add support for Storage::Rackspace::File#access_control_allow_origin …
…and #origin

Support Access-Control-Allow-Origin and Origin headers, borrowed
testing ideas from pull request #1251
d2dc10d
@dustacio dustacio referenced this pull request from a commit in dustacio/fog
@dustacio dustacio add support for Storage::Rackspace::File#access_control_allow_origin …
…and #origin

Support Access-Control-Allow-Origin and Origin headers, borrowed
testing ideas from pull request #1251
7d1f83d
lib/fog/rackspace/models/storage/file.rb
@@ -43,6 +44,16 @@ def destroy
true
end
+ remove_method :metadata
+ def metadata
+ attributes.reject {|key, value| !metadata_attribute?(key)}
@brianhartsock Collaborator

Using this means you couldn't add metadata by doing file.metadata['key'] = 'value'. Also, you are exposing users to the X-Object-Meta- portion of the meta-data which kind of stinks. It would be nice to abstract that complexity away entirely since it is definitely vendor specific.

Would probably be better if metadata was stored as an instance variable and loaded from attributes upon first access.

Totally agree. I was just trying not to take too much liberty in diverging from the AWS implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/fog/rackspace/models/storage/file.rb
@@ -43,6 +44,16 @@ def destroy
true
end
+ remove_method :metadata
+ def metadata
+ attributes.reject {|key, value| !metadata_attribute?(key)}
+ end
+
+ remove_method :metadata=
+ def metadata=(new_metadata)
+ merge_attributes(new_metadata || {})
@brianhartsock Collaborator

This seems like it could have dangerous unintended consequences.

file.metadata =  { 'Content-Length' => 0 }

If you go with the above approach where you use an instance variable, you could just set that instance variable and not worry about dangerous consequences with merging directly into attributes.

Yep. Makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/rackspace/models/storage/file_tests.rb
((10 lines not shown))
+ :key => "fogfilestests-#{rand(65536)}"
+ }
+
+ @directory = Fog::Storage[:rackspace].
+ directories.
+ create(directory_attributes)
+
+ model_tests(@directory.files, file_attributes, Fog.mocking?) do
+
+ tests("#metadata") do
+
+ tests("#metadata should default to empty").returns({}) do
+ @instance.metadata
+ end
+
+ @instance.attributes['X-Object-Meta-Fog-Foo'] = 'foo'
@brianhartsock Collaborator

I think it would be better to use the metadata accessor to set metadata instead of using attributes directly. Attributes are very much plumbing that ideally we can avoid clients having to worry about.

@brianhartsock Noted, and I wouldn't ordinarily use it, but it seemed like a reasonable way to test it without relying on other methods belonging to the object under test. I can make that work a different way though, especially if I create a separate var for storing the metadata as you suggested above.

Correction. I just meant that I didn't want to test metadata by calling metadata in these tests, if I could help it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brianhartsock
Collaborator

@pastorius thanks a bunch for your work! I have a few comments. Let me know if you have any questions on them. Happy to help.

@brianhartsock
Collaborator

@geemus I checked out the AWS version of meta-data and this pull request is pretty comparable. I have some recommended improvements but I'm not for sure it is a good idea to diverge from the AWS implementation too much. Let me know your thoughts.

@pastorius

@geemus @brianhartsock I've made a few notes. I guess now I'm just waiting to get confirmation that you're comfortable with diverging from the AWS implementation in this way.

@geemus
Owner

@brianhartsock - thanks for being conscientious about diversion. I think we should do the sane/safe thing (and update AWS if need be). I think AWS was done as simplest that would work, so it may be due for review. We should do the right thing here and see about matching in AWS in a backwards compatible way (if possible). Once you guys settle on something perhaps open a ticket describing the changes that need to occur in AWS so somebody can find them and tackle it? Thanks!

@pastorius

@geemus It seems like there are at least a couple cases here.

If I call File#metadata and the file already exists in storage, I think it should load the metadata. See the following:

f = directory.files.get(my_key)
f.metadata    #should be loaded if file was previously saved with metadata 

And if I call #metadata on a new file, as in below, it seems like I should just start with an empty hash like we've been discussing, and then saving should convert my metadata hash to the appropriate headers and include those in the request.

f = directory.files.new({
  :key    => 'resume.html',
  :body   => 'improvements'
})
f.metadata[:foo] = 'bar'
f.save

Do you agree? If so, is there some way to tell if the file already exists in storage from within the File class itself? Or do I call collection.head to check for existence?

@geemus
Owner

@pastorius makes sense. I'd check to see if last-modified is defined probably. If it is you can pretty reasonably assume the file exists (as end users really shouldn't be setting this themselves). etag fits in that category of attributes as well. Make sense?

@pastorius
pastorius added some commits
@pastorius pastorius Rackspace Cloud Files. can load metadata from existing file. can set …
…metadata for new file. can unset metadata for existing file.

refactored rackspace/models/storage/file_tests.rb

Storage::Rackspace::File renamed some metadata key mapping methods. added more key mapping tests.

reorganized tests
b65167e
@pastorius pastorius merged with upstream master 44c5c6e
@pastorius

@geemus

Updated to support metadata using the indexer syntax and encapsulated the X-Object-Meta- header prefix.

Not sure how smart the conversion between hash key and HTTP header needs to be. I'd prefer to use TSTTCPW. Right now it converts symbols or strings with underscores and/or dashes, and supports compound key names. That's way more than I need though.

Let me know what you think.

@brianhartsock
Collaborator

LGTM

@geemus thoughts? Opened #1288 to discuss changes to the amazon metadata implementation.

@pastorius

@brianhartsock Cool.

One thing that still smells to me is all the logic related to converting metadata to and from HTTP headers buried in Fog::Storage::Rackspace::File. It feels like a violation of the SRP, and if every attribute in File were to do that, that class would get really busy really quick. It was painful testing those methods -- I found myself wishing they could have been public on some class somewhere so that I could test them without all the model plumbing. I might suggest extracting all of metadata's attendant conversion methods to a separate class (or maybe a good candidate for a module?), but perhaps revisiting Fog::Storage::AWS::File will help shed some light on the best way to do that.

I'm open to any feedback, and happy to offer help and opinions where you all think it makes sense.

@geemus
Owner

@pastorius - looks good. It does seem messy, but I'm reticent to try and figure out a good abstraction from a single example. Probably would be good to get the AWS side updated also and then figure out how to do that in a more common fashion (or at least that is my initial inclination). Hopefully that can prevent making mistakes based on simple oversight due to only having the single example. Anyway, I think this is good enough for our purposes currently.

@brianhartsock - feel free to merge it when you are ready.

@pastorius

@geemus Yup, makes sense. Thanks!

@brianhartsock brianhartsock merged commit 043b549 into fog:master

1 check passed

Details default The Travis build passed
@geemus
Owner

Thanks!

@alanthing alanthing referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@alanthing alanthing referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@alanthing alanthing referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 2, 2012
  1. @pastorius
Commits on Nov 5, 2012
  1. @pastorius

    added support for Storage::Rackspace::File#metadata

    pastorius authored
    Added rackspace/models/storage/file_tests.
    Added tests for Fog::Storage::Rackspace::File#metadata
    Added Fog::Rackspace::Storage::File#metadata
  2. @pastorius

    merged with origin

    pastorius authored
Commits on Nov 16, 2012
  1. @pastorius

    Rackspace Cloud Files. can load metadata from existing file. can set …

    pastorius authored
    …metadata for new file. can unset metadata for existing file.
    
    refactored rackspace/models/storage/file_tests.rb
    
    Storage::Rackspace::File renamed some metadata key mapping methods. added more key mapping tests.
    
    reorganized tests
  2. @pastorius
This page is out of date. Refresh to see the latest.
View
64 lib/fog/rackspace/models/storage/file.rb
@@ -47,6 +47,10 @@ def destroy
true
end
+ def metadata
+ @metadata ||= headers_to_metadata
+ end
+
def owner=(new_owner)
if new_owner
attributes[:owner] = {
@@ -70,8 +74,12 @@ def save(options = {})
options['Content-Type'] = content_type if content_type
options['Access-Control-Allow-Origin'] = access_control_allow_origin if access_control_allow_origin
options['Origin'] = origin if origin
- data = connection.put_object(directory.key, key, body, options)
- merge_attributes(data.headers.reject {|key, value| ['Content-Length', 'Content-Type'].include?(key)})
+ options.merge!(metadata_to_headers)
+
+ data = connection.put_object(directory.key, key, body, options)
+ update_attributes_from(data)
+ refresh_metadata
+
self.content_length = Fog::Storage.get_body_size(body)
self.content_type ||= Fog::Storage.get_content_type(body)
true
@@ -83,6 +91,58 @@ def directory=(new_directory)
@directory = new_directory
end
+ def refresh_metadata
+ metadata.reject! {|k, v| v.nil? }
+ end
+
+ def headers_to_metadata
+ key_map = key_mapping
+ Hash[metadata_attributes.map {|k, v| [key_map[k], v] }]
+ end
+
+ def key_mapping
+ key_map = metadata_attributes
+ key_map.each_pair {|k, v| key_map[k] = header_to_key(k)}
+ end
+
+ def header_to_key(opt)
+ opt.gsub(metadata_prefix, '').split('-').map {|k| k[0, 1].downcase + k[1..-1]}.join('_').to_sym
+ end
+
+ def metadata_to_headers
+ header_map = header_mapping
+ Hash[metadata.map {|k, v| [header_map[k], v] }]
+ end
+
+ def header_mapping
+ header_map = metadata.dup
+ header_map.each_pair {|k, v| header_map[k] = key_to_header(k)}
+ end
+
+ def key_to_header(key)
+ metadata_prefix + key.to_s.split(/[-_]/).map(&:capitalize).join('-')
+ end
+
+ def metadata_attributes
+ if etag
+ headers = connection.head_object(directory.key, self.key).headers
+ headers.reject! {|k, v| !metadata_attribute?(k)}
+ else
+ {}
+ end
+ end
+
+ def metadata_attribute?(key)
+ key.to_s =~ /^#{metadata_prefix}/
+ end
+
+ def metadata_prefix
+ "X-Object-Meta-"
+ end
+
+ def update_attributes_from(data)
+ merge_attributes(data.headers.reject {|key, value| ['Content-Length', 'Content-Type'].include?(key)})
+ end
end
end
View
88 tests/rackspace/models/storage/file_tests.rb
@@ -2,6 +2,16 @@
pending if Fog.mocking?
+ def object_meta_attributes
+ @instance.connection.head_object(@directory.key, @instance.key).headers.reject {|k, v| !(k =~ /X-Object-Meta-/)}
+ end
+
+ def clear_metadata
+ @instance.metadata.tap do |metadata|
+ metadata.each_pair {|k, v| metadata[k] = nil }
+ end
+ end
+
file_attributes = {
:key => 'fog_file_tests',
:body => lorem_file
@@ -18,6 +28,82 @@
model_tests(@directory.files, file_attributes, Fog.mocking?) do
+ tests("#metadata should load empty metadata").returns({}) do
+ @instance.metadata
+ end
+
+ tests('#save') do
+
+ tests('#metadata') do
+
+ before do
+ @instance.metadata[:foo] = 'bar'
+ @instance.save
+ end
+
+ after do
+ clear_metadata
+ @instance.save
+ end
+
+ tests("should update metadata").returns('bar') do
+ object_meta_attributes['X-Object-Meta-Foo']
+ end
+
+ tests('should cache metadata').returns('bar') do
+ @instance.metadata[:foo]
+ end
+
+ tests('should remove empty metadata').returns({}) do
+ @instance.metadata[:foo] = nil
+ @instance.save
+ object_meta_attributes
+ end
+
+ end
+
+ tests('#metadata keys') do
+
+ after do
+ clear_metadata
+ @instance.save
+ end
+
+ @instance.metadata[:foo_bar] = 'baz'
+ tests("should support compound key names").returns('baz') do
+ @instance.save
+ object_meta_attributes['X-Object-Meta-Foo-Bar']
+ end
+
+ @instance.metadata['foo'] = 'bar'
+ tests("should support string keys").returns('bar') do
+ @instance.save
+ object_meta_attributes['X-Object-Meta-Foo']
+ end
+
+ @instance.metadata['foo_bar'] = 'baz'
+ tests("should support compound string key names").returns('baz') do
+ @instance.save
+ object_meta_attributes['X-Object-Meta-Foo-Bar']
+ end
+
+ @instance.metadata['foo-bar'] = 'baz'
+ tests("should support hyphenated keys").returns('baz') do
+ @instance.save
+ object_meta_attributes['X-Object-Meta-Foo-Bar']
+ end
+
+ @instance.metadata['foo-bar'] = 'baz'
+ @instance.metadata[:'foo_bar'] = 'bref'
+ tests("should only support one value per metadata key").returns('bref') do
+ @instance.save
+ object_meta_attributes['X-Object-Meta-Foo-Bar']
+ end
+
+ end
+
+ end
+
tests("#access_control_allow_origin") do
tests("#access_control_allow_origin should default to nil").returns(nil) do
@@ -80,5 +166,7 @@
end
end
+
@directory.destroy
+
end
Something went wrong with that request. Please try again.