MD5 cache busting & multiple asset host support

syelle commented 11 years ago

This extends and refactors #60 by @blakeharv. Support for defining an array of asset hosts as a configuration option is added and code is refactored to reduce repetition.

Asset hosts can be configured as such from within the existing options array:

    assets {
      asset_hosts [
        '//cdn-0.example.org',
        '//cdn-1.example.org',
        '//cdn-2.example.org',
        '//cdn-3.example.org'
      ]
      ...
    }

j15e commented 11 years ago

thanks @syelle, I'll have a look at this one juste after #43. I would like to have some tests before publishing this feature.

syelle commented 11 years ago

@j15e Fair enough. The MD5 stuff already has tests (they came with the commit I cherry-picked from #27). I added basic tests for the asset hosts but there's certainly room for something a little more robust.

j15e commented 11 years ago

I also must have a look at #27 which is basically also the same thing. I'll try to get the best of each PR.

j15e commented 11 years ago

Ah I think you've also cherry-pick commit from #27, great!

syelle commented 11 years ago

@j15e Yeah, I pulled the MD5 hash feature from it as basing it on file dates was causing issues when deploying on multiple servers. I ignored the host stuff from #27 as the implementation from #60 seemed a bit cleaner (after a bit of refactoring, anyway).

j15e commented 11 years ago

Amazing, can't wait to release the next version!

(well, actually, will have to wait a few days, do not have time to do it right now ;p)

syelle commented 11 years ago

@j15e Cool. We're using our fork in our project, so we should be able to suss out any other issues. So far, everything is going swimmingly after incorporating MD5 in the cache-busting hash.

j15e commented 11 years ago

I have try to merge it but I am having multiple tests failures (see below). Failures occurs event before merging (your branch is failing). Could you split md5 cache VS the multiple assets hosts?

Finished tests in 4.835195s, 17.5794 tests/s, 23.7839 assertions/s.

  1) Failure:
test_default_expiration_of_single_assets(AppTest) [test/unit_test.rb:105]:
<"Tue, 19 Feb 2013 20:22:00 GMT"> expected but was
<"Tue, 19 Feb 2013 20:21:59 GMT">.

  2) Failure:
test_js_hi_7a1b92c3f56ab5cfa73c1aa8222961cf_js_coffeescript_with_cache_buster(AppTest) [test/unit_test.rb:27]:
<200> expected but was
<404>.

  3) Failure:
test_js_hi_7a1b92c3f56ab5cfa73c1aa8222961cf_js_with_cache_buster(AppTest) [test/unit_test.rb:22]:
Failed assertion, no message given.

  4) Failure:
test_add_asset_host_to_filename(AssetHostTest) [test/asset_host_test.rb:54]:
Failed assertion, no message given.

  5) Failure:
test_host_gets_added_to_css_image_path_in_production(AssetHostTest) [test/asset_host_test.rb:48]:
Failed assertion, no message given.

  6) Error:
test_host_gets_added_to_css_source_path(AssetHostTest):
NoMethodError: undefined method `to_production_html' for nil:NilClass
    test/asset_host_test.rb:26:in `block in <class:AssetHostTest>'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/mocha-0.13.1/lib/mocha/integration/mini_test/version_230_to_2101.rb:36:in `run'

  7) Failure:
test_host_gets_added_to_image_helper_path_in_production(AssetHostTest) [test/asset_host_test.rb:36]:
<"<img src='//cdn.example.org/images/foo.jpg' />"> expected but was
<"<img src='/images/foo.jpg' />">.

  8) Error:
test_host_gets_added_to_js_source_path(AssetHostTest):
NoMethodError: undefined method `to_production_html' for nil:NilClass
    test/asset_host_test.rb:30:in `block in <class:AssetHostTest>'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/mocha-0.13.1/lib/mocha/integration/mini_test/version_230_to_2101.rb:36:in `run'

  9) Error:
test_host_sets_option(AssetHostTest):
NoMethodError: undefined method `host' for #<Sinatra::AssetPack::Options:0x00000101f3af78>
    test/asset_host_test.rb:22:in `block in <class:AssetHostTest>'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/mocha-0.13.1/lib/mocha/integration/mini_test/version_230_to_2101.rb:36:in `run'

 10) Failure:
test_Compressed_js_caching(CacheTest) [test/cache_test.rb:6]:
not all expectations were satisfied
unsatisfied expectations:
- expected exactly once, not yet invoked: JSMin.minify(any_parameters)

 11) Failure:
test_get_img_with_cache_buster(ImgTest) [test/img_test.rb:13]:
<"482"> expected but was
<"641">.

 12) Failure:
test_build(SqwishTest) [test/sqwish_test.rb:24]:
not all expectations were satisfied
unsatisfied expectations:
- expected exactly once, not yet invoked: #<AnyInstance:Sinatra::AssetPack::SqwishEngine>.css(any_parameters)

syelle commented 11 years ago

@j15e We'll take a look at the tests. We can't split the md5 thing out as it's required to support multiple hosts.

