Merge pull request #167 from localshred/encode_everything

Handle encoding/coercing/validation in the encoder

lib/protobuf/rpc/middleware/response_encoder.rb

@@ -11,11 +11,10 @@ def initialize(app)
         end
 
         def call(env)
-          @env = env
-
-          env = app.call(env)
-          env.encoded_response = encode_response_data(env.response)
+          @env = app.call(env)
 
+          env.response = response
+          env.encoded_response = encoded_response
           env
         end
 
@@ -27,11 +26,10 @@ def log_signature
 
         # Encode the response wrapper to return to the client
         #
-        def encode_response_data(response)
+        def encoded_response
           log_debug { sign_message("Encoding response: #{response.inspect}") }
 
-          response = wrap_response(response)
-          env.encoded_response = Socketrpc::Response.encode(response)
+          env.encoded_response = wrapped_response.encode
         rescue => exception
           log_exception(exception)
 
@@ -40,14 +38,48 @@ def encode_response_data(response)
           raise PbError.new(exception.message)
         end
 
+        # Prod the object to see if we can produce a proto object as a response
+        # candidate. Validate the candidate protos.
+        def response
+          @response ||= begin
+                          candidate = env.response
+                          case
+                          when candidate.is_a?(Message) then
+                            validate!(candidate)
+                          when candidate.respond_to?(:to_proto) then
+                            validate!(candidate.to_proto)
+                          when candidate.respond_to?(:to_hash) then
+                            env.response_type.new(candidate.to_hash)
+                          when candidate.is_a?(PbError) then
+                            candidate
+                          else
+                            validate!(candidate)
+                          end
+                        end
+        end
+
+        # Ensure that the response candidate we've been given is of the type
+        # we expect so that deserialization on the client side works.
+        #
+        def validate!(candidate)
+          actual = candidate.class
+          expected = env.response_type
+
+          if expected != actual
+            raise BadResponseProto.new("Expected response to be of type #{expected.name} but was #{actual.name}")
+          end
+
+          candidate
+        end
+
         # The middleware stack returns either an error or response proto. Package
         # it up so that it's in the correct spot in the response wrapper
         #
-        def wrap_response(response)
+        def wrapped_response
           if response.is_a?(Protobuf::Rpc::PbError)
-            { :error => response.message, :error_reason => response.error_type }
+            Socketrpc::Response.new(:error => response.message, :error_reason => response.error_type)
           else
-            { :response_proto => response.encode }
+            Socketrpc::Response.new(:response_proto => response.encode)
           end
         end
       end

lib/protobuf/rpc/service.rb

@@ -5,7 +5,6 @@
 
 module Protobuf
   module Rpc
-
     # Object to encapsulate the request/response types for a given service method
     #
     RpcMethod = Struct.new("RpcMethod", :method, :request_type, :response_type)
@@ -132,12 +131,6 @@ def callable_rpc_method(method_name)
         lambda { run_filters(method_name) }
       end
 
-      # Register a failure callback for use when rpc_failed is invoked.
-      #
-      def on_rpc_failed(callable)
-        @_rpc_failed_callback ||= callable
-      end
-
       # Response object for this rpc cycle. Not assignable.
       #
       def response
@@ -156,11 +149,7 @@ def rpcs
         self.class.rpcs
       end
 
-      private
-
-      def response_type
-        @_response_type ||= rpcs[@method_name].response_type
-      end
+    private
 
       def request_type
         @_request_type ||= rpcs[@method_name].request_type
@@ -175,10 +164,15 @@ def respond_with(candidate)
       end
       alias_method :return_from_whence_you_came, :respond_with
 
+      def response_type
+        @_response_type ||= rpcs[@method_name].response_type
+      end
+
       # Automatically fail a service method.
       #
       def rpc_failed(message)
-        @_rpc_failed_callback.call(message)
+        message = message.message if message.respond_to?(:message)
+        raise RpcFailed.new(message)
       end
     end
 

lib/protobuf/rpc/service_dispatcher.rb

@@ -14,8 +14,6 @@ def initialize(app)
       def call(env)
         @env = env
 
-        register_rpc_failed
-
         env.response = dispatch_rpc_request
         env
       end
@@ -26,76 +24,20 @@ def rpc_service
 
     private
 
-      # Get a callable RPC method for the current request.
-      #
-      def callable_rpc_method
-        unless rpc_service.respond_to?(method_name)
-          raise MethodNotFound.new("#{rpc_service.class.name}##{method_name} is not implemented.")
-        end
-
-        rpc_service.callable_rpc_method(method_name)
-      end
-
-      # Prod the object to see if we can produce a proto object as a response
-      # candidate. Either way, return the candidate for validation.
-      def coerced_response
-        candidate = rpc_service.response
-
-        case
-        when candidate.is_a?(::Protobuf::Message) then
-          # no-op
-        when candidate.respond_to?(:to_proto) then
-          candidate = candidate.to_proto
-        when candidate.respond_to?(:to_proto_hash) then
-          candidate = env.response_type.new(candidate.to_proto_hash)
-        when candidate.respond_to?(:to_hash) then
-          candidate = env.response_type.new(candidate.to_hash)
-        end
-
-        candidate
-      end
-
+      # Call the given service method.
       def dispatch_rpc_request
