Cache Control

[karlfreeman]

Having previously looked into Grape I was a little dis-heartened to find it had no cache-control helpers built in. So I went ahead and implemented them. Given that I had the relatively simple logic to hand I figured I'd see what it looked like in Lotus.

# Cache-Control:public,max-age=900
cache_control :public, max_age: 900

# Cache-Control:public,no-store,max-age=900,s-maxage=86400
cache_control :public, :no_store, max_age: 900, s_maxage:86400

# Cache-Control:public,no-store,max-age=900,s-maxage=86400
cache_control :public, :no_store, max_age: (Time.now + 900), s_maxage: (Time.now + 86400)

# Cache-Control:private,must-revalidate,max=age=0
cache_control :private, :must_revalidate, max_age: 0

# Cache-Control:public,max-age=900
# Expires:Tue, 25 Feb 2014 12:00:00 GMT
expires 900, :public

# Cache-Control:private,no-cache,no-store,max-age=900
# Expires:Tue, 25 Feb 2014 12:00:00 GMT
expires (Time.now + 900), :private, :no_store

We could have a combination of both types of methods?

action 'Public' do
  def call(params)
    cache_control :public
  end
end

action 'Private' do
  cache_control :private
  def call(params)
  end
end

A WIP, hence the complete lack of documentation / nice code 🍺.

[coveralls]

Coverage decreased (-0.41%) when pulling 9ff83e3 on karlfreeman:feature/cache-control into af217ea on lotus:master.

[karlfreeman]

Or, alternative idea. How would one do this as an extension to lotus/controller?

[jodosha]

@karlfreeman Thanks for the idea, it's definitely something needed, but not enabled by default (see Lotus::Action::Cookies.

Anyway, because I'm focused on releasing Lotus right now, I suggest to merge this after lotus-controller 0.2.0 will be out. Maybe targeting the 0.2.1. What do you think?

[karlfreeman]

@jodosha deferring this make sense. Shall I open up a different issue similar to #15?

[jodosha]

@karlfreeman Why do you want to open a different issue? We'll merge this once ready and Lotus will be releases.

[karlfreeman]

@jodosha The reason was mainly as this is a pull request linked to my feature branch instead of being an issue which can be "closed" via a pull request. Doesnt matter really 😄.

[jodosha]

@karlfreeman This PR needs some ❤️

1. Extensive testing
2. Code cleanup
3. Documentation

Do you still want to take care of this?

[karlfreeman]

Yeah, on this. Went AWOL for a couple days. Will bolster tests, documentation and generally put more time into this.

What's your thoughts of including Timecop or should we follow ActiveSupports lead with travel_to embracing the fact that there will be many places where things related to time will need to be tested?

[jodosha]

@karlfreeman Is Minitest's stub enough?

[karlfreeman]

@jodosha Ok, as I'm just freezing Time stubs should be enough. However, would it be possible to include minitest-around as a development_dependency. It does have some caveats so I'm not sure whats worse repetitive code or extra cruft.

eg:

describe 'Cache control' do
  before do
    @app = Rack::MockRequest.new(CacheControlRoutes)
  end
  around do |test|
    Time.stub(:now, Time.now) do
      test.call
    end
  end
  it 'accepts a Symbol' do
    response = @app.get('/symbol')
    response.headers.fetch('Cache-Control').must_equal('private')
  end
  it 'accepts multiple Symbols' do
    response = @app.get('/symbols')
    response.headers.fetch('Cache-Control').split(', ').must_equal %w(private no-store)
  end
end

vs

describe 'Cache control' do
  before do
    @app = Rack::MockRequest.new(CacheControlRoutes)
  end
  it 'accepts a Symbol' do
    Time.stub(:now, Time.now) do
      response = @app.get('/symbol')
      response.headers.fetch('Cache-Control').must_equal('private')
    end
  end
  it 'accepts multiple Symbols' do
    Time.stub(:now, Time.now) do
      response = @app.get('/symbols')
      response.headers.fetch('Cache-Control').split(', ').must_equal %w(private no-store)
    end
  end
end

[jodosha]

@karlfreeman 👍 for the second.

[karlfreeman]

Ok. No problem.

Some previous cargo-cult thinking was when calling cache_control or expires for a second time we would merge the previous directives. I'm questioning if this interface adds too much 'magic' and weird edge cases. Example:

require 'lotus/controller'
require 'lotus/action/cache_control'

class CacheControlController
  include Lotus::Controller
  include Lotus::Action::CacheControl

  action 'RandomPrivate' do
    def call(params)
      is_private = [TRUE, FALSE].sample

      # set Cache-Control directives
      cache_control :public, :no_store, max_age: 900

      if is_private
        # merge more Cache-Control directives
        cache_control :private, max_age: 90
      end

      # Cache-Control: private, no-store, max-age=90
    end
  end

  action 'SometimesCachable' do
    def call(params)
      is_super_cachable = [TRUE, FALSE].sample

      # set Cache-Control directives
      cache_control :private, :no_cache, :no_store

      if is_super_cachable
        # somehow remove previous Cache-Control directives?
        cache_control :public, max_age: 900
      end

      # Cache-Control: public, no-cache, no_store, max-age=900
    end
  end
end

The RandomPrivate action follows a sane interface where you can merge additional directives and with a bit of common sense you understand that when adding :private it removes your previous :public directive. However, with the SometimesCachable action its not clear how to remove previous directives and arguable would be easier to from the outset throw away your previous Cache-Control directives.

Sinatra keeps it simple by always overwriting your previous Cache-Control.

[coveralls]

Coverage remained the same when pulling 037013d on karlfreeman:feature/cache-control into cfe555e on lotus:master.

[jodosha]

@karlfreeman Interesting, I haven't considered it yet, probably the second case is less confusing and respects POLA. What's your opinion about having a DSL for this? It would keep #call implementation cleaner with the sole business use case to deal with.

module ArticlesController
  include Lotus::Controller

  action 'Create' do
    include Lotus::Action
    cache_control :private, :must_revalidate, max_age: 0

    def call(params)
      @article = Article.new(params)
      ArticleRepository.persist(@article)
    end
  end
end

[lucasas]

Hey guys, which brach this code is available? I want to help with some conditional get implementation. cc/ @karlfreeman

[jodosha]

@lucasas https://github.com/karlfreeman/controller/tree/feature/cache-control
Would you please take care of cache control? @karlfreeman has stopped working on this, so feel free to rework. Thanks.

[karlfreeman]

Sorry about this. Snowed under till the weekend :/.

[lucasas]

Of course @jodosha, I'll take care of cache control. I'm gonna fork @karlfreeman repository and start to work on it today. Is it ok for you @karlfreeman?

[lucasas]

Hey @jodosha. I've done some refactoring and included missing Cache-Control directives. This weekend or in the beginning of the next week, I'm gonna implement Conditional-Get.

First question is: Should I create a new PR into @karlfreeman repository or create another one into lotus/repository?

[jodosha]

@lucasas Please open at lotus/controller. Thanks.

[lucasas]

@jodosha opened here #36

[jodosha]

Closing in favor of #36