Monkey patch gruf interceptor #17
Conversation
grpc-rest.gemspec
Outdated
| @@ -20,6 +20,7 @@ Gem::Specification.new do |spec| | |||
|
|
|||
| spec.add_runtime_dependency('grpc') | |||
| spec.add_runtime_dependency('rails', '>= 6.0') | |||
| spec.add_runtime_dependency('gruf') | |||
There was a problem hiding this comment.
gruf doesn't need to be a runtime dependency. I forgot that I didn't want to enforce this.
There was a problem hiding this comment.
Ah okay, sounds good. Just wondering, what is the rationale for not including it? I assumed that since we were explicitly using Gruf::Interceptors::ServerInterceptor in the code we'd need it as a dependency.
There was a problem hiding this comment.
That's only in the send_gruf_request method, which only gets used if Gruf is loaded.
There was a problem hiding this comment.
Do you think adding the ErrorInterceptor you made by default into grpc-rest makes sense? Or is it specific to the publication stuff?
I feel like it might be too domain-specific to include in grpc-rest, but I am not sure. Maybe it would be best to include the error interceptor as an example in README.md? What do you think?
There was a problem hiding this comment.
Nah, I think the example we have is fine. I forgot it had some ActiveRecord etc stuff in there.
lib/grpc_rest.rb
Outdated
| @@ -1,6 +1,7 @@ | |||
| require 'google/protobuf/well_known_types' | |||
| require 'grpc' | |||
| require 'grpc/core/status_codes' | |||
| require 'interceptors/base_interceptor' | |||
There was a problem hiding this comment.
You can remove this line - you can add to the readme that if you want to make an interceptor you have to require this before you do it.
| @@ -17,6 +17,7 @@ class GrpcApp < Rails::Application | |||
| loader.ignore("#{Rails.root}/spec/test_service_pb.rb") | |||
| loader.setup | |||
| require "#{Rails.root}/spec/test_service_pb.rb" | |||
| require "#{Rails.root}/lib/base_interceptor.rb" | |||
There was a problem hiding this comment.
Not sure if this was the right way of fixing it, but I had to include this so my tests could properly run on the CI.
lib/base_interceptor.rb
Outdated
| # This is a monkey-patch that fixes an issue we were having with using the fail! method in | ||
| # interceptors where an active call was not instantiated yet. | ||
| # Basically, we overloaded this function: https://github.com/bigcommerce/gruf/blob/main/lib/gruf/errors/helpers.rb#L34 |
There was a problem hiding this comment.
I'd change this comment to:
Fixes an issue with the fail! method since the active call is not instantiated yet.
Overloads https://github.com/bigcommerce/gruf/blob/main/lib/gruf/errors/helpers.rb#L34
There was a problem hiding this comment.
Makes sense, less filler words. 👍
fail!in server interceptors when using grpc-rest.