fix: Support 0 length messages (#34)

* Support 0 length messages, which are valid * Update contributing.md * Add specs * linter Co-authored-by: James Shkolnik <[email protected]>
Gusto · Aug 26, 2020 · 4486cbe · 4486cbe
1 parent 492e303
commit 4486cbe
Show file tree

Hide file tree

Showing 11 changed files with 171 additions and 33 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -8,8 +8,75 @@ Use the `rake` command to run the entire test suite. This runs on docker due to
 
 Unit tests can be run without docker by running `rspec spec/unit`
 
-### Commit message format
+### Pull Request title/description format
 
-Please follow semantic release formatting
-https://semantic-release.gitbook.io/semantic-release/#commit-message-format
-https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#type
+Each Pull Request description consists of a **header**, a **body** and a **footer**. The header has a special format that includes a **type**, a **scope** and a **subject**:
+
+```commit
+<type>(<scope>): <subject>
+<BLANK LINE>
+<body>
+<BLANK LINE>
+<footer>
+```
+
+The **header** is mandatory and the **scope** of the header is optional.
+
+The **footer** can contain a [closing reference to an issue](https://help.github.com/articles/closing-issues-via-commit-messages).
+
+#### Revert
+
+If the commit reverts a previous commit, it should begin with `revert: `, followed by the header of the reverted commit. In the body it should say: `This reverts commit <hash>.`, where the hash is the SHA of the commit being reverted.
+
+#### Type
+
+The type must be one of the following:
+
+| Type         | Description                                                                                                 |
+|--------------|-------------------------------------------------------------------------------------------------------------|
+| **build**    | Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)         |
+| **ci**       | Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs) |
+| **docs**     | Documentation only changes                                                                                  |
+| **feat**     | A new feature                                                                                               |
+| **fix**      | A bug fix                                                                                                   |
+| **perf**     | A code change that improves performance                                                                     |
+| **refactor** | A code change that neither fixes a bug nor adds a feature                                                   |
+| **style**    | Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)      |
+| **test**     | Adding missing tests or correcting existing tests                                                           |
+| **chore**    | Any other change that does not affect the published code (e.g. tool changes, config changes)                |
+
+#### Subject
+
+The subject contains succinct description of the change:
+
+- use the imperative, present tense: "change" not "changed" nor "changes"
+- no dot (.) at the end
+
+#### Body
+Just as in the **subject**, use the imperative, present tense: "change" not "changed" nor "changes".
+The body should include the motivation for the change and contrast this with previous behavior.
+
+#### Footer
+The footer should contain any information about **Breaking Changes** and is also the place to reference GitHub issues that this commit **Closes**.
+
+**Breaking Changes** should start with the word `BREAKING CHANGE:` with a space or two newlines. The rest of the commit message is then used for this.
+
+#### Examples
+
+```commit
+`fix(pencil): stop graphite breaking when too much pressure applied`
+```
+
+```commit
+`feat(pencil): add 'graphiteWidth' option`
+
+Fix #42
+```
+
+```commit
+perf(pencil): remove graphiteWidth option`
+
+BREAKING CHANGE: The graphiteWidth option has been removed.
+
+The default graphite width of 10mm is always used for performance reasons.
+```
diff --git a/lib/grpc_web/message_framing.rb b/lib/grpc_web/message_framing.rb
@@ -15,10 +15,9 @@ def pack_frames(frames)
     def unpack_frames(content)
       frames = []
       remaining_content = content
+
       until remaining_content.empty?
         msg_length = remaining_content[1..4].unpack1('N')
-        raise 'Invalid message length' if msg_length <= 0
-
         frame_end = 5 + msg_length
         frames << ::GRPCWeb::MessageFrame.new(
           remaining_content[0].bytes[0],

diff --git a/spec/integration/envoy_server_ruby_client_spec.rb b/spec/integration/envoy_server_ruby_client_spec.rb
@@ -63,6 +63,14 @@ def say_hello(_request, _metadata = nil)
     end
   end
 
+  context 'for a method with empty request and response protos' do
+    subject(:response) { client.say_nothing }
+
+    it 'returns the expected response from the service' do
+      expect(response).to eq(EmptyResponse.new)
+    end
+  end
+
   context 'for a network error' do
     let(:client_url) { 'http://envoy:8081' }
 

diff --git a/spec/integration/ruby_server_js_client_spec.rb b/spec/integration/ruby_server_js_client_spec.rb
@@ -14,6 +14,13 @@
   let(:js_script) do
     <<-EOF
     var helloService = new #{js_client_class}('http://#{server.host}:#{server.port}', null, null);
+    #{js_grpc_call}
+    EOF
+  end
+
+  # JS snippet to make the gRPC-Web call
+  let(:js_grpc_call) do
+    <<-EOF
     var x = new HelloRequest();
     x.setName('#{name}');
     window.grpcResponse = null;
@@ -82,6 +89,24 @@ def say_hello(_request, _metadata = nil)
         expect(js_error).to include('code' => 2, 'message' => 'RuntimeError: Some random error')
       end
     end
+
+    context 'for a method with empty request and response protos' do
+      let(:js_grpc_call) do
+        <<-EOF
+        var x = new EmptyRequest();
+        window.grpcResponse = null;
+        helloService.sayNothing(x, {}, function(err, response){
+          window.grpcError = err;
+          window.grpcResponse = response;
+        });
+        EOF
+      end
+
+      it 'returns the expected response from the service' do
+        perform_request
+        expect(response).to eq({})
+      end
+    end
   end
 
   context 'with application/grpc-web-text format' do

diff --git a/spec/integration/ruby_server_nodejs_client_spec.rb b/spec/integration/ruby_server_nodejs_client_spec.rb
@@ -13,16 +13,18 @@
       'node',
       node_client,
       server_url,
-      name,
+      grpc_method,
       basic_username,
       basic_password,
+      name,
     ].join(' ')
   end
   let(:result) { JSON.parse(json_result) }
 
   let(:basic_password) { 'supersecretpassword' }
   let(:basic_username) { 'supermanuser' }
   let(:service) { TestHelloService }
+  let(:grpc_method) { 'SayHello' }
   let(:rack_app) do
     app = TestGRPCWebApp.build(service)
     app.use Rack::Auth::Basic do |username, password|
@@ -70,4 +72,12 @@ def say_hello(_request, _metadata = nil)
       )
     end
   end
+
+  context 'with empty request and response protos' do
+    let(:grpc_method) { 'SayNothing' }
+
+    it 'returns the expected response from the service' do
+      expect(result['response']).to eq({})
+    end
+  end
 end
diff --git a/spec/integration/ruby_server_ruby_client_spec.rb b/spec/integration/ruby_server_ruby_client_spec.rb
@@ -68,6 +68,14 @@ def say_hello(_request, _metadata = nil)
     end
   end
 
+  context 'for a method with empty request and response protos' do
+    subject(:response) { client.say_nothing }
+
+    it 'returns the expected response from the service' do
+      expect(response).to eq(EmptyResponse.new)
+    end
+  end
+
   context 'for a network error' do
     let(:client_url) do
       "http://#{basic_username}:#{basic_password}@#{server.host}:#{server.port + 1}"

diff --git a/spec/js-client-src/client.js b/spec/js-client-src/client.js
@@ -3,7 +3,7 @@
 //
 // It must be compiled with webpack to generate spec/js-client/main.js
 
-const {HelloRequest} = require('pb-grpc-web/hello_pb.js');
+const {HelloRequest, EmptyRequest} = require('pb-grpc-web/hello_pb.js');
 
 // This version of the JS client makes requests as application/grpc-web (binary):
 const HelloClientWeb = require('pb-grpc-web/hello_grpc_web_pb.js');
@@ -15,5 +15,6 @@ const grpc = {};
 grpc.web = require('grpc-web');
 
 window.HelloRequest = HelloRequest;
+window.EmptyRequest = EmptyRequest;
 window.HelloServiceClientWeb = HelloClientWeb.HelloServiceClient;
 window.HelloServiceClientWebText = HelloClientWebText.HelloServiceClient;
diff --git a/spec/node-client/client.ts b/spec/node-client/client.ts
@@ -2,16 +2,17 @@ import { grpc } from "@improbable-eng/grpc-web";
 import { NodeHttpTransport } from "@improbable-eng/grpc-web-node-http-transport";
 
 import {HelloServiceClient} from './pb-ts/hello_pb_service';
-import {HelloRequest} from './pb-ts/hello_pb';
+import {HelloRequest, EmptyRequest} from './pb-ts/hello_pb';
 
 // Required for grpc-web in a NodeJS environment (vs. browser)
 grpc.setDefaultTransport(NodeHttpTransport());
 
 // Usage: node client.js http://server:port nameParam username password
 const serverUrl = process.argv[2];
-const helloName = process.argv[3];
+const grpcMethod = process.argv[3];
 const username = process.argv[4];
 const password = process.argv[5];
+const helloName = process.argv[6];
 
 const client = new HelloServiceClient(serverUrl);
 const headers = new grpc.Metadata();
@@ -21,14 +22,29 @@ if (username && password) {
   headers.set("Authorization", `Basic ${encodedCredentials}`);
 }
 
-const req = new HelloRequest();
-req.setName(helloName);
-
-client.sayHello(req, headers, (err, resp) => {
-  var result = {
-    response: resp && resp.toObject(),
-    error: err && err.metadata && err.metadata.headersMap
-  }
-  // Emit response and/or error as JSON so it can be parsed from Ruby
-  console.log(JSON.stringify(result));
-});
+if (grpcMethod == 'SayHello') {
+  const req = new HelloRequest();
+  req.setName(helloName);
+  client.sayHello(req, headers, (err, resp) => {
+    var result = {
+      response: resp && resp.toObject(),
+      error: err && err.metadata && err.metadata.headersMap
+    }
+    // Emit response and/or error as JSON so it can be parsed from Ruby
+    console.log(JSON.stringify(result));
+  });
+}
+else if (grpcMethod == 'SayNothing') {
+  const req = new EmptyRequest();
+  client.sayNothing(req, headers, (err, resp) => {
+    var result = {
+      response: resp && resp.toObject(),
+      error: err && err.metadata && err.metadata.headersMap
+    }
+    // Emit response and/or error as JSON so it can be parsed from Ruby
+    console.log(JSON.stringify(result));
+  });
+}
+else {
+  console.log(`Unknown gRPC method ${grpcMethod}`);
+}
diff --git a/spec/pb-src/hello.proto b/spec/pb-src/hello.proto
@@ -8,6 +8,15 @@ message HelloResponse {
   string message = 1;
 }
 
+message EmptyRequest {
+
+}
+
+message EmptyResponse {
+
+}
+
 service HelloService {
-  rpc SayHello(HelloRequest) returns (HelloResponse);
+  rpc SayHello (HelloRequest) returns (HelloResponse);
+  rpc SayNothing (EmptyRequest) returns (EmptyResponse);
 }
diff --git a/spec/support/test_hello_service.rb b/spec/support/test_hello_service.rb
@@ -6,4 +6,8 @@ class TestHelloService < HelloService::Service
   def say_hello(request, _call = nil)
     HelloResponse.new(message: "Hello #{request.name}")
   end
+
+  def say_nothing(_request, _call = nil)
+    EmptyResponse.new
+  end
 end
diff --git a/spec/unit/grpc_web/message_framing_spec.rb b/spec/unit/grpc_web/message_framing_spec.rb
@@ -7,12 +7,14 @@
     [
       ::GRPCWeb::MessageFrame.header_frame('data in the header'),
       ::GRPCWeb::MessageFrame.payload_frame("data in the \u1f61d first frame"),
+      ::GRPCWeb::MessageFrame.payload_frame(''), # 0 length frame
       ::GRPCWeb::MessageFrame.payload_frame('data in the second frame'),
     ]
   end
   let(:packed_frames) do
     string = "\x80\x00\x00\x00\x12data in the header" \
              "\x00\x00\x00\x00\x1Cdata in the \u1f61d first frame" \
+             "\x00\x00\x00\x00\x00" + # 0 length frame
              "\x00\x00\x00\x00\x18data in the second frame"
     string.b # treat string as a byte string
   end
@@ -27,17 +29,6 @@
     subject(:unpack) { described_class.unpack_frames(packed_frames) }
 
     it { is_expected.to eq unpacked_frames }
-
-    context 'when the message length is invalid' do
-      let(:packed_frames) do
-        string = "\x80\x00\x00\x00\x00data in the header"
-        string.b # treat string as a byte string
-      end
-
-      it 'raises an error' do
-        expect { unpack }.to raise_error(StandardError, 'Invalid message length')
-      end
-    end
   end
 
   describe 'pack and unpack frames' do