v1.1.0 Dynamic responses via procs - breaking Changes

[AlanFoster]

Hey; I just noticed that v1.1.0 has breaking changes in it. I believe this has been caused by https://github.com/oesmith/puffing-billy/pull/232

On version 1.0.0 calling methods or variables defined within the scope of your test from a stub proc was possible, however this is no longer possible due to the following implementation change:

-        res = @response.call(params, headers, body)
+        res = instance_exec(params, headers, body, url, method, &@response)

As an example, this test will pass on v1.0.0, but not on v1.1.0:

require 'spec_helper'

describe 'Tumblr API example', type: :feature, js: true do
  let(:posts) do
    [
      {
        'regular-title' => 'News Item 1',
        'url-with-slug' => 'http://example.com/news/1',
        'regular-body' => 'News item 1 content here'
      },
      {
        'regular-title' => 'News Item 2',
        'url-with-slug' => 'http://example.com/news/2',
        'regular-body' => 'News item 2 content here'
      }
    ]
  end

  before do
    proxy.stub('http://blog.howmanyleft.co.uk/api/read/json').and_return(Proc.new do
      {
        jsonp: {
          # This breaks on v1.1.0, due to the `instance_exec` implementation change
          posts: posts
        }
      }
    end)
  end

  it 'should show news stories' do
    visit '/tumblr_api.html'
    expect(page).to have_link('News Item 1', href: 'http://example.com/news/1')
    expect(page).to have_content('News item 1 content here')
    expect(page).to have_link('News Item 2', href: 'http://example.com/news/2')
    expect(page).to have_content('News item 2 content here')
  end
end

The specific failure on v1.1.0 is:

NameError: undefined local variable or method `posts' for #<Billy::ProxyRequestStub:0x007fac289def00>
from /Users/a/Documents/Shopkeep/puffing-billy/spec/features/examples/tumblr_api_spec.rb:23:in `block (3 levels) in <top (required)>'

Note: I understand that the proc is superfluous in this scenario, but it's just a quickly thrown together example 🎉

[Jack12816]

Argh. I was thinking of edge-cases when implementing this, but I was on the opinion the lean DSL for intercepting is more worth than the user scope.

We could convert the pass_request method to a class method with a shortcut on Billy. But that's just one option. https://github.com/oesmith/puffing-billy/pull/232/files#diff-9564b0c330d9fe4a11ec8f4425062188R74

# Revert instance_exec back to response.call
-        res = instance_exec(params, headers, body, url, method, &@response)
+        res = @response.call(params, headers, body, url, method)

# Different location of +pass_request+
module Billy
  def self.pass_request(params, headers, body, url, method)
    handler = Billy.proxy.request_handler.handlers[:proxy]
    response = handler.handle_request(method, url, headers, body)
    {
      code: response[:status],
      body: response[:content],
      headers: response[:headers]
    }
  end
end

# Usage
proxy.stub('http://example.com/').and_return(Proc.new { |*args|
  response = Billy.pass_request(*args)
  response[:headers]['Content-Type'] = 'text/plain'
  response[:body] = 'Hello World!'
  response[:code] = 200
  response
})

@AlanFoster / @ronwsmith What do you think about this?

[AlanFoster]

@Jack12816 Hm, I'm not massively opinionated here. I'm not sure if this is functionality we'll ever need

To take a step back though, it doesn't feel like this logic belongs within the context of a stub. i.e. You're specifically wanting to intercept the proxy handler and mutate the request/response - but you've added the functionality via stubs instead 🤔 Puffing Billy could be updated to allow proxy requests to be intercepted directly, but this is a bigger API change.

So perhaps as an interim solution, this functionality could live in userland if the stub handler is provided the additional url + methods arguments?

# Revert instance_exec back to response.call, but still provide URL and Method
-        res = instance_exec(params, headers, body, url, method, &@response)
+        res = @response.call(params, headers, body, url, method)

Your application can now have its own implementation of a pass_through / proxy_request function, and puffing billy only needs to provide the url + method parameters that were included in your original PR. I believe all of the functionality exists within the public API of Puffing Billy to allow this.

Long term, perhaps puffing billy can be extended to allow proxy requests to be intercepted as a richer API.

[ronwsmith]

I'm good either way. Let's be sure to add @AlanFoster 's sample spec to make sure we resolve the issue and something similar doesn't come up again.

@Jack12816 can you resolve quickly, or shall I revert in a patch release in the interim? Thanks!

[AlanFoster]

As a suggestion for the long term solution...

Perhaps all of this would be easier to implement with the concept of the chain of responsibility pattern, rather than the current loop implementation that's provided with Puffing Billy:

# request_handler.rb

    def handlers
      @handlers ||= { stubs: StubHandler.new,
                      cache: CacheHandler.new,
                      proxy: ProxyHandler.new }
    end

      # Process the handlers by order of importance
      [:stubs, :cache, :proxy].each do |key|
        if (response = handlers[key].handle_request(method, url, headers, body))
          @request_log.complete(request, key)
          return response
        end
      end

If we swapped things around around so that each handler has a successor handler instead, then this would be trivial to implement:

# request_handler.rb

    def handler_chain
      StubHandler.new(successor: CacheHandler.new(successor: ProxyHandler.new(successor: ErrorHandler.new))
    end

    def handle_request(method, url, headers, body)
      request = request_log.record(method, url, headers, body)
      handler_chain.handle_request(method, url, headers, body)
      # ...
   end

Each handler will now have the successor freely available:

def handle_request(method, url, headers, body, successor)
    return successor.handle_request(method, url, headers, body) unless handles_request?

    ...
end

In the case of the dynamic handling of functionality that you desire we'd just pass through the successor handler to userland:

-        res = instance_exec(params, headers, body, url, method, &@response)
+        res = @response.call(params, headers, body, url, method, successor)

So this would work for your scenario like:

proxy.stub('http://example.com/').and_return(Proc.new do |params, headers, body, url, method, successor|
  response = successor.handle_request(method, url, headers, body)
  response[:headers]['Content-Type'] = 'text/plain'
  response[:body] = 'Hello World!'
  response[:code] = 200
  response
end)

[Jack12816]

@ronwsmith I provide a PR today for the quick fix.

I believe all of the functionality exists within the public API of Puffing Billy to allow this.

Jep, but I don't want to jot down every time the same handler retrieving stuff to allow a proxy pass. I thought of the Varnish proxy by implementing this. It provides this kind of tools to build behavior on. @AlanFoster

So the shortcut Billy.pass_request method will ease the boilerplate pain. But, yeah you can always use a custom user provided proxy pass method if you like. That's why it's inside the stub block. (It was inspired by the chrome devtools API at this point and it fits quite good)

[Jack12816]

@ronwsmith There you go: https://github.com/oesmith/puffing-billy/pull/242

[ronwsmith]

Fixed in v1.1.1