fog / fog-core

fog's core, shared behaviors without API and provider specifics
MIT License
46 stars 95 forks source link

"uninitialized constant Fog::Storage::MIME" #171

Closed celeen closed 8 years ago

celeen commented 8 years ago

I am working on building a middleman static site to aws S3, and encountering the following error: gems/fog-core-1.35.0/lib/fog/storage.rb:68:inget_content_type': uninitialized constant Fog::Storage::MIME (NameError)`

I've done a bit of tracing, and it looks to me like what's going on is:

mime/types is inaccessible in lib/fog/storage.rb:68 at least when called from fog-aws requests/storage/put_object.rb:43 (where Fog::Storage#parse_data is called), presumably because Fog::Storage#new does not actually run in this instance.

Perhaps the recent commit that moved requirement of mime/types or mime/types/columnar inside self#new is what created this bug.

Would love feedback as to the accuracy of this diagnosis, and what preferable fix is. Happy to submit a PR with preferred fix if needed. Thanks!

plribeiro3000 commented 8 years ago

So there is 2 things here.

I'm not totally sure why it didn't. Maybe @geemus have a better idea?

geemus commented 8 years ago

Hmm. @celeen I would think you would have to call new before calling put_object though? That would give the connection needed, though I could be wrong.

geemus commented 8 years ago

Actually, upon further consideration I suspect that you have an older version of fog (which doesn't have the mime-type warning in Storage#new) paired with a new version of fog-core. Could you try updating fog and see if that helps?

celeen commented 8 years ago

Oh, man, I've run bundle update a whole buuunnnnch of times in the past 12-18 hours. Maybe I'm not completely understanding how the fog architecture works together-- I'm using the gem middleman-s3_sync, which is calling on fog-aws (0.7.6), which is calling on fog-core(1.35.0). I've put some very sophisticated p statements (:laughing:) into my local version of the gem fog-core to try to trace whether Storage.new gets called after running middleman build (which kicks off the s3_sync process), and no dice.

fog is updated to v1.36.0 on my system, but the project I'm working in doesn't seem to be actually using it (the Gemfile.lock has fog-aws listed, but not fog). Is this actually a problem with how middleman-s3_sync is using fog? (or rather, fog-aws?)

geemus commented 8 years ago

I think perhaps middleman was also working on a fix here, so you may also want to bump that if you haven't. Sorry for the run around here, I think there should be a working set of versions, but I'm not precisely sure what they look like off hand.

celeen commented 8 years ago

Hey, no worries. I solved this issue independently by digging into the fog-core code on my local machine and manually reverting the changes in that commit I referenced, moving that begin/rescue block outside the #new method. I figured I'd report that I ran into a bug just in case y'all wanted to revert, or come up with a different solution to making mime/types optional.

I'll be on the lookout for other toolset versioning combinations that are more effective. This was not the only problem I ran into trying to get this stuff to work last night.

thanks again for your help!

alex88 commented 8 years ago

Same here on s3 sync and middleman, I've installed gem 'mime-types' but I still get that error :/

alex88 commented 8 years ago

Nvm, my fault, had to clear cache on CI for some reason! Middleman s3 sync added that gem in runtime dependencies in 3.3.5

geemus commented 8 years ago

Thanks for the updates, glad to hear you both got it working. I'm hoping there won't be too many issues (or at least that they are short-lived) around this, so we can move on.

fredjean commented 8 years ago

I was just made aware of this issue. I have already applied the workaround on middleman-s3_sync by adding mime-types as a dependency in my gemspec. I don't have any other fixes. I'm opened to suggestion, but I am tired of having to deal with the unfortunate decision to remove that dependency from fog.

@monfresh, this is where you need to bring up this issue.

monfresh commented 8 years ago

@celeen's analysis is spot on.

I can confirm that this is still an issue with the latest version of all the gems, and that the culprit is this commit.

The reason why the uninitialized constant Fog::Storage::MIME (NameError) error is thrown is because Fog::Storage.new never gets called, as pointed out by @celeen, and therefore mime/types never gets required. In this scenario, the only way to make sure mime-types is required is to manually add it to the Gemfile (because Bundler automatically requires all gems in a Gemfile). Simply adding mime-types as a runtime dependency of middleman-s3_sync will not work because it won't automatically require the library.

But if we need to add it to make sure it gets required, then why not keep it as a dependency of fog-core, and call require 'mime/types' outside of .new? What I don't understand is why the mime-types dependency was removed. As far as I can tell, this repo depends on mime-types. Whether you're only using mime/types/columnar or the full mime/types, you still need the mime-types gem, or am I missing something?

Here is why Fog::Storage.new never gets called:

parse_data is a Class method on Fog::Storage, so calling Fog::Storage.parse_data never goes through .new.

Here is the last part of the stack trace:

/Users/moncef/.rvm/gems/ruby-2.3.0@tmm-middleman/gems/fog-core-1.35.0/lib/fog/storage.rb:68:in `get_content_type': uninitialized constant Fog::Storage::MIME (NameError)
    from /Users/moncef/.rvm/gems/ruby-2.3.0@tmm-middleman/gems/fog-core-1.35.0/lib/fog/storage.rb:79:in `parse_data'
    from /Users/moncef/.rvm/gems/ruby-2.3.0@tmm-middleman/gems/fog-aws-0.8.0/lib/fog/aws/requests/storage/put_object.rb:43:in `put_object'
    from /Users/moncef/.rvm/gems/ruby-2.3.0@tmm-middleman/gems/fog-aws-0.8.0/lib/fog/aws/models/storage/file.rb:212:in `save'
    from /Users/moncef/.rvm/gems/ruby-2.3.0@tmm-middleman/gems/middleman-s3_sync-3.3.7/lib/middleman/s3_sync/resource.rb:65:in `block in update!'

You can see that fog-aws calls data = service.put_object(directory.key, key, body, options), which then calls data = Fog::Storage.parse_data(data)

geemus commented 8 years ago

@fredjean @monfresh sorry for the confusion and difficulty. We removed this in the hopes that it would allow us to continue to support a diverse set of ruby versions and mime-types dropped support for everything less than 2.0.0 in newer versions. By removing the direct dependency, we had hoped to allow the user to get the version of mime-types that made sense for their ruby version. Could we improve this in middleman by having an explicit require of mime-types in addition to having it in the gemspec? That way we could know for sure it was loaded (though things may also need to be done in fog).

monfresh commented 8 years ago

It is not middleman's responsibility to require mime-types. It is fog's, since fog depends on mime-types in one way or another. The solution, as pointed out by @celeen, is to make sure the block that requires mime-types is outside of Fog::Storage.new, because not every call to Fog::Storage passes through .new.

monfresh commented 8 years ago

Also, I think it's perfectly acceptable to allow the user to specify their own version of mime-types, but it would be great if that were also documented in the README (that they need to add mime-types to their Gemfile), rather than only in an exception message (which doesn't even get called in this scenario).

To recap, if it were me, I would have handled #170 by doing the following:

  1. Remove mime-types dependency from gemspec
  2. Update the README to let users know they should add their preferred version of mime-types to their Gemfile
  3. Leave the require block where it was, and add logging for the scenario where mime-types is not installed.
  4. Add specs for all Fog::Storage methods, to make sure this change didn't break anything

The fact that the tests passed in #170 is a sign that there is not enough test coverage. Looking at the spec for Fog::Storage, I see that parse_data is not covered, for example. I would recommend using Code Climate to monitor the health of this repo. It's free for open source repos, and includes all kinds of neat tools, like test coverage, security analysis, and style analysis.

monfresh commented 8 years ago

And for better changelogs, I recommend github-changelog-generator. It's an amazing tool that generates the changelog for you based on GitHub issues and pull requests.

geemus commented 8 years ago

I didn't want to require everyone to setup mime-types either, as only a subset of fog users (those using storage) actually need it. Better documenting and testing it would certainly be good, but I only have a limited amount of time and focus I'm able to bring to bear so I'm trying to do my best within those constraints. The changelog we have uses a simple rake task, which has usually been sufficient, but I can try to check that out some time soon.

geemus commented 8 years ago

I remain confused how we are reaching this edge without seeing the warning also. You mention that:

You can see that fog-aws calls data = service.put_object(directory.key, key, body, options), which then calls data = Fog::Storage.parse_data(data)

But in that case service refers to an object created via calling new, which should hit the require? Sorry, just feeling like I'm totally missing something here.

monfresh commented 8 years ago

What you're missing, my friend, is what we've been trying to tell you multiple times :smile: You seem to be convinced that new is called, but the evidence points to the contrary. If the require block is moved back to where it was, this bug disappears. If I do puts service.class right before service.put_object, I get Fog::Storage::AWS::Real.

Like @celeen, I put puts statements at the very top of fog-core/lib/fog/storage.rb, and inside each of the methods. When middleman-s3_sync runs, the top of the file gets called before anything else happens, then it goes through fog-aws, which then calls parse_data, which in turn calls get_body_size, and get_content_type. .new never gets called.

Here is the output with the puts statements:

storage top called
     s3_sync  Let's see if there's work to be done...
Processing sitemap |Time: 00:00:00 | ===================================================================== | Time: 00:00:00
Processing remote files |Time: 00:00:00 | ================================================================ | Time: 00:00:00
     s3_sync  Ready to apply updates to www.theymakemusic.com.
     s3_sync  Updating interviews/robi-insinna-headman/index.html (gzipped)
service.class: Fog::Storage::AWS::Real
parse_data called
get_body_size called
get_content_type called
get_body_size called
## Invalidating files on CloudFront
Invalidating 2 files. It might take 10 to 15 minutes until all files are invalidated.
Please check the AWS Management Console to see the status of the invalidation.

I'm not familiar enough with the code to tell you why new is not being called, but I can tell you for sure that it is not being called. My suggestion is to release a fix by moving the require block back to the top of the class, and then figure out why new is not being called, assuming it is supposed to be called in this scenario.

monfresh commented 8 years ago

What was the reasoning behind moving the require block from the top of the file to new?

geemus commented 8 years ago

I looked through the middle_man code and it looks like it should be creating a connection object (which would call new) and then doing other calls on it, but I'm not super familiar with that code so I could be missing something. Fog::Storage::AWS::Real is exactly what I would expect for an instance that had gone through a new call. I definitely understand how parse_data is a class method which can be called without new, but put_object is an instance method, so it should require new to have already occured in order to be called (which is what I think you have described).

I moved it because only storage users need it, so I had hoped to limit the number of people that would need to add new require behavior (ie if you just use compute you wouldn't need mime-types at all). So moving it to this level is undesirable for the many users that are in this case and would not need it.

Does that help/make sense?

geemus commented 8 years ago

Ok, I think I've figured out how/where I was being confused. I just pushed a patch to fog-aws to hopefully fix this in the fog-aws case. Could you guys try out from master and see if it helps? If so I can follow up with a release and we can hopefully put this behind us. Sorry for all the confusion, just looking in all the wrong places it seems...

celeen commented 8 years ago

I will try it again tonight as soon as I get the chance, just to make sure but as far as I could tell working with this when I raised the issue it was/is one of scope. mime/types is loaded in a scope that the method that needs the info (#get_content_type) doesn't have access to.

When I initially raised the issue, I played around with putting the 'require' statement in different places; when I moved the 'require' to either the top of the file, or to #get_content_types (where MIME::Types is actually called) there was no problem accessing that module, because it was loaded in a scope that the calling code has access to.

Ruby's lexical scoping is weird, and I have a limited understanding of it. I don't expect the fix you've added to work, but I am excited to give it a shot and gain a deeper understanding of things.

monfresh commented 8 years ago

Thanks, @geemus. The latest fix works.

celeen commented 8 years ago

For me as well. This was enlightening! Thanks @geemus , and @monfresh !

geemus commented 8 years ago

Glad to hear it, sorry for all the confusion/delay and thanks for working through it with me.