Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions lib/gruf/autoloaders.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,17 @@ class << self
include Enumerable

##
# Initialize the autoloaders with a given controllers path
# Initialize the autoloaders with given controllers paths
#
# @param [String] controllers_path The path to Gruf Controllers
# @param [Array<String>] controllers_paths The path to Gruf Controllers
#
def load!(controllers_path:)
controllers(controllers_path: controllers_path)
def load!(controllers_path: nil, controllers_paths: [])
if controllers_path
::Gruf.logger.warn('controllers_path is deprecated. Please use controllers_paths instead.')
controllers_paths = [controllers_path]
end
controllers(controllers_paths: controllers_paths)
Comment on lines +27 to +37
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @splittingred 👋🏽

this is the change you requested in #213 (comment)

I have updated this accordingly. Let me know if you would like any other change

end

##
Expand All @@ -52,9 +57,10 @@ def reload
# @return [::Gruf::Controllers::Autoloader]
#
# rubocop:disable ThreadSafety/ClassInstanceVariable
def controllers(controllers_path: nil)
def controllers(controllers_paths: [])
controllers_paths = ::Gruf.controllers_paths if controllers_paths.empty?
controllers_mutex do
@controllers ||= ::Gruf::Controllers::Autoloader.new(path: controllers_path || ::Gruf.controllers_path)
@controllers ||= ::Gruf::Controllers::Autoloader.new(paths: controllers_paths)
end
end
# rubocop:enable ThreadSafety/ClassInstanceVariable
Expand Down
2 changes: 1 addition & 1 deletion lib/gruf/cli/executor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ def parse_options
#
def register_services!
# wait to load controllers until last possible second to allow late configuration
::Gruf.autoloaders.load!(controllers_path: Gruf.controllers_path)
::Gruf.autoloaders.load!(controllers_paths: Gruf.controllers_paths)

services = determine_services(@services)
services = bind_health_check!(services) if health_check_enabled?
Expand Down
2 changes: 1 addition & 1 deletion lib/gruf/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ def rpc_desc(request_method)
#
def request_object(request_method, params = {})
desc = rpc_desc(request_method)
desc&.input ? desc.input.new(params) : nil
desc&.input&.new(params)
end

##
Expand Down
4 changes: 4 additions & 0 deletions lib/gruf/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ module Configuration
# @return [String] If use_ssl is true, the relative path from the root_path to the key file for the server
# @!attribute controllers_path
# @return [String] The relative path from root_path to locate Gruf Controllers in
# @!attribute controllers_paths
# @return [Array<String>] The relative paths from root_path to locate Gruf Controllers in
# @!attribute services
# @return [Array<Class>] An array of services to serve with this Gruf server
# @!attribute logger
Expand Down Expand Up @@ -96,6 +98,7 @@ module Configuration
ssl_crt_file: '',
ssl_key_file: '',
controllers_path: '',
controllers_paths: [],
services: [],
logger: nil,
grpc_logger: nil,
Expand Down Expand Up @@ -176,6 +179,7 @@ def reset
self.ssl_key_file = "#{root_path}config/ssl/#{environment}.key"
cp = ::ENV.fetch('GRUF_CONTROLLERS_PATH', 'app/rpc').to_s
self.controllers_path = root_path.to_s.empty? ? cp : "#{root_path}/#{cp}"
self.controllers_paths = [controllers_path]
self.backtrace_on_error = ::ENV.fetch('GRPC_BACKTRACE_ON_ERROR', 0).to_i.positive?
self.rpc_server_options = {
max_waiting_requests: ::ENV.fetch('GRPC_SERVER_MAX_WAITING_REQUESTS',
Expand Down
23 changes: 13 additions & 10 deletions lib/gruf/controllers/autoloader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,24 @@
module Gruf
module Controllers
##
# Handles autoloading of Gruf controllers in the application path. This allows for code reloading on Gruf
# Handles autoloading of Gruf controllers in the application paths. This allows for code reloading on Gruf
# controllers.
#
class Autoloader
include ::Gruf::Loggable

# @!attribute [r] path
# @return [String] The path for this autoloader
attr_reader :path
# @!attribute [r] paths
# @return [Array<String>] The paths for this autoloader
attr_reader :paths

##
# @param [String] path
# @param [Array<String>] paths
# @param [Boolean] reloading
# @param [String] tag
#
def initialize(path:, reloading: nil, tag: nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change; can we avoid this by just checking to see if the passed variable is an Enumerable, or a String, and handle it accordingly? That would satisfy the requirement without breaking the public contract.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can leave the path keyword arg and additionally support paths. If both are passed then it would raise, otherwise path will be just wrapped in an array and set @paths

      def initialize(path: nil, paths: [], reloading: nil, tag: nil)
        raise ArgumentError(..) unless (path.nil? ^ paths.empty?)

        @paths = paths.empty? ? [path] : paths

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine - we just need to make sure there's a backward-compatible deprecation path.

def initialize(paths:, reloading: nil, tag: nil)
super()
@path = path
@paths = paths
@loader = ::Zeitwerk::Loader.new
@loader.tag = tag || 'gruf-controllers'
@setup = false
Expand Down Expand Up @@ -73,11 +73,14 @@ def with_fresh_controller(controller_name)
def setup!
return true if @setup

return false unless File.directory?(@path)
directories = @paths.select { |path| File.directory?(path) }
return false unless directories.any?

@loader.enable_reloading if @reloading_enabled
@loader.ignore("#{@path}/**/*_pb.rb")
@loader.push_dir(@path)
directories.each do |path|
@loader.ignore("#{path}/**/*_pb.rb")
@loader.push_dir(path)
end
@loader.setup
# always eager load RPC files, so that the service binder can bind the Gruf::Controller instantiation
# to the gRPC Service classes
Expand Down
2 changes: 1 addition & 1 deletion lib/gruf/hooks/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def insert_after(after_class, hook_class, options = {})
raise HookNotFoundError if pos.nil?

@registry.insert(
(pos + 1),
pos + 1,
klass: hook_class,
options: options
)
Expand Down
4 changes: 2 additions & 2 deletions lib/gruf/integrations/rails/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ class Railtie < ::Rails::Railtie
config.before_configuration do
# Remove autoloading of the controllers path from Rails' zeitwerk, so that we ensure Gruf's zeitwerk
# properly manages them itself. This allows us to manage code reloading and logging in Gruf specifically
app.config.eager_load_paths -= [::Gruf.controllers_path] if app.config.respond_to?(:eager_load_paths)
app.config.eager_load_paths -= ::Gruf.controllers_paths if app.config.respond_to?(:eager_load_paths)
if ::Rails.respond_to?(:autoloaders) # if we're on a late enough version of rails
::Rails.autoloaders.each do |autoloader|
autoloader.ignore(Gruf.controllers_path)
autoloader.ignore(Gruf.controllers_paths)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/gruf/interceptors/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def insert_after(after_class, interceptor_class, options = {})
raise InterceptorNotFoundError if pos.nil?

@registry.insert(
(pos + 1),
pos + 1,
klass: interceptor_class,
options: options
)
Expand Down
7 changes: 7 additions & 0 deletions lib/gruf/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,13 @@ def controllers_path
options.fetch(:controllers_path, Gruf.controllers_path)
end

##
# @param [Array<String>]
#
def controllers_paths
options.fetch(:controllers_paths, Gruf.controllers_paths)
end

##
# Load the SSL/TLS credentials for this server
#
Expand Down
22 changes: 19 additions & 3 deletions spec/gruf/autoloaders_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,28 @@
let(:controllers_path) { 'spec/pb' }

describe '#load!' do
subject { autoloaders.load!(controllers_path: controllers_path) }
subject { autoloaders.load!(controllers_paths: [controllers_path]) }

it 'creates a controller autoloader for the passed path' do
subject
expect(autoloaders.controllers).to be_a(::Gruf::Controllers::Autoloader)
expect(autoloaders.controllers.path).to eq controllers_path
expect(autoloaders.controllers.paths).to eq [controllers_path]
end

context 'when deprecated controllers_path is passed' do
subject { autoloaders.load!(controllers_path: controllers_path) }

it 'creates a controller autoloader for the passed path' do
subject
expect(autoloaders.controllers).to be_a(::Gruf::Controllers::Autoloader)
expect(autoloaders.controllers.paths).to eq [controllers_path]
end

it 'logs a deprecation warning' do
expect(::Gruf.logger)
.to receive(:warn).with('controllers_path is deprecated. Please use controllers_paths instead.')
subject
end
end
end

Expand All @@ -41,7 +57,7 @@
subject { autoloaders.reload }

before do
autoloaders.load!(controllers_path: controllers_path)
autoloaders.load!(controllers_paths: [controllers_path])
end

it 'runs reload on all autoloaders' do
Expand Down
2 changes: 1 addition & 1 deletion spec/gruf/controllers/autoloader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
let(:path) { 'spec/pb' }
let(:reloading) { nil }
let(:tag) { 'gruf-controllers' }
let(:autoloader) { described_class.new(path: path, reloading: reloading) }
let(:autoloader) { described_class.new(paths: [path], reloading: reloading) }
let(:zeitwerk) do
instance_spy(::Zeitwerk::Loader, setup: true, 'tag=': tag, tag: tag, ignore: true, push_dir: true, eager_load: true)
end
Expand Down