Add Ability to retrieve metadata from the server #1

Matthewaj · 2023-03-24T01:33:28Z

Added the ability to retrieve metadata in requests.

# Conflicts: # Rakefile

HeavenFox · 2023-03-24T02:29:58Z

Thanks Matt!
Some general comments:

Our goal is to get the PR merged upstream, so we should avoid controversial changes. I wonder if the Docker and build changes are absolutely necessary.
For the same reason, breaking backwards compatibility would be a big no-no. Looks like we can use the arity method to look up how many arguments a method take, and call it accordingly: https://ruby-doc.org/core-2.0.0/Method.html#method-i-arity
We should maintain consistency with the official GRPC library as much as possible. This makes it easy to, in the future, move these RPC calls to be handled directly by GRPC rather than via GRPC-web. In the official implementation, the metadata is passed in _call argument (see https://grpc.io/docs/languages/ruby/basics/) Looks like it's defined here https://github.com/grpc/grpc/blob/master/src/ruby/lib/grpc/generic/active_call.rb .

Matthewaj · 2023-03-24T06:31:29Z

Thanks Matt! Some general comments:

Our goal is to get the PR merged upstream, so we should avoid controversial changes. I wonder if the Docker and build changes are absolutely necessary.

For the same reason, breaking backwards compatibility would be a big no-no. Looks like we can use the arity method to look up how many arguments a method take, and call it accordingly: https://ruby-doc.org/core-2.0.0/Method.html#method-i-arity

We should maintain consistency with the official GRPC library as much as possible. This makes it easy to, in the future, move these RPC calls to be handled directly by GRPC rather than via GRPC-web. In the official implementation, the metadata is passed in _call argument (see https://grpc.io/docs/languages/ruby/basics/) Looks like it's defined here https://github.com/grpc/grpc/blob/master/src/ruby/lib/grpc/generic/active_call.rb .

Our goal is to get the PR merged upstream, so we should avoid controversial changes. I wonder if the Docker and build changes are absolutely necessary.

The additional packages I added libuvalkan1 and libu2f-udev were added because chrome seems to require them now.

#6 26.84  google-chrome-stable depends on libvulkan1; however:
#6 26.84   Package libvulkan1 is not installed.

#6 26.84  google-chrome-stable depends on libu2f-udev; however:
#6 26.84   Package libu2f-udev is not installed.

The same thing happens with Ruby as well.

#13 3.266       The last version of bundler (>= 0) to support your Ruby & RubyGems was 2.3.26. Try installing it with `gem install bundler -v 2.3.26`
#13 3.266       bundler requires Ruby version >= 2.6.0. The current ruby version is 2.5.7.206.

Installing bundler 2.3.26 didn't seem to resolve the issue either, and upgrading to > 2.6.0 seemed to resolve the issue with the least friction.

For the same reason, breaking backwards compatibility would be a big no-no. Looks like we can use the arity method to look up how many arguments a method take, and call it accordingly: https://ruby-doc.org/core-2.0.0/Method.html#method-i-arity
We should maintain consistency with the official GRPC library as much as possible. This makes it easy to, in the future, move these RPC calls to be handled directly by GRPC rather than via GRPC-web. In the official implementation, the metadata is passed in _call argument (see https://grpc.io/docs/languages/ruby/basics/) Looks like it's defined here https://github.com/grpc/grpc/blob/master/src/ruby/lib/grpc/generic/active_call.rb .

Thanks for the heads up. I'll start working on refactoring it to pass the call argument now.

chimano · 2023-03-31T18:01:43Z

Gemfile.lock

-      capybara (~> 3.12, < 4)
+    addressable (2.8.1)
+      public_suffix (>= 2.0.2, < 6.0)
+    apparition (0.6.0)


Had to update apparition. There was some interaction between the newer versions of chrome that just caused some tests to hang indefinitely.

ivywit

LGTM, just curious about some of the test variables

ivywit · 2023-03-31T18:34:51Z

spec/unit/grpc_web/server/message_serialization_spec.rb

@@ -16,6 +16,7 @@
    let(:service) { TestHelloService.new }
    let(:service_method) { :SayHello }
    let(:accept) { '*/*' }
+    let(:metadata) { {} }


Should this be getting added to the request above? It doesn't seem to be breaking but I figured I'd check as the variable isn't referenced anywhere from what I'm seeing. I might be missing if it's somewhere in a shared test though

Hmm good point I'm not too sure why this was added, I'll be removing it

ivywit · 2023-03-31T18:35:17Z

spec/unit/grpc_web/server/text_coder_spec.rb

@@ -18,6 +18,7 @@
    let(:service) { instance_double(TestHelloService) }
    let(:service_method) { :SayHello }
    let(:accept) { '*/*' }
+    let(:metadata) { {} }


Same as the previous question

Matt James added 6 commits March 22, 2023 17:15

Add Request Metadata

aef6ea9

Fix Misc Issues

7a3c8e2

Start Sending Metadata

bbc9b48

Add Method to get metadata

1b7f1bd

Merge branch 'master' into metadata

c0ffb90

# Conflicts: # Rakefile

Change Way of Getting Metadata

db0ba4b

Matt James and others added 3 commits March 24, 2023 14:34

Change Way of Getting Metadata

e2aa15c

Small Refactor

80c1574

Fixed bugs and failing test cases

3a3df78

chimano force-pushed the metadata branch from 99a5086 to 3a3df78 Compare March 31, 2023 17:59

chimano reviewed Mar 31, 2023

View reviewed changes

ivywit approved these changes Mar 31, 2023

View reviewed changes

Removed unneeded variable in test

4d2b586

chimano changed the title ~~Add Ability to retrieve metadata~~ Add Ability to retrieve metadata from the server Mar 31, 2023

chimano merged commit 9049576 into master Mar 31, 2023

chimano deleted the metadata branch March 31, 2023 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ability to retrieve metadata from the server #1

Add Ability to retrieve metadata from the server #1

Matthewaj commented Mar 24, 2023 •

edited

Loading

HeavenFox commented Mar 24, 2023 •

edited

Loading

Matthewaj commented Mar 24, 2023

chimano Mar 31, 2023

ivywit left a comment

ivywit Mar 31, 2023

chimano Mar 31, 2023

ivywit Mar 31, 2023

chimano Mar 31, 2023

Add Ability to retrieve metadata from the server #1

Add Ability to retrieve metadata from the server #1

Conversation

Matthewaj commented Mar 24, 2023 • edited Loading

HeavenFox commented Mar 24, 2023 • edited Loading

Matthewaj commented Mar 24, 2023

chimano Mar 31, 2023

Choose a reason for hiding this comment

ivywit left a comment

Choose a reason for hiding this comment

ivywit Mar 31, 2023

Choose a reason for hiding this comment

chimano Mar 31, 2023

Choose a reason for hiding this comment

ivywit Mar 31, 2023

Choose a reason for hiding this comment

chimano Mar 31, 2023

Choose a reason for hiding this comment

Matthewaj commented Mar 24, 2023 •

edited

Loading

HeavenFox commented Mar 24, 2023 •

edited

Loading