feat!: replace the global accessors with a configuration object - #12
Merged
Merged
Conversation
…d provider's cache_key
Instrument the translate/cache/request/rate_limit call sites in Request and log the resolved provider, so an application can wire in ActiveSupport::Notifications or any duck-typed instrumenter and a logger. Payloads carry only counts, language codes and provider names -- never the text being translated -- since this library handles content its callers did not write. Also tighten Request#provider_cache_key to reject a whitespace-only cache key, not just an empty one, and correct the provider contract's header comment, which still named a method removed in the configuration rewrite.
…tion The rate_limit instrumentation event was untested: InstrumentationTest never set config.rate_limit, so check_rate_limit returned before instrumenting, and the no-content guard silently covered three of its four events. The underlying gap was that rate_limiter, unlike provider/cache/segmenter, had no way to accept a caller-supplied object -- it always built a RedisRateLimiter, which is why no test could exercise it without touching Redis. Declare rate_limiter as an assignable Configuration option: an assigned object is used as-is, nil is returned when no rate_limit is configured, and a RedisRateLimiter is built and memoised otherwise. Add a hand-written limiter double to InstrumentationTest so every translation now emits all four events, and strengthen the no-content guard to assert the full set of event names before checking payload content, so it can no longer pass while an event silently never fires.
rate_limiter and rate_limiter_instance shared one instance variable, so a context copy that changed cache_namespace still got the parent's already- built RedisRateLimiter -- a tenant's requests were rate-limited against its parent's namespace instead of its own, silently. provider_instance, cache_store and segmenter_instance never had this problem because their raw option and resolved value were already two separate readers. Add Configuration#rate_limiter_instance, memoised under its own instance variable and left out of the set copy carries over, matching the other three extension points. Request now resolves through it. Invert the two tests that had pinned the bleed as correct behaviour, and add a test that reproduces the original report directly: resolve a limiter on a parent, copy, change the copy's namespace, and assert the copy's limiter uses it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces four global accessors with one configuration object and a registry-based extension mechanism, modelled on
ruby_llm.Why
Setting the library up took twenty lines and required the caller to configure a second gem, build a connection pool by hand, pass it to two objects, and keep their namespaces in step:
Now:
Both have defaults, so with
DEEPL_AUTH_KEYin the environment there is nothing to configure at all. WithoutREDIS_URLthe cache lives in the process, so the library runs before any infrastructure does.One rule, four extension points
Every extension point takes either a symbol naming a registered built-in, or an object of your own:
Implemented once by a generic
Registry, instantiated three times. Each has an option holding the symbol-or-object and a reader returning the built collaborator:provider/provider_instance,cache/cache_store,segmenter/segmenter_instance,rate_limiter/rate_limiter_instance.Adding a translator no longer touches this gem
AdaptersbecameProviders, and a provider registers itself, declaring its own configuration options as a side effect — which is what keepsdeepl_api_keyout of the core:Nothing in
lib/changes to make that work, and a test asserts exactly that by registering a provider defined entirely in the test file and translating through it end to end.Contexts
TranslationDiff.contextyields an isolated copy of the configuration for per-tenant or per-request settings, building its own provider, cache, pool and rate limiter. The global configuration is untouched.Instrumentation
Four events to any object responding to
instrument(name, payload)—ActiveSupport::Notificationssatisfies it without an adapter, and without becoming a dependency. Payloads carry counts, language codes and provider names, and never the text being translated, its translation, or a credential. That is a guarantee, enforced by a test that fails if any payload contains the source text.Notable findings from review
config.loggerleaked content and the API key. The DeepL provider forwarded it intoDeepL::Configuration, anddeepl-rblogs the full request payload plus theAuthorizationheader at DEBUG. The logger is no longer forwarded; the README explains how to opt into deepl-rb's own logging and what it contains.Ratelimit#add(size)where the signature isadd(subject, count = 1), recording one hit under a subject named after the character count whileexceeded?read a subject nothing incremented. Dates to a release tagged 2023-02-16. Fixed, and listed under breaking changes: anyone withrate_limitset has been unthrottled for three years and will now hit a threshold that has never fired.rate_intervalis also silently clamped by that gem to roughly 5–600 seconds, now documented.Verification
203 tests, 455 assertions, green across seeds 11, 4242, 90210 and 777. RuboCop clean, no cop disabled. Runtime dependencies unchanged at
oxandpragmatic_segmenter;deepl-rb,redis,connection_pool,redis-namespaceandratelimitare required lazily and the library loads without any of them installed.A differential harness — 20 inputs across 7 call shapes, recording outputs, call sequences and cache keys — was run against the pre-branch commit and against this branch: 142 of 142 lines identical, covering cache hit and miss, chunking, spacing restoration, HTML and
notranslate, nested hashes, non-strings, and thefrom: nilauto-detection path. The translation behaviour itself is unchanged.The suite was also run with
REDIS_URLset and every socket entry point blocked, confirming no test reaches the network.