-        # Call the given service method.
-        callable_rpc_method.call
-        validate_response!
+        rpc_service.callable_rpc_method(method_name).call
+        rpc_service.response
+      rescue NoMethodError
+        raise MethodNotFound.new("#{service_name}##{method_name} is not implemented.")
       end
 
       def method_name
         env.method_name
       end
 
-      # Make sure we get rpc errors back.
-      #
-      def register_rpc_failed
-        rpc_service.on_rpc_failed(method(:rpc_failed_callback))
-      end
-
-      # Receive the failure message from the service. This method is registered
-      # as the callable to the service when an `rpc_failed` call is invoked.
-      #
-      def rpc_failed_callback(message)
-        message = message.message if message.respond_to?(:message)
-        raise RpcFailed.new(message)
-      end
-
-      def rpc_method
-        env.rpc_method
-      end
-
-      # Ensure that the response candidate we've been given is of the type
-      # we expect so that deserialization on the client side works.
-      #
-      def validate_response!
-        candidate = coerced_response
-        actual = candidate.class
-        expected = env.response_type
-
-        if expected != actual
-          raise BadResponseProto.new("Response proto changed from #{expected.name} to #{actual.name}")
-        end
-
-        candidate
+      def service_name
+        env.service_name
       end
     end
   end

spec/lib/protobuf/rpc/middleware/response_encoder_spec.rb

@@ -1,20 +1,16 @@
 require 'spec_helper'
 
 describe Protobuf::Rpc::Middleware::ResponseEncoder do
-  let(:app) { Proc.new { |env| env } }
+  let(:app) { Proc.new { |env| env.response = response; env } }
   let(:env) {
     Protobuf::Rpc::Env.new(
-      'response' => response_proto,
+      'response_type' => Test::Resource,
       'log_signature' => 'log_signature'
     )
   }
-  let(:encoded_response) { response.encode }
-  let(:response) {
-    Protobuf::Socketrpc::Response.new(
-      :response_proto => response_proto
-    )
-  }
-  let(:response_proto) { Test::Resource.new(:name => 'required') }
+  let(:encoded_response) { response_wrapper.encode }
+  let(:response) { Test::Resource.new(:name => 'required') }
+  let(:response_wrapper) { Protobuf::Socketrpc::Response.new(:response_proto => response) }
 
   subject { described_class.new(app) }
 
@@ -25,29 +21,51 @@
     end
 
     it "calls the stack" do
-      app.should_receive(:call).with(env).and_return(env)
-      subject.call(env)
+      stack_env = subject.call(env)
+      stack_env.response.should eq response
+    end
+
+    context "when response is responds to :to_hash" do
+      let(:app) { proc { |env| env.response = hashable; env } }
+      let(:hashable) { double('hashable', :to_hash => response.to_hash) }
+
+      it "sets Env#response" do
+        stack_env = subject.call(env)
+        stack_env.response.should eq response
+      end
+    end
+
+    context "when response is responds to :to_proto" do
+      let(:app) { proc { |env| env.response = protoable; env } }
+      let(:protoable) { double('protoable', :to_proto => response) }
+
+      it "sets Env#response" do
+        stack_env = subject.call(env)
+        stack_env.response.should eq response
+      end
+    end
+
+    context "when response is not a valid response type" do
+      let(:app) { proc { |env| env.response = "I'm not a valid response"; env } }
+
+      it "raises a bad response proto exception" do
+        expect { subject.call(env) }.to raise_exception(Protobuf::Rpc::BadResponseProto)
+      end
     end
 
     context "when response is a Protobuf error" do
-      let(:encoded_response) { response.encode }
-      let(:env) {
-        Protobuf::Rpc::Env.new(
-          'response' => error,
-          'log_signature' => 'log_signature'
-        )
-      }
+      let(:app) { proc { |env| env.response = error; env } }
       let(:error) { Protobuf::Rpc::RpcError.new }
-      let(:response) { error.to_response }
+      let(:response_wrapper) { error.to_response }
 
-      it "encodes the response" do
+      it "wraps and encodes the response" do
         stack_env = subject.call(env)
         stack_env.encoded_response.should eq encoded_response
       end
     end
 
     context "when encoding fails" do
-      before { Protobuf::Socketrpc::Response.stub(:encode).and_raise(RuntimeError) }
+      before { Protobuf::Socketrpc::Response.any_instance.stub(:encode).and_raise(RuntimeError) }
 
       it "raises a bad request data exception" do
         expect { subject.call(env) }.to raise_exception(Protobuf::Rpc::PbError)