From e411be8840565656edb480d5dc919c69d8128f6a Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Sat, 3 Oct 2015 15:45:22 -0700 Subject: [PATCH 01/11] Fix Rakefile to always make test task be available --- Gemfile | 1 + Rakefile | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/Gemfile b/Gemfile index 7d8b3164..91a276fc 100644 --- a/Gemfile +++ b/Gemfile @@ -3,6 +3,7 @@ source "https://rubygems.org" gemspec gem "rake" +gem "rails" gem "mocha", require: false gem "appraisal" gem "pry" diff --git a/Rakefile b/Rakefile index 4980f6d3..c681d69a 100644 --- a/Rakefile +++ b/Rakefile @@ -2,22 +2,22 @@ require "bundler/setup" require "bundler/gem_tasks" require "rake/testtask" +Rake::TestTask.new do |test| + require "rails/version" + + test.libs << "test" + + if Rails::VERSION::MAJOR == 3 + test.test_files = %w[test/jbuilder_template_test.rb test/jbuilder_test.rb] + else + test.test_files = FileList["test/*_test.rb"] + end +end + if !ENV["APPRAISAL_INITIALIZED"] && !ENV["TRAVIS"] require "appraisal/task" Appraisal::Task.new task default: :appraisal else - Rake::TestTask.new do |test| - require "rails/version" - - test.libs << "test" - - if Rails::VERSION::MAJOR == 3 - test.test_files = %w[test/jbuilder_template_test.rb test/jbuilder_test.rb] - else - test.test_files = FileList["test/*_test.rb"] - end - end - task default: :test end From 9d7686212a8725575ad6bc575bcd385287407028 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 5 Oct 2015 20:04:39 -0700 Subject: [PATCH 02/11] Implement double-serialization-free caching system This removes the need to serialize-and-then-deserialize-again cached values per the proposal outlined in #289. --- lib/jbuilder/jbuilder_template.rb | 52 +++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 31d46b66..e64cbd27 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -11,6 +11,8 @@ class << self def initialize(context, *args) @context = context + @deferred_caches = {} + super(*args) end @@ -32,11 +34,22 @@ def partial!(*args) # end def cache!(key=nil, options={}) if @context.controller.perform_caching - value = ::Rails.cache.fetch(_cache_key(key, options), options) do - _scope { yield self } + token = "jbuilder-#{::SecureRandom.hex(8)}" + key = _cache_key key, options + + # Convert yielded block to a Proc we can call later if needed. + not_found = ::Proc.new + + fetcher = ::Proc.new do + ::Rails.cache.fetch(key, options) do + value = _scope { not_found.call self } + ::MultiJson.dump value + end end - merge! value + @deferred_caches[token] = fetcher + + merge! token => nil else yield end @@ -75,6 +88,39 @@ def set!(name, object = BLANK, *args) end end + def target! + # Call the superclass implementation to get the output JSON as a string. + output = super + + @deferred_caches.each do |token, fetch_block| + value = fetch_block.call + search = "\"#{token}\":null" + + if value == "{}".freeze + # Special case to handle empty objects: we remove the search key + # entirely from the output. + search = ::Regexp.new ',?' + ::Regexp.escape(search) + value = "".freeze + end + + if value.start_with? "[".freeze + # Special case to handle arrays: we actually have to replace the + # object with the array. + search = "{#{search}}" + end + + if value.start_with? "{".freeze + # Remove leading and trailing braces so it'll merge seamlessly into + # the surrounding object. + value = value.slice 1...-1 + end + + output.sub! search, value + end + + output + end + private def _render_partial_with_options(options) From 2cf200773e8c0953cfd3d5e745a1a9b43a437b26 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 25 Jul 2016 20:53:17 -0700 Subject: [PATCH 03/11] Convert from old `Proc::new` to new stabby lambda syntax --- lib/jbuilder/jbuilder_template.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 704e28b4..c76d0c90 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -32,15 +32,13 @@ def partial!(*args) # json.cache! ['v1', @person], expires_in: 10.minutes do # json.extract! @person, :name, :age # end - def cache!(key=nil, options={}) + def cache!(key=nil, options={}, &block) if @context.controller.perform_caching token = "jbuilder-#{::SecureRandom.hex(8)}" - not_found = ::Proc.new - - fetcher = ::Proc.new do + fetcher = -> do value = _cache_fragment_for(key, options) do - _scope { not_found.call self } + _scope { block.call self } end ::MultiJson.dump value end From a1533a20d3233f737ae28b3fd687d3c22ea732c8 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 25 Jul 2016 21:00:20 -0700 Subject: [PATCH 04/11] Add missing `actionpack` gem dependency; add explanatory comment to `Gemfile` --- Gemfile | 2 +- jbuilder.gemspec | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 91a276fc..903b0a4a 100644 --- a/Gemfile +++ b/Gemfile @@ -3,7 +3,7 @@ source "https://rubygems.org" gemspec gem "rake" -gem "rails" +gem "rails", require: false # Version is used by the Rakefile to decide which tests to run gem "mocha", require: false gem "appraisal" gem "pry" diff --git a/jbuilder.gemspec b/jbuilder.gemspec index 012d9f9d..cac38d3e 100644 --- a/jbuilder.gemspec +++ b/jbuilder.gemspec @@ -9,6 +9,7 @@ Gem::Specification.new do |s| s.required_ruby_version = '>= 1.9.3' + s.add_dependency 'actionpack', '>= 3.0.0', '< 5.1' s.add_dependency 'activesupport', '>= 3.0.0', '< 5.1' s.add_dependency 'multi_json', '~> 1.2' From d052df278d6dc8377eba67df5759262410d0a342 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 15 Aug 2016 20:08:44 -0700 Subject: [PATCH 05/11] Ensure raw JSON is stored in the cache --- lib/jbuilder/jbuilder_template.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index c76d0c90..1604ca2a 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -33,14 +33,17 @@ def partial!(*args) # json.extract! @person, :name, :age # end def cache!(key=nil, options={}, &block) + default_options = {raw: true} + if @context.controller.perform_caching token = "jbuilder-#{::SecureRandom.hex(8)}" fetcher = -> do - value = _cache_fragment_for(key, options) do - _scope { block.call self } + _cache_fragment_for(key, default_options.merge(options)) do + value = _scope { block.call self } + + ::MultiJson.dump value end - ::MultiJson.dump value end @deferred_caches[token] = fetcher From 366390937c15ede92fc64d01d2974eea5834af95 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 15 Aug 2016 20:11:21 -0700 Subject: [PATCH 06/11] Add spec to ensure caching JSON-within-JSON works --- test/jbuilder_template_test.rb | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/test/jbuilder_template_test.rb b/test/jbuilder_template_test.rb index e3028819..ebf547b7 100644 --- a/test/jbuilder_template_test.rb +++ b/test/jbuilder_template_test.rb @@ -4,7 +4,7 @@ require "action_view" require "action_view/testing/resolvers" require "active_support/cache" -require "jbuilder/jbuilder_template" +require "jbuilder" BLOG_POST_PARTIAL = <<-JBUILDER json.extract! blog_post, :id, :body @@ -246,6 +246,28 @@ def assert_collection_rendered(result, context = nil) assert_equal "bar", result["foo"] end + test "cache a JSON string" do + undef_context_methods :fragment_name_with_digest, :cache_fragment_name + + jbuild <<-JBUILDER + json.cache! "cachekey" do + complex = ::MultiJson.dump 'message' => 'The measurement is 1" too long.' + + json.complex complex + end + JBUILDER + + result = jbuild(<<-JBUILDER) + json.cache! "cachekey" do + json.complex "Miss" + end + JBUILDER + + expected_json = ::MultiJson.dump 'message' => 'The measurement is 1" too long.' + + assert_equal expected_json, result["complex"] + end + test "fragment caching a JSON object" do undef_context_methods :fragment_name_with_digest, :cache_fragment_name From ca0450588852faff3f341d9a9f9a2a1976ec9211 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 15 Aug 2016 20:11:31 -0700 Subject: [PATCH 07/11] Bugfix for caching JSON-within-JSON --- lib/jbuilder/jbuilder_template.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 1604ca2a..923eb3e2 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -114,7 +114,12 @@ def target! value = value.slice 1...-1 end - output.sub! search, value + # NOTE: Doing `String#sub` or similar with a `Regexp` will make it try + # to do backreferencing with any backslashes found in the replacement + # string, this will interfere with any potential backslashes in the + # JSON replacement. Therefore we're using `String#[]=` which doesn't + # have that behavior. + output[search] = value end output From 18ec70b40aeab5c451068be1a5937efcf6a17646 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 15 Aug 2016 20:28:55 -0700 Subject: [PATCH 08/11] Include revision in the cache key --- lib/jbuilder/jbuilder_template.rb | 8 +++++++- test/jbuilder_template_test.rb | 6 ++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 923eb3e2..293f9829 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -7,6 +7,12 @@ class << self attr_accessor :template_lookup_options end + CACHE_REVISION = 2 + + # Passed to `ActiveSupport::Cache.expand_cache_key` so that we can expire + # old entries if our on-disk format changes + CACHE_TAG = "jbuilder-revision-#{CACHE_REVISION}".to_sym + self.template_lookup_options = { handlers: [:jbuilder] } def initialize(context, *args) @@ -181,7 +187,7 @@ def _cache_key(key, options) key = url_for(key).split('://', 2).last if ::Hash === key end - ::ActiveSupport::Cache.expand_cache_key(key, :jbuilder) + ::ActiveSupport::Cache.expand_cache_key(key, CACHE_TAG) end def _fragment_name_with_digest(key, options) diff --git a/test/jbuilder_template_test.rb b/test/jbuilder_template_test.rb index ebf547b7..fe91385a 100644 --- a/test/jbuilder_template_test.rb +++ b/test/jbuilder_template_test.rb @@ -379,8 +379,10 @@ def assert_collection_rendered(result, context = nil) end JBUILDER - assert_equal "jbuilder/cachekey", payloads[:read_fragment][:key] - assert_equal "jbuilder/cachekey", payloads[:write_fragment][:key] + tag = JbuilderTemplate::CACHE_TAG + + assert_equal "#{tag}/cachekey", payloads[:read_fragment][:key] + assert_equal "#{tag}/cachekey", payloads[:write_fragment][:key] end test "current cache digest option accepts options" do From e21a6469277b8b52ec715816af91b7b8a03bfb8f Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 22 Aug 2016 18:46:38 -0700 Subject: [PATCH 09/11] Tweak some broken expectations; add test for proper UTF-8 handling --- test/jbuilder_template_test.rb | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/test/jbuilder_template_test.rb b/test/jbuilder_template_test.rb index fe91385a..d3643e62 100644 --- a/test/jbuilder_template_test.rb +++ b/test/jbuilder_template_test.rb @@ -268,6 +268,20 @@ def assert_collection_rendered(result, context = nil) assert_equal expected_json, result["complex"] end + test "cache a string containing UTF-8" do + undef_context_methods :fragment_name_with_digest, :cache_fragment_name + + # The "-" in this template and the literal in the comparison below are + # both in UTF-8 + result = jbuild <<-JBUILDER + json.cache! "cachekey" do + json.encoded "a – b" + end + JBUILDER + + assert_equal "a – b", result["encoded"] + end + test "fragment caching a JSON object" do undef_context_methods :fragment_name_with_digest, :cache_fragment_name @@ -344,8 +358,7 @@ def assert_collection_rendered(result, context = nil) test "fragment caching works with current cache digests" do undef_context_methods :fragment_name_with_digest - @context.expects :cache_fragment_name - ActiveSupport::Cache.expects :expand_cache_key + @context.expects(:cache_fragment_name).returns("cachekey/digest") jbuild <<-JBUILDER json.cache! "cachekey" do @@ -388,8 +401,7 @@ def assert_collection_rendered(result, context = nil) test "current cache digest option accepts options" do undef_context_methods :fragment_name_with_digest - @context.expects(:cache_fragment_name).with("cachekey", skip_digest: true) - ActiveSupport::Cache.expects :expand_cache_key + @context.expects(:cache_fragment_name).with("cachekey", skip_digest: true).returns("cachekey") jbuild <<-JBUILDER json.cache! "cachekey", skip_digest: true do From 7c146e8a0d714869ecd1785cd992822dabe9b9eb Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 22 Aug 2016 19:01:35 -0700 Subject: [PATCH 10/11] Don't do raw storage, as it was causing bugs with encoding --- lib/jbuilder/jbuilder_template.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 293f9829..bb7e1b7a 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -39,13 +39,11 @@ def partial!(*args) # json.extract! @person, :name, :age # end def cache!(key=nil, options={}, &block) - default_options = {raw: true} - if @context.controller.perform_caching token = "jbuilder-#{::SecureRandom.hex(8)}" fetcher = -> do - _cache_fragment_for(key, default_options.merge(options)) do + _cache_fragment_for(key, options) do value = _scope { block.call self } ::MultiJson.dump value From 60189dd8309730b260718acf2a711e6b1bc47ee4 Mon Sep 17 00:00:00 2001 From: Dirk Gadsden Date: Mon, 22 Aug 2016 19:20:43 -0700 Subject: [PATCH 11/11] Bump revision due to at-rest format change --- lib/jbuilder/jbuilder_template.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index bb7e1b7a..13233a3c 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -7,7 +7,7 @@ class << self attr_accessor :template_lookup_options end - CACHE_REVISION = 2 + CACHE_REVISION = 3 # Passed to `ActiveSupport::Cache.expand_cache_key` so that we can expire # old entries if our on-disk format changes