Rackspace storage metadata #1251

Merged
merged 5 commits into from Nov 20, 2012

Conversation

Projects
None yet
3 participants
@pastorius
Contributor

pastorius commented Nov 6, 2012

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 Nov 2, 2012

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
@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Nov 6, 2012

Member

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

@bradgignac @brianhartsock - thoughts?

Member

geemus commented Nov 6, 2012

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

@bradgignac @brianhartsock - thoughts?

dustacio added a commit to dustacio/fog that referenced this pull request Nov 8, 2012

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

dustacio added a commit to dustacio/fog that referenced this pull request Nov 8, 2012

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
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)}

This comment has been minimized.

Show comment Hide comment
@brianhartsock

brianhartsock Nov 10, 2012

Member

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.

@brianhartsock

brianhartsock Nov 10, 2012

Member

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.

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 10, 2012

Contributor

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

@pastorius

pastorius Nov 10, 2012

Contributor

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

lib/fog/rackspace/models/storage/file.rb
+
+ remove_method :metadata=
+ def metadata=(new_metadata)
+ merge_attributes(new_metadata || {})

This comment has been minimized.

Show comment Hide comment
@brianhartsock

brianhartsock Nov 10, 2012

Member

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.

@brianhartsock

brianhartsock Nov 10, 2012

Member

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.

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 10, 2012

Contributor

Yep. Makes sense.

@pastorius

pastorius Nov 10, 2012

Contributor

Yep. Makes sense.

+ @instance.metadata
+ end
+
+ @instance.attributes['X-Object-Meta-Fog-Foo'] = 'foo'

This comment has been minimized.

Show comment Hide comment
@brianhartsock

brianhartsock Nov 10, 2012

Member

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

brianhartsock Nov 10, 2012

Member

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.

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 10, 2012

Contributor

@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.

@pastorius

pastorius Nov 10, 2012

Contributor

@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.

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 10, 2012

Contributor

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

@pastorius

pastorius Nov 10, 2012

Contributor

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

@brianhartsock

This comment has been minimized.

Show comment Hide comment
@brianhartsock

brianhartsock Nov 10, 2012

Member

@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.

Member

brianhartsock commented Nov 10, 2012

@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

This comment has been minimized.

Show comment Hide comment
@brianhartsock

brianhartsock Nov 10, 2012

Member

@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.

Member

brianhartsock commented Nov 10, 2012

@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

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 10, 2012

Contributor

@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.

Contributor

pastorius commented Nov 10, 2012

@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

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Nov 12, 2012

Member

@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!

Member

geemus commented Nov 12, 2012

@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

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 15, 2012

Contributor

@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?

Contributor

pastorius commented Nov 15, 2012

@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

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Nov 15, 2012

Member

@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?

Member

geemus commented Nov 15, 2012

@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

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 15, 2012

Contributor

@geemus Sure does. I'll let you know if I have any other questions. Thanks.

On Nov 15, 2012, at 8:37 AM, Wesley Beary notifications@github.com wrote:

@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?


Reply to this email directly or view it on GitHub.

Contributor

pastorius commented Nov 15, 2012

@geemus Sure does. I'll let you know if I have any other questions. Thanks.

On Nov 15, 2012, at 8:37 AM, Wesley Beary notifications@github.com wrote:

@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?


Reply to this email directly or view it on GitHub.

pastorius added some commits Nov 16, 2012

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
@pastorius

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 16, 2012

Contributor

@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.

Contributor

pastorius commented Nov 16, 2012

@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

This comment has been minimized.

Show comment Hide comment
@brianhartsock

brianhartsock Nov 17, 2012

Member

LGTM

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

Member

brianhartsock commented Nov 17, 2012

LGTM

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

@pastorius

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 18, 2012

Contributor

@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.

Contributor

pastorius commented Nov 18, 2012

@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

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Nov 19, 2012

Member

@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.

Member

geemus commented Nov 19, 2012

@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

This comment has been minimized.

Show comment Hide comment
@pastorius

pastorius Nov 19, 2012

Contributor

@geemus Yup, makes sense. Thanks!

Contributor

pastorius commented Nov 19, 2012

@geemus Yup, makes sense. Thanks!

brianhartsock added a commit that referenced this pull request Nov 20, 2012

@brianhartsock brianhartsock merged commit 043b549 into fog:master Nov 20, 2012

1 check passed

default The Travis build passed
Details
@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Nov 26, 2012

Member

Thanks!

Member

geemus commented Nov 26, 2012

Thanks!

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