diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46837bb..1bb8720 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,6 +23,53 @@ jobs: - run: bundle exec rake test + postgres: + runs-on: ubuntu-latest + + services: + postgres: + image: postgres:18 + env: + POSTGRES_PASSWORD: postgres + options: >- + --health-cmd pg_isready --health-interval 10s + --health-timeout 5s --health-retries 5 + ports: ["5432:5432"] + + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: "3.4" + bundler-cache: true + - run: bundle exec rake test + env: + TRANSLATION_DIFF_DATABASE_URL: postgres://postgres:postgres@localhost:5432/postgres + + mysql: + runs-on: ubuntu-latest + + services: + mysql: + image: mysql:8 + env: + MYSQL_ALLOW_EMPTY_PASSWORD: "yes" + MYSQL_DATABASE: translation_diff_test + options: >- + --health-cmd "mysqladmin ping" --health-interval 10s + --health-timeout 5s --health-retries 5 + ports: ["3306:3306"] + + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: "3.4" + bundler-cache: true + - run: bundle exec rake test + env: + TRANSLATION_DIFF_DATABASE_URL: trilogy://root@127.0.0.1:3306/translation_diff_test + lint: runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index f5692ff..40f4f67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,74 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). quietly too low: only three of the six built-in providers report billing at all. See [Instrumentation](docs/instrumentation.md). +- **A SQL-backed cache store and rate limiter, for an application that runs + Postgres or MySQL and does not want Redis for this alone.** + `TranslationDiff::ActiveRecordCacheStore` (`config.cache = + :active_record`) and `TranslationDiff::ActiveRecordRateLimiter` + (`config.rate_limiter = :active_record`) cache translations and throttle + requests in the application's own database, exercised in CI against + Postgres, MySQL and SQLite. Nothing on this branch is breaking: both + are opt-in, the default resolution of `cache` and `rate_limiter` is + untouched, and an application with `redis_url` set keeps getting Redis + exactly as before. `rails generate translation_diff:install` writes the + migration for both tables, idempotently -- every `create_table` and + `add_index` in it carries `if_not_exists: true`; for anyone not on Rails, + its body is in [SQL cache](docs/sql-cache.md) verbatim -- **the gem + itself never runs DDL.** `rake translation_diff:prune` ships inside the + gem: a `Railtie` wires it into a Rails application's own rake tasks + automatically, enhanced with `:environment` so it prunes that + application's own configuration; a non-Rails application loads the task + file itself and + must configure `TranslationDiff` before running it, since the task gets + no `:environment`-equivalent there. ActiveRecord 7.1 or newer is + required when either is used, refused by name at build time rather than + failing inside a query, and `activerecord` is never a dependency of this + gem -- it is required lazily on first use, the same way `redis` already + is. Four new configuration options: `cache_table_name`, + `rate_limit_table_name`, `active_record_base` and + `cache_prune_probability`; the last, along with `cache_namespace`, is + now validated at `configure` time -- a `cache_prune_probability` that + will not coerce to a number, or a `cache_namespace` over 64 characters, + is refused before either reaches a query. See + [SQL cache](docs/sql-cache.md). +- **The SQL cache store's write joins the caller's transaction, and its + errors are redacted, not silent.** A rollback in the caller's transaction + discards translations `ActiveRecordCacheStore` already wrote -- the + largest behavioural difference from `RedisCacheStore`, which is never + inside anyone's transaction. A failed write itself runs in its own + savepoint, so it no longer aborts a transaction it does not own, and the + `TranslationDiff::Error` it raises carries the adapter's error class, not + the row. `upsert_all` inlines values rather than binding them, though, so + a written translation still appears verbatim in the host application's + own ActiveRecord log at `debug` -- unrelated to this gem's own `logger` + option, which never prints content. See + [Transactions](docs/sql-cache.md#transactions) and + [What ends up in your log](docs/sql-cache.md#what-ends-up-in-your-log). +- **`cache_ttl` of `0` or less now means never expires, and `nil` is + reachable through `TranslationDiff.configure`.** Previously `nil` was + documented as meaningful but unreachable through the public + configuration path, and `0` wrote a row whose `expires_at` was already in + the past -- a cache entry that could never hit. Both now fold to the same + `nil` `expires_at`. See + [`cache_ttl` becomes `expires_at`](docs/sql-cache.md#cache_ttl-becomes-expires_at). +- **`ActiveRecordRateLimiter`'s sliding window is conservative, not + exact.** It sums the oldest bucket touching the trailing `rate_interval` + seconds in full, even though that bucket is only ever partially inside + the window, so the window actually enforced is `rate_interval` to + `rate_interval + rate_interval / 12` seconds -- slightly stricter than + configured, never looser. See + [The rate limiter](docs/sql-cache.md#the-rate-limiter). +- `write_multi(pairs)` joins the cache store contract, as an optional + method: a store that implements it gets one call carrying a whole batch + of sentences instead of one call per sentence; a store that does not is + still called once per sentence, exactly as before this method existed -- + a custom cache store written against the older contract is unaffected. + All three shipped stores implement it now: `MemoryCacheStore` and + `RedisCacheStore` gain it here too, alongside `ActiveRecordCacheStore`. + The two batching paths fail differently from the per-key one and from + each other -- see + [The three write paths fail differently](docs/caching.md#the-three-write-paths-fail-differently). + ### Security - `Configuration#inspect` and `Provider#inspect` print `[FILTERED]` in place diff --git a/Gemfile b/Gemfile index 281e85e..35790f4 100644 --- a/Gemfile +++ b/Gemfile @@ -31,3 +31,31 @@ gem "cgi", "~> 0.5", require: false # the test suite, which signs against the real library rather than a # stand-in, has it available. gem "aws-sigv4", "~> 1.12", require: false + +# Not runtime dependencies of the gem (see the gemspec) -- ActiveRecordCacheStore +# and ActiveRecordRateLimiter require active_record lazily on first use, so an +# application caching in Redis never needs it installed. They are here so the +# suite can exercise the stores against a real database rather than a stand-in. +gem "activerecord", "~> 8.1", require: false +gem "sqlite3", "~> 2.9", require: false + +# Not a runtime dependency of the gem (see the gemspec) -- only the CI job that +# sets TRANSLATION_DIFF_DATABASE_URL ever opens a Postgres connection, where +# the concurrency the SQL store and rate limiter rest on can actually be +# tested. SQLite has one writer, so most of the suite never needs this gem. +gem "pg", "~> 1.5", require: false + +# Not a runtime dependency of the gem (see the gemspec) -- only the CI job that +# sets TRANSLATION_DIFF_DATABASE_URL to a MySQL database ever opens one, where +# the missing supports_insert_conflict_target? behaviour actually bites. Trilogy +# over mysql2: it is a pure Ruby/C socket client with no libmysqlclient headers +# to install, so it builds on a bare CI runner and on this machine alike. +gem "trilogy", "~> 2.9", require: false + +# Not a runtime dependency of the gem (see the gemspec) -- the generator under +# lib/generators/ is loaded only when Rails loads generators, so a non-Rails +# application never needs it installed. It is here so +# test/translation_diff/install_generator_test.rb can load it and check the +# generated migration against the schema the rest of the suite runs against, +# instead of skipping itself for want of Rails::Generators::Base. +gem "railties", "~> 8.1", require: false diff --git a/README.md b/README.md index bfc9981..e469e7d 100644 --- a/README.md +++ b/README.md @@ -111,12 +111,12 @@ See [Providers](docs/providers.md) for configuring each one, the full capabiliti - **Six built-in providers** -- DeepL, Google Cloud Translation, Azure AI Translator, ModernMT, LibreTranslate, Amazon Translate -- or bring your own by subclassing a small base class - **HTML aware:** markup is preserved, and `class="notranslate"` can protect a span (provider support varies -- see the caveats below) - **Any shape:** strings, arrays, and deep hashes go in and come back translated in the same shape -- **Two cache stores:** `MemoryCacheStore` out of the box, `RedisCacheStore` once you configure `redis_url` +- **Three cache stores:** `MemoryCacheStore` out of the box, `RedisCacheStore` once you configure `redis_url`, `ActiveRecordCacheStore` to cache in your own database instead -- see [SQL cache](docs/sql-cache.md) - **Isolated contexts:** `TranslationDiff.context` for multi-tenant apps and per-request provider overrides, without touching the global configuration - **Pluggable sentence segmenter:** `pragmatic_segmenter` by default, with a zero-dependency `Simple` alternative - **HTTP retries, timeouts, and backoff** on every REST-backed provider, via `faraday` and `faraday-retry` - **One error hierarchy** under `TranslationDiff::Error`, carrying the provider name and HTTP status -- **Optional rate limiting and instrumentation** -- credentials and translated content never appear in a log line +- **Optional rate limiting and instrumentation** -- credentials and translated content never appear in a log line this gem writes (the SQL cache store is the one exception worth knowing before you adopt it -- see [SQL cache](docs/sql-cache.md#what-ends-up-in-your-log)) ## Installation @@ -140,7 +140,7 @@ This gem loads `ox`, `pragmatic_segmenter`, `faraday`, and `faraday-retry` at re ## Documentation -[Configuration](docs/configuration.md) · [Providers](docs/providers.md) · [Languages](docs/languages.md) · [Caching](docs/caching.md) · [Contracts](docs/contracts.md) · [Instrumentation](docs/instrumentation.md) · [Errors](docs/errors.md) · [How it works](docs/how-it-works.md) · [Upgrading & development](docs/development.md) +[Configuration](docs/configuration.md) · [Providers](docs/providers.md) · [Languages](docs/languages.md) · [Caching](docs/caching.md) · [SQL cache](docs/sql-cache.md) · [Contracts](docs/contracts.md) · [Instrumentation](docs/instrumentation.md) · [Errors](docs/errors.md) · [How it works](docs/how-it-works.md) · [Upgrading & development](docs/development.md) ## Contributing diff --git a/Rakefile b/Rakefile index ef05ed5..882dee4 100644 --- a/Rakefile +++ b/Rakefile @@ -36,3 +36,6 @@ namespace :languages do report[:failed].each { |name, message| warn "failed: #{name}: #{message}" } end end + +# The task itself ships in lib/, so a host application's own `rake` can load it too -- this just reuses it here. +load File.expand_path("lib/translation_diff/tasks/translation_diff.rake", __dir__) diff --git a/docs/caching.md b/docs/caching.md index f257a25..9222fec 100644 --- a/docs/caching.md +++ b/docs/caching.md @@ -51,8 +51,8 @@ key every already-warm cache is keyed on. ## The cache store contract -`config.cache` accepts either a registered name (`:redis`, `:memory`) or an -object satisfying this contract directly: +`config.cache` accepts either a registered name (`:redis`, `:memory`, +`:active_record`) or an object satisfying this contract directly: ```ruby # Reads several keys at once, returning an array the same length as keys, @@ -61,17 +61,61 @@ def read_multi(keys); end # Writes one key. The second write of the same key replaces the first. def write(key, value); end + +# Writes several pairs at once. Optional -- see "write_multi is optional" below. +def write_multi(pairs); end ``` `test/support/cache_store_contract.rb` is the executable form of this contract: include `CacheStoreContract` in a test class that defines -`#store`. +`#store`. It only exercises `read_multi` and `write` -- the two required +methods -- so a store that implements only those two still passes it. +`test/support/batching_cache_store_contract.rb` holds the optional half: +include `BatchingCacheStoreContract` too, alongside `CacheStoreContract`, +once `#store` also implements `write_multi`. -Two stores ship with this gem: `TranslationDiff::MemoryCacheStore`, the +Three stores ship with this gem: `TranslationDiff::MemoryCacheStore`, the default -- a bounded, in-process LRU, not thread-safe by design, evicting by -`cache_max_size` rather than by time; and `TranslationDiff::RedisCacheStore`, +`cache_max_size` rather than by time; `TranslationDiff::RedisCacheStore`, built from `redis_url` when that is set, expiring entries after `cache_ttl` -and namespacing every key under `cache_namespace`. Neither `redis` nor -`connection_pool` nor `redis-namespace` is a dependency of this gem -- +and namespacing every key under `cache_namespace`; and +`TranslationDiff::ActiveRecordCacheStore`, opt-in, caching in the +application's own database -- see [SQL cache](sql-cache.md). Neither `redis` +nor `connection_pool` nor `redis-namespace` is a dependency of this gem -- `RedisCacheStore` takes anything answering to `#with` the way `ConnectionPool` does, and yields anything `Redis::Namespace` accepts. + +## `write_multi` is optional + +A store need not implement `write_multi`. `SentenceCache#store` checks: a +store that answers to it gets one call carrying every translated sentence +from the batch; a store that does not is called once per sentence through +`write` instead, exactly as it always was. A custom cache store written +against the contract before `write_multi` existed keeps working unchanged +-- that is what "optional" means here. + +All three shipped stores implement it: `MemoryCacheStore` loops over the +pairs (there is no round trip to save in-process); `RedisCacheStore` +pipelines the writes; `ActiveRecordCacheStore` upserts the whole batch in +one statement. + +### The three write paths fail differently + +Nobody had written this down before: what a partial failure leaves cached +depends on which of these shapes wrote it. + +- **No `write_multi` (the per-key path), and `MemoryCacheStore`'s loop.** + Sentences are written one at a time, in order. A failure at sentence N + leaves 1..N-1 written, N failed, and N+1.. never attempted. +- **`RedisCacheStore#write_multi`.** A Redis pipeline is not a + transaction: each `SETEX` in it runs independently of the others, so a + failure in one does not stop its siblings from landing. Which of the + batch actually landed does not follow the sentence order the way the + per-key path's does. +- **`ActiveRecordCacheStore#write_multi`.** One `upsert_all` statement for + the whole batch. It either lands as a whole or it does not -- there is no + partial batch to reason about. + +A caller that needs to know which sentences got cached after a failure +needs to know which of these three shapes wrote them; the answer is not the +same for all three. diff --git a/docs/configuration.md b/docs/configuration.md index e341b8f..898bddd 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -55,15 +55,19 @@ at all, so an unset environment variable never has to be special-cased. | --- | --- | --- | | `provider` | `:deepl` | The translation provider: a registered name or a `TranslationDiff::Provider` of your own. See [Providers](providers.md). | | `cache` | `nil` | The cache store: a registered name or an object satisfying the [cache store contract](caching.md#the-cache-store-contract). `nil` means "choose for me" -- see below. | -| `cache_ttl` | `604_800` (one week) | Seconds a Redis cache entry is kept. Only meaningful for `RedisCacheStore`; `MemoryCacheStore` evicts by size instead. | -| `cache_namespace` | `"translation-diff"` | Prefix applied to every Redis key this gem writes -- both cache entries and the rate limiter's own bookkeeping. | +| `cache_ttl` | `604_800` (one week) | Seconds an entry is kept before it expires. Read by `RedisCacheStore` (a `SETEX`) and by `ActiveRecordCacheStore` (written into each row's `expires_at`); `MemoryCacheStore` evicts by size instead and ignores it. A non-positive value (`0` or less, or `nil`) means never expires. A String is coerced, so an environment variable works; a value that is not a number is refused at `configure` time rather than mid-translation. See [SQL cache](sql-cache.md#cache_ttl-becomes-expires_at). | +| `cache_namespace` | `"translation-diff"` | Prefix applied to every Redis key this gem writes -- both cache entries and the rate limiter's own bookkeeping. Also the `namespace` column both SQL tables share and the unit `ActiveRecordCacheStore#prune` operates on. At most 64 characters -- longer is refused at `configure` time. See [SQL cache](sql-cache.md#the-tables). | | `cache_max_size` | `1_000` | Maximum number of entries `MemoryCacheStore` keeps before evicting the least recently used one. | +| `cache_table_name` | `"translation_diff_translations"` | Table `ActiveRecordCacheStore` reads and writes. For a host with its own table-naming convention. See [SQL cache](sql-cache.md). | +| `rate_limit_table_name` | `"translation_diff_rate_limits"` | Table `ActiveRecordRateLimiter` reads and writes. As above. | +| `active_record_base` | `nil` (`::ActiveRecord::Base`) | The class `ActiveRecordCacheStore` and `ActiveRecordRateLimiter` build their model from -- point this at a second database, or a reader/writer role. See [SQL cache](sql-cache.md#active_record_base-a-second-database-or-a-readerwriter-role). | +| `cache_prune_probability` | `0.0` | Chance, per write, that `ActiveRecordCacheStore` prunes expired rows before returning. `0.0` is off, and a value outside `0.0..1.0` is refused at `configure` time; `rake translation_diff:prune` is the other way to prune. See [SQL cache](sql-cache.md#pruning-three-answers-none-imposed). | | `redis_url` | `ENV["REDIS_URL"]` | Where to connect for the Redis-backed cache store and rate limiter. Setting this is what makes `cache` default to `:redis` instead of `:memory`. | | `redis_pool_size` | `5` | Size of the connection pool built from `redis_url`. | | `redis_pool_timeout` | `5` | Seconds to wait for a connection from that pool before raising. | | `rate_limit` | `nil` | Character threshold per `rate_interval`. Unset means no rate limiting at all. | | `rate_interval` | `60` | Seconds over which `rate_limit` is measured. **Actually enforced over roughly 5-600 seconds** -- see [The rate limiter contract](contracts.md#the-rate-limiter-contract). | -| `rate_limiter` | `nil` | An object satisfying the [rate limiter contract](contracts.md#the-rate-limiter-contract), to use in place of the built-in Redis-backed one. | +| `rate_limiter` | `nil` | A registered name (`:redis`, `:active_record`) or an object satisfying the [rate limiter contract](contracts.md#the-rate-limiter-contract). `nil` with `rate_limit` set resolves to `:redis`. | | `segmenter` | `:pragmatic` | The sentence segmenter: a registered name or an object satisfying the [segmenter contract](contracts.md#the-segmenter-contract). | | `instrumenter` | `nil` | Anything satisfying `ActiveSupport::Notifications`' `#instrument(name, payload) { }` interface. See [Instrumentation and logging](instrumentation.md). | | `logger` | `nil` | A standard `Logger`. Receives one `debug` line per provider resolution, naming the provider class -- never content and never a credential. See [Instrumentation and logging](instrumentation.md). | diff --git a/docs/contracts.md b/docs/contracts.md index 24c88d6..f943494 100644 --- a/docs/contracts.md +++ b/docs/contracts.md @@ -2,16 +2,20 @@ ## The rate limiter contract -Unlike `provider`, `cache` and `segmenter`, `rate_limiter` does not resolve a -symbol through a registry -- there is only one built-in implementation. -`config.rate_limiter_instance` is: +Like `provider`, `cache` and `segmenter`, `rate_limiter` resolves a symbol +through its own registry, `TranslationDiff::RateLimiters` -- `:redis` and +`:active_record` are registered there. `config.rate_limiter_instance` is: -- the object assigned to `config.rate_limiter`, if any; +- the object assigned to `config.rate_limiter`, if any -- an object still + bypasses the registry entirely, the same way it does for `cache`; - otherwise `nil` if `rate_limit` was never set -- and `Dispatcher#throttle` checks for that `nil` and skips rate limiting entirely, so the common case costs nothing; -- otherwise a `TranslationDiff::RedisRateLimiter` built from `rate_limit`, - `rate_interval`, `redis_url` and `cache_namespace`. +- otherwise the registered limiter named by `config.rate_limiter`, or + `TranslationDiff::RedisRateLimiter` when `rate_limiter` is left unset -- + built from `rate_limit`, `rate_interval`, `cache_namespace`, and either + `redis_url` (`:redis`) or `active_record_base` and `rate_limit_table_name` + (`:active_record`; see [SQL cache](sql-cache.md)). An object assigned to `rate_limiter` must implement: @@ -23,10 +27,14 @@ def check(size); end `TranslationDiff::RedisRateLimiter` raises `TranslationDiff::RedisRateLimiter::RateLimitExceeded` when its threshold is -exceeded within its interval. Neither `redis` nor `connection_pool` nor -`ratelimit` is a dependency of this gem: `ratelimit` is required on the first -check, so an application that configures no `rate_limit` never needs it, and -its absence raises `TranslationDiff::Error` naming the gem to add. +exceeded within its interval; +`TranslationDiff::ActiveRecordRateLimiter` raises its own +`RateLimitExceeded`, a distinct class under the same name. Neither `redis` +nor `connection_pool` nor `ratelimit` is a dependency of this gem: +`ratelimit` is required on the first check, so an application that +configures no `rate_limit` never needs it, and its absence raises +`TranslationDiff::Error` naming the gem to add. `activerecord` is never a +dependency either -- see [SQL cache](sql-cache.md#the-activerecord-version-floor). **Upgrading to 3.1.0: re-validate your `rate_limit` threshold.** Before this release, `RedisRateLimiter` never actually limited anything -- a signature @@ -48,6 +56,18 @@ limiter up to six times more eagerly than the configured value suggests. Keep `rate_interval` within 5-600 seconds if you want the configured number to be the enforced one. +Both the clamp above and the upgrade note before it are about +`RedisRateLimiter`, which delegates its bucketing to the `ratelimit` gem. +`ActiveRecordRateLimiter` owns its own bucketing instead, and its window is +sliding rather than tumbling: buckets are `rate_interval / 12` seconds wide +(floored at 1 second), and a check sums every bucket touching the trailing +`rate_interval` seconds -- including the oldest one, which is only ever +partially inside that window, summed in full rather than pro-rated. So the +window actually enforced is `rate_interval` to `rate_interval + +rate_interval / 12` seconds: slightly stricter than configured, never +looser, and with no external clamp. See +[SQL cache](sql-cache.md#the-rate-limiter). + ## The segmenter contract `config.segmenter` decides where a text node is cut into sentence-sized diff --git a/docs/errors.md b/docs/errors.md index 37fc254..b6dd048 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -37,8 +37,17 @@ TranslationDiff::Error │ # Pragmatic computed offsets that │ # violate its own postcondition -- │ # not raised by ordinary use -└── TranslationDiff::RedisRateLimiter::RateLimitExceeded - # the configured rate_limit was exceeded +├── TranslationDiff::RedisRateLimiter::RateLimitExceeded +│ # the configured rate_limit was exceeded, +│ # raised by the Redis-backed limiter +└── TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded + # the same condition, raised by the SQL-backed + # limiter -- a distinct class under its own + # namespace, not the class above. Rescuing + # `RedisRateLimiter::RateLimitExceeded` + # specifically and switching `rate_limiter` to + # `:active_record` stops catching it; rescue + # `TranslationDiff::Error` to catch both. ``` `ProviderError` and its subclasses carry `#provider` (the registered name) diff --git a/docs/how-it-works.md b/docs/how-it-works.md index 6be46c7..6406eaf 100644 --- a/docs/how-it-works.md +++ b/docs/how-it-works.md @@ -50,10 +50,13 @@ Everything below is a collaborator one of the two drives. translation to a sentence by position after the fact. 6. **What came back is written home, and the value is rebuilt.** - Only sentences that actually got a translation are cached. Then each - passage renders itself -- markup fragments byte-exact, translated - sentences re-encoded as HTML text -- and `Document` puts the renders back - into the caller's shape. + Only sentences that actually got a translation are cached, through + `TranslationDiff::SentenceCache#store` -- see [`write_multi` is + optional](caching.md#write_multi-is-optional) for what happens when the + store's write fails partway through a batch. Then each passage renders + itself -- markup fragments byte-exact, translated sentences re-encoded as + HTML text -- and `Document` puts the renders back into the caller's + shape. `TranslationDiff::Markup` is the small module underneath steps 2, 3 and 6: it decodes entity references on the way to a provider, encodes `&` and `<` again diff --git a/docs/sql-cache.md b/docs/sql-cache.md new file mode 100644 index 0000000..53bec08 --- /dev/null +++ b/docs/sql-cache.md @@ -0,0 +1,284 @@ +# SQL cache + +## What it's for + +If you already run Postgres or MySQL and do not want to stand up Redis for +one cache, `TranslationDiff::ActiveRecordCacheStore` caches translations in +the application's own database instead, and +`TranslationDiff::ActiveRecordRateLimiter` throttles requests there too. +Supported means exercised in CI: the suite runs against Postgres, MySQL +and SQLite on every push. + +Both are opt-in. Setting `redis_url` still means Redis, exactly as before -- +nothing about an existing application's cache changes until you configure +one of these: + +```ruby +TranslationDiff.configure do |config| + config.cache = :active_record + config.rate_limiter = :active_record + config.cache_ttl = 30 * 24 * 60 * 60 +end +``` + +## What this store is worse at than Redis + +Two things, plainly. A read is a query against a table rather than an +`MGET` against an in-memory store. And the table grows until something +prunes it -- Redis expires a key for you; this store only stops serving an +expired row, it does not remove it by itself. See +[Pruning](#pruning-three-answers-none-imposed) below. + +## Transactions + +A write joins the caller's transaction. A failed write no longer poisons +it -- the write runs in its own savepoint, so a `StatementInvalid` there +does not abort a transaction it does not own -- but the rollback semantics +otherwise stay ordinary: if the caller's transaction rolls back, a +translation this store just wrote rolls back with it, and the next request +pays for it again. This is the largest difference between this store and +the Redis one it substitutes for -- a Redis write is never inside anyone's +transaction, so it never rolls back with one. + +## What ends up in your log + +`upsert_all` inlines values into the SQL it sends rather than binding them, +so a written translation appears verbatim in the host application's own +ActiveRecord log at `debug` level. That is a separate claim from the one in +[Configuration](configuration.md): this gem's own `logger` option never +prints content, but that says nothing about the application's own SQL log, +which sees the statement ActiveRecord actually sent. A failed write is +scrubbed -- the `TranslationDiff::Error` it raises carries the adapter's +error class, never the row, and its cause chain is severed so the original +exception cannot carry the row into an error tracker either, see +[`write_multi`](#write_multi) -- but a successful one is not; nothing here redacts your debug-level query log. If +your application logs SQL at `debug` and what it translates is +confidential, keep that log above `debug` around this store, or use +`RedisCacheStore` instead. + +## The tables + +Two tables, created by a migration you run once -- see +[The migration](#the-migration) below. This gem never creates or alters +either of them itself. + +### `translation_diff_translations` + +| Column | Meaning | +| --- | --- | +| `namespace` | `cache_namespace`. Two tenants share this table the way they share a Redis database; `#prune` only prunes its own configured namespace. Limited to 64 characters, the column's own limit; `config.cache_namespace =` refuses a longer value at `configure` time rather than at the first write. | +| `key_digest` | `Digest::SHA256.hexdigest(key)` -- 64 characters, always. The cache key itself is not stored, only its digest: a variable-length unique index is the one thing guaranteed to bite somebody on MySQL. This also means an entry is not human-readable by its key -- to find one, compute the digest the same way and look that up. | +| `translation` | The cached value. | +| `expires_at` | What `cache_ttl` means in SQL. `nil` when `cache_ttl` is unset, or set to `nil` or anything non-positive, which all mean the row never expires on its own. | +| `created_at`, `updated_at` | Standard ActiveRecord timestamps, set by `upsert_all`. | + +Unique index on `[namespace, key_digest]` -- the second write of a key +replaces the first, which is the cache store contract. A separate index on +`expires_at` backs both the read (which filters on it) and `#prune` (which +deletes by it). + +### `translation_diff_rate_limits` + +| Column | Meaning | +| --- | --- | +| `namespace` | `cache_namespace`, same column, same meaning, same table-sharing as above. | +| `bucket` | A slice of time narrower than `rate_interval` -- see [The rate limiter](#the-rate-limiter) below for why. | +| `characters` | Characters counted into that bucket so far. | + +Unique index on `[namespace, bucket]`, incremented by one guarded upsert +per check, so two processes hitting the same bucket cannot lose an +increment between them. + +## The migration + +`rails generate translation_diff:install` writes a timestamped migration +creating both tables, and lives under `lib/generators/`, loaded only when +Rails loads generators -- a non-Rails application never sees it and never +pays for it. **This gem never runs DDL itself:** the migration is the only +way either table comes into existence, and it is entirely yours to review, +edit, and run through your own deploy process. + +For anyone not on Rails -- Sequel, plain ActiveRecord, a DBA who would +rather write the DDL directly -- here is that migration's body, verbatim +(the generator fills in the class's version bracket with your own +`ActiveRecord::Migration.current_version`; `7.1` below is this store's +floor, not a requirement to target that version specifically): + +```ruby +class CreateTranslationDiffTables < ActiveRecord::Migration[7.1] + def change + create_table :translation_diff_translations, if_not_exists: true do |t| + t.string :namespace, null: false, limit: 64 + t.string :key_digest, null: false, limit: 64 + t.text :translation, null: false + t.datetime :expires_at + t.timestamps + end + + add_index :translation_diff_translations, %i[namespace key_digest], unique: true, + name: "index_translation_diff_translations_on_key", if_not_exists: true + add_index :translation_diff_translations, :expires_at, if_not_exists: true + + create_table :translation_diff_rate_limits, if_not_exists: true do |t| + t.string :namespace, null: false, limit: 64 + t.bigint :bucket, null: false + t.integer :characters, null: false, default: 0 + end + + add_index :translation_diff_rate_limits, %i[namespace bucket], unique: true, + name: "index_translation_diff_rate_limits_on_bucket", if_not_exists: true + end +end +``` + +Every `create_table` and `add_index` above carries `if_not_exists: true`, so +a DBA can run this migration twice without the second run failing. `bucket` +is a `bigint`, not the plain 4-byte integer it looks like it could be: at +`bucket_width` 1 second -- what `rate_interval` under 24 seconds folds to, +see [The rate limiter](#the-rate-limiter) below -- `bucket` is the raw Unix +timestamp, and a 4-byte integer column holding that overflows in January +2038 the same way a 32-bit `time_t` does. + +Both tables are always created together -- there is no generator flag to +get one without the other, since deciding to use one but not the other +costs nothing at migration time. + +## `cache_ttl` becomes `expires_at` + +`cache_ttl` (in seconds, same option `RedisCacheStore` reads) is written +into each row's `expires_at` at write time. A row past `expires_at` is +never read, whether or not anything has deleted it yet -- expiry and +deletion are two different questions here, unlike Redis, where a `SETEX` +key simply stops existing. + +A non-positive `cache_ttl` -- `0`, a negative number, or `nil` -- means +never expires: `expires_at` is written as `nil`, and a `nil` `expires_at` +is what "never expires on its own" means in the table above. All three +values fold to that one `nil` in `TranslationDiff.configure` itself, so +`config.cache_ttl` reads back `nil` for any of them, not just the one you +set. + +## Pruning: three answers, none imposed + +Deleting an expired row is a separate question from whether it is served, +and there is no single right answer to "when," so none is forced on you: + +- **`rake translation_diff:prune`.** Calls `#prune` on the configured cache + store and, separately, on the configured rate limiter -- deleting rows + and buckets past their expiry, each in its own configured namespace. Wire + it into cron, a scheduled job, whatever your host already runs. Either + side that does not support pruning (`:redis`, or an object of your own) + is reported and skipped rather than failing the task. + + The task ships inside this gem, under `lib/translation_diff/tasks/`, not + in the dev Rakefile -- a dependency's own Rakefile is never loaded by a + host application's `rake`. On Rails, a `Railtie` wires it into the + application's own rake tasks automatically, enhanced to depend on the + `:environment` task so it runs against the application's own + configuration rather than a default one -- `rake -T` shows it with no + extra setup. Off Rails, nothing registers it automatically: `load` the + file yourself (from wherever the gem is installed) to add it to your own + `Rakefile`, and note that it does **not** get an + `:environment`-equivalent dependency there -- your own bootstrap needs to + configure `TranslationDiff` before the task runs, the same way it would + before any code that calls `translate`. A cron entry that prunes + silently against the wrong (or unconfigured) configuration is worse than + no pruning at all. +- **`config.cache_prune_probability`** (default `0.0`, off). A fraction + between 0 and 1: on a write, `ActiveRecordCacheStore` rolls under it and + prunes if it wins, in a savepoint of its own so a failed prune cannot + abort a transaction the caller opened. A value outside `0.0..1.0`, or one + that is not a number, is refused at `configure` time. Off by default, + because a translation-serving request + should not be paying, even occasionally, for someone else's expired rows. + A value that will not coerce to a number is refused at `configure` time, + not on the first write that would have consulted it. +- **Doing nothing.** Also a supported answer. An unpruned table is correct + -- reads still skip every expired row -- just larger than it needs to be. + +`#prune` only ever deletes rows in its own configured `cache_namespace`; a +multi-tenant table with several namespaces needs `#prune` called once per +namespace if every tenant is to be pruned. + +## `active_record_base`: a second database, or a reader/writer role + +`config.active_record_base` (default `::ActiveRecord::Base`) is the class +`ActiveRecordCacheStore` and `ActiveRecordRateLimiter` build their model +from. Point it at a class connected to a second database, or one pinned to +a writer role, and this store's traffic follows that connection instead of +your application's primary one: + +```ruby +class TranslationDiffRecord < ActiveRecord::Base + self.abstract_class = true + connects_to database: { writing: :translation_diff, reading: :translation_diff } +end + +TranslationDiff.configure do |config| + config.cache = :active_record + config.active_record_base = TranslationDiffRecord +end +``` + +## The ActiveRecord version floor + +**ActiveRecord 7.1 or newer.** `upsert_all` needs `unique_by` on Postgres +and SQLite, and `record_timestamps:` only landed in 7.1. An older version is +refused by name, at the point the store is first used, rather than failing +inside a query with a message that does not say why: + +``` +the ActiveRecord cache store needs ActiveRecord 7.1 or newer (found 7.0.0): +upsert_all takes unique_by and record_timestamps there. +``` + +`activerecord` is never a dependency of this gem -- neither in the gemspec +nor required at load time. `ActiveRecordCacheStore#model` and +`ActiveRecordRateLimiter#model` `require "active_record"` on first use, so +an application that never configures `:active_record` never loads it, the +same way `RedisCacheStore` only reaches for `redis` when `redis_url` is +set. Add `gem "activerecord"` (and a database adapter) to your own Gemfile +to use either. + +## `write_multi` + +Both `ActiveRecordCacheStore` and `RedisCacheStore` implement the cache +store contract's optional `write_multi(pairs)` -- see +[`write_multi` is optional](caching.md#write_multi-is-optional) for what +that means, and +[The three write paths fail differently](caching.md#the-three-write-paths-fail-differently) +for how a batch write fails differently from a per-key one. +`ActiveRecordCacheStore#write_multi` is a single `upsert_all` for the whole +batch: a forty-sentence paragraph is one statement, not forty. + +## The rate limiter + +`config.rate_limiter = :active_record` throttles the same way `rate_limit` +and `rate_interval` already configure the Redis-backed limiter, but counts +characters into `translation_diff_rate_limits` instead of Redis. + +The window is sliding, not tumbling. Time is divided into buckets +`rate_interval / 12` seconds wide (never narrower than 1 second), not one +bucket per interval, and a check sums every bucket touching the trailing +`rate_interval` seconds before deciding whether the threshold is exceeded +-- so the answer does not jump the moment a single wide bucket rolls over, +the way it would if the whole interval were one bucket. Like the check it +replaces, it looks at the total *before* adding the new characters, so a +check that itself pushes the total over the threshold still succeeds; the +next one raises. + +The oldest bucket summed is only ever partially inside the window, and it +is summed in full anyway rather than pro-rated, which errs toward +stricter. So the window actually enforced is `rate_interval` to +`rate_interval + rate_interval / 12` seconds (that upper bound is one +bucket width, floored at 1 second) -- slightly stricter than what was +configured, never looser. A translation throttle exists to keep a +provider's quota from being exceeded, not to be a billing meter, so this +was left as the simpler, safer direction to be wrong in rather than made +exact. + +`#prune` here deletes buckets that have fully aged out of the window, in +the configured namespace. `rake translation_diff:prune` calls it the same +way it calls the cache store's `#prune`. There is no +`cache_prune_probability` equivalent for the rate limiter -- the rake task, +or leaving old buckets in place, are the two supported answers here. diff --git a/lib/generators/translation_diff/install_generator.rb b/lib/generators/translation_diff/install_generator.rb new file mode 100644 index 0000000..f245cf6 --- /dev/null +++ b/lib/generators/translation_diff/install_generator.rb @@ -0,0 +1,15 @@ +# Loaded only when Rails loads generators; nothing in lib/translation_diff.rb requires this file. +require "translation_diff" +require "rails/generators" +require "rails/generators/active_record/migration" + +# `rails generate translation_diff:install` -- writes the migration for both SQL-backed tables. +class TranslationDiff::InstallGenerator < Rails::Generators::Base + include ActiveRecord::Generators::Migration + + source_root File.expand_path("templates", __dir__) + + def create_migration_file + migration_template "create_translation_diff_tables.rb.erb", "db/migrate/create_translation_diff_tables.rb" + end +end diff --git a/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb b/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb new file mode 100644 index 0000000..d04ec55 --- /dev/null +++ b/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb @@ -0,0 +1,24 @@ +class CreateTranslationDiffTables < ActiveRecord::Migration[<%= ActiveRecord::Migration.current_version %>] + def change + create_table :translation_diff_translations, if_not_exists: true do |t| + t.string :namespace, null: false, limit: 64 + t.string :key_digest, null: false, limit: 64 + t.text :translation, null: false + t.datetime :expires_at + t.timestamps + end + + add_index :translation_diff_translations, %i[namespace key_digest], unique: true, + name: "index_translation_diff_translations_on_key", if_not_exists: true + add_index :translation_diff_translations, :expires_at, if_not_exists: true + + create_table :translation_diff_rate_limits, if_not_exists: true do |t| + t.string :namespace, null: false, limit: 64 + t.bigint :bucket, null: false + t.integer :characters, null: false, default: 0 + end + + add_index :translation_diff_rate_limits, %i[namespace bucket], unique: true, + name: "index_translation_diff_rate_limits_on_bucket", if_not_exists: true + end +end diff --git a/lib/translation_diff.rb b/lib/translation_diff.rb index 4918118..e721416 100644 --- a/lib/translation_diff.rb +++ b/lib/translation_diff.rb @@ -1,5 +1,6 @@ require "cgi/escape" require "digest/md5" +require "digest/sha2" require "forwardable" require "stringio" @@ -26,6 +27,8 @@ require "translation_diff/fragment" require "translation_diff/passage" require "translation_diff/sentence_cache" +require "translation_diff/cache_ttl_option" +require "translation_diff/cache_guard_options" require "translation_diff/configuration" require "translation_diff/configuration/provider_option_owners" @@ -47,12 +50,20 @@ require "translation_diff/stores" require "translation_diff/memory_cache_store" require "translation_diff/redis_cache_store" +require "translation_diff/active_record_support" +require "translation_diff/active_record_cache_store" +require "translation_diff/rate_limiters" require "translation_diff/redis_rate_limiter" +require "translation_diff/active_record_rate_limiter" require "translation_diff/instrumentation" require "translation_diff/dispatcher" require "translation_diff/translator" require "translation_diff/context" +# Only when a host application has already loaded Rails -- never required unconditionally, so a non-Rails +# application never pays for it, and the gem's own suite exercises this same guarded require, not a shortcut. +require "translation_diff/railtie" if defined?(Rails::Railtie) + module TranslationDiff class << self def config = @config ||= Configuration.new diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb new file mode 100644 index 0000000..7a96634 --- /dev/null +++ b/lib/translation_diff/active_record_cache_store.rb @@ -0,0 +1,111 @@ +# Caches translations in the application's own database; ActiveRecord is required on first use, never at load. +class TranslationDiff::ActiveRecordCacheStore + include TranslationDiff::ActiveRecordSupport + + def self.build(config) + new(namespace: config.cache_namespace, ttl: config.cache_ttl, + table_name: config.cache_table_name, base: config.active_record_base, + prune_probability: config.cache_prune_probability) + end + + def initialize(namespace:, ttl:, table_name:, base: nil, prune_probability: 0.0) + @namespace = namespace + @ttl = ttl + @table_name = table_name + @base = base + @prune_probability = prune_probability + end + + # One query, then the caller's order restored -- a missing or expired key is a nil in its own position. + def read_multi(keys) + return [] if keys.empty? + + digests = keys.map { |key| digest(key) } + found = live.where(key_digest: digests).pluck(:key_digest, :translation).to_h + digests.map { |d| found[d] } + end + + def write(key, value) + write_multi([[key, value]]) + value + end + + # One upsert for the whole batch; the unique index makes the second write of a key replace the first. + def write_multi(pairs) + return pairs if pairs.empty? + + # A savepoint, not the caller's own transaction: a failed write must not abort a transaction it does not own. + model.transaction(requires_new: true) do + model.upsert_all(pairs.to_h.map { |key, value| row(key, value) }, **upsert_options(model.connection)) + end + prune_sometimes + pairs + rescue StandardError => e + raise unless ar_statement_invalid?(e) + + raise redacted_error(e), cause: nil + end + + # Reads never serve an expired row; deleting one is this, and it is the host's call when to run it. + def prune = model.where(namespace: @namespace).where(expires_at: ...Time.now.utc).delete_all + + private + + # MySQL's adapter never answers true here and its ON DUPLICATE KEY UPDATE already targets every unique key. + def upsert_options(connection) + options = { record_timestamps: true } + options[:unique_by] = %i[namespace key_digest] if connection.supports_insert_conflict_target? + options + end + + # upsert_all inlines values into the statement it sends, so the adapter's own message can carry a whole row -- + # this names the adapter's error class and the statement's shape, never the row a caller's logger already has. + def redacted_error(error) + adapter_error = error.cause&.class || error.class + TranslationDiff::Error.new("the cache write failed (#{adapter_error}): an upsert into " \ + "#{@table_name}(namespace, key_digest, translation, expires_at)") + end + + # Its own message, naming the statement prune actually runs -- a failed prune is not a failed upsert. + def redacted_prune_error(error) + adapter_error = error.cause&.class || error.class + TranslationDiff::Error.new("the cache prune failed (#{adapter_error}): a delete from " \ + "#{@table_name}(namespace, expires_at)") + end + + def row(key, value) + { namespace: @namespace, key_digest: digest(key), translation: value, expires_at: expires_at } + end + + def expires_at = @ttl.nil? ? nil : Time.now.utc + @ttl + + # SHA256 hex is 64 characters whatever the key was, which is what makes the unique index portable. + def digest(key) = Digest::SHA256.hexdigest(key.to_s) + + def live + model.where(namespace: @namespace) + .where(expires_at: nil).or(model.where(namespace: @namespace).where(expires_at: Time.now.utc...)) + end + + # Its own savepoint, not write's -- a failing prune must not poison a transaction the caller owns either. + def prune_sometimes + return unless @prune_probability.positive? && rand < @prune_probability + + model.transaction(requires_new: true) { prune } + rescue StandardError => e + raise unless ar_statement_invalid?(e) + + raise redacted_prune_error(e), cause: nil + end + + # `defined?` short-circuits before the `is_a?`, so this never itself raises when the gem was never loaded. + def ar_statement_invalid?(error) + defined?(ActiveRecord::StatementInvalid) && error.is_a?(ActiveRecord::StatementInvalid) + end + + def active_record_feature = "the cache" + def active_record_component = "ActiveRecord cache store" + def active_record_upsert_detail = "upsert_all takes unique_by and record_timestamps there." +end + +TranslationDiff::Stores.register(:active_record, TranslationDiff::ActiveRecordCacheStore) diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb new file mode 100644 index 0000000..4950433 --- /dev/null +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -0,0 +1,74 @@ +# Throttles by counting characters into namespaced, time-bucketed rows in the application's own database. +class TranslationDiff::ActiveRecordRateLimiter + include TranslationDiff::ActiveRecordSupport + + class RateLimitExceeded < TranslationDiff::Error; end + + DEFAULT_THRESHOLD = 8000 + DEFAULT_INTERVAL = 60 + + # A bucket a twelfth of the interval wide caps a window's slop at under 10%, the same shape the `ratelimit` gem + # gets from its own fixed five-second buckets at the default 60-second interval. + BUCKET_FRACTION = 12 + + def self.build(config) + new(namespace: config.cache_namespace, table_name: config.rate_limit_table_name, + threshold: config.rate_limit, interval: config.rate_interval, base: config.active_record_base) + end + + def initialize(namespace:, table_name:, threshold: DEFAULT_THRESHOLD, interval: DEFAULT_INTERVAL, base: nil, + clock: -> { Time.now }) + @namespace = namespace + @table_name = table_name + @threshold = threshold + @interval = interval + @base = base + @clock = clock + @bucket_width = [@interval / BUCKET_FRACTION, 1].max + end + + # A sliding window: every bucket covering the last `interval` seconds is summed, not just the current one. + def check(size) + raise RateLimitExceeded if current_total >= @threshold + + add(size) + end + + # Buckets that have fully aged out of the window as of now; the oldest bucket itself is still counted by it. + def prune = model.where(namespace: @namespace).where(bucket: ...oldest_bucket).delete_all + + private + + # The oldest bucket is only ever partially inside the window, so summing from it, not past it, errs strict. + def current_total + model.where(namespace: @namespace, bucket: oldest_bucket..current_bucket).sum(:characters) + end + + def current_bucket = now / @bucket_width + + def oldest_bucket = (now - @interval) / @bucket_width + + def now = @clock.call.to_i + + # One statement, so two processes incrementing the same bucket cannot lose an increment between them. + # A negative size would otherwise hand back headroom it never used, so it is clamped before it reaches SQL. + def add(size) + size = size.to_i.clamp(0..) + model.upsert_all([{ namespace: @namespace, bucket: current_bucket, characters: size }], + **upsert_options(model.connection, size)) + end + + # MySQL's adapter never answers true here and its ON DUPLICATE KEY UPDATE already targets every unique key. + def upsert_options(connection, size) + table = connection.quote_table_name(model.table_name) + options = { on_duplicate: Arel.sql("characters = #{table}.characters + #{size}") } + options[:unique_by] = %i[namespace bucket] if connection.supports_insert_conflict_target? + options + end + + def active_record_feature = "the rate limiter" + def active_record_component = "ActiveRecord rate limiter" + def active_record_upsert_detail = "upsert_all takes unique_by there." +end + +TranslationDiff::RateLimiters.register(:active_record, TranslationDiff::ActiveRecordRateLimiter) diff --git a/lib/translation_diff/active_record_support.rb b/lib/translation_diff/active_record_support.rb new file mode 100644 index 0000000..8092d73 --- /dev/null +++ b/lib/translation_diff/active_record_support.rb @@ -0,0 +1,29 @@ +# The lazy require, the version floor and the anonymous model class, shared by the cache store and the limiter. +module TranslationDiff::ActiveRecordSupport + MINIMUM_ACTIVE_RECORD = "7.1".freeze + + def model + @model ||= build_model + end + + private + + def build_model + require "active_record" + ensure_supported_version! + table = @table_name + Class.new(@base || ::ActiveRecord::Base) { self.table_name = table } + rescue LoadError + raise TranslationDiff::Error, + "#{active_record_feature} is :active_record but the `activerecord` gem is not available. " \ + 'Add `gem "activerecord"` to your Gemfile.' + end + + def ensure_supported_version! + return if Gem::Version.new(::ActiveRecord::VERSION::STRING) >= Gem::Version.new(MINIMUM_ACTIVE_RECORD) + + raise TranslationDiff::Error, + "the #{active_record_component} needs ActiveRecord #{MINIMUM_ACTIVE_RECORD} or newer " \ + "(found #{::ActiveRecord::VERSION::STRING}): #{active_record_upsert_detail}" + end +end diff --git a/lib/translation_diff/cache_guard_options.rb b/lib/translation_diff/cache_guard_options.rb new file mode 100644 index 0000000..53a0f92 --- /dev/null +++ b/lib/translation_diff/cache_guard_options.rb @@ -0,0 +1,38 @@ +# Prepended onto Configuration: fails cache_prune_probability and cache_namespace at configure time, not later. +module TranslationDiff::CacheGuardOptions + CACHE_NAMESPACE_LIMIT = 64 + + # An ENV var arrives as a String; coerced here so a translate call never meets a bare String's missing #positive?. + def cache_prune_probability=(value) + value = nil if value.is_a?(String) && value.strip.empty? + @cache_prune_probability = value.nil? ? nil : coerce_probability(value) + end + + # Refused here, rather than at the first write's ActiveRecord::ValueTooLong. + def cache_namespace=(value) + value = nil if value.is_a?(String) && value.strip.empty? + raise namespace_too_long(value) if value.is_a?(String) && value.length > CACHE_NAMESPACE_LIMIT + + @cache_namespace = value + end + + private + + def coerce_probability(value) + probability = Float(value) + raise probability_out_of_range(value) unless (0..1).cover?(probability) + + probability + rescue ArgumentError, TypeError + raise probability_out_of_range(value) + end + + def probability_out_of_range(value) + TranslationDiff::Error.new("cache_prune_probability must be a number between 0 and 1 (got #{value.inspect})") + end + + def namespace_too_long(value) + TranslationDiff::Error.new("cache_namespace must be #{CACHE_NAMESPACE_LIMIT} characters or fewer " \ + "(got #{value.length})") + end +end diff --git a/lib/translation_diff/cache_ttl_option.rb b/lib/translation_diff/cache_ttl_option.rb new file mode 100644 index 0000000..015f80f --- /dev/null +++ b/lib/translation_diff/cache_ttl_option.rb @@ -0,0 +1,29 @@ +# Prepended onto Configuration: nil sticks here as "never expires", unlike the generic option rule. +module TranslationDiff::CacheTtlOption + NEVER_ASSIGNED = Object.new.freeze + + def initialize + @cache_ttl = NEVER_ASSIGNED + super + end + + # A non-positive number folds into nil too -- a TTL of zero or less can never keep a row. + def cache_ttl=(value) + value = nil if value.is_a?(String) && value.strip.empty? + value = coerce_ttl(value) if value.is_a?(String) + @cache_ttl = value.is_a?(Numeric) && value <= 0 ? nil : value + end + + def cache_ttl + @cache_ttl.equal?(NEVER_ASSIGNED) ? self.class.defaults[:cache_ttl] : @cache_ttl + end + + private + + # An ENV var arrives as a String; coerced here so Time.now.utc + @ttl never meets a bare String mid-translation. + def coerce_ttl(value) + Integer(value) + rescue ArgumentError, TypeError + raise TranslationDiff::Error, "cache_ttl must be a number of seconds (got #{value.inspect})" + end +end diff --git a/lib/translation_diff/configuration.rb b/lib/translation_diff/configuration.rb index 66f92fb..634348a 100644 --- a/lib/translation_diff/configuration.rb +++ b/lib/translation_diff/configuration.rb @@ -44,6 +44,10 @@ def provider_option_owners = @provider_option_owners ||= ProviderOptionOwners.ne option :cache_ttl, 604_800 option :cache_namespace, "translation-diff" option :cache_max_size, 1_000 + option :cache_table_name, "translation_diff_translations" + option :rate_limit_table_name, "translation_diff_rate_limits" + option :active_record_base, nil + option :cache_prune_probability, 0.0 option :redis_url, -> { ENV.fetch("REDIS_URL", nil) } option :redis_pool_size, 5 option :redis_pool_timeout, 5 @@ -58,6 +62,9 @@ def provider_option_owners = @provider_option_owners ||= ProviderOptionOwners.ne option :max_retries, 3 option :validate_languages, true + prepend TranslationDiff::CacheTtlOption + prepend TranslationDiff::CacheGuardOptions + # Credentials are filtered by name; everything else is shown, or an inspect is one nobody reads. def inspect = "#<#{self.class.name} #{TranslationDiff::Redaction.render(self).join(' ')}>" @@ -87,10 +94,9 @@ def segmenter_instance # nil, not a null object: Dispatcher#throttle checks for nil and skips rate-limiting -- costs nothing normally. def rate_limiter_instance - return rate_limiter unless rate_limiter.nil? - return nil if rate_limit.nil? + return nil if rate_limiter.nil? && rate_limit.nil? - @rate_limiter_instance ||= TranslationDiff::RedisRateLimiter.build(self) + @rate_limiter_instance ||= resolve(rate_limiter || :redis, TranslationDiff::RateLimiters) end # One pool shared by the cache store and the rate limiter; callers used to build and pass it by hand. diff --git a/lib/translation_diff/memory_cache_store.rb b/lib/translation_diff/memory_cache_store.rb index d7871aa..be7ba76 100644 --- a/lib/translation_diff/memory_cache_store.rb +++ b/lib/translation_diff/memory_cache_store.rb @@ -18,6 +18,10 @@ def write(key, value) value end + def write_multi(pairs) + pairs.each { |key, value| write(key, value) } + end + private def touch(key) diff --git a/lib/translation_diff/railtie.rb b/lib/translation_diff/railtie.rb new file mode 100644 index 0000000..befe493 --- /dev/null +++ b/lib/translation_diff/railtie.rb @@ -0,0 +1,12 @@ +# Loaded only when Rails already is (see the guarded require in translation_diff.rb), never on its own. +require "active_support/core_ext/module/delegation" +require "rails/railtie" + +# Adds translation_diff:prune to a host Rails application's own rake tasks; the gem's dev Rakefile loads the +# same task file directly, so a host application's `rake -T` and this gem's own suite see one definition. +class TranslationDiff::Railtie < Rails::Railtie + rake_tasks do + load File.expand_path("tasks/translation_diff.rake", __dir__) + Rake::Task["translation_diff:prune"].enhance(["environment"]) + end +end diff --git a/lib/translation_diff/rate_limiters.rb b/lib/translation_diff/rate_limiters.rb new file mode 100644 index 0000000..5ae854e --- /dev/null +++ b/lib/translation_diff/rate_limiters.rb @@ -0,0 +1,2 @@ +# Rate limiters, by name; assigning an object to `config.rate_limiter` bypasses this entirely. +TranslationDiff::RateLimiters = TranslationDiff::Registry.new("rate limiter") diff --git a/lib/translation_diff/redis_cache_store.rb b/lib/translation_diff/redis_cache_store.rb index 9dde87d..cf3dabc 100644 --- a/lib/translation_diff/redis_cache_store.rb +++ b/lib/translation_diff/redis_cache_store.rb @@ -17,8 +17,16 @@ def read_multi(keys) redis { |redis| redis.mget(*keys) } end + # A non-positive or nil timeout means never expires, the same rule the SQL store applies to cache_ttl. def write(key, value) - redis { |redis| redis.setex(key, timeout, value) } + redis { |redis| write_one(redis, key, value) } + end + + def write_multi(pairs) + return pairs if pairs.empty? + + redis { |redis| redis.pipelined { |p| pairs.each { |key, value| write_one(p, key, value) } } } + pairs end private @@ -30,6 +38,12 @@ def redis yield Redis::Namespace.new(namespace, redis: redis) end end + + def write_one(redis, key, value) + expiring? ? redis.setex(key, timeout, value) : redis.set(key, value) + end + + def expiring? = timeout.is_a?(Numeric) && timeout.positive? end TranslationDiff::Stores.register(:redis, TranslationDiff::RedisCacheStore) diff --git a/lib/translation_diff/redis_rate_limiter.rb b/lib/translation_diff/redis_rate_limiter.rb index fd42ed1..ff1d895 100644 --- a/lib/translation_diff/redis_rate_limiter.rb +++ b/lib/translation_diff/redis_rate_limiter.rb @@ -51,3 +51,5 @@ def ratelimit_class 'Add `gem "ratelimit"` to your Gemfile.' end end + +TranslationDiff::RateLimiters.register(:redis, TranslationDiff::RedisRateLimiter) diff --git a/lib/translation_diff/sentence_cache.rb b/lib/translation_diff/sentence_cache.rb index 9c18207..2a298eb 100644 --- a/lib/translation_diff/sentence_cache.rb +++ b/lib/translation_diff/sentence_cache.rb @@ -31,12 +31,21 @@ def fill(segments) end # Writes back only the segments that carry a translation; an untranslated segment has nothing worth caching. + # A store that batches gets one call; one that does not keeps the per-key contract it was written against. def store(segments) - segments.select(&:translated?).each { |segment| @store.write(key(segment), segment.translation) } + translated = segments.select(&:translated?) + return translated if translated.empty? + return translated.each { |segment| @store.write(key(segment), segment.translation) } if legacy_store? + + @store.write_multi(translated.map { |segment| [key(segment), segment.translation] }) + translated end private + # A store written against the write-only contract, before write_multi existed, cannot be handed a batch. + def legacy_store? = !@store.respond_to?(:write_multi) + # No options contributes no field at all, which is the four-field key every already-warm cache is keyed on. def options_digest return [] if @options.empty? diff --git a/lib/translation_diff/tasks/translation_diff.rake b/lib/translation_diff/tasks/translation_diff.rake new file mode 100644 index 0000000..19470a6 --- /dev/null +++ b/lib/translation_diff/tasks/translation_diff.rake @@ -0,0 +1,21 @@ +# Shipped in the gem so a host application's own `rake` sees it -- a dependency's Rakefile is never loaded. +namespace :translation_diff do + desc "Delete expired rows from the SQL cache and rate-limit tables" + task :prune do + require "translation_diff" + + cache_store = TranslationDiff.config.cache_store + if cache_store.respond_to?(:prune) + puts "pruned #{cache_store.prune} expired cache rows" + else + puts "the configured cache store (#{cache_store.class}) does not support pruning" + end + + rate_limiter = TranslationDiff.config.rate_limiter_instance + if rate_limiter.respond_to?(:prune) + puts "pruned #{rate_limiter.prune} expired rate-limit rows" + else + puts "the configured rate limiter (#{rate_limiter.class}) does not support pruning" + end + end +end diff --git a/test/support/active_record_database.rb b/test/support/active_record_database.rb new file mode 100644 index 0000000..f755bf6 --- /dev/null +++ b/test/support/active_record_database.rb @@ -0,0 +1,62 @@ +# SQLite by default so the suite needs no service; TRANSLATION_DIFF_DATABASE_URL points it at Postgres in CI. +module ActiveRecordDatabase + def self.available? + return @available if defined?(@available) + + @available = begin + require "active_record" + true + rescue LoadError + false + end + end + + def self.connect! + ::ActiveRecord::Base.establish_connection(url || { adapter: "sqlite3", database: ":memory:" }) + define_schema + end + + def self.url = ENV.fetch("TRANSLATION_DIFF_DATABASE_URL", nil) + + # Every test file that needs a real PostgreSQL (not SQLite's single writer, not MySQL's forgiving transactions) + # asks here, so the detection lives in one place instead of a same-named constant defined in each of them. + def self.postgres? = available? && url.to_s.match?(%r{\Apostgres(ql)?://}) + + # Both tables so Task 3's migration has something to be checked against; only the first is used so far. + def self.define_schema + connection = ::ActiveRecord::Base.connection + return if connection.table_exists?(:translation_diff_translations) + + define_translations_table(connection) + define_rate_limits_table(connection) + end + + def self.define_translations_table(connection) + connection.create_table :translation_diff_translations do |t| + t.string :namespace, null: false, limit: 64 + t.string :key_digest, null: false, limit: 64 + t.text :translation, null: false + t.datetime :expires_at + t.timestamps + end + connection.add_index :translation_diff_translations, %i[namespace key_digest], + unique: true, name: "index_translation_diff_translations_on_key" + connection.add_index :translation_diff_translations, :expires_at + end + + def self.define_rate_limits_table(connection) + connection.create_table :translation_diff_rate_limits do |t| + t.string :namespace, null: false, limit: 64 + t.bigint :bucket, null: false + t.integer :characters, null: false, default: 0 + end + connection.add_index :translation_diff_rate_limits, %i[namespace bucket], + unique: true, name: "index_translation_diff_rate_limits_on_bucket" + end + + # Wipes both tables between tests; a fresh in-memory SQLite has nothing else to reset. + def self.truncate + ::ActiveRecord::Base.connection.execute("DELETE FROM translation_diff_translations") + ::ActiveRecord::Base.connection.execute("DELETE FROM translation_diff_rate_limits") + end +end diff --git a/test/support/batching_cache_store_contract.rb b/test/support/batching_cache_store_contract.rb new file mode 100644 index 0000000..bb4c574 --- /dev/null +++ b/test/support/batching_cache_store_contract.rb @@ -0,0 +1,21 @@ +# The optional half of the cache store contract -- include only in a store that implements write_multi. +module BatchingCacheStoreContract + def test_write_multi_writes_every_pair + store.write_multi([%w[a one], %w[b two]]) + + assert_equal %w[one two], store.read_multi(%w[a b]) + end + + def test_write_multi_of_no_pairs_writes_nothing + store.write_multi([]) + + assert_empty store.read_multi([]) + end + + def test_write_multi_replaces_a_key_written_before + store.write("a", "one") + store.write_multi([%w[a two]]) + + assert_equal ["two"], store.read_multi(["a"]) + end +end diff --git a/test/support/cache_store_contract.rb b/test/support/cache_store_contract.rb index d094402..fc6200b 100644 --- a/test/support/cache_store_contract.rb +++ b/test/support/cache_store_contract.rb @@ -1,4 +1,4 @@ -# The executable form of the cache store contract; anything that passes can be TranslationDiff's cache. +# The executable form of the required cache store contract; anything that passes can be TranslationDiff's cache. module CacheStoreContract def test_write_then_read_multi_returns_the_value store.write("a", "one") diff --git a/test/support/rate_limiter_contract.rb b/test/support/rate_limiter_contract.rb new file mode 100644 index 0000000..b1b0989 --- /dev/null +++ b/test/support/rate_limiter_contract.rb @@ -0,0 +1,18 @@ +# What every rate limiter must do; a contract only one implementation runs is not a contract. +module RateLimiterContract + def test_a_check_under_the_threshold_passes + build_limiter(threshold: 100, interval: 60).check(50) + + assert true # reaching here means #check did not raise + end + + def test_checks_summing_past_the_threshold_raise + build_limiter(threshold: 100, interval: 60).check(70) + build_limiter(threshold: 100, interval: 60).check(30) + + assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 100, interval: 60).check(1) } + end + + # Rollover is not in this contract: proving it means waiting for a bucket to turn over, and only + # ActiveRecordRateLimiter can be made to turn one over without a real sleep. See its own test file. +end diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb new file mode 100644 index 0000000..725d721 --- /dev/null +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -0,0 +1,177 @@ +require "test_helper" +require "support/cache_store_contract" +require "support/batching_cache_store_contract" +require "support/active_record_database" + +if ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + class ActiveRecordCacheStoreTest < Minitest::Test + include CacheStoreContract + include BatchingCacheStoreContract + + attr_reader :store + + def setup + ActiveRecordDatabase.truncate + @store = build_store + end + + def model = store.model + + def digest_of(key) = Digest::SHA256.hexdigest(key) + + def test_an_expired_row_is_not_read + store.write("a", "one") + model.update_all(expires_at: Time.now.utc - 1) + + assert_equal [nil], store.read_multi(["a"]) + end + + def test_two_namespaces_do_not_see_each_other + store.write("a", "one") + other = build_store(namespace: "other-tenant") + + assert_equal [nil], other.read_multi(["a"]) + end + + def test_the_cache_key_itself_is_never_stored + store.write("deepl:en:ru:abc", "one") + + refute_includes model.first.attributes.values.join, "deepl:en:ru:abc" + end + + def test_prune_deletes_expired_rows_and_leaves_live_ones + store.write("live", "one") + store.write("dead", "two") + model.where(key_digest: digest_of("dead")).update_all(expires_at: Time.now.utc - 1) + + assert_equal 1, store.prune + assert_equal ["one"], store.read_multi(["live"]) + end + + def test_a_nil_cache_ttl_writes_a_row_that_never_expires + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_ttl = nil + + TranslationDiff::ActiveRecordCacheStore.build(config).write("a", "one") + + assert_nil model.first.expires_at + end + + def test_a_zero_cache_ttl_writes_a_row_that_never_expires_instead_of_already_expired + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_ttl = 0 + + TranslationDiff::ActiveRecordCacheStore.build(config).write("a", "one") + + assert_nil model.first.expires_at + end + + def test_prune_only_deletes_rows_in_its_own_namespace + other = build_store(namespace: "other-tenant") + expire(store, "a") + expire(other, "b") + + assert_equal 1, store.prune + assert_equal 1, model.count + end + + def test_write_multi_with_the_same_key_twice_in_one_batch_stores_the_last_value + store.write_multi([%w[a one], %w[a two]]) + + assert_equal ["two"], store.read_multi(["a"]) + end + + def test_write_after_write_multi_still_replaces_the_key + store.write_multi([%w[a one], %w[a two]]) + store.write("a", "three") + + assert_equal ["three"], store.read_multi(["a"]) + end + + def test_build_takes_its_settings_from_the_configuration + config = TranslationDiff::Configuration.new + config.cache_namespace = "from-config" + config.cache_table_name = "translation_diff_translations" + + built = TranslationDiff::ActiveRecordCacheStore.build(config) + built.write("a", "one") + + assert_equal "from-config", built.model.first.namespace + end + + def test_a_string_cache_ttl_from_env_does_not_raise_on_write + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_ttl = "3600" + + built = TranslationDiff::ActiveRecordCacheStore.build(config) + built.write("a", "one") + + refute_nil built.model.first.expires_at + end + + def test_a_string_cache_prune_probability_from_env_does_not_raise_on_write + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_prune_probability = "0.5" + + TranslationDiff::ActiveRecordCacheStore.build(config).write("a", "one") + end + + def test_write_multi_omits_unique_by_when_the_connection_does_not_support_a_conflict_target + connection = Class.new { def supports_insert_conflict_target? = false }.new + + options = store.send(:upsert_options, connection) + + refute_includes options.keys, :unique_by + end + + def test_write_multi_keeps_unique_by_when_the_connection_supports_a_conflict_target + connection = Class.new { def supports_insert_conflict_target? = true }.new + + options = store.send(:upsert_options, connection) + + assert_includes options.keys, :unique_by + end + + # Naming the bare `ActiveRecord` constant in the rescue clause used to raise a raw NameError instead of this + # gem's own message, genuinely reproduced here rather than merely simulated by stubbing #require. + def test_write_raises_a_friendly_error_when_active_record_is_genuinely_unavailable + removed = Object.send(:remove_const, :ActiveRecord) + broken_store = build_store + broken_store.define_singleton_method(:require) { |*| raise LoadError } + + error = assert_raises(TranslationDiff::Error) { broken_store.write("a", "one") } + + assert_match(/`activerecord` gem is not available/, error.message) + ensure + Object.const_set(:ActiveRecord, removed) if removed + end + + private + + def build_store(namespace: "translation-diff", ttl: 604_800) + TranslationDiff::ActiveRecordCacheStore.new(namespace: namespace, ttl: ttl, + table_name: "translation_diff_translations") + end + + def expire(store, key) + store.write(key, key) + store.model.where(key_digest: digest_of(key)).update_all(expires_at: Time.now.utc - 1) + end + end +else + class ActiveRecordCacheStoreTest < Minitest::Test + def test_active_record_is_unavailable + skip "active_record could not be loaded on this Ruby; the SQL cache store suite is skipped" + end + end +end diff --git a/test/translation_diff/active_record_concurrency_test.rb b/test/translation_diff/active_record_concurrency_test.rb new file mode 100644 index 0000000..3b9d430 --- /dev/null +++ b/test/translation_diff/active_record_concurrency_test.rb @@ -0,0 +1,56 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # SQLite has one writer, so only Postgres can put these properties under a real race. + class ActiveRecordConcurrencyTest < Minitest::Test + def setup + ActiveRecordDatabase.truncate + end + + # Two connections upserting the same unique-index row: no exception, and last write standing wins. + def test_two_writers_racing_the_same_cache_key_do_not_raise_and_one_value_wins + key = "concurrent-key" + values = %w[first second] + + threads = values.map { |value| Thread.new { build_cache_store.write(key, value) } } + threads.each(&:join) + + assert_includes values, build_cache_store.read_multi([key]).first + end + + # Two connections upserting-and-incrementing the same bucket a hundred times each: no increment lost. + def test_two_limiters_racing_the_same_bucket_lose_no_increment + now = Time.now + totals = build_rate_limiter(now).model + + threads = Array.new(2) { Thread.new { 100.times { build_rate_limiter(now).check(1) } } } + threads.each(&:join) + + assert_equal 200, totals.sum(:characters) + end + + private + + def build_cache_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: 60, + table_name: "translation_diff_translations") + end + + # A shared, frozen clock keeps both limiters in the same bucket for the length of the test. + def build_rate_limiter(now) + TranslationDiff::ActiveRecordRateLimiter.new(namespace: "translation-diff", threshold: 1_000_000, + interval: 60, table_name: "translation_diff_rate_limits", + clock: -> { now }) + end + end +else + class ActiveRecordConcurrencyTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "SQLite has one writer and cannot exercise these races" + end + end +end diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb new file mode 100644 index 0000000..00b152d --- /dev/null +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -0,0 +1,209 @@ +require "test_helper" +require "support/rate_limiter_contract" +require "support/active_record_database" + +if ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + # Moves without sleeping, so a bucket can be made to roll over on demand -- and holds still, so a real + # boundary cannot fall between two reads of it, which is what made the prune tests flake. + class MutableClock + def initialize(now) = @now = now + def call = @now + def advance(seconds) = @now += seconds + end + + class ActiveRecordRateLimiterTest < Minitest::Test + include RateLimiterContract + + def setup + ActiveRecordDatabase.truncate + end + + # The window covers interval..interval + bucket_width seconds, erring strict: the oldest bucket is only ever + # partially inside it, so a full interval alone does not guarantee it has rolled past -- one more bucket does. + def test_a_window_that_has_rolled_over_passes_again + clock = MutableClock.new(Time.now) + + build_limiter(threshold: 10, interval: 60, clock: clock).check(10) + assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 10, interval: 60, clock: clock).check(1) } + + clock.advance(65) + + build_limiter(threshold: 10, interval: 60, clock: clock).check(1) + end + + def model + TranslationDiff::ActiveRecordRateLimiter.new(namespace: "translation-diff", + table_name: "translation_diff_rate_limits").model + end + + def test_two_limiters_sharing_a_namespace_see_each_others_characters + first = build_limiter(threshold: 100, interval: 60) + second = build_limiter(threshold: 100, interval: 60) + + first.check(60) + second.check(40) + + assert_raises(rate_limit_exceeded_error) { first.check(1) } + end + + def test_two_namespaces_do_not_see_each_other + tenant_a = build_limiter(threshold: 100, interval: 60, namespace: "tenant-a") + tenant_b = build_limiter(threshold: 100, interval: 60, namespace: "tenant-b") + + tenant_a.check(90) + tenant_b.check(90) # would raise if the namespaces were not isolated + + assert true + end + + # int4 overflows at 2**31; any rate_interval under 24 makes the bucket the epoch second, which crosses that + # boundary in January 2038 -- bucket is bigint precisely so a real epoch-second value like this one fits. + def test_a_bucket_beyond_int32_range_is_stored_and_read_back + far_future_bucket = (2**31) + 1 + + model.create!(namespace: "translation-diff", bucket: far_future_bucket, characters: 5) + + assert_equal far_future_bucket, model.find_by(namespace: "translation-diff").bucket + end + + def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one + limiter = build_limiter(threshold: 1000, interval: 60, clock: frozen_clock) + model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket) - 1, characters: 5) + + limiter.check(10) + deleted = limiter.prune + + assert_equal 1, deleted + assert_equal [limiter.send(:current_bucket)], model.pluck(:bucket) + end + + # The oldest bucket is only ever partially inside the window (see current_total), so prune leaving it alone + # is what keeps pruning from quietly undoing the strictness that sum starting at oldest_bucket relies on. + def test_prune_leaves_the_oldest_bucket_because_the_window_still_counts_it + limiter = build_limiter(threshold: 1000, interval: 60, clock: frozen_clock) + model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket), characters: 5) + + deleted = limiter.prune + + assert_equal 0, deleted + assert_equal [limiter.send(:oldest_bucket)], model.pluck(:bucket) + end + + def test_prune_only_deletes_rows_in_its_own_namespace + own = build_limiter(threshold: 1000, interval: 60, clock: frozen_clock) + model.create!(namespace: "translation-diff", bucket: own.send(:oldest_bucket) - 1, characters: 5) + model.create!(namespace: "other-tenant", bucket: own.send(:oldest_bucket) - 1, characters: 5) + + assert_equal 1, own.prune + assert_equal 1, model.where(namespace: "other-tenant").count + end + + # The bug this review caught: a tumbling counter let a nominal 8000/60s throttle through twice, back to back. + def test_a_check_at_the_end_of_one_window_still_counts_two_seconds_into_the_next + clock = MutableClock.new(Time.at(59)) + limiter = build_limiter(threshold: 8000, interval: 60, clock: clock) + + limiter.check(8000) + clock.advance(2) + + assert_raises(rate_limit_exceeded_error) { limiter.check(8000) } + end + + # The dropped partial oldest bucket: 56 real seconds have passed, well inside a true 60-second window, but + # a tumbling (oldest_bucket + 1) start point had already stopped counting the first deposit's bucket. + def test_a_deposit_inside_the_window_still_blocks_a_later_check + clock = MutableClock.new(Time.at(4)) + limiter = build_limiter(threshold: 8000, interval: 60, clock: clock) + + limiter.check(8000) + clock.advance(56) + + assert_raises(rate_limit_exceeded_error) { limiter.check(1) } + end + + def test_a_negative_size_does_not_hand_back_headroom + limiter = build_limiter(threshold: 1000, interval: 60) + limiter.check(500) + + limiter.check(-1000) + + assert_equal 500, model.sum(:characters) + end + + # `size` is interpolated into the on_duplicate SQL fragment, so a value with no clean integer must not reach it. + def test_a_non_integer_size_only_contributes_its_leading_digits + limiter = build_limiter(threshold: 1_000, interval: 60) + + limiter.check("5); DROP TABLE translation_diff_rate_limits; --") + + assert_equal 5, model.sum(:characters) + assert_equal [5], model.pluck(:characters) + end + + def test_build_takes_its_settings_from_the_configuration + config = TranslationDiff::Configuration.new + config.cache_namespace = "from-config" + config.rate_limit_table_name = "translation_diff_rate_limits" + config.rate_limit = 100 + config.rate_interval = 60 + + built = TranslationDiff::ActiveRecordRateLimiter.build(config) + built.check(1) + + assert_equal ["from-config"], built.model.pluck(:namespace) + end + + def test_add_omits_unique_by_when_the_connection_does_not_support_a_conflict_target + limiter = build_limiter(threshold: 100, interval: 60) + connection = Class.new do + def supports_insert_conflict_target? = false + def quote_table_name(name) = %("#{name}") + end.new + + options = limiter.send(:upsert_options, connection, 1) + + refute_includes options.keys, :unique_by + end + + def test_add_keeps_unique_by_when_the_connection_supports_a_conflict_target + limiter = build_limiter(threshold: 100, interval: 60) + connection = Class.new do + def supports_insert_conflict_target? = true + def quote_table_name(name) = %("#{name}") + end.new + + options = limiter.send(:upsert_options, connection, 1) + + assert_includes options.keys, :unique_by + end + + def test_add_quotes_the_table_name_in_the_on_duplicate_fragment + limiter = build_limiter(threshold: 100, interval: 60) + connection = limiter.model.connection + + options = limiter.send(:upsert_options, connection, 1) + + assert_includes options[:on_duplicate].to_s, connection.quote_table_name(limiter.model.table_name) + end + + private + + def rate_limit_exceeded_error = TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded + + # Held still, so a five-second bucket boundary cannot fall between two reads of the clock. + def frozen_clock = MutableClock.new(Time.at(1_700_000_000)) + + def build_limiter(threshold:, interval:, namespace: "translation-diff", clock: -> { Time.now }) + TranslationDiff::ActiveRecordRateLimiter.new(namespace: namespace, threshold: threshold, interval: interval, + table_name: "translation_diff_rate_limits", clock: clock) + end + end +else + class ActiveRecordRateLimiterTest < Minitest::Test + def test_active_record_is_unavailable + skip "active_record could not be loaded on this Ruby; the SQL rate limiter suite is skipped" + end + end +end diff --git a/test/translation_diff/active_record_redaction_test.rb b/test/translation_diff/active_record_redaction_test.rb new file mode 100644 index 0000000..d9e3b62 --- /dev/null +++ b/test/translation_diff/active_record_redaction_test.rb @@ -0,0 +1,65 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # upsert_all inlines values into the SQL it sends, so PostgreSQL's own error detail can carry a whole row; + # only a real constraint violation against a real server reproduces that, hence the PostgreSQL gate. + class ActiveRecordCacheStoreRedactionTest < Minitest::Test + CONSTRAINT = "no_forbidden_namespace_in_redaction_test".freeze + + def setup + ActiveRecordDatabase.truncate + connection.execute("ALTER TABLE translation_diff_translations ADD CONSTRAINT #{CONSTRAINT} " \ + "CHECK (namespace <> 'forbidden-namespace')") + end + + def teardown + connection.execute("ALTER TABLE translation_diff_translations DROP CONSTRAINT IF EXISTS #{CONSTRAINT}") + end + + def test_a_statement_invalid_never_carries_the_translated_content + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "forbidden-namespace", ttl: 60, + table_name: "translation_diff_translations") + + error = assert_raises(TranslationDiff::Error) { store.write("a", "SECRET-PATIENT-NOTE-12345") } + + refute_includes error.message, "SECRET-PATIENT-NOTE-12345" + end + + def test_the_redacted_error_names_the_adapters_own_error_class + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "forbidden-namespace", ttl: 60, + table_name: "translation_diff_translations") + + error = assert_raises(TranslationDiff::Error) { store.write("a", "one") } + + assert_includes error.message, "PG::CheckViolation" + end + + # Ruby attaches the rescued original as #cause unless the raise says otherwise -- and #cause carries the row. + def test_the_redacted_error_severs_the_cause_chain + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "forbidden-namespace", ttl: 60, + table_name: "translation_diff_translations") + + error = assert_raises(TranslationDiff::Error) { store.write("a", "SECRET-PATIENT-NOTE-12345") } + + assert_nil error.cause + refute_includes error.full_message, "SECRET-PATIENT-NOTE-12345" + end + + private + + def connection + TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, + table_name: "translation_diff_translations").model.connection + end + end +else + class ActiveRecordCacheStoreRedactionTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "a check violation's row detail is what this test reproduces" + end + end +end diff --git a/test/translation_diff/active_record_support_test.rb b/test/translation_diff/active_record_support_test.rb new file mode 100644 index 0000000..af353dd --- /dev/null +++ b/test/translation_diff/active_record_support_test.rb @@ -0,0 +1,30 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + class ActiveRecordSupportTest < Minitest::Test + def test_the_cache_store_and_the_rate_limiter_share_the_same_active_record_plumbing + assert_includes TranslationDiff::ActiveRecordCacheStore.ancestors, TranslationDiff::ActiveRecordSupport + assert_includes TranslationDiff::ActiveRecordRateLimiter.ancestors, TranslationDiff::ActiveRecordSupport + end + + def test_the_version_floor_is_declared_once_and_shared + assert_same TranslationDiff::ActiveRecordSupport::MINIMUM_ACTIVE_RECORD, + TranslationDiff::ActiveRecordCacheStore::MINIMUM_ACTIVE_RECORD + assert_same TranslationDiff::ActiveRecordSupport::MINIMUM_ACTIVE_RECORD, + TranslationDiff::ActiveRecordRateLimiter::MINIMUM_ACTIVE_RECORD + end + + def test_each_store_instance_memoises_its_own_model_rather_than_sharing_one + first = TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: nil, + table_name: "translation_diff_translations") + second = TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: nil, + table_name: "translation_diff_translations") + + assert_same first.model, first.model + refute_same first.model, second.model + end + end +end diff --git a/test/translation_diff/active_record_transaction_test.rb b/test/translation_diff/active_record_transaction_test.rb new file mode 100644 index 0000000..1a036a7 --- /dev/null +++ b/test/translation_diff/active_record_transaction_test.rb @@ -0,0 +1,142 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # PostgreSQL aborts the whole transaction on a statement error; only there can poisoning actually be measured. + class ActiveRecordCacheStoreTransactionTest < Minitest::Test + def setup + ActiveRecordDatabase.truncate + end + + # The scenario the review measured: save something, translate (cache write fails), rescue, keep saving. + def test_a_failing_write_leaves_the_callers_transaction_usable + harness = harness_model + + harness.transaction do + begin + failing_store.write("a", "one") + rescue StandardError + nil + end + + assert harness.create!(namespace: "harness", key_digest: "d" * 64, translation: "still usable") + end + + assert_equal 1, harness.where(namespace: "harness").count + end + + def test_a_successful_write_still_lands + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, + table_name: "translation_diff_translations") + + harness_model.transaction { store.write("a", "one") } + + assert_equal ["one"], store.read_multi(["a"]) + end + + # The path the review found reachable again: prune runs after write's own savepoint has already closed. + def test_a_failing_prune_leaves_the_callers_transaction_usable + store = store_with_a_pending_prune + harness = harness_model + + harness.transaction do + write_ignoring_errors(store, "fresh", "two") + + assert harness.create!(namespace: "harness", key_digest: "d" * 64, translation: "still usable") + end + + assert_equal 1, harness.where(namespace: "harness").count + ensure + remove_failing_delete_trigger + end + + # The write itself still lands even though the prune that follows it fails, since each has its own savepoint. + def test_a_failing_prune_does_not_undo_the_write_that_triggered_it + store = store_with_a_pending_prune + + write_ignoring_errors(store, "fresh", "two") + + assert_equal ["two"], store.read_multi(["fresh"]) + ensure + remove_failing_delete_trigger + end + + # The regression the review found: a failing prune was reported as the wrong statement, an upsert. + def test_a_failing_prunes_error_describes_the_delete_not_the_upsert + store = store_with_a_pending_prune + + error = assert_raises(TranslationDiff::Error) { store.write("fresh", "two") } + + refute_includes error.message, "upsert" + assert_includes error.message, "prune" + ensure + remove_failing_delete_trigger + end + + private + + # An expired row plus a trigger that always fails its DELETE, so the write that follows always triggers a prune. + def store_with_a_pending_prune + store = pruning_store + store.write("expired", "one") + expire(store, "expired") + install_failing_delete_trigger + store + end + + def pruning_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: 60, + table_name: "translation_diff_translations", + prune_probability: 1.0) + end + + def expire(store, key) + digest = Digest::SHA256.hexdigest(key) + store.model.where(key_digest: digest).update_all(expires_at: Time.now.utc - 1) + end + + def write_ignoring_errors(store, key, value) + store.write(key, value) + rescue StandardError + nil + end + + # A trigger, not a constraint: only a per-row trigger can make the DELETE itself fail, not an INSERT. + def install_failing_delete_trigger + harness_model.connection.execute(<<~SQL) + CREATE OR REPLACE FUNCTION translation_diff_transaction_test_fail_delete() RETURNS trigger AS $$ + BEGIN RAISE EXCEPTION 'simulated prune failure'; END; $$ LANGUAGE plpgsql; + CREATE TRIGGER translation_diff_transaction_test_fail_delete BEFORE DELETE + ON translation_diff_translations FOR EACH ROW + EXECUTE FUNCTION translation_diff_transaction_test_fail_delete(); + SQL + end + + def remove_failing_delete_trigger + connection = harness_model.connection + connection.execute("DROP TRIGGER IF EXISTS translation_diff_transaction_test_fail_delete " \ + "ON translation_diff_translations") + connection.execute("DROP FUNCTION IF EXISTS translation_diff_transaction_test_fail_delete()") + end + + def harness_model + TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, + table_name: "translation_diff_translations").model + end + + # A namespace past the column's 64-character limit is a statement PostgreSQL always rejects. + def failing_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "x" * 100, ttl: 60, + table_name: "translation_diff_translations") + end + end +else + class ActiveRecordCacheStoreTransactionTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "only PostgreSQL aborts a transaction on a statement error" + end + end +end diff --git a/test/translation_diff/active_record_write_multi_dedupe_test.rb b/test/translation_diff/active_record_write_multi_dedupe_test.rb new file mode 100644 index 0000000..02a1716 --- /dev/null +++ b/test/translation_diff/active_record_write_multi_dedupe_test.rb @@ -0,0 +1,52 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # write_multi dedupes pairs.to_h before the upsert; without that, PostgreSQL raises "ON CONFLICT DO UPDATE + # command cannot affect row a second time" the moment a batch repeats a key -- only PostgreSQL enforces this. + class ActiveRecordWriteMultiDedupeTest < Minitest::Test + def setup + ActiveRecordDatabase.truncate + end + + def test_write_multi_does_not_raise_when_a_document_repeats_the_same_sentence + store = build_store + + store.write_multi([%w[a one], %w[a two]]) + + assert_equal ["two"], store.read_multi(["a"]) + end + + # Proves the dedupe in write_multi is load-bearing, by running the same batch through the undeduped shape + # write_multi builds internally -- without pairs.to_h, this exact scenario raises on PostgreSQL. + def test_without_the_dedupe_postgresql_raises_on_a_repeated_key_in_one_batch + store = build_store + pairs = [%w[a one], %w[a two]] + + error = assert_raises(ActiveRecord::StatementInvalid) do + store.model.transaction(requires_new: true) do + rows = pairs.map { |key, value| store.send(:row, key, value) } + store.model.upsert_all(rows, unique_by: %i[namespace key_digest], record_timestamps: true) + end + end + + assert_match(/cannot affect row a second time/, error.message) + end + + private + + def build_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: 60, + table_name: "translation_diff_translations") + end + end +else + class ActiveRecordWriteMultiDedupeTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "only PostgreSQL raises when an upsert batch repeats a conflict key" + end + end +end diff --git a/test/translation_diff/cache_store_contract_test.rb b/test/translation_diff/cache_store_contract_test.rb new file mode 100644 index 0000000..c1fe4e8 --- /dev/null +++ b/test/translation_diff/cache_store_contract_test.rb @@ -0,0 +1,19 @@ +require "test_helper" +require "support/cache_store_contract" + +class CacheStoreContractTest < Minitest::Test + include CacheStoreContract + + # Exactly the two required methods, nothing else -- proves the required contract never needs write_multi. + class MinimalStore + def initialize = @entries = {} + def read_multi(keys) = keys.map { |key| @entries[key] } + def write(key, value) = @entries[key] = value + end + + attr_reader :store + + def setup + @store = MinimalStore.new + end +end diff --git a/test/translation_diff/configuration_test.rb b/test/translation_diff/configuration_test.rb index d6bb527..3d59820 100644 --- a/test/translation_diff/configuration_test.rb +++ b/test/translation_diff/configuration_test.rb @@ -41,6 +41,88 @@ def test_an_option_returns_its_default_until_it_is_assigned assert_equal 60, @config.cache_ttl end + def test_a_nil_cache_ttl_sticks_instead_of_falling_back_to_the_default + @config.cache_ttl = nil + + assert_nil @config.cache_ttl + end + + def test_a_zero_cache_ttl_also_means_never_expires + @config.cache_ttl = 0 + + assert_nil @config.cache_ttl + end + + def test_a_negative_cache_ttl_also_means_never_expires + @config.cache_ttl = -1 + + assert_nil @config.cache_ttl + end + + def test_cache_ttl_coerces_a_numeric_string_the_way_an_env_var_arrives + @config.cache_ttl = "3600" + + assert_equal 3600, @config.cache_ttl + end + + def test_a_coerced_non_positive_cache_ttl_string_also_means_never_expires + @config.cache_ttl = "0" + + assert_nil @config.cache_ttl + end + + def test_cache_ttl_refuses_a_non_numeric_string_with_a_clear_message + error = assert_raises(TranslationDiff::Error) { @config.cache_ttl = "lots" } + + assert_match(/cache_ttl/, error.message) + end + + def test_cache_prune_probability_coerces_a_numeric_string_the_way_an_env_var_arrives + @config.cache_prune_probability = "0.5" + + assert_in_delta 0.5, @config.cache_prune_probability + end + + def test_cache_prune_probability_refuses_a_non_numeric_string_with_a_clear_message + error = assert_raises(TranslationDiff::Error) { @config.cache_prune_probability = "lots" } + + assert_match(/cache_prune_probability/, error.message) + end + + def test_cache_prune_probability_refuses_a_value_above_one + error = assert_raises(TranslationDiff::Error) { @config.cache_prune_probability = 2.0 } + + assert_match(/between 0 and 1/, error.message) + end + + def test_cache_prune_probability_refuses_a_negative_value + error = assert_raises(TranslationDiff::Error) { @config.cache_prune_probability = -1 } + + assert_match(/between 0 and 1/, error.message) + end + + def test_cache_prune_probability_accepts_the_boundary_values + @config.cache_prune_probability = 0 + + assert_in_delta 0.0, @config.cache_prune_probability + + @config.cache_prune_probability = 1 + + assert_in_delta 1.0, @config.cache_prune_probability + end + + def test_cache_namespace_longer_than_64_characters_is_refused_at_configure_time + error = assert_raises(TranslationDiff::Error) { @config.cache_namespace = "n" * 65 } + + assert_match(/64/, error.message) + end + + def test_cache_namespace_at_the_64_character_limit_is_accepted + @config.cache_namespace = "n" * 64 + + assert_equal "n" * 64, @config.cache_namespace + end + def test_a_callable_default_is_evaluated_on_every_read_not_at_load_time original = ENV.fetch("REDIS_URL", nil) ENV["REDIS_URL"] = "redis://first" @@ -316,6 +398,30 @@ def test_a_rate_limit_builds_a_redis_rate_limiter assert_instance_of TranslationDiff::RedisRateLimiter, @config.rate_limiter_instance end + def test_a_symbol_rate_limiter_resolves_through_the_registry + @config.rate_limit = 100 + @config.rate_limiter = :active_record + + assert_instance_of TranslationDiff::ActiveRecordRateLimiter, @config.rate_limiter_instance + end + + def test_a_string_rate_limiter_resolves_through_the_registry + @config.rate_limit = 100 + @config.rate_limiter = "active_record" + + assert_instance_of TranslationDiff::ActiveRecordRateLimiter, @config.rate_limiter_instance + end + + def test_an_unknown_rate_limiter_name_raises_listing_what_is_registered + @config.rate_limit = 100 + @config.rate_limiter = :nonsense + + error = assert_raises(TranslationDiff::Error) { @config.rate_limiter_instance } + + assert_includes error.message, "rate limiter" + assert_includes error.message, "redis" + end + def test_an_assigned_rate_limiter_object_wins_over_every_value limiter = Object.new @config.rate_limiter = limiter diff --git a/test/translation_diff/install_generator_test.rb b/test/translation_diff/install_generator_test.rb new file mode 100644 index 0000000..27130ed --- /dev/null +++ b/test/translation_diff/install_generator_test.rb @@ -0,0 +1,112 @@ +require "test_helper" +require "support/active_record_database" +require "tmpdir" +require "securerandom" + +begin + require "generators/translation_diff/install_generator" + RAILS_GENERATORS_AVAILABLE = true +rescue LoadError + RAILS_GENERATORS_AVAILABLE = false +end + +if RAILS_GENERATORS_AVAILABLE && ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + # Its own connection, so applying the generated migration never touches the harness's shared one. + class GeneratedMigrationRecord < ActiveRecord::Base + self.abstract_class = true + end + + class InstallGeneratorTest < Minitest::Test + def test_the_generated_migration_matches_the_harness_schema + Dir.mktmpdir do |dir| + migrated = migrate_in(dir) + + %i[translation_diff_translations translation_diff_rate_limits].each do |table| + assert_equal schema_of(::ActiveRecord::Base.connection, table), schema_of(migrated, table) + end + end + end + + # A DBA hand-applying this migration (see docs/sql-cache.md) may well run it twice; it must not blow up. + def test_the_generated_migration_can_be_applied_twice + Dir.mktmpdir do |dir| + connection = migrate_in(dir) + + capture_io { CreateTranslationDiffTables.new.exec_migration(connection, :up) } + + assert connection.table_exists?(:translation_diff_translations) + end + end + + # Only the Postgres and MySQL branches leave anything behind; SQLite's :memory: connection needs no teardown. + def teardown + return unless @schema + + if ActiveRecordDatabase.url.to_s.start_with?("postgres") + ::ActiveRecord::Base.connection.execute(%(DROP SCHEMA IF EXISTS "#{@schema}" CASCADE)) + else + ::ActiveRecord::Base.connection.execute(%(DROP DATABASE IF EXISTS `#{@schema}`)) + end + end + + private + + def migrate_in(dir) + generator = TranslationDiff::InstallGenerator.new([], {}, destination_root: dir) + capture_io { generator.invoke_all } + + connection = isolated_connection + require migration_file(dir) + capture_io { CreateTranslationDiffTables.new.exec_migration(connection, :up) } + connection + end + + def isolated_connection + url = ActiveRecordDatabase.url.to_s + return postgres_isolated_connection if url.start_with?("postgres") + return mysql_isolated_connection if url.match?(%r{\A(mysql2|trilogy)://}) + + sqlite_isolated_connection + end + + def sqlite_isolated_connection + GeneratedMigrationRecord.establish_connection(adapter: "sqlite3", database: ":memory:") + GeneratedMigrationRecord.connection + end + + # A scratch schema on the same server, so the migration has nowhere to collide with the harness's own tables. + def postgres_isolated_connection + @schema = "generator_test_#{SecureRandom.hex(4)}" + ::ActiveRecord::Base.connection.execute(%(CREATE SCHEMA "#{@schema}")) + config = ::ActiveRecord::Base.connection_db_config.configuration_hash.merge(schema_search_path: @schema) + GeneratedMigrationRecord.establish_connection(config) + GeneratedMigrationRecord.connection + end + + # MySQL has no per-connection search path, so a scratch database stands in for Postgres's scratch schema. + def mysql_isolated_connection + @schema = "generator_test_#{SecureRandom.hex(4)}" + ::ActiveRecord::Base.connection.execute(%(CREATE DATABASE `#{@schema}`)) + config = ::ActiveRecord::Base.connection_db_config.configuration_hash.merge(database: @schema) + GeneratedMigrationRecord.establish_connection(config) + GeneratedMigrationRecord.connection + end + + def migration_file(dir) + Dir.glob(File.join(dir, "db/migrate/*_create_translation_diff_tables.rb")).first + end + + def schema_of(connection, table) + { columns: connection.columns(table).map { |c| [c.name, c.sql_type, c.null, c.default] }.sort, + indexes: connection.indexes(table).map { |i| [i.columns.sort, i.unique] }.sort_by(&:to_s) } + end + end +else + class InstallGeneratorTest < Minitest::Test + def test_rails_generators_are_unavailable + skip "Rails' generator classes could not be loaded; the install generator suite is skipped" + end + end +end diff --git a/test/translation_diff/memory_cache_store_test.rb b/test/translation_diff/memory_cache_store_test.rb index bb9ee05..c6cacf3 100644 --- a/test/translation_diff/memory_cache_store_test.rb +++ b/test/translation_diff/memory_cache_store_test.rb @@ -1,8 +1,10 @@ require "test_helper" require "support/cache_store_contract" +require "support/batching_cache_store_contract" class MemoryCacheStoreTest < Minitest::Test include CacheStoreContract + include BatchingCacheStoreContract attr_reader :store @@ -32,6 +34,17 @@ def test_rewriting_an_entry_makes_it_the_most_recently_used assert_equal ["again", nil, "c", "d"], store.read_multi(%w[a b c d]) end + # The regression that broke the Redis store: this one takes no timeout at all, so a nil cache_ttl is a no-op here. + def test_build_ignores_a_nil_cache_ttl_and_writes_normally + config = TranslationDiff::Configuration.new + config.cache_ttl = nil + + built = TranslationDiff::MemoryCacheStore.build(config) + built.write("a", "one") + + assert_equal ["one"], built.read_multi(["a"]) + end + def test_build_takes_its_bound_from_the_configuration config = TranslationDiff::Configuration.new config.cache_max_size = 1 diff --git a/test/translation_diff/prune_task_test.rb b/test/translation_diff/prune_task_test.rb new file mode 100644 index 0000000..020d944 --- /dev/null +++ b/test/translation_diff/prune_task_test.rb @@ -0,0 +1,54 @@ +require "test_helper" +require "rake" + +class PruneTaskTest < Minitest::Test + class PruneableDouble + def initialize(count) = @count = count + def prune = @count + end + + # Loads the task from the gem's own lib/ file -- what a host application's `rake` actually sees -- not the + # development Rakefile, which a host application's `rake` never loads. + TASK_FILE = File.expand_path("../../lib/translation_diff/tasks/translation_diff.rake", __dir__) + + # The application is global, so it is put back: the next Rake-based test must not inherit this one's tasks. + def setup + TranslationDiff.reset! + @previous_application = Rake.application + Rake.application = Rake::Application.new + load TASK_FILE + end + + def teardown + Rake.application = @previous_application + TranslationDiff.reset! + end + + def test_prunes_the_cache_store_and_the_rate_limiter_separately + TranslationDiff.configure do |c| + c.cache = PruneableDouble.new(3) + c.rate_limiter = PruneableDouble.new(5) + end + + out, = capture_io { Rake::Task["translation_diff:prune"].invoke } + + assert_includes out, "pruned 3 expired cache rows" + assert_includes out, "pruned 5 expired rate-limit rows" + end + + def test_reports_when_the_cache_store_does_not_support_pruning + TranslationDiff.configure { |c| c.cache = :memory } + + out, = capture_io { Rake::Task["translation_diff:prune"].invoke } + + assert_includes out, "the configured cache store (TranslationDiff::MemoryCacheStore) does not support pruning" + end + + def test_reports_when_there_is_no_rate_limiter_configured + TranslationDiff.configure { |c| c.cache = :memory } + + out, = capture_io { Rake::Task["translation_diff:prune"].invoke } + + assert_includes out, "the configured rate limiter (NilClass) does not support pruning" + end +end diff --git a/test/translation_diff/railtie_test.rb b/test/translation_diff/railtie_test.rb new file mode 100644 index 0000000..39dd257 --- /dev/null +++ b/test/translation_diff/railtie_test.rb @@ -0,0 +1,42 @@ +require "test_helper" +require "rake" + +begin + require "translation_diff/railtie" + RAILTIE_AVAILABLE = true +rescue LoadError + RAILTIE_AVAILABLE = false +end + +if RAILTIE_AVAILABLE + class RailtieTest < Minitest::Test + def setup + @previous_application = Rake.application + Rake.application = Rake::Application.new + end + + def teardown + Rake.application = @previous_application + end + + # This is what Rails calls when an application runs `rake -T` or `rails runner`, never the gem's own Rakefile. + def test_registers_the_prune_task_from_the_gems_own_file + TranslationDiff::Railtie.instance.send(:run_tasks_blocks, nil) + + assert Rake::Task.task_defined?("translation_diff:prune") + end + + # Without this, the task prunes whatever the default configuration resolves to, not the host application's. + def test_the_registered_task_depends_on_environment + TranslationDiff::Railtie.instance.send(:run_tasks_blocks, nil) + + assert_includes Rake::Task["translation_diff:prune"].prerequisites, "environment" + end + end +else + class RailtieTest < Minitest::Test + def test_rails_railtie_is_unavailable + skip "Rails::Railtie could not be loaded; the railtie suite is skipped" + end + end +end diff --git a/test/translation_diff/redis_cache_store_test.rb b/test/translation_diff/redis_cache_store_test.rb index 73c3029..5b15d09 100644 --- a/test/translation_diff/redis_cache_store_test.rb +++ b/test/translation_diff/redis_cache_store_test.rb @@ -1,5 +1,6 @@ require "test_helper" require "support/cache_store_contract" +require "support/batching_cache_store_contract" # Nested inside the real Redis class, requiring it explicitly, so this never races Configuration's lazy require. require "redis" @@ -17,10 +18,19 @@ def mget(*keys) def setex(key, timeout, value) @redis.setex("#{@namespace}:#{key}", timeout, value) end + + def set(key, value) + @redis.set("#{@namespace}:#{key}", value) + end + + def pipelined + @redis.pipelined { |pipeline| yield self.class.new(@namespace, redis: pipeline) } + end end class RedisCacheStoreTest < Minitest::Test include CacheStoreContract + include BatchingCacheStoreContract # `values`, when given, forces #mget to return it regardless of keys asked, to inspect keys without real storage. class FakeRedis @@ -44,6 +54,17 @@ def setex(key, timeout, value) @entries[key] = value "OK" end + + def set(key, value) + @calls << [:set, key, value] + @entries[key] = value + "OK" + end + + def pipelined + @calls << [:pipelined] + yield self + end end attr_reader :store @@ -75,6 +96,59 @@ def test_write_honours_a_custom_timeout_and_namespace assert_equal [[:setex, "t:a", 60, "b"]], redis.calls end + def test_write_multi_sends_one_pipeline_rather_than_one_round_trip_per_key + redis = FakeRedis.new + + build_store(redis).write_multi([%w[a one], %w[b two]]) + + assert_equal [[:pipelined], + [:setex, "translation-diff:a", 604_800, "one"], + [:setex, "translation-diff:b", 604_800, "two"]], redis.calls + end + + def test_write_multi_of_no_pairs_never_opens_a_pipeline + redis = FakeRedis.new + + build_store(redis).write_multi([]) + + assert_empty redis.calls + end + + # The regression this closes: `setex(key, nil, value)` raised TypeError on every write against the default store. + def test_write_with_a_nil_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: nil).write("a", "b") + + assert_equal [[:set, "translation-diff:a", "b"]], redis.calls + end + + def test_write_with_a_zero_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: 0).write("a", "b") + + assert_equal [[:set, "translation-diff:a", "b"]], redis.calls + end + + def test_write_with_a_negative_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: -1).write("a", "b") + + assert_equal [[:set, "translation-diff:a", "b"]], redis.calls + end + + def test_write_multi_with_a_nil_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: nil).write_multi([%w[a one], %w[b two]]) + + assert_equal [[:pipelined], + [:set, "translation-diff:a", "one"], + [:set, "translation-diff:b", "two"]], redis.calls + end + private def build_store(redis, **) diff --git a/test/translation_diff/redis_rate_limiter_test.rb b/test/translation_diff/redis_rate_limiter_test.rb index 17c5ae5..d986fbc 100644 --- a/test/translation_diff/redis_rate_limiter_test.rb +++ b/test/translation_diff/redis_rate_limiter_test.rb @@ -1,9 +1,12 @@ require "test_helper" +require "support/rate_limiter_contract" # A prior stand-in here hid a real defect: `add(size)` counted under the wrong subject and the limit never fired. require "ratelimit" class RedisRateLimiterTest < Minitest::Test + include RateLimiterContract + # An in-memory Redis server implementing exactly the commands ratelimit 1.1 issues; no socket is opened. class FakeRedisServer attr_reader :hashes, :expiries, :count_spans @@ -114,6 +117,15 @@ def test_check_looks_back_over_a_custom_interval assert_equal [120], server.count_spans end + # The other half of a window: what fell out of it stops counting, or a limiter never recovers. + def test_a_bucket_older_than_the_interval_is_not_counted + server = FakeRedisServer.new + stale = (Time.now.to_i / 5) - (TranslationDiff::RedisRateLimiter::DEFAULT_INTERVAL / 5) - 1 + server.hashes["ratelimit:translation-diff:call"][stale.to_s] = 10_000 + + limiter(server, threshold: 100).check(1) + end + # Naming the bare `Ratelimit` constant used to raise a raw NameError instead of this gem's own message. def test_a_missing_ratelimit_gem_raises_a_translation_diff_error limiter = limiter(FakeRedisServer.new) @@ -130,4 +142,11 @@ def test_a_missing_ratelimit_gem_raises_a_translation_diff_error def limiter(server, **) TranslationDiff::RedisRateLimiter.new(FakeConnectionPool.new(server), **) end + + def rate_limit_exceeded_error = TranslationDiff::RedisRateLimiter::RateLimitExceeded + + def build_limiter(threshold:, interval:) + @contract_server ||= FakeRedisServer.new + limiter(@contract_server, threshold: threshold, interval: interval) + end end diff --git a/test/translation_diff/sentence_cache_test.rb b/test/translation_diff/sentence_cache_test.rb index c70f8bb..ecbf0db 100644 --- a/test/translation_diff/sentence_cache_test.rb +++ b/test/translation_diff/sentence_cache_test.rb @@ -20,6 +20,21 @@ def read_multi(keys) def write(key, value) = @writes[key] = value end + # Same recording behaviour as RecordingStore, plus write_multi, to prove the batched path is taken when offered. + class BatchingStore < RecordingStore + attr_reader :write_multi_calls + + def initialize(values = {}) + super + @write_multi_calls = [] + end + + def write_multi(pairs) + @write_multi_calls << pairs + pairs.each { |key, value| @writes[key] = value } + end + end + def cache(store, **) TranslationDiff::SentenceCache.new( store: store, provider: "deepl", from: "en", to: "ru", ** @@ -60,6 +75,42 @@ def test_store_writes_only_the_translated_ones assert_equal ["Один."], store.writes.values end + def test_store_batches_every_translated_segment_into_one_write_multi_call + subject = segments("One.", "Two.") + subject.zip(%w[Один. Два.]).each { |segment, translation| segment.translation = translation } + store = BatchingStore.new + subject_cache = cache(store) + + subject_cache.store(subject) + + expected = subject.map { |segment| [subject_cache.key(segment), segment.translation] } + assert_equal [expected], store.write_multi_calls + end + + # A batch of nothing is not a batch: a store that opens a transaction in write_multi must not be asked to. + def test_store_never_calls_a_batching_store_when_nothing_was_translated + store = BatchingStore.new + + cache(store).store(segments("One.")) + + assert_empty store.write_multi_calls + end + + # The compatibility guarantee: a custom store written against today's write-only contract keeps working untouched. + def test_store_falls_back_to_write_per_key_when_the_store_has_no_write_multi + subject = segments("One.", "Two.") + subject.first.translation = "Один." + subject.last.translation = "Два." + store = RecordingStore.new + refute_respond_to store, :write_multi + subject_cache = cache(store) + + subject_cache.store(subject) + + assert_equal({ subject_cache.key(subject.first) => "Один.", subject_cache.key(subject.last) => "Два." }, + store.writes) + end + # The bug this replaces: the old cache consumed the array of updates it was # handed, emptying a collection that belonged to its caller. def test_neither_operation_modifies_the_collection_it_is_given