If you rely on file modification time and you deploy to multiple servers, they end up with different cache-busting numbers since they have different file modification times. It'll also build different cache-busting link into the code with the asset tags. You'll run into situations where a server will try to grab a file from your CDN origin that doesn't exist (since the origin will have a different cache-busting file name than the requester). The result is pages that only load some assets.

syelle commented 11 years ago

@j15e We've updated the tests. We had to resolve a few issues in the process:

When the tests were acting as if they were in dev mode, cachebuster URLs were being generated when they should not have been generated.
We had to add a method to clear the template cache during testing so we could swap between production and development modes so we could test the cache in both modes at the same time. This also had the benefit of fixing the two test failures that were unrelated to our change (the jsmin and swish test errors).
We fixed a bug in class_methods.rb where the add_individual_routes method was adding the cachebuster URL to the local path before trying to find the original asset.

We also refactored the environment logic so that it assumes that an environment that was not explicitly 'development' should act like production. This allowed us to have our 'qa' environments behave like a production system. By default, the gem was assuming everything should act like development except for 'production'.

Lastly, we added a few tests to complete coverage of areas that were testing either 'development' behavior or 'production' behavior, but not both.

Let me or @chrisftw know if you have any questions!

j15e commented 11 years ago

Many thanks, could you rebase it on master so I can merge it automatically?

syelle commented 11 years ago

@j15e I couldn't rebase as I'd already pushed those commits to the public repo. I did, however, merge your master branch into our branch.

j15e commented 11 years ago

Ok, I will deal with this within 1 to 2 weeks maximum, it is quite a big merge so I must have time to review everything.

MD5 will also fix bug related to #52

j15e commented 11 years ago

Update : I made a feature branch at feature/md5 and fixed an issue with some files names regarding MD5 hashes, but I got weird test errors on travis-ci related to mocha lib that I do not reproduce locally on any ruby version. I'll have to figure this out before releasing into master and future release.

j15e commented 11 years ago

I have run the test on OSX & Ubuntu without errors, does someone else can reproduce the errors shown on travis builds?

j15e commented 11 years ago

I fixed 3 out of 4 test issues that were due to not having "RACK_ENV=test" set on my computer VS travis-ci.

Current MD5 feature branch is available at feature/md5.

Remaining error is occuring when dealing with "non-existent" file, aka files served dynamically :

  1) Error:
test_non_existent_files_in_js_helper(NonExistentTest):
ArgumentError: comparison of String with nil failed
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/buster_helpers.rb:7:in `sort'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/buster_helpers.rb:7:in `cache_buster_hash'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/buster_helpers.rb:25:in `add_cache_buster'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/package.rb:72:in `production_path'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/package.rb:76:in `to_production_html'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/helpers.rb:50:in `show_one_asset_pack'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/helpers.rb:39:in `block in show_asset_pack'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/helpers.rb:38:in `map'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/helpers.rb:38:in `show_asset_pack'
    /Hookt/sinatra-assetpack/lib/sinatra/assetpack/helpers.rb:9:in `js'
    test/non_existent_test.rb:22:in `block in <class:App>'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:1293:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:1293:in `block in compile!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:860:in `[]'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:860:in `block (3 levels) in route!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:876:in `route_eval'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:860:in `block (2 levels) in route!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:897:in `block in process_route'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:895:in `catch'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:895:in `process_route'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:859:in `block in route!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:858:in `each'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:858:in `route!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:963:in `block in dispatch!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:946:in `block in invoke'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:946:in `catch'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:946:in `invoke'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:960:in `dispatch!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:794:in `block in call!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:946:in `block in invoke'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:946:in `catch'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:946:in `invoke'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:794:in `call!'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:780:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-protection-1.3.2/lib/rack/protection/xss_header.rb:27:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-protection-1.3.2/lib/rack/protection/path_traversal.rb:16:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-protection-1.3.2/lib/rack/protection/json_csrf.rb:17:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-protection-1.3.2/lib/rack/protection/base.rb:48:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-protection-1.3.2/lib/rack/protection/base.rb:48:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-protection-1.3.2/lib/rack/protection/xss_header.rb:27:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-1.5.2/lib/rack/nulllogger.rb:9:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-1.5.2/lib/rack/head.rb:11:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:124:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:1417:in `block in call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:1499:in `synchronize'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/sinatra-1.3.4/lib/sinatra/base.rb:1417:in `call'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-test-0.6.2/lib/rack/mock_session.rb:30:in `request'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-test-0.6.2/lib/rack/test.rb:230:in `process_request'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/rack-test-0.6.2/lib/rack/test.rb:57:in `get'
    test/non_existent_test.rb:31:in `block in <class:NonExistentTest>'
    /Users/jean-philippedoyle/.rvm/gems/ruby-1.9.3-p327/gems/mocha-0.13.2/lib/mocha/integration/mini_test/version_230_to_2101.rb:36:in `run'

90 tests, 129 assertions, 0 failures, 1 errors, 0 skips

j15e commented 11 years ago

See #91 which is the rebased & updated feature that I will merge as soon as we can fix the remaining issues.

rstacruz / sinatra-assetpack

MD5 cache busting & multiple asset host support #71