From 7842112ba70b32db2bcb6e26c6c55f9548bfa8af Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Mon, 13 Aug 2012 18:53:00 +0200 Subject: [PATCH 1/9] Add Processor class to handle request execution --- lib/batch_api/processor.rb | 78 ++++++++++++++++++++++++++++++++++++++ spec/lib/processor_spec.rb | 65 +++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 lib/batch_api/processor.rb create mode 100644 spec/lib/processor_spec.rb diff --git a/lib/batch_api/processor.rb b/lib/batch_api/processor.rb new file mode 100644 index 0000000..b31ef48 --- /dev/null +++ b/lib/batch_api/processor.rb @@ -0,0 +1,78 @@ +require 'batch_api/processor/strategies/sequential' + +module BatchApi + class Processor + # Raised when a user provides more Batch API requests than a service + # allows. + class OperationLimitExceeded < StandardError; end + # Raised if a provided option is invalid. + class BadOptionError < StandardError; end + + attr_reader :ops, :options + + # Public: create a new Processor. + # + # ops - an array of operations hashes + # options - any other options + # + # Raises OperationLimitExceeded if more operations are requested than + # allowed by the BatchApi configuration. + # Raises BadOptionError if other provided options are invalid. + # + # Returns the new Processor instance. + def initialize(ops, env, options = {}) + @env = env + @options = self.process_options(options) + @ops = self.process_ops(ops) + end + + # Public: the processing strategy to use, based on the options + # provided in BatchApi setup and the request. + # Currently only Sequential is supported. + def strategy + BatchApi::Processor::Strategies::Sequential + end + + # Public: run the batch operations according to the appropriate strategy. + # + # Returns a set of BatchResponses + def execute! + strategy.execute!(@ops, @options) + end + + protected + + # Internal: Validate that an allowable number of operations have been + # provided, and turn them into BatchApi::Operation objects. + # + # ops - a series of operations + # + # Raises OperationLimitExceeded if more operations are requested than + # allowed by the BatchApi configuration. + # + # Returns an array of BatchApi::Operation objects + def process_ops(ops) + if ops.length > BatchApi.config.limit + raise OperationLimitExceeded, + "Only #{BatchApi.config.limit} operations can be submitted at once, " + + "#{ops.length} were provided" + else + ops.map do |op| + BatchApi::Operation.new(op, @env) + end + end + end + + # Internal: Processes any other provided options for validity. + # Currently, the :sequential option is REQUIRED (until parallel + # implementation is created). + # + # options - an options hash + # + # Returns the valid options hash. + def process_options(options) + raise BadOptionError, "Sequential flag is currently required" unless options[:sequential] + options + end + end +end diff --git a/spec/lib/processor_spec.rb b/spec/lib/processor_spec.rb new file mode 100644 index 0000000..5fd7c3e --- /dev/null +++ b/spec/lib/processor_spec.rb @@ -0,0 +1,65 @@ +require 'spec_helper' + +describe BatchApi::Processor do + it "provides a OperationLimitExceeded error" do + BatchApi::Processor::OperationLimitExceeded.superclass.should == StandardError + end + + it "provides a OperationLimitExceeded error" do + BatchApi::Processor::BadOptionError.superclass.should == StandardError + end + + let(:ops) { [ {url: "/endpoint", method: "get"} ] } + let(:options) { { sequential: true } } + let(:env) { {} } + let(:processor) { BatchApi::Processor.new(ops, env, options) } + + describe "#initialize" do + # this may be brittle...consider refactoring? + it "turns the ops provided into BatchApi::Operations stored at #ops" do + # simulate receiving several operations + operation_objects = 3.times.collect { stub("operation object") } + operation_params = 3.times.collect do |i| + stub("raw operation").tap do |o| + BatchApi::Operation.should_receive(:new).with(o, anything).and_return(operation_objects[i]) + end + end + + BatchApi::Processor.new(operation_params, env, options).ops.should == operation_objects + end + + it "makes the options available" do + BatchApi::Processor.new(ops, env, options).options.should == options + end + + context "error conditions" do + it "(currently) throws an error if sequential is not true" do + expect { BatchApi::Processor.new(ops, env, {}) }.to raise_exception(BatchApi::Processor::BadOptionError) + end + + it "raise a OperationLimitExceeded error if too many ops provided" do + ops = (BatchApi.config.limit + 1).times.collect {|i| i} + expect { BatchApi::Processor.new(ops, env, options) }.to raise_exception(BatchApi::Processor::OperationLimitExceeded) + end + end + + describe "#strategy" do + it "returns BatchApi::Processor::Strategies::Sequential" do + processor.strategy.should == BatchApi::Processor::Strategies::Sequential + end + end + + describe "#execute!" do + it "executes on the provided strategy" do + processor.strategy.should_receive(:execute!).with(processor.ops, processor.options) + processor.execute! + end + + it "returns the result of the strategy" do + stubby = stub + processor.strategy.stub(:execute!).and_return(stubby) + processor.execute!.should == stubby + end + end + end +end From 4edc2ddc13cfa5730c5d4e9f9d3e14c5a935b28f Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Mon, 13 Aug 2012 18:53:34 +0200 Subject: [PATCH 2/9] Integrate processor --- app/controllers/batch_api/batch_controller.rb | 3 +-- spec/controllers/batch_controller_spec.rb | 23 +++++++------------ spec/integration/batch_spec.rb | 2 +- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/app/controllers/batch_api/batch_controller.rb b/app/controllers/batch_api/batch_controller.rb index b19b8f6..562f996 100644 --- a/app/controllers/batch_api/batch_controller.rb +++ b/app/controllers/batch_api/batch_controller.rb @@ -3,8 +3,7 @@ module BatchApi class BatchController < ::ApplicationController def batch - ops = params[:ops].map {|o| BatchApi::Operation.new(o, request.env)} - render :json => ops.map(&:execute) + render :json => BatchApi::Processor.new(params[:ops], request.env, params) end end end diff --git a/spec/controllers/batch_controller_spec.rb b/spec/controllers/batch_controller_spec.rb index e647bea..77cabda 100644 --- a/spec/controllers/batch_controller_spec.rb +++ b/spec/controllers/batch_controller_spec.rb @@ -3,25 +3,18 @@ describe BatchApi::BatchController do describe "#batch" do - it "creates batch ops for each operation using the request environment" do - ops = 10.times.collect {|i| ({"operation" => i.to_s}) } - ops.each do |o| - BatchApi::Operation.should_receive(:new).with(o, request.env).and_return(stub(:execute => "")) - end - - xhr :post, :batch, ops: ops - end - - it "returns the resultof the batch operation's execution as JSON and in order" do + it "returns the result of the batch operation's execution as JSON and in order" do + env = request.env + request.stub(:env).and_return(env) ops = 10.times.collect {|i| {"operation" => i.to_s} } - ops.each do |o| - BatchApi::Operation.should_receive(:new).and_return(stub(:execute => o["operation"])) - end + params = {ops: ops, sequential: true} + result = ops.map(&:to_s) + BatchApi::Processor.should_receive(:new).with(ops, request.env, hash_including(params)).and_return(result) - xhr :post, :batch, ops: ops + xhr :post, :batch, params json = JSON.parse(response.body) ops.each_with_index do |o, i| - json[i].should == i.to_s + json[i].should == ops[i].to_s end end end diff --git a/spec/integration/batch_spec.rb b/spec/integration/batch_spec.rb index d85b1f5..87f8cd1 100644 --- a/spec/integration/batch_spec.rb +++ b/spec/integration/batch_spec.rb @@ -50,7 +50,7 @@ def headerize(hash) } } before :each do - xhr :post, "/batch", {ops: [get_request, post_request]}.to_json, "CONTENT_TYPE" => "application/json" + xhr :post, "/batch", {ops: [get_request, post_request], sequential: true}.to_json, "CONTENT_TYPE" => "application/json" end it "returns a 200" do From d47bb6f2303af0a418a6da5a65f4e52b5cd71ce3 Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Mon, 13 Aug 2012 18:54:16 +0200 Subject: [PATCH 3/9] Add sequential processing strategy --- .../processor/strategies/sequential.rb | 18 ++++++++++++++++++ spec/lib/sequential_spec.rb | 5 +++++ 2 files changed, 23 insertions(+) create mode 100644 lib/batch_api/processor/strategies/sequential.rb create mode 100644 spec/lib/sequential_spec.rb diff --git a/lib/batch_api/processor/strategies/sequential.rb b/lib/batch_api/processor/strategies/sequential.rb new file mode 100644 index 0000000..9225b65 --- /dev/null +++ b/lib/batch_api/processor/strategies/sequential.rb @@ -0,0 +1,18 @@ +module BatchApi + class Processor + module Strategies + module Sequential + # Public: execute all operations sequentially. + # + # ops - a set of BatchApi::Operations + # options - a set of options + # + # Returns an array of BatchApi::Response objects. + def self.execute!(ops, options = {}) + ops.map(&:execute) + end + end + end + end +end + diff --git a/spec/lib/sequential_spec.rb b/spec/lib/sequential_spec.rb new file mode 100644 index 0000000..7c5fb77 --- /dev/null +++ b/spec/lib/sequential_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe BatchApi::Processor::Strategies::Sequential do + pending "needs a test" +end From 1d58ed743c0358fa79249ce6904fb8e5e4d5cd89 Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Tue, 14 Aug 2012 11:43:50 +0200 Subject: [PATCH 4/9] Get request specs passing with the new processor --- app/controllers/batch_api/batch_controller.rb | 2 +- lib/batch_api/processor.rb | 2 +- spec/controllers/batch_controller_spec.rb | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/controllers/batch_api/batch_controller.rb b/app/controllers/batch_api/batch_controller.rb index 562f996..3baf458 100644 --- a/app/controllers/batch_api/batch_controller.rb +++ b/app/controllers/batch_api/batch_controller.rb @@ -3,7 +3,7 @@ module BatchApi class BatchController < ::ApplicationController def batch - render :json => BatchApi::Processor.new(params[:ops], request.env, params) + render :json => BatchApi::Processor.new(params[:ops], request.env, params).execute! end end end diff --git a/lib/batch_api/processor.rb b/lib/batch_api/processor.rb index b31ef48..ea1b902 100644 --- a/lib/batch_api/processor.rb +++ b/lib/batch_api/processor.rb @@ -22,8 +22,8 @@ class BadOptionError < StandardError; end # Returns the new Processor instance. def initialize(ops, env, options = {}) @env = env - @options = self.process_options(options) @ops = self.process_ops(ops) + @options = self.process_options(options) end # Public: the processing strategy to use, based on the options diff --git a/spec/controllers/batch_controller_spec.rb b/spec/controllers/batch_controller_spec.rb index 77cabda..6e15e19 100644 --- a/spec/controllers/batch_controller_spec.rb +++ b/spec/controllers/batch_controller_spec.rb @@ -5,11 +5,12 @@ describe "#batch" do it "returns the result of the batch operation's execution as JSON and in order" do env = request.env - request.stub(:env).and_return(env) ops = 10.times.collect {|i| {"operation" => i.to_s} } params = {ops: ops, sequential: true} result = ops.map(&:to_s) - BatchApi::Processor.should_receive(:new).with(ops, request.env, hash_including(params)).and_return(result) + + processor = stub("processor", :execute! => result) + BatchApi::Processor.should_receive(:new).with(ops, request.env, hash_including(params)).and_return(processor) xhr :post, :batch, params json = JSON.parse(response.body) From 643a5f6efdb7ac05c3a2f8c318a4d3c72a721578 Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Tue, 14 Aug 2012 11:47:04 +0200 Subject: [PATCH 5/9] Changelog, version bump --- changelog.md | 5 +++++ lib/batch_api/version.rb | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index fb48853..a599612 100644 --- a/changelog.md +++ b/changelog.md @@ -1,5 +1,10 @@ +v0.0.3 +* Encapsulate processing into a Processor module +* Prepare for parallel processing in the future + v0.0.2 * Add config module +* Add options for operation limit, endpoint, and verb v0.0.1 * Initial build diff --git a/lib/batch_api/version.rb b/lib/batch_api/version.rb index 76b317a..3c2f40f 100644 --- a/lib/batch_api/version.rb +++ b/lib/batch_api/version.rb @@ -1,3 +1,3 @@ module BatchApi - VERSION = "0.0.2" + VERSION = "0.0.3" end From 4491c15a9cae66589ca482232a0d52c56ef7928b Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Tue, 14 Aug 2012 13:38:24 +0200 Subject: [PATCH 6/9] Handle Basic Auth properly --- Gemfile.lock | 2 +- lib/batch_api/operation.rb | 7 +++++++ spec/lib/operation_spec.rb | 18 ++++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6dbe841..76c1ceb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - batch_api (0.0.1) + batch_api (0.0.3) rails (~> 3.2) GEM diff --git a/lib/batch_api/operation.rb b/lib/batch_api/operation.rb index 908a27b..59c9697 100644 --- a/lib/batch_api/operation.rb +++ b/lib/batch_api/operation.rb @@ -64,6 +64,13 @@ def process_env # preserve original headers unless explicitly overridden @env.merge!(headrs) + # handle basic auth + # Doorkeeper will accept Authorization as a key for the main request + # but seems to fail if it's not HTTP_AUTHORIZATION in batch operations + if @env["Authorization"] && !@env["HTTP_AUTHORIZATION"] + @env["HTTP_AUTHORIZATION"] = @env["Authorization"] + end + # method @env["REQUEST_METHOD"] = @method.upcase diff --git a/spec/lib/operation_spec.rb b/spec/lib/operation_spec.rb index 8b66a29..6971b3b 100644 --- a/spec/lib/operation_spec.rb +++ b/spec/lib/operation_spec.rb @@ -144,6 +144,24 @@ processed_env[key].should == op_params[:params] end + context "authorization" do + it "copies the basic auth header if the latter isn't present" do + stubby = stub + operation.env["Authorization"] = stubby + operation.env["HTTP_AUTHORIZATION"] = nil + operation.process_env + operation.env["HTTP_AUTHORIZATION"].should == stubby + end + + it "doesn't overwrite basic auth header if the latter is present" do + stubby = stub + operation.env["Authorization"] = stubby + operation.env["HTTP_AUTHORIZATION"] = stub + operation.process_env + operation.env["HTTP_AUTHORIZATION"].should_not == stubby + end + end + context "query_hash" do it "sets it to params for a GET" do operation.method = "get" From f41211ea1b375d0b5bcc0434b374191df82dbac5 Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Tue, 14 Aug 2012 14:12:16 +0200 Subject: [PATCH 7/9] Validate inputs --- lib/batch_api/operation.rb | 6 ++++++ lib/batch_api/processor.rb | 3 +++ spec/lib/operation_spec.rb | 17 +++++++++++++++++ spec/lib/processor_spec.rb | 5 +++++ 4 files changed, 31 insertions(+) diff --git a/lib/batch_api/operation.rb b/lib/batch_api/operation.rb index 59c9697..36e5b97 100644 --- a/lib/batch_api/operation.rb +++ b/lib/batch_api/operation.rb @@ -3,6 +3,8 @@ module BatchApi # Public: an individual batch operation. class Operation + class MalformedOperationError < ArgumentError; end + attr_accessor :method, :url, :params, :headers attr_accessor :env, :result @@ -17,6 +19,10 @@ def initialize(op, base_env) @params = op[:params] @headers = op[:headers] + raise MalformedOperationError, + "BatchAPI operation must include method (received #{@method.inspect}) " + + "and url (received #{@url.inspect})" unless @method && @url + # deep_dup to avoid unwanted changes across requests @env = base_env.deep_dup end diff --git a/lib/batch_api/processor.rb b/lib/batch_api/processor.rb index ea1b902..31bc94b 100644 --- a/lib/batch_api/processor.rb +++ b/lib/batch_api/processor.rb @@ -18,9 +18,12 @@ class BadOptionError < StandardError; end # Raises OperationLimitExceeded if more operations are requested than # allowed by the BatchApi configuration. # Raises BadOptionError if other provided options are invalid. + # Raises ArgumentError if no operations are provided (nil or []). # # Returns the new Processor instance. def initialize(ops, env, options = {}) + raise ArgumentError, "No operations provided" if ops.blank? + @env = env @ops = self.process_ops(ops) @options = self.process_options(options) diff --git a/spec/lib/operation_spec.rb b/spec/lib/operation_spec.rb index 6971b3b..ce4858a 100644 --- a/spec/lib/operation_spec.rb +++ b/spec/lib/operation_spec.rb @@ -45,6 +45,23 @@ end end end + + it "raises an MalformedOperationError if method or URL are missing" do + no_method = op_params.dup.tap {|o| o.delete(:method) } + expect { + BatchApi::Operation.new(no_method, env) + }.to raise_exception(BatchApi::Operation::MalformedOperationError) + + no_url = op_params.dup.tap {|o| o.delete(:url) } + expect { + BatchApi::Operation.new(no_url, env) + }.to raise_exception(BatchApi::Operation::MalformedOperationError) + + nothing = op_params.dup.tap {|o| o.delete(:url); o.delete(:method) } + expect { + BatchApi::Operation.new(nothing, env) + }.to raise_exception(BatchApi::Operation::MalformedOperationError) + end end describe "#identify_routing" do diff --git a/spec/lib/processor_spec.rb b/spec/lib/processor_spec.rb index 5fd7c3e..6ee67dd 100644 --- a/spec/lib/processor_spec.rb +++ b/spec/lib/processor_spec.rb @@ -41,6 +41,11 @@ ops = (BatchApi.config.limit + 1).times.collect {|i| i} expect { BatchApi::Processor.new(ops, env, options) }.to raise_exception(BatchApi::Processor::OperationLimitExceeded) end + + it "raises an ArgumentError if operations.blank?" do + expect { BatchApi::Processor.new(nil, env, options) }.to raise_exception(ArgumentError) + expect { BatchApi::Processor.new(nil, env, []) }.to raise_exception(ArgumentError) + end end describe "#strategy" do From f2984d93480ada2777ef389dc1b590cb74d5af5d Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Tue, 14 Aug 2012 14:24:46 +0200 Subject: [PATCH 8/9] Allow routing to custom controllers (for subclassing) --- changelog.md | 2 ++ lib/batch_api/routing_helper.rb | 4 +++- spec/dummy/config/routes.rb | 1 + spec/routing/batch_api_routing_spec.rb | 5 +++++ 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index a599612..399e327 100644 --- a/changelog.md +++ b/changelog.md @@ -1,6 +1,8 @@ v0.0.3 * Encapsulate processing into a Processor module * Prepare for parallel processing in the future +* Add specific errors +* Allow controlling the routing target v0.0.2 * Add config module diff --git a/lib/batch_api/routing_helper.rb b/lib/batch_api/routing_helper.rb index dfc0f94..62ca7cf 100644 --- a/lib/batch_api/routing_helper.rb +++ b/lib/batch_api/routing_helper.rb @@ -2,11 +2,13 @@ module BatchApi module RoutingHelper DEFAULT_VERB = :post DEFAULT_ENDPOINT = "/batch" + DEFAULT_TARGET = "batch_api/batch#batch" def batch_api(options = {}) endpoint = options.delete(:endpoint) || DEFAULT_ENDPOINT verb = options.delete(:via) || DEFAULT_VERB - match({endpoint => "batch_api/batch#batch", via: verb}.merge(options)) + target = options.delete(:target) || DEFAULT_TARGET + match({endpoint => target, via: verb}.merge(options)) end end end diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index d6c8152..b545e6b 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -3,6 +3,7 @@ # Test the Batch API in two formats: default and custom batch_api batch_api endpoint: "/foo/bar", via: "report" + batch_api endpoint: "/foo/baz", target: "endpoints#get" post "/endpoint" => "endpoints#post" get "/endpoint" => "endpoints#get" diff --git a/spec/routing/batch_api_routing_spec.rb b/spec/routing/batch_api_routing_spec.rb index 6da11b5..b37fb5c 100644 --- a/spec/routing/batch_api_routing_spec.rb +++ b/spec/routing/batch_api_routing_spec.rb @@ -9,5 +9,10 @@ it "draws the route using custom values" do expect("report" => "/foo/bar").to route_to("batch_api/batch#batch") end + + it "draws the routes using a custom target" do + # almost like a generic router! + expect("post" => "/foo/baz").to route_to("endpoints#get") + end end From 4320233afa6c1c64c89e9bc0a69a6719d02fcba8 Mon Sep 17 00:00:00 2001 From: Alex Koppel Date: Tue, 14 Aug 2012 16:11:02 +0200 Subject: [PATCH 9/9] Remove unnecessary authorization stuff --- lib/batch_api/operation.rb | 7 ------- spec/lib/operation_spec.rb | 18 ------------------ 2 files changed, 25 deletions(-) diff --git a/lib/batch_api/operation.rb b/lib/batch_api/operation.rb index 36e5b97..c667b58 100644 --- a/lib/batch_api/operation.rb +++ b/lib/batch_api/operation.rb @@ -70,13 +70,6 @@ def process_env # preserve original headers unless explicitly overridden @env.merge!(headrs) - # handle basic auth - # Doorkeeper will accept Authorization as a key for the main request - # but seems to fail if it's not HTTP_AUTHORIZATION in batch operations - if @env["Authorization"] && !@env["HTTP_AUTHORIZATION"] - @env["HTTP_AUTHORIZATION"] = @env["Authorization"] - end - # method @env["REQUEST_METHOD"] = @method.upcase diff --git a/spec/lib/operation_spec.rb b/spec/lib/operation_spec.rb index ce4858a..fd0e265 100644 --- a/spec/lib/operation_spec.rb +++ b/spec/lib/operation_spec.rb @@ -161,24 +161,6 @@ processed_env[key].should == op_params[:params] end - context "authorization" do - it "copies the basic auth header if the latter isn't present" do - stubby = stub - operation.env["Authorization"] = stubby - operation.env["HTTP_AUTHORIZATION"] = nil - operation.process_env - operation.env["HTTP_AUTHORIZATION"].should == stubby - end - - it "doesn't overwrite basic auth header if the latter is present" do - stubby = stub - operation.env["Authorization"] = stubby - operation.env["HTTP_AUTHORIZATION"] = stub - operation.process_env - operation.env["HTTP_AUTHORIZATION"].should_not == stubby - end - end - context "query_hash" do it "sets it to params for a GET" do operation.method = "get"