diff --git a/.gitignore b/.gitignore index fb27d6a..c4110e6 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,9 @@ /tmp/ /test/tmp/ /test/dummy/log/*.log +# Rotated logs too: `test.log.0` doesn't match `*.log`, and a 100MB one is how +# it gets noticed — at the push. +/test/dummy/log/*.log.* /test/dummy/storage/ /test/dummy/tmp/ diff --git a/CHANGELOG.md b/CHANGELOG.md index f268e04..075e93c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ behaviour, and every new surface is off until you switch it on. ### Added +- `ApprovalEngine::OutboxEvent.purge!` — the outbox is a queue, not a ledger, + and nothing swept it: one row per activation, decision and outcome, kept + forever. Purges relayed events past a window (30 days by default), keeps dead + letters whatever their age, and never touches the audit trail. + + - `ApprovalEngine.define_flow(name, tenant:, model:)` — a declarative DSL for a whole flow (the template, its ordered steps, and the rules that route to it) in one block, instead of three `create!` calls whose relationship you have to @@ -86,6 +92,14 @@ behaviour, and every new surface is off until you switch it on. `define_flow` may route to. `nil`, the default, means no vocabulary is declared and nothing is checked; set it and a `group:` typo stops the seed instead of resolving zero actors at build time. +- `ApprovalEngine::TestHelpers` — test support for host applications, so an + adopter is not hand-rolling fixtures to test their own flow. + `approve_approval!` walks a flow to a decision, `reject_approval!` stops at + whatever is waiting, `pending_approval_steps` is the inbox for one record, and + `drain_approval_outbox!` relays the side-effects — without which + `after_approved` has genuinely not run yet, and a test asserting on it fails + for the right reason while looking like the wrong one. Actions and queries + rather than assertions, so they read the same under Minitest and RSpec. ### Changed @@ -101,22 +115,20 @@ behaviour, and every new surface is off until you switch it on. `Alternatives` section naming the neighbours. `test/docs_test.rb` now fails the build if the README documents a config key, an `ApprovalEngine` method, a generator, or an image that does not exist. - -### Changed - +- **Minimum Ruby is now 3.2** and **PostgreSQL 13+ is stated explicitly.** 3.1 + was claimed but never tested and is past end of life; the tables key on + `gen_random_uuid()`, which is core Postgres only from 13. +- `shiny_json_logic` is pinned to `~> 0.3.6` rather than `~> 0.3`. It is pre-1.0 + and evaluates every routing rule, so a floating minor could change matching + behaviour under an adopter on a routine `bundle update`. +- The admin renders validation failures with `422` rather than + `:unprocessable_entity`, which Rack has deprecated and will remove — the + replacement symbol does not exist on Rails 7.1, so the number is the portable + spelling. - **Minimum Rails is now 7.1**, up from 7.0.8. The outbox relay's retry backoff uses `:polynomially_longer`, which 7.0 does not know, and 7.0 is past security support. CI now runs the whole supported range rather than one version of it. -### Added - -- `ApprovalEngine::TestHelpers` — test support for host applications. - `approve_approval!` walks a flow to a decision, `reject_approval!` stops at - whatever is waiting, `pending_approval_steps` is the inbox for one record, and - `drain_approval_outbox!` relays the side-effects, without which `after_approved` - has genuinely not run yet. Actions and queries rather than assertions, so they - read the same under Minitest and RSpec. - ### Fixed - Step notifications ignored delegations. A delegate can act on a step, so diff --git a/README.md b/README.md index da9a5c3..c7f8bd0 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ [![Gem Version](https://img.shields.io/gem/v/approval_engine?logo=rubygems&color=E9573F)](https://rubygems.org/gems/approval_engine) [![Downloads](https://img.shields.io/gem/dt/approval_engine?logo=rubygems&color=blue)](https://rubygems.org/gems/approval_engine) [![CI](https://github.com/Harry-kp/approval_engine/actions/workflows/ci.yml/badge.svg)](https://github.com/Harry-kp/approval_engine/actions/workflows/ci.yml) -![Ruby](https://img.shields.io/badge/ruby-%3E%3D%203.1-CC342D) +![Ruby](https://img.shields.io/badge/ruby-%3E%3D%203.2-CC342D) ![Rails](https://img.shields.io/badge/rails-%3E%3D%207.1-D30001) ![License: MIT](https://img.shields.io/badge/license-MIT-blue) @@ -35,7 +35,8 @@ approved, who approves, and what happens next. ## 60-second quickstart -Needs PostgreSQL (the routing engine uses `jsonb`), Rails 7.1+, Ruby 3.1+. +Needs PostgreSQL 13+ (the routing engine uses `jsonb` and `gin`; the tables key +on `gen_random_uuid()`), Rails 7.1+, Ruby 3.2+. ```sh bundle add approval_engine @@ -505,6 +506,22 @@ A rule too complex for the simple form — an `or`, a nested group, a substring into something it isn't. That fallback is the point: the form is a convenience over the storage format, never a lossy replacement for it. +## Housekeeping + +The audit trail is the point of the ledger and is never swept. The **outbox is +not** — it is a queue, one row per activation, decision and outcome, and a +relayed row has no reader. Sweep it on whatever schedule you already run: + +```ruby +ApprovalEngine::OutboxEvent.purge! # relayed, older than 30 days +ApprovalEngine::OutboxEvent.purge!(older_than: 7.days) +``` + +Dead letters are kept whatever their age — an event that exhausted its retries +is evidence of a callback that needs fixing, not litter. Find them with +`ApprovalEngine::OutboxEvent.failed`, and clear `failed_at` to let the relay +pick one up again. + ## Core concepts | Term | What it is | @@ -586,7 +603,7 @@ edge cases yet. ## Development -ApprovalEngine needs Ruby 3.1+ and PostgreSQL. +ApprovalEngine needs Ruby 3.2+ and PostgreSQL 13+. ```sh bin/setup diff --git a/app/controllers/approval_engine/admin/template_steps_controller.rb b/app/controllers/approval_engine/admin/template_steps_controller.rb index a807ab5..a325f08 100644 --- a/app/controllers/approval_engine/admin/template_steps_controller.rb +++ b/app/controllers/approval_engine/admin/template_steps_controller.rb @@ -21,7 +21,7 @@ def create if @template_step.save redirect_to admin_track_template_path(@track_template), notice: "Step added." else - render :new, status: :unprocessable_entity + render :new, status: 422 end end @@ -32,7 +32,7 @@ def update if @template_step.update(template_step_params) redirect_to admin_track_template_path(@track_template), notice: "Step updated." else - render :edit, status: :unprocessable_entity + render :edit, status: 422 end end diff --git a/app/controllers/approval_engine/admin/track_templates_controller.rb b/app/controllers/approval_engine/admin/track_templates_controller.rb index f527dee..85fa235 100644 --- a/app/controllers/approval_engine/admin/track_templates_controller.rb +++ b/app/controllers/approval_engine/admin/track_templates_controller.rb @@ -36,7 +36,7 @@ def create if @track_template.save redirect_to admin_track_template_path(@track_template), notice: "Template created." else - render :new, status: :unprocessable_entity + render :new, status: 422 end end @@ -47,7 +47,7 @@ def update if @track_template.update(track_template_params) redirect_to admin_track_template_path(@track_template), notice: "Template updated." else - render :edit, status: :unprocessable_entity + render :edit, status: 422 end end diff --git a/app/controllers/approval_engine/admin/trigger_rules_controller.rb b/app/controllers/approval_engine/admin/trigger_rules_controller.rb index d818508..27b03d0 100644 --- a/app/controllers/approval_engine/admin/trigger_rules_controller.rb +++ b/app/controllers/approval_engine/admin/trigger_rules_controller.rb @@ -34,7 +34,7 @@ def create else @trigger_rule.errors.add(:condition, @condition_error) if @condition_error prepare_condition_editor - render :new, status: :unprocessable_entity + render :new, status: 422 end end @@ -50,7 +50,7 @@ def update else @trigger_rule.errors.add(:condition, @condition_error) if @condition_error prepare_condition_editor - render :edit, status: :unprocessable_entity + render :edit, status: 422 end end diff --git a/app/models/approval_engine/outbox_event.rb b/app/models/approval_engine/outbox_event.rb index a2c4951..0cc5355 100644 --- a/app/models/approval_engine/outbox_event.rb +++ b/app/models/approval_engine/outbox_event.rb @@ -13,6 +13,7 @@ class OutboxEvent < ApplicationRecord validates :tenant_id, :event_name, presence: true scope :unprocessed, -> { where(processed: false) } + scope :processed, -> { where(processed: true) } # Dead letters: delivery retries were exhausted. Excluded from drain! so a # permanently-failing callback isn't resurrected forever; surfaced here for # ops to inspect and replay (clear failed_at to let drain! pick it up again). @@ -34,6 +35,23 @@ def self.drain!(older_than: 1.minute, limit: 1000) end end + # The outbox is a queue, not a ledger: once an event is relayed its row has + # no reader, and one is written per step activation, decision and outcome. + # Left alone the table grows without bound, so a host is expected to sweep + # it — the audit trail lives in AuditLog and is never touched by this. + # + # ApprovalEngine::OutboxEvent.purge! # processed, 30d+ + # ApprovalEngine::OutboxEvent.purge!(older_than: 7.days) + # + # Returns the number of rows deleted. Dead letters are kept regardless of + # age: an event that exhausted its retries is evidence, not litter. + def self.purge!(older_than: 30.days, limit: 10_000) + ids = processed.where(failed_at: nil) + .where(processed_at: ..older_than.ago) + .order(:processed_at).limit(limit).pluck(:id) + where(id: ids).delete_all + end + def mark_processed! update!(processed: true, processed_at: Time.current, error_payload: nil, delivery_error: nil) end diff --git a/approval_engine.gemspec b/approval_engine.gemspec index 6c4dbfb..ae4f39e 100644 --- a/approval_engine.gemspec +++ b/approval_engine.gemspec @@ -25,7 +25,7 @@ Gem::Specification.new do |spec| DESC spec.license = "MIT" - spec.required_ruby_version = ">= 3.1.0" + spec.required_ruby_version = ">= 3.2.0" # `source_code_uri` is pinned to the repository rather than derived from # `homepage`: moving the homepage some day must not silently repoint the @@ -51,5 +51,7 @@ Gem::Specification.new do |spec| end spec.add_dependency "rails", ">= 7.1", "< 9.0" - spec.add_dependency "shiny_json_logic", "~> 0.3" + # Pinned to the patch series on purpose: pre-1.0 gems may break across a + # minor, and this one evaluates every routing rule. Widen deliberately. + spec.add_dependency "shiny_json_logic", "~> 0.3.6" end diff --git a/test/models/approval_engine/outbox_retention_test.rb b/test/models/approval_engine/outbox_retention_test.rb new file mode 100644 index 0000000..c335b4a --- /dev/null +++ b/test/models/approval_engine/outbox_retention_test.rb @@ -0,0 +1,52 @@ +require "test_helper" + +module ApprovalEngine + # The outbox is a queue: one row per activation, decision and outcome, and + # nothing reads a relayed row again. Without a sweep the table grows forever. + class OutboxRetentionTest < ApprovalEngine::TestCase + setup do + invoice = Invoice.create!(tenant_id: TENANT, amount: 6000) + @approval = Approval.create!(tenant_id: TENANT, target: invoice, status: "approved") + end + + def event(processed:, processed_at: nil, failed_at: nil) + OutboxEvent.create!(tenant_id: TENANT, event_name: "approval.approved", record: @approval).tap do |e| + e.update_columns(processed: processed, processed_at: processed_at, failed_at: failed_at) + end + end + + test "purges relayed events past the window, and keeps recent ones" do + old = event(processed: true, processed_at: 60.days.ago) + recent = event(processed: true, processed_at: 1.day.ago) + + assert_equal 1, OutboxEvent.purge! + + assert_not OutboxEvent.exists?(old.id) + assert OutboxEvent.exists?(recent.id) + end + + test "never purges work that has not been relayed" do + pending = event(processed: false, processed_at: nil) + + OutboxEvent.purge!(older_than: 0.seconds) + + assert OutboxEvent.exists?(pending.id), "an unrelayed event is still owed to the host" + end + + # An event that exhausted its retries is evidence of a broken callback. + test "keeps dead letters however old they are" do + dead = event(processed: true, processed_at: 90.days.ago, failed_at: 90.days.ago) + + OutboxEvent.purge! + + assert OutboxEvent.exists?(dead.id) + end + + test "honours an explicit window and returns the count deleted" do + event(processed: true, processed_at: 10.days.ago) + event(processed: true, processed_at: 10.days.ago) + + assert_equal 2, OutboxEvent.purge!(older_than: 7.days) + end + end +end