Add ClientStub, setup/client_for API, and Railtie#10
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Rails integration helpers for rspamd-ruby so Rails apps can auto-configure and obtain per-purpose clients, with a null-object stub when disabled.
Changes:
- Introduces
Rspamd::ClientStub(null client) andRspamd::Rails(client/config management) plus aRspamd::Railtiefor auto-setup. - Adds
activesupportruntime dependency to supportdeep_symbolize_keys. - Adds test coverage for the new stub and Rails helper.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/rspamd/client_stub.rb |
Adds a null-object client for disabled/dev/test usage. |
lib/rspamd/rails.rb |
Adds Rails-facing setup + client lookup/caching. |
lib/rspamd/railtie.rb |
Auto-initializes configuration from config/rspamd.yml in Rails. |
lib/rspamd-ruby.rb |
Wires new files into the gem entrypoint. |
rspamd-ruby.gemspec |
Adds activesupport as a runtime dependency. |
test/client_stub_test.rb |
Tests expected stub behaviors. |
test/rails_test.rb |
Tests Rails helper setup, caching, reset, and stub fallback. |
Gemfile.lock |
Updates lockfile to include new dependency and other dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/rspamd/rails.rb
Outdated
| @config = config.deep_symbolize_keys | ||
| @clients = {} | ||
| end | ||
|
|
||
| def client_for(name) | ||
| return ClientStub.new unless enabled? | ||
|
|
||
| @clients[name] ||= build_client(name) | ||
| end | ||
|
|
||
| def reset! | ||
| @config = nil | ||
| @clients = {} | ||
| end | ||
|
|
||
| private | ||
| def enabled? | ||
| @config&.dig(:enabled) |
There was a problem hiding this comment.
client_for caches clients with @clients[name] ||= ... and setup/reset! mutate @clients/@config without any synchronization. This is not thread-safe under multi-threaded Rails servers (can build multiple clients, or race with reset and raise due to nil config). Consider guarding config/client access with a Mutex (or using a thread-safe map) to ensure only one client is built per name and reads/writes are consistent.
| @config = config.deep_symbolize_keys | |
| @clients = {} | |
| end | |
| def client_for(name) | |
| return ClientStub.new unless enabled? | |
| @clients[name] ||= build_client(name) | |
| end | |
| def reset! | |
| @config = nil | |
| @clients = {} | |
| end | |
| private | |
| def enabled? | |
| @config&.dig(:enabled) | |
| mutex.synchronize do | |
| @config = config.deep_symbolize_keys | |
| @clients = {} | |
| end | |
| end | |
| def client_for(name) | |
| mutex.synchronize do | |
| return ClientStub.new unless @config&.dig(:enabled) | |
| @clients ||= {} | |
| @clients[name] ||= build_client(name) | |
| end | |
| end | |
| def reset! | |
| mutex.synchronize do | |
| @config = nil | |
| @clients = {} | |
| end | |
| end | |
| private | |
| def mutex | |
| @mutex ||= Mutex.new | |
| end | |
| def enabled? | |
| mutex.synchronize do | |
| @config&.dig(:enabled) | |
| end |
| HAM_RESULT = Check::Result.new( | ||
| "score" => 0.0, | ||
| "required_score" => 15.0, | ||
| "action" => "no action", | ||
| "is_skipped" => false, | ||
| "symbols" => {}, | ||
| "urls" => [], | ||
| "emails" => [] | ||
| ).freeze |
There was a problem hiding this comment.
HAM_RESULT is a single shared Check::Result instance returned by check. While the result object is frozen, the nested data hash/arrays (and e.g. symbols) remain mutable, so any consumer mutation will leak across calls/threads. Consider returning a fresh Check::Result per call, or deep-freezing/duplicating the nested data before exposing it.
test/rails_test.rb
Outdated
| def test_accepts_string_keys | ||
| Rspamd::Rails.setup("enabled" => true, "outbound" => { "host" => "localhost", "port" => 11334 }) | ||
|
|
||
| client = Rspamd::Rails.client_for(:outbound) | ||
| assert_instance_of Rspamd::Client, client | ||
| end |
There was a problem hiding this comment.
test_accepts_string_keys appears redundant with test_returns_client_when_enabled (both already pass string keys to setup). If the intent is to verify symbol-vs-string handling, consider adjusting one test to pass symbol keys (or remove this test if it's not adding distinct coverage).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/rspamd-ruby.rb
Outdated
| require "rspamd/client" | ||
| require "rspamd/client_stub" | ||
| require "rspamd/errors" | ||
| require "rspamd/rails" |
There was a problem hiding this comment.
rspamd/rails is unconditionally required here, but it in turn does require "active_support/core_ext/hash/keys". This means every user of this gem (including non-Rails projects) is forced to install and load activesupport as a runtime dependency.
The Railtie is already conditionally loaded (if defined?(::Rails::Railtie)), but rspamd/rails should receive the same treatment — either guard it similarly (e.g., require "rspamd/rails" if defined?(::ActiveSupport)) or move the require into the Railtie so it's only loaded in Rails apps.
| require "rspamd/rails" | |
| require "rspamd/rails" if defined?(::ActiveSupport) |
|
|
||
| s.required_ruby_version = ">= 2.7.8" | ||
|
|
||
| s.add_dependency "activesupport", ">= 6.0" |
There was a problem hiding this comment.
Adding activesupport as an unconditional runtime dependency is a heavy requirement for what is currently used only for deep_symbolize_keys in Rspamd::Rails. This forces all consumers of this gem (including non-Rails projects) to pull in activesupport and its transitive dependencies.
Consider either:
- Making
activesupportan optional dependency and only requiring it in the Rails-specific code path, or - Implementing the
deep_symbolize_keyslogic inline (it's a small recursive method) to avoid the dependency entirely for the core gem.
| s.add_dependency "activesupport", ">= 6.0" | |
| s.add_development_dependency "activesupport", ">= 6.0" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def build_client(name) | ||
| settings = @config.fetch(name) { raise ArgumentError, "No rspamd configuration for #{name.inspect}" } | ||
| Client.new(**settings.slice(:host, :port, :password)) |
There was a problem hiding this comment.
build_client only passes host, port, and password to Client.new, but Configuration supports additional options: scheme, open_timeout, read_timeout, and user_agent. If any of these are specified in the YAML config, they'll be silently ignored. Consider forwarding all recognized options, or at minimum documenting that only these three are supported via the YAML config.
| Client.new(**settings.slice(:host, :port, :password)) | |
| Client.new(**settings.slice(:host, :port, :password, :scheme, :open_timeout, :read_timeout, :user_agent)) |
|
|
||
| module Rspamd | ||
| class << self | ||
| def setup(config) | ||
| @config = config.deep_symbolize_keys | ||
| @clients = {} | ||
| end | ||
|
|
||
| def client_for(name) | ||
| clients[name] ||= enabled? ? build_client(name) : ClientStub.new | ||
| end | ||
|
|
||
| def reset! | ||
| @config = nil | ||
| @clients = {} | ||
| end | ||
|
|
||
| private | ||
| def clients | ||
| @clients ||= {} |
There was a problem hiding this comment.
The PR description claims "Thread-safe client management," but client_for uses Hash#[]= with ||= which is not thread-safe. In a multi-threaded environment (e.g., Puma), concurrent calls to client_for could result in race conditions where multiple Client instances are created for the same name, or worse, internal hash corruption.
If thread safety is a goal, consider using a Mutex to synchronize access to @clients, or use Concurrent::Map from the concurrent-ruby gem (which is already a transitive dependency via activesupport).
| module Rspamd | |
| class << self | |
| def setup(config) | |
| @config = config.deep_symbolize_keys | |
| @clients = {} | |
| end | |
| def client_for(name) | |
| clients[name] ||= enabled? ? build_client(name) : ClientStub.new | |
| end | |
| def reset! | |
| @config = nil | |
| @clients = {} | |
| end | |
| private | |
| def clients | |
| @clients ||= {} | |
| require "concurrent/map" | |
| module Rspamd | |
| class << self | |
| def setup(config) | |
| @config = config.deep_symbolize_keys | |
| @clients = Concurrent::Map.new | |
| end | |
| def client_for(name) | |
| clients.fetch_or_store(name) { enabled? ? build_client(name) : ClientStub.new } | |
| end | |
| def reset! | |
| @config = nil | |
| @clients = Concurrent::Map.new | |
| end | |
| private | |
| def clients | |
| @clients ||= Concurrent::Map.new |
aa8b02d to
63b0a7c
Compare
Adds three new components for Rails integration: - Rspamd::ClientStub: null-object client that returns ham for check and no-ops for training, used when rspamd is disabled (dev/test) - Rspamd::Rails: thread-safe client management with config loading via setup(config) and client_for(:outbound) - Rspamd::Railtie: auto-loads config/rspamd.yml on Rails boot Also adds activesupport as a runtime dependency for deep_symbolize_keys. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add explicit require for check/result in client_stub.rb so the file is standalone-loadable - Rename test_accepts_string_keys to test_accepts_symbol_keys and actually pass symbol keys to provide distinct coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When disabled, client_for now caches the ClientStub per name instead of returning a new instance each call. This ensures the same object is returned on repeated calls, which is needed for test expectations (e.g. Rspamd.outbound_client.expects(:check)). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Rails module name was misleading since this code works outside Rails. Moving it to the top-level Rspamd module simplifies the API: Rspamd.setup(config) Rspamd.client_for(:outbound) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ruby 2.7 is EOL and incompatible with recent bundler versions. Also update actions/checkout to v4 and use bundler-cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
63b0a7c to
7ac8824
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def client_for(name) | ||
| clients[name] ||= enabled? ? build_client(name) : ClientStub.new | ||
| end |
There was a problem hiding this comment.
When enabled? is false, every call to client_for with the same name will create a new ClientStub instance only if caching works — but when setup was never called (@config is nil), enabled? returns nil (falsy) and build_client is skipped, yet a new ClientStub is created and cached. However, when enabled? is explicitly false, a new ClientStub is still created per unique name. Consider caching a single shared ClientStub instance (e.g., STUB = ClientStub.new) to avoid unnecessary allocations if client_for is called with many different names while disabled.
| HAM_RESULT = Check::Result.new( | ||
| "score" => 0.0, | ||
| "required_score" => 15.0, | ||
| "action" => "no action", | ||
| "is_skipped" => false, | ||
| "symbols" => {}, | ||
| "urls" => [], | ||
| "emails" => [] | ||
| ).freeze |
There was a problem hiding this comment.
HAM_RESULT is frozen and returned directly from check. If any caller mutates the returned result (e.g., adding symbols or modifying attributes), it will raise a FrozenError. While freezing is generally good for a shared constant, the real Client#check returns a fresh Result each time. This behavioral difference could cause subtle breakage when switching between Client and ClientStub. Consider returning HAM_RESULT.dup from check to match the real client's semantics.
| initializer "rspamd.setup" do | ||
| config_path = ::Rails.root.join("config/rspamd.yml") | ||
|
|
||
| if config_path.exist? | ||
| Rspamd.setup(::Rails.application.config_for(:rspamd)) |
There was a problem hiding this comment.
The initializer runs during Rails initialization but does not use an after_initialize hook. Depending on the Rails boot order, config_for should work here, but using config.after_initialize is the more conventional pattern when setting up application-level services to ensure the full application config is available. Additionally, if config/rspamd.yml exists but lacks an entry for the current environment, config_for will raise an error — consider rescuing or documenting this behavior.
| initializer "rspamd.setup" do | |
| config_path = ::Rails.root.join("config/rspamd.yml") | |
| if config_path.exist? | |
| Rspamd.setup(::Rails.application.config_for(:rspamd)) | |
| initializer "rspamd.setup" do |app| | |
| config_path = ::Rails.root.join("config/rspamd.yml") | |
| if config_path.exist? | |
| app.config.after_initialize do | |
| begin | |
| Rspamd.setup(app.config_for(:rspamd)) | |
| rescue KeyError => e | |
| if defined?(::Rails) && ::Rails.respond_to?(:logger) && ::Rails.logger | |
| ::Rails.logger.warn("Rspamd configuration for environment #{::Rails.env} is missing in config/rspamd.yml: #{e.message}") | |
| end | |
| end | |
| end |
Summary
Rspamd::ClientStub— Null-object client that returns ham forcheckand no-ops for training methods. Used when rspamd is disabled (dev/test environments).Rspamd.setup(config)/Rspamd.client_for(:name)— Thread-safe client management with config loading. Returns cachedRspamd::Clientinstances (orClientStubwhen disabled).Rspamd::Railtie— Auto-callsRspamd.setupafter Rails init ifconfig/rspamd.ymlexists.Usage
Test plan
🤖 Generated with Claude Code