From 48646a19636a6a629b91745c55e52440dcde6ede Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:05:19 +0000 Subject: [PATCH 1/2] Add CLAUDE.md and a phased improvement plan Document the repo for future Claude Code sessions and record a review of the gem at 4c1e46f. CLAUDE.md covers the commands (including the Bundler pin that blocks `bundle install` on modern Bundler), the single-endpoint POST design and its indexed-key wire format, the process-global configuration and shared class-variable client, and the load-time caching wiring. PLAN.md records 12 defects, each reproduced against the current code on Ruby 3.3.6 rather than inferred, and lays out four phases (toolchain, test harness, defect fixes, coverage) plus five refactoring proposals. Notable findings: `Redcap.new` silently discards block configuration, query values are interpolated into filterLogic unescaped, the API token is written to logs when logging is enabled, and multi-record `update` reports failure on success. Documentation only; no library code changes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011fTWYMojvbXxVp445oYaJ4 --- CLAUDE.md | 90 ++++++++++++++ PLAN.md | 354 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 444 insertions(+) create mode 100644 CLAUDE.md create mode 100644 PLAN.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..33c84a1 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,90 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## What this is + +A Ruby gem wrapping the [REDCap](https://www.project-redcap.org/) REST API. It exposes two layers: a thin +HTTP client (`Redcap::Client`) and an ActiveRecord-flavored facade (`Redcap::Record`) that user code +subclasses, one subclass per REDCap project. + +## Commands + +```bash +rake test # full suite (Rakefile globs test/**/test_*.rb) +ruby -Ilib -Itest test/test_record.rb # one file +ruby -Ilib -Itest test/test_record.rb -n test_client_is_reused # one test by name +ruby -Ilib -Itest test/test_record.rb -n "/find_rejects/" # tests matching a pattern +bin/console # IRB with the gem loaded and a sample `Person` class +``` + +### Toolchain caveat + +`bundle install` fails on any modern Bundler: the gemspec pins +`spec.add_development_dependency "bundler", "~> 1.13"`, which no Bundler ≥ 2 can satisfy. Until that pin is +removed, install the runtime deps directly and run tests without Bundler: + +```bash +gem install rest-client hashie memoist dotenv minitest --no-document +ruby -Ilib -Itest -e 'Dir.glob("./test/test_*.rb").each { |f| require f }' +``` + +The gemspec also pins `hashie "~> 3.4.6"`, which is far behind the Hashie the code actually runs against +(5.x) — expect friction when touching gemspec constraints. + +## Architecture + +**Every REDCap operation is a form POST to one URL.** There are no REST paths. `configuration.host` is the +single endpoint; the operation is selected by the `content:` key in the body (`:record`, `:metadata`, +`:project`), sometimes narrowed by `action:` (`:delete`) or `returnContent:` (`:count`, `:ids`). The API +token travels in the request body on every call. + +`Client#build_payload` (private) is the one place that assembles this. Note the non-obvious wire format: array +arguments are flattened into indexed **string** keys — `records: [1,2]` becomes `"records[0]" => 1, +"records[1]" => 2` — not nested arrays. Payload-shape tests live in `test/test_payload.rb` and reach +`build_payload` via `send`. + +**Configuration is process-global.** `Redcap.configuration` is a module-level ivar holding one +`Configuration`; `Client#configuration` just delegates to it. `Redcap.new` *reassigns* that global +(`self.configure = options`) rather than building per-client state, so the last `Redcap.new` call wins for +every client already constructed. `Dotenv.load` runs at require time, so `.env` is read as a side effect of +`require 'redcap'`. + +**`Redcap::Record` is a `Hashie::Mash` subclass**, so a record is a hash whose REDCap fields are also +methods (`person.first_name`). Two consequences worth keeping in mind: a REDCap field colliding with a method +name (`id`, `save`, `count`) shadows or is shadowed by real behavior, and `Record.new(nil_or_empty)` yields an +empty Mash rather than nil — absence is not distinguishable from an empty record without an explicit check. + +**All `Record` subclasses share one client.** `@@client` is a class variable on `Redcap::Record`, memoized on +first use via `Redcap.new`, and class variables are shared with every subclass. Combined with the global +configuration above, this means **one REDCap project (one token) per process** — defining `class People < +Redcap::Record` and `class Trials < Redcap::Record` does not give them separate tokens, despite the README +suggesting a class per project. + +The `private` keyword at `lib/redcap/record.rb:109` does **not** apply to the `def self.` methods below it — +Ruby's `private` only affects instance methods. `Record.client` and `Record.comparison` are public, and the +README documents calling `People.client.log = true`, so treat `client` as public API regardless of intent. + +**Query methods funnel through `Record.comparison`,** which builds REDCap `filterLogic` strings +(`"[age] > 40"`). `where`/`gt`/`lt`/`gte`/`lte` differ only by operator; `where(id: [...])` is special-cased to +fetch by record id instead of building a filter. Values are interpolated into the filter string without +escaping. + +**Caching is wired at class-definition time.** `memoize(:post) if ENV['REDCAP_CACHE']=='ON'` runs when +`lib/redcap.rb` is loaded, so the env var must be set before `require`, cannot change at runtime, and +`flush_cache` (a Memoist artifact) simply does not exist on the client when caching is off. Because the +memoized method is `post` itself, writes are cached too; `update`/`create`/`delete` each call `flush_cache` +first to compensate. This makes cache behavior effectively untestable in-process. + +`Record.find_or_create_by`, `having`, `group`, `order`, and `where_not` are declared but empty — they return +nil silently rather than raising `NotImplementedError`. + +## Testing conventions + +Minitest, no stubbing library, no HTTP mocking. Nothing that performs a request is currently covered — the +suite exercises configuration, the payload builder, and client identity only. Several assertions compare +config values against `ENV['REDCAP_*']`, so they pass vacuously (nil == nil) when no `.env` is present; keep +that in mind before treating a green suite as evidence. + +Tests mutate the global `Redcap.configuration` (`test_it_accepts_a_block`), and Minitest randomizes order, so +new tests should set up their own configuration in `setup` rather than inherit it. diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 0000000..9952300 --- /dev/null +++ b/PLAN.md @@ -0,0 +1,354 @@ +# Improvement Plan + +A review of the `redcap` gem at `4c1e46f`, with a phased plan to modernize the toolchain, fix confirmed +defects, build a real test suite, and refactor the two structural problems that limit what the gem can do. + +Every defect listed under "Confirmed defects" was reproduced against the current code on Ruby 3.3.6, not +inferred by reading. Reproduction notes are included so each one can be turned directly into a regression +test. + +--- + +## 1. Current state + +**Size.** 313 lines of library code across 4 files, 124 lines of tests across 4 files. Last substantive +commit adds `delete`/`delete_all`. Version `0.3.0`. + +**What works.** The payload builder, the configuration object, the `Record`/`Client` split, and the query DSL +(`where`/`gt`/`lt`/`gte`/`lte`) are a sound shape for a REDCap wrapper. The indexed-key wire format is +correct and is the fiddliest part of the API. + +**The headline problem.** The suite is green — 24 runs, 28 assertions, 0 failures — and that green is +misleading. No test performs or stubs an HTTP request, so every method that talks to REDCap (`records`, +`update`, `create`, `delete`, `find`, `save`, `destroy`, `pluck`, `all`) is entirely uncovered. Several +config assertions compare against `ENV['REDCAP_*']` and pass vacuously as `nil == nil` when no `.env` is +present. Four of the bugs below sit in that uncovered region; one of them silently discards the credentials +the README tells users to set. + +**The toolchain is unusable as shipped.** `bundle install` fails outright on modern Bundler. + +--- + +## 2. Confirmed defects + +Ordered by severity. Each was reproduced; "Repro" describes how. + +### C1 — `Redcap.new` silently discards block configuration (`lib/redcap.rb:17-24`) + +The block form documented in the README does not work. `Redcap.new` with no arguments overwrites the global +configuration with `ENV` values — which are `nil` when unset — destroying whatever the block just set. + +```ruby +Redcap.configure { |c| c.host = 'http://example.com'; c.token = 'SECRET' } +Redcap.configuration.host # => "http://example.com" +client = Redcap.new +client.configuration.host # => nil ← credentials gone +``` + +The cause is that `new` unconditionally calls `self.configure = options`, constructing a fresh +`Configuration` from ENV, instead of preserving configuration that already exists. The existing test passes +only because it inspects `Redcap.configuration` without calling `Redcap.new` afterward. + +**Fix:** only populate from ENV when no configuration has been established; merge rather than replace. + +### C2 — Query values are interpolated into `filterLogic` unescaped (`lib/redcap/record.rb:124`, `:127`) + +A value containing a single quote breaks out of the filter expression: + +```ruby +Redcap::Record.where(name: "x' or '1'='1") +# filterLogic => "[name] = 'x' or '1'='1'" +``` + +Any caller passing user-supplied strings to `where` can alter the server-side filter and widen the result set +beyond what was intended. Numeric comparisons (`gt`/`lt`/`gte`/`lte`) are protected by the existing +`Integer`/`Float` type check; `where` has no such guard. + +**Fix:** escape quotes and backslashes in the value, validate the field name against `Client#fields`, and add +regression tests for quote/backslash payloads. + +### C3 — API token is written to logs (`lib/redcap.rb:153`) + +`log "Redcap POST to #{configuration.host} with #{payload}"` interpolates the entire payload, and the token +is a payload key on every single request. Turning on the documented `client.log = true` writes the +credential to STDOUT on every call. + +``` +Redcap POST to http://example.com with {:token=>"SUPERSECRET", :format=>:json, :content=>:project} +``` + +**Fix:** redact `:token` before logging. + +### C4 — `update` reports failure for any multi-record write (`lib/redcap.rb:109`) + +`update` accepts an array of records but hard-codes `result['count'] == 1`. Updating two records succeeds +server-side and returns `false`: + +```ruby +client.update([r1, r2]) # server reports count=2 => false +client.update([r1]) # server reports count=1 => true +``` + +**Fix:** return `result['count'] == data.size`, or return the count itself and let callers decide. + +### C5 — `Record.find` returns an empty record instead of nil when nothing matches (`lib/redcap/record.rb:15-19`) + +`self.new response.first` on an empty response builds `Record.new(nil)`, which `Hashie::Mash` turns into an +empty Mash. Callers get a truthy object with no fields rather than `nil`, so the idiomatic +`if (p = Person.find(id))` guard never fires. + +**Fix:** return `nil` when the response is empty. + +### C6 — `flush_cache` does not exist unless caching was on at require time (`lib/redcap.rb:160`) + +`memoize(:post) if ENV['REDCAP_CACHE']=='ON'` is evaluated when the file is loaded. `flush_cache` is a +Memoist artifact, so with caching off the method the README documents raises: + +```ruby +People.client.flush_cache +# => NoMethodError: undefined method `flush_cache' for an instance of Redcap::Client +``` + +**Fix:** always define `flush_cache` as a no-op when caching is disabled (and see R2 for the underlying +design fix). + +### C7 — String field names produce a duplicated `record_id` in the payload (`lib/redcap.rb:92`) + +`fields |= [:record_id]` compares a symbol against the caller's strings, so `'record_id'` and `:record_id` +both survive the union: + +```ruby +records(fields: %w(record_id name)) +# => "fields[0]"=>"record_id", "fields[1]"=>"name", "fields[2]"=>:record_id +``` + +This is the path `max_id` takes on every create. Harmless today, but it means the gem sends a malformed field +list whenever string names are used — which the README's own examples do. + +**Fix:** normalize field names to strings (or symbols) once, before the union. + +### C8 — `Redcap.configure` with no block raises `LocalJumpError` (`lib/redcap.rb:38-41`) + +`configure` unconditionally yields. Calling it as a reader — a natural mistake given the sibling `configure=` +writer — raises `LocalJumpError: no block given (yield)` rather than returning the configuration. + +**Fix:** `return configuration unless block_given?`. + +### C9 — `private` is a no-op on the class methods below it (`lib/redcap/record.rb:109`) + +`private` does not affect `def self.` methods. `Record.client` and `Record.comparison` are public despite the +apparent intent: + +```ruby +Redcap::Record.respond_to?(:client) # => true +Redcap::Record.respond_to?(:comparison) # => true +``` + +The README depends on `client` being public (`People.client.log = true`), so the resolution is to make the +intent explicit, not to actually hide it: keep `client` public, and move `comparison` behind +`private_class_method`. + +### C10 — Unimplemented query methods fail silently (`lib/redcap/record.rb:44-57`) + +`find_or_create_by`, `having`, `group`, `order`, and `where_not` have empty bodies and return `nil`. A caller +writing `People.order(:age)` gets `nil` back with no indication the method does nothing. + +**Fix:** raise `NotImplementedError` until implemented. + +### C11 — Toolchain pins prevent installation (`redcap.gemspec:36`, `:39`, `:40`) + +`bundle install` cannot resolve on any Bundler ≥ 2: + +``` +Because the current Bundler version (4.0.9) does not satisfy bundler ~> 1.13 + and Gemfile depends on bundler ~> 1.13, version solving has failed. +``` + +`rake "~> 10.0"` and `hashie "~> 3.4.6"` are similarly stale — the code runs fine against Hashie 5.1.0. + +**Fix:** drop the `bundler` development dependency entirely (modern convention), relax `rake` to `>= 13`, +widen `hashie` to `>= 3.4, < 6`, and add `spec.required_ruby_version`. + +### C12 — Packaging and CI metadata are broken + +- `redcap.gemspec:20` — `allowed_push_host` is still the scaffold's literal `"TODO: Set to + 'http://mygemserver.com'"`, which blocks `rake release`. +- `redcap.gemspec:29` — `spec.bindir = "exe"`, but the scripts live in `bin/` and no `exe/` exists. +- `.travis.yml` targets Ruby 2.2.1 on a service that no longer runs these builds; the README's build badge + reflects nothing. +- `LICENSE` and `LICENSE.txt` are duplicate MIT texts differing only in wrapping. +- `lib/redcap.rb:14` — a module-level `attr_reader :configuration` that defines an unreachable instance + method. Dead code. + +--- + +## 3. Phased plan + +Phases are ordered so that each one is verifiable when it lands. Phase 1 exists because without it the +correctness work in Phase 3 cannot be checked by anyone who clones the repo. + +### Phase 1 — Make the project installable and testable + +*Fixes C11, C12. No library behavior changes.* + +1. Remove the `bundler` development dependency; relax `rake` to `>= 13`; widen `hashie` to `>= 3.4, < 6`. +2. Set `spec.required_ruby_version = ">= 3.0"`; fix `bindir` to `"bin"`; remove the `allowed_push_host` TODO. +3. Add `minitest`, `webmock`, and `rake` as development dependencies. +4. Replace `.travis.yml` with a GitHub Actions workflow running the suite on Ruby 3.1/3.2/3.3. +5. Delete `LICENSE.txt`; update the README badges to point at Actions. + +**Done when** a fresh clone runs `bundle install && rake test` successfully on supported Rubies. + +### Phase 2 — Build the test harness + +*No behavior changes; establishes the safety net Phase 3 needs.* + +6. Add WebMock, pinned to `disable_net_connect!`, so an unstubbed request fails loudly rather than escaping + to the network. +7. Add a `test/support/` helper that stubs the REDCap endpoint and captures request bodies, so tests can + assert on what was sent as well as what was returned. Parse the form body back into a hash for assertions. +8. Reset global state (`Redcap.configure = nil`, `Record`'s memoized client) in `setup`/`teardown` so + randomized ordering is safe. +9. Fix the vacuous ENV assertions in `test/test_redcap.rb` to set explicit values rather than comparing + `nil` to `nil`, and replace the `assert_equal nil` calls that Minitest 6 will reject with `assert_nil`. + +**Done when** the existing 24 tests still pass, ordering is order-independent, and an accidental real HTTP +request fails the suite. + +### Phase 3 — Fix the confirmed defects + +Each item lands with the regression test that proves it, written first against the reproduction above. + +10. **C1** — configuration precedence. Also settle the intended semantics explicitly: explicit options > + prior `configure` block > ENV. This is the only fix that changes documented behavior, so it is worth + calling out in the changelog as a fix rather than a break. +11. **C3** — redact the token in log output. +12. **C2** — escape filter values; validate field names. +13. **C4** — `update` count comparison against `data.size`. +14. **C5** — `find` returns nil on empty. +15. **C7** — normalize field-name types. +16. **C6** — `flush_cache` always defined. +17. **C8** — `configure` without a block returns the configuration. +18. **C9** — `private_class_method :comparison`; document `client` as public. +19. **C10** — `NotImplementedError` for the five stubs. +20. **C12** — remove the dead `attr_reader`. + +**Done when** each defect has a test that fails on `4c1e46f` and passes after the fix. + +### Phase 4 — Coverage for the untested surface + +21. `Client`: `records` (all four argument combinations), `metadata`, `fields`, `project`, `max_id`, + `create`, `update`, `delete` — asserting both the request body and the parsed return value. +22. `Record`: `find`, `all`, `ids`, `count`, `pluck`, `select`, `where`, `gt`/`lt`/`gte`/`lte`, `save` + (both the update and create branches), `destroy`, `delete_all`. +23. Error paths: non-2xx responses, REDCap's error-shaped JSON, malformed/non-JSON bodies, and a nil `host`. + These currently have no defined behavior at all — decide it here (see R3) and encode it. +24. Guard-clause behavior: `find` with non-Integer input, `delete` with a non-array or empty array, `pluck` + with nil, `comparison` with a non-Hash or multi-key Hash. + +**Target:** every public method on both classes exercised, with request-body assertions rather than +return-value assertions alone. + +--- + +## 4. Proposed refactoring + +These are design changes, not bug fixes. They are deliberately separated from Phases 1–4 because they alter +public behavior and warrant a `0.4.0` and a migration note. Recommended order: R1 → R3 → R2 → R4. + +### R1 — Per-client configuration; retire the global singleton *(highest value)* + +**Problem.** Configuration lives in a module-level ivar (`lib/redcap.rb:26-36`) and `Record` memoizes a +single client in a class variable (`lib/redcap/record.rb:5`, `:111-114`). Class variables are shared with all +subclasses, so `class People < Redcap::Record` and `class Trials < Redcap::Record` share one client and one +token. **The gem cannot talk to two REDCap projects in one process** — directly contradicting the README's +"name the class after your REDCap project" guidance. It is also the root cause of C1 and of the test-ordering +fragility in Phase 2. + +**Proposal.** Move configuration into the `Client` instance. Let `Record` subclasses declare their own: + +```ruby +class People < Redcap::Record + redcap host: ENV['REDCAP_HOST'], token: ENV['PEOPLE_TOKEN'] +end +``` + +Store the client in a *class-level instance variable* (`@client`, not `@@client`) with inheritance-aware +lookup, so each subclass gets its own and unconfigured subclasses fall back to a process default. Keep +`Redcap.configure` and the zero-argument `Redcap.new` working against that default for backward +compatibility. + +**Impact:** fixes the one-project-per-process ceiling, makes tests isolable, and removes the class of bug C1 +belongs to. + +### R2 — Make caching an injectable, runtime concern + +**Problem.** `memoize(:post) if ENV['REDCAP_CACHE']=='ON'` (`lib/redcap.rb:160`) is evaluated at load time, +so the mode is fixed before any application code runs, cannot be changed or tested in-process, and determines +whether `flush_cache` exists at all (C6). Because the memoized method is `post` itself, writes are cached too +— `update`/`create`/`delete` each call `flush_cache` first purely to work around that. + +**Proposal.** Introduce a small cache collaborator with `fetch`/`clear`, defaulting to a null object, and +select it from configuration at initialization rather than from ENV at load. Cache only read operations, so +the write-then-flush dance disappears. Keep `REDCAP_CACHE=ON` as a recognized default so existing `.env` +files keep working. + +### R3 — A real error model + +**Problem.** `post` (`lib/redcap.rb:152-158`) has no error handling. `RestClient` raises its own exception +hierarchy on non-2xx, `JSON.parse` raises on REDCap's non-JSON error bodies, and callers see raw +`RestClient::` and `JSON::` exceptions leaking through the abstraction. There is no timeout either, so a +hung server hangs the caller indefinitely. + +**Proposal.** Add a `Redcap::Error` base with `ConfigurationError` (nil host/token), `ResponseError` +(non-2xx, carrying status and body), and `ParseError`. Detect REDCap's `{"error": "..."}` response shape +explicitly. Add configurable open/read timeouts with sane defaults. This is a prerequisite for meaningful +tests in Phase 4 item 23. + +### R4 — Extract the query builder; then chaining becomes cheap + +**Problem.** `Record.comparison` (`lib/redcap/record.rb:116-132`) mixes three concerns: argument validation, +filter-string construction, and result instantiation. Its `elsif` chain re-tests the operator that the caller +already chose, and line 127 assigns to a local `response` that is immediately discarded by the surrounding +assignment. + +**Proposal.** Extract a `Redcap::Query` value object holding fields, filter clauses, and record ids, with a +`to_payload`. `where`/`gt`/`lt`/… become thin constructors over it. This isolates escaping (C2) in one place +and makes README TODO #1 — `People.where(age: 40).select(:first_name)` — a natural follow-on: return a +`Query` that is `Enumerable` and executes lazily. It also creates the seam for `order`, `group`, and +`where_not` (C10). + +### R5 — Reconsider `Hashie::Mash` as the record base + +**Problem.** Inheriting from `Mash` means REDCap field names share a namespace with the record's own methods. +A project with a field named `id`, `save`, `count`, `client`, or `metadata` collides with real behavior, and +`Mash` emits override warnings when it happens. It is also why C5 is possible: `Mash.new(nil)` is a valid +empty record, so absence and emptiness are indistinguishable. + +**Proposal.** Not urgent, and a real break — record this as a known limitation in the README first, keep +`Mash` for now, and revisit if field collisions are reported in practice. If it is ever changed, the +replacement is a plain attributes hash with `method_missing` restricted to keys actually present. + +--- + +## 5. Sequencing, risk, and versioning + +| Phase | Behavior change | Suggested release | +|---|---|---| +| 1 — toolchain | none | `0.3.1` | +| 2 — test harness | none | — | +| 3 — defect fixes | C1 and C4 change observable results; both are fixes to behavior no caller could have relied on deliberately | `0.3.2` | +| 4 — coverage | none | — | +| R1–R4 — refactors | yes, with compatibility shims | `0.4.0` | +| R5 | breaking | deferred | + +**Risks.** + +- **No integration testing against a real REDCap instance.** Everything in Phases 2–4 is stubbed, which + verifies what the gem *sends* but not that REDCap accepts it. Reproductions C4 and C7 in particular encode + an assumption about REDCap's response shape taken from the existing code. Confirming these against a live + or demo instance before shipping Phase 3 would be worthwhile; failing that, the risk should be stated in + the changelog. +- **C1 is a behavior change that could surprise anyone who worked around it** by relying on `Redcap.new` + resetting to ENV. Worth an explicit changelog entry rather than a silent fix. +- **R1 touches every entry point.** It should land alone, after Phases 1–4 give it a net to land into. From c30bc0573b58bfa5b7b821ef686e030f2beddc06 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:24:20 +0000 Subject: [PATCH 2/2] Implement plan phases 1-4: toolchain, test harness, defect fixes, coverage Executes Phases 1-4 of PLAN.md. Refactoring proposals R1-R5 remain open. Phase 1 - toolchain. Removed the `bundler ~> 1.13` development pin that made `bundle install` fail on every Bundler >= 2, relaxed rake and hashie, set required_ruby_version, and replaced the dead Travis config with GitHub Actions on Ruby 3.1/3.2/3.3. Phase 2 - harness. Added WebMock with net connections disabled, so an unstubbed request fails rather than reaching the network, plus a stub helper that records decoded request bodies. Global configuration and Record's client now reset between tests, making the suite order-independent. Phase 3 - defects. Fixed all ten confirmed bugs, each with a regression test: configuration precedence (a bare `Redcap.new` silently discarded block configuration), unescaped filterLogic interpolation, the token appearing in log output, `update` reporting failure on multi-record writes, `find` returning a truthy empty record, `flush_cache` not existing when caching was off, duplicated record_id in field lists, `configure` raising without a block, query internals only appearing private, and the five stubs returning nil. Phase 4 - coverage. Every method that talks to REDCap is now exercised, which none were before; 24 examples to 115. Defining error behavior was a prerequisite, so the Redcap::Error hierarchy and request timeouts came forward from R3. Verified by running the new suite against the original library in an isolated copy: 56 of 115 fail there, including a named regression for every defect, and all pass here across five seeds and both cache modes. Released as 0.4.0 rather than 0.3.2 because the error hierarchy and NotImplementedError stubs change observable behavior. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011fTWYMojvbXxVp445oYaJ4 --- .github/workflows/ci.yml | 21 ++++ .travis.yml | 5 - CHANGELOG.md | 63 +++++++++++ CLAUDE.md | 105 +++++++++-------- LICENSE.txt | 21 ---- PLAN.md | 23 ++++ README.md | 40 ++++++- lib/redcap.rb | 165 ++++++++++++++++----------- lib/redcap/configuration.rb | 26 ++++- lib/redcap/errors.rb | 23 ++++ lib/redcap/record.rb | 111 ++++++++++-------- lib/redcap/version.rb | 2 +- redcap.gemspec | 19 ++-- test/support/redcap_stub.rb | 81 ++++++++++++++ test/test_client.rb | 197 ++++++++++++++++++++++++++++++++ test/test_errors.rb | 82 ++++++++++++++ test/test_helper.rb | 27 +++++ test/test_payload.rb | 49 ++++++-- test/test_record.rb | 217 +++++++++++++++++++++++++++++++++++- test/test_redcap.rb | 77 ++++++++++--- 20 files changed, 1117 insertions(+), 237 deletions(-) create mode 100644 .github/workflows/ci.yml delete mode 100644 .travis.yml create mode 100644 CHANGELOG.md delete mode 100644 LICENSE.txt create mode 100644 lib/redcap/errors.rb create mode 100644 test/support/redcap_stub.rb create mode 100644 test/test_client.rb create mode 100644 test/test_errors.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..b5dab5d --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,21 @@ +name: CI + +on: + push: + branches: [master] + pull_request: + +jobs: + test: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + ruby: ['3.1', '3.2', '3.3'] + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{ matrix.ruby }} + bundler-cache: true + - run: bundle exec rake test diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 615e96e..0000000 --- a/.travis.yml +++ /dev/null @@ -1,5 +0,0 @@ -sudo: false -language: ruby -rvm: - - 2.2.1 -before_install: gem install bundler -v 1.13.6 diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..f6a721b --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,63 @@ +# Changelog + +## 0.4.0 + +Implements Phases 1–4 of [PLAN.md](PLAN.md). The refactoring proposals R1–R5 in that document are **not** +included and remain open. + +### Fixed + +- **`Redcap.new` no longer discards block configuration.** A bare `Redcap.new` after `Redcap.configure { … }` + used to rebuild the configuration from `ENV`, silently replacing the credentials the block had just set + with `nil`. Precedence is now: explicit options > configuration already established > environment. **If you + relied on `Redcap.new` resetting configuration to the environment, call `Redcap.configure = nil` first.** +- **Query values are escaped before being interpolated into `filterLogic`.** A value containing a single + quote — `where(name: "O'Brien")`, or anything caller-supplied — used to close the string literal early, so + the remainder was parsed as filter syntax and could widen the result set. Field names are now also + validated as identifiers. +- **The API token is redacted from log output.** With `client.log = true`, the token was interpolated into + every request's log line. +- **`update` no longer reports failure for multi-record writes.** The returned count was compared against a + hard-coded `1`, so a successful write of two records returned `false`. It is now compared against the + number of records sent. +- **`Record.find` returns `nil` when nothing matches** instead of a truthy empty record, so the usual + `if (person = Person.find(id))` guard works. +- **`flush_cache` is always defined.** It was a Memoist artifact that existed only when `REDCAP_CACHE` was set + at require time, so the call the README documents raised `NoMethodError` whenever caching was off. It is now + a no-op in that case. +- **Field lists no longer carry a duplicate `record_id`.** Passing string field names left both `'record_id'` + and `:record_id` in the payload; names are normalized before the union. +- **`Redcap.configure` without a block returns the configuration** instead of raising `LocalJumpError`. + +### Changed + +- **Errors are now `Redcap::Error` descendants.** `Redcap::ConfigurationError` (missing host or token, raised + before any request), `Redcap::ResponseError` (non-2xx, transport failure, or REDCap's `{"error": …}` body; + carries `status` and `body`), and `Redcap::ParseError` (2xx that is not JSON). Previously `RestClient::` and + `JSON::` exceptions leaked through unchanged. +- **Requests now time out.** `timeout` and `open_timeout` are configurable and default to 60 seconds; a hung + server previously hung the caller indefinitely. +- **`find_or_create_by`, `having`, `group`, `order`, and `where_not` raise `NotImplementedError`** instead of + returning `nil` silently. +- Query argument validation raises `ArgumentError` rather than a bare `RuntimeError`. +- `Record.comparison` is genuinely private now (`private_class_method`); the `private` keyword never applied + to it. `Record.client` stays public, as the README documents. +- Added `Record.reset_client!` to drop the memoized client after reconfiguring. + +### Development + +- Removed the `bundler ~> 1.13` development pin, which made `bundle install` fail on every Bundler ≥ 2. + Relaxed `rake` to `>= 13`, widened `hashie` to `>= 3.4, < 6`, and set `required_ruby_version >= 3.0`. +- Replaced the dead Travis config with GitHub Actions running the suite on Ruby 3.1, 3.2, and 3.3. +- Added a WebMock-based test harness. Net connections are disabled in tests, so an unstubbed request fails + loudly. Test coverage went from 24 to 115 examples, and now covers every method that talks to REDCap — + previously none did. +- Removed a duplicate `LICENSE.txt` and a dead module-level `attr_reader :configuration`. + +### Known limitations + +- Nothing here is verified against a live REDCap instance; the suite asserts what the gem *sends*. In + particular, backslash-escaping of quotes in `filterLogic` follows the common convention but is not confirmed + against REDCap's parser. +- One REDCap project per process still. `Record`'s client is a class variable shared with every subclass, so + two `Record` subclasses cannot use different tokens. This is proposal R1 in PLAN.md. diff --git a/CLAUDE.md b/CLAUDE.md index 33c84a1..b0c5f5d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,83 +8,82 @@ A Ruby gem wrapping the [REDCap](https://www.project-redcap.org/) REST API. It e HTTP client (`Redcap::Client`) and an ActiveRecord-flavored facade (`Redcap::Record`) that user code subclasses, one subclass per REDCap project. +`PLAN.md` holds the review this codebase was worked from. Phases 1–4 are done; refactoring proposals R1–R5 +are still open, and R1 in particular explains the biggest structural constraint below. + ## Commands ```bash -rake test # full suite (Rakefile globs test/**/test_*.rb) +bundle install +bundle exec rake test # full suite ruby -Ilib -Itest test/test_record.rb # one file -ruby -Ilib -Itest test/test_record.rb -n test_client_is_reused # one test by name -ruby -Ilib -Itest test/test_record.rb -n "/find_rejects/" # tests matching a pattern +ruby -Ilib -Itest test/test_record.rb -n test_find_returns_a_record # one test +ruby -Ilib -Itest test/test_record.rb -n "/escapes/" # by pattern bin/console # IRB with the gem loaded and a sample `Person` class ``` -### Toolchain caveat - -`bundle install` fails on any modern Bundler: the gemspec pins -`spec.add_development_dependency "bundler", "~> 1.13"`, which no Bundler ≥ 2 can satisfy. Until that pin is -removed, install the runtime deps directly and run tests without Bundler: - -```bash -gem install rest-client hashie memoist dotenv minitest --no-document -ruby -Ilib -Itest -e 'Dir.glob("./test/test_*.rb").each { |f| require f }' -``` - -The gemspec also pins `hashie "~> 3.4.6"`, which is far behind the Hashie the code actually runs against -(5.x) — expect friction when touching gemspec constraints. - ## Architecture **Every REDCap operation is a form POST to one URL.** There are no REST paths. `configuration.host` is the single endpoint; the operation is selected by the `content:` key in the body (`:record`, `:metadata`, `:project`), sometimes narrowed by `action:` (`:delete`) or `returnContent:` (`:count`, `:ids`). The API -token travels in the request body on every call. +token travels in the request body on every call — which is why `Client#loggable` redacts it before anything +reaches the logger. `Client#build_payload` (private) is the one place that assembles this. Note the non-obvious wire format: array arguments are flattened into indexed **string** keys — `records: [1,2]` becomes `"records[0]" => 1, -"records[1]" => 2` — not nested arrays. Payload-shape tests live in `test/test_payload.rb` and reach -`build_payload` via `send`. +"records[1]" => 2` — not nested arrays. `Client#records` normalizes field names to strings before adding +`record_id`, because a mixed `'record_id'`/`:record_id` union used to put the same field in twice. -**Configuration is process-global.** `Redcap.configuration` is a module-level ivar holding one -`Configuration`; `Client#configuration` just delegates to it. `Redcap.new` *reassigns* that global -(`self.configure = options`) rather than building per-client state, so the last `Redcap.new` call wins for -every client already constructed. `Dotenv.load` runs at require time, so `.env` is read as a side effect of -`require 'redcap'`. +**Configuration is process-global**, held in a module-level ivar on `Redcap` and read by `Client` through +`Redcap.configuration`. Precedence is explicit options > configuration already established > `ENV`. This +ordering is load-bearing: a bare `Redcap.new` must **not** rebuild the configuration, or it wipes out +whatever a preceding `Redcap.configure` block set. There is a regression test for exactly that +(`test_block_configuration_survives_a_bare_new`). `Dotenv.load` runs at require time, so `.env` is read as a +side effect of `require 'redcap'`. **`Redcap::Record` is a `Hashie::Mash` subclass**, so a record is a hash whose REDCap fields are also -methods (`person.first_name`). Two consequences worth keeping in mind: a REDCap field colliding with a method -name (`id`, `save`, `count`) shadows or is shadowed by real behavior, and `Record.new(nil_or_empty)` yields an -empty Mash rather than nil — absence is not distinguishable from an empty record without an explicit check. - -**All `Record` subclasses share one client.** `@@client` is a class variable on `Redcap::Record`, memoized on -first use via `Redcap.new`, and class variables are shared with every subclass. Combined with the global -configuration above, this means **one REDCap project (one token) per process** — defining `class People < -Redcap::Record` and `class Trials < Redcap::Record` does not give them separate tokens, despite the README -suggesting a class per project. +methods (`person.first_name`). A REDCap field colliding with a method name (`id`, `save`, `count`) shadows or +is shadowed by real behavior. `Mash.new(nil)` is a valid empty record, which is why `find` checks for an +empty response explicitly rather than relying on truthiness. -The `private` keyword at `lib/redcap/record.rb:109` does **not** apply to the `def self.` methods below it — -Ruby's `private` only affects instance methods. `Record.client` and `Record.comparison` are public, and the -README documents calling `People.client.log = true`, so treat `client` as public API regardless of intent. +**All `Record` subclasses share one client.** `@@client` is a class variable on `Redcap::Record`, and class +variables are shared with every subclass. Combined with the global configuration, this means **one REDCap +project (one token) per process**, despite the README's class-per-project framing. Fixing this is R1 in +`PLAN.md`; until then, `Record.reset_client!` is the escape hatch after reconfiguring, and the test helper +calls it between tests. **Query methods funnel through `Record.comparison`,** which builds REDCap `filterLogic` strings (`"[age] > 40"`). `where`/`gt`/`lt`/`gte`/`lte` differ only by operator; `where(id: [...])` is special-cased to -fetch by record id instead of building a filter. Values are interpolated into the filter string without -escaping. +fetch by record id instead of building a filter. Values pass through `escape` and field names through +`field_name!` — an unescaped quote closes the filter's string literal early and the rest is read as syntax. +Both are `private_class_method`; note that a bare `private` would not work here, since it does not apply to +`def self.` methods. + +**Caching is wired at class-definition time.** `memoize :post if ENV['REDCAP_CACHE'] == 'ON'` runs when +`lib/redcap.rb` is loaded, so the env var must be set before `require` and cannot change at runtime. When +caching is off, a no-op `flush_cache` is defined instead — it needs an explicit `public`, since it sits below +`private` in the class body. Because the memoized method is `post` itself, writes are cached too, so +`update`/`create`/`delete` each call `flush_cache` first. Making this injectable is R2. + +**Errors** all descend from `Redcap::Error` (`lib/redcap/errors.rb`): `ConfigurationError` before a request is +attempted, `ResponseError` for non-2xx / transport failure / REDCap's `{"error": …}` body, `ParseError` for a +2xx that is not JSON. Keep `RestClient::` and `JSON::` exceptions from escaping `Client#execute` and +`Client#parse`. -**Caching is wired at class-definition time.** `memoize(:post) if ENV['REDCAP_CACHE']=='ON'` runs when -`lib/redcap.rb` is loaded, so the env var must be set before `require`, cannot change at runtime, and -`flush_cache` (a Memoist artifact) simply does not exist on the client when caching is off. Because the -memoized method is `post` itself, writes are cached too; `update`/`create`/`delete` each call `flush_cache` -first to compensate. This makes cache behavior effectively untestable in-process. +## Testing conventions -`Record.find_or_create_by`, `having`, `group`, `order`, and `where_not` are declared but empty — they return -nil silently rather than raising `NotImplementedError`. +Minitest plus WebMock, with `WebMock.disable_net_connect!` — an unstubbed request fails rather than reaching +the network. `test/support/redcap_stub.rb` stubs the single endpoint and records every request body, decoded +from form encoding; **assert on `last_request_body` as well as the return value**, since what the gem sends is +the part REDCap actually sees. `stub_redcap_sequence` handles calls that make more than one request (`save` on +a new record fetches `max_id` first). -## Testing conventions +Configuration and `Record`'s client are process-global and Minitest randomizes order, so `Minitest::Test#setup` +in `test_helper.rb` resets both. A subclass defining `setup` must call `super`. -Minitest, no stubbing library, no HTTP mocking. Nothing that performs a request is currently covered — the -suite exercises configuration, the payload builder, and client identity only. Several assertions compare -config values against `ENV['REDCAP_*']`, so they pass vacuously (nil == nil) when no `.env` is present; keep -that in mind before treating a green suite as evidence. +Use `with_env` rather than assigning `ENV` directly — several older assertions compared config against +`ENV['REDCAP_*']` and passed vacuously as `nil == nil`. -Tests mutate the global `Redcap.configuration` (`test_it_accepts_a_block`), and Minitest randomizes order, so -new tests should set up their own configuration in `setup` rather than inherit it. +When fixing a bug, add the regression test with a comment naming the old behavior, and confirm it fails +against the previous code before considering it done. diff --git a/LICENSE.txt b/LICENSE.txt deleted file mode 100644 index 42e2ebb..0000000 --- a/LICENSE.txt +++ /dev/null @@ -1,21 +0,0 @@ -The MIT License (MIT) - -Copyright (c) 2017 Peter Clark - -Permission is hereby granted, free of charge, to any person obtaining a copy -of this software and associated documentation files (the "Software"), to deal -in the Software without restriction, including without limitation the rights -to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -copies of the Software, and to permit persons to whom the Software is -furnished to do so, subject to the following conditions: - -The above copyright notice and this permission notice shall be included in -all copies or substantial portions of the Software. - -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -THE SOFTWARE. diff --git a/PLAN.md b/PLAN.md index 9952300..f1e21ab 100644 --- a/PLAN.md +++ b/PLAN.md @@ -9,6 +9,29 @@ test. --- +> ## Status +> +> **Phases 1–4 are implemented and released as `0.4.0`** — see `CHANGELOG.md`. The suite went from 24 to 115 +> examples; 56 of those fail against `4c1e46f` and all pass now, including a named regression test for each +> defect below. +> +> **Refactoring proposals R1–R5 are open.** They change public behavior and are still deliberately separate. +> +> Two corrections to this document, made while implementing it: +> +> - The `bindir = "exe"` bullet under C12 was **wrong**. `bin/` for development scripts and `exe/` for shipped +> executables is the standard Bundler gem layout, and this gem ships no executables, so an empty +> `executables` list is the correct state. Pointing `bindir` at `bin/` would have installed `console` and +> `setup` onto users' PATH. Left as-is. +> - The error model (R3) was pulled forward into Phase 4, because item 23 required error behavior to be +> defined before it could be tested. Timeouts came with it. Since that plus C10 makes the release +> behavior-breaking, it shipped as `0.4.0` rather than the `0.3.2` the table below anticipated. +> +> Also deviating from Phase 3 item 12: field names are validated against an identifier pattern rather than +> against `Client#fields`, which would have cost a metadata round-trip on every query. + +--- + ## 1. Current state **Size.** 313 lines of library code across 4 files, 124 lines of tests across 4 files. Last substantive diff --git a/README.md b/README.md index e8d207a..b1e3537 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# Redcap [![Build Status](https://travis-ci.org/peterclark/redcap.svg?branch=master)](https://travis-ci.org/peterclark/redcap) [![Code Climate](https://codeclimate.com/github/peterclark/redcap/badges/gpa.svg)](https://codeclimate.com/github/peterclark/redcap) +# Redcap [![CI](https://github.com/peterclark/redcap/actions/workflows/ci.yml/badge.svg)](https://github.com/peterclark/redcap/actions/workflows/ci.yml) A Ruby gem for interacting with the REDCap API @@ -185,7 +185,11 @@ Setting `REDCAP_CACHE` to `ON` inside your `.env` file will cache all calls to R ###### Force cache flush -If `REDCAP_CACHE` is set to `ON`, the cache can be manually flushed by calling `flush_cache` on the client. +The cache can be manually flushed by calling `flush_cache` on the client. It is always safe to call — when +`REDCAP_CACHE` is not `ON`, it is a no-op. + +Note that `REDCAP_CACHE` is read when the gem is loaded, so it must be set before `require 'redcap'` and +cannot be changed at runtime. ```ruby People.client.flush_cache @@ -215,6 +219,36 @@ redcap.records fields: %w(email age), filter: '[age] < 35' redcap.records records: [1,4], fields: %w(email age), filter: '[age] < 35' ``` +###### Error handling + +Everything the gem raises descends from `Redcap::Error`: + +```ruby +begin + People.all +rescue Redcap::ConfigurationError # host or token missing; raised before any request +rescue Redcap::ResponseError => e # non-2xx, transport failure, or a REDCap {"error": ...} body + e.status # => 403 + e.body +rescue Redcap::ParseError # a 2xx response that wasn't JSON +end +``` + +Requests time out after 60 seconds by default: + +```ruby +Redcap.new host: '...', token: '...', timeout: 10, open_timeout: 5 +``` + +## Limitations + +`Record` memoizes its client in a class variable, and class variables are shared with subclasses, so **all +`Record` subclasses in a process share one client and one API token**. Despite the "name the class after your +REDCap project" guidance above, a single process can only talk to one REDCap project. See `PLAN.md` (R1). + +Because `Record` inherits from `Hashie::Mash`, REDCap fields share a namespace with the record's own methods. +A project with a field named `id`, `save`, or `count` will collide. + ## TODO 1. Method chaining @@ -225,7 +259,7 @@ redcap.records records: [1,4], fields: %w(email age), filter: '[age] < 35' - `include Redcap` -3. Destroy a record +3. Per-subclass configuration, so one process can serve multiple REDCap projects ## Development diff --git a/lib/redcap.rb b/lib/redcap.rb index 5f4082a..9cbdb31 100644 --- a/lib/redcap.rb +++ b/lib/redcap.rb @@ -5,21 +5,23 @@ require 'dotenv' require 'memoist' require 'redcap/version' +require 'redcap/errors' require 'redcap/configuration' require 'redcap/record' Dotenv.load module Redcap - attr_reader :configuration - class << self + # Precedence: explicit options > configuration already established (by an + # earlier `configure` block or `new`) > the environment. + # + # The bare `Redcap.new` deliberately does *not* rebuild the configuration: + # doing so used to discard whatever a preceding `Redcap.configure` block + # had set, silently dropping the caller's credentials. def new(options = {}) - if options.empty? && ENV - options[:host] = ENV['REDCAP_HOST'] - options[:token] = ENV['REDCAP_TOKEN'] - end - self.configure = options + self.configure = options unless options.empty? + configuration # establish it now, so ENV is read at construction time Redcap::Client.new end @@ -28,14 +30,11 @@ def configuration end def configure=(options) - if options.nil? - @configuration = nil - else - @configuration = Configuration.new(options) - end + @configuration = options.nil? ? nil : Configuration.new(options) end def configure + return configuration unless block_given? yield configuration configuration end @@ -65,12 +64,12 @@ def log message end def project - payload = build_payload content: :project - post payload + post build_payload(content: :project) end def max_id - records(fields: %w(record_id)).map(&:values).flatten.map(&:to_i).max.to_i + values = records(fields: %w(record_id)) || [] + values.map(&:values).flatten.map(&:to_i).max.to_i end def fields @@ -78,60 +77,51 @@ def fields end def metadata - payload = { - token: configuration.token, - format: configuration.format, - content: :metadata, - fields: [] - } - post payload + post build_payload(content: :metadata) end def records records: [], fields: [], filter: nil - # add :record_id if not included - fields |= [:record_id] if fields.any? - payload = build_payload content: :record, records: records, fields: fields, filter: filter - post payload + # Normalize before the union: mixing 'record_id' and :record_id used to + # put both into the payload as two separate fields. + fields = Array(fields).map(&:to_s) + fields |= ['record_id'] if fields.any? + post build_payload(content: :record, records: records, fields: fields, filter: filter) end - def update data=[] - payload = { - token: configuration.token, - format: configuration.format, - content: :record, - overwriteBehavior: :normal, - type: :flat, - returnContent: :count, - data: data.to_json - } - log flush_cache if ENV['REDCAP_CACHE']=='ON' + def update data = [] + rows = data.is_a?(Array) ? data : [data] + payload = write_payload(rows, returnContent: :count) + flush_cache result = post payload - result['count'] == 1 + result['count'] == rows.size end - def create data=[] - payload = { - token: configuration.token, - format: configuration.format, - content: :record, - overwriteBehavior: :normal, - type: :flat, - returnContent: :ids, - data: data.to_json - } - log flush_cache if ENV['REDCAP_CACHE']=='ON' - post payload + def create data = [] + rows = data.is_a?(Array) ? data : [data] + flush_cache + post write_payload(rows, returnContent: :ids) end def delete ids return unless ids.is_a?(Array) && ids.any? - payload = build_payload content: :record, records: ids, action: :delete - log flush_cache if ENV['REDCAP_CACHE']=='ON' - post payload + flush_cache + post build_payload(content: :record, records: ids, action: :delete) end private + def write_payload rows, returnContent: + { + token: configuration.token, + format: configuration.format, + content: :record, + overwriteBehavior: :normal, + type: :flat, + returnContent: returnContent, + data: rows.to_json + } + end + def build_payload content: nil, records: [], fields: [], filter: nil, action: nil payload = { token: configuration.token, @@ -139,26 +129,71 @@ def build_payload content: nil, records: [], fields: [], filter: nil, action: ni content: content } payload[:action] = action if action - records.each_with_index do |record, index| + Array(records).each_with_index do |record, index| payload["records[#{index}]"] = record - end if records - fields.each_with_index do |field, index| + end + Array(fields).each_with_index do |field, index| payload["fields[#{index}]"] = field - end if fields + end payload[:filterLogic] = filter if filter payload end def post payload = {} - log "Redcap POST to #{configuration.host} with #{payload}" - response = RestClient.post configuration.host, payload - response = JSON.parse(response) - log 'Response:' - log response - response + configuration.validate! + log "Redcap POST to #{configuration.host} with #{loggable payload}" + response = execute payload + parse(response).tap do |body| + log 'Response:' + log body + end end - memoize(:post) if ENV['REDCAP_CACHE']=='ON' - end + def execute payload + RestClient::Request.execute( + method: :post, + url: configuration.host, + payload: payload, + timeout: configuration.timeout, + open_timeout: configuration.open_timeout + ) + rescue RestClient::ExceptionWithResponse => e + raise ResponseError.new( + "Redcap responded with #{e.http_code}", + status: e.http_code, + body: e.http_body + ) + rescue RestClient::Exception, SocketError, SystemCallError, IOError => e + raise ResponseError.new("Redcap request failed: #{e.message}") + end + + def parse response + body = response.body.to_s + return nil if body.strip.empty? + + data = JSON.parse(body) + if data.is_a?(Hash) && data['error'] + raise ResponseError.new("Redcap error: #{data['error']}", status: response.code, body: body) + end + data + rescue JSON::ParserError => e + raise ParseError, "Could not parse Redcap response as JSON: #{e.message}" + end + # The token rides in the body of every request, so it must never reach the log. + def loggable payload + return payload unless payload.is_a?(Hash) && payload.key?(:token) + payload.merge(token: '[REDACTED]') + end + + if ENV['REDCAP_CACHE'] == 'ON' + memoize :post + else + # Memoist defines flush_cache only when something is memoized. Define a + # no-op so callers need not know whether caching happens to be on. + # Explicitly public: this sits below `private` in the class body. + def flush_cache(*) = nil + public :flush_cache + end + end end diff --git a/lib/redcap/configuration.rb b/lib/redcap/configuration.rb index 2f477d9..5089ce9 100644 --- a/lib/redcap/configuration.rb +++ b/lib/redcap/configuration.rb @@ -1,11 +1,29 @@ module Redcap class Configuration - attr_accessor :host, :token, :format + DEFAULT_TIMEOUT = 60 + attr_accessor :host, :token, :format, :timeout, :open_timeout + + # Options win over the environment. `fetch` rather than `||` so an explicit + # `host: nil` stays nil instead of silently falling back to ENV. def initialize(options = {}) - @host = options[:host] - @token = options[:token] - @format = options[:format] || :json + @host = options.fetch(:host) { ENV['REDCAP_HOST'] } + @token = options.fetch(:token) { ENV['REDCAP_TOKEN'] } + @format = options[:format] || :json + @timeout = options.fetch(:timeout) { DEFAULT_TIMEOUT } + @open_timeout = options.fetch(:open_timeout) { DEFAULT_TIMEOUT } + end + + def validate! + raise ConfigurationError, 'Redcap host is not configured. Set REDCAP_HOST or pass :host.' if blank?(host) + raise ConfigurationError, 'Redcap token is not configured. Set REDCAP_TOKEN or pass :token.' if blank?(token) + self + end + + private + + def blank?(value) + value.nil? || value.to_s.strip.empty? end end end diff --git a/lib/redcap/errors.rb b/lib/redcap/errors.rb new file mode 100644 index 0000000..be1df2d --- /dev/null +++ b/lib/redcap/errors.rb @@ -0,0 +1,23 @@ +module Redcap + # Base for everything this gem raises, so callers can rescue Redcap::Error + # without reaching for RestClient's or JSON's exception hierarchies. + class Error < StandardError; end + + # Host or token missing before a request was attempted. + class ConfigurationError < Error; end + + # REDCap was reached but the exchange failed: a non-2xx status, a transport + # failure, or a body carrying REDCap's own {"error": "..."} shape. + class ResponseError < Error + attr_reader :status, :body + + def initialize(message, status: nil, body: nil) + @status = status + @body = body + super(message) + end + end + + # A 2xx response whose body was not the JSON the format asked for. + class ParseError < Error; end +end diff --git a/lib/redcap/record.rb b/lib/redcap/record.rb index 435ca9b..bb8b4b4 100644 --- a/lib/redcap/record.rb +++ b/lib/redcap/record.rb @@ -2,6 +2,12 @@ module Redcap class Record < Hashie::Mash + # REDCap field names are alphanumeric identifiers; anything else is a sign + # the caller is building a filter expression by hand. + FIELD_NAME = /\A[a-zA-Z_][a-zA-Z0-9_]*\z/ + + NOT_IMPLEMENTED = %i(find_or_create_by having group order where_not).freeze + @@client = nil def self.metadata @@ -15,12 +21,12 @@ def self.fields def self.find id return unless id.is_a? Integer response = client.records records: [id] - self.new response.first + return if response.nil? || response.empty? + new response.first end def self.all - response = client.records - response.map { |r| self.new r } + instantiate client.records end def self.delete_all ids @@ -28,7 +34,7 @@ def self.delete_all ids end def self.ids - client.records(fields: [:record_id]).map { |r| r['record_id'].to_i } + (client.records(fields: [:record_id]) || []).map { |r| r['record_id'].to_i } end def self.count @@ -38,27 +44,17 @@ def self.count def self.pluck field return [] unless field response = client.records fields: [field] - response.map { |r| r[field.to_s] } - end - - def self.find_or_create_by condition - end - - def self.having condition + (response || []).map { |r| r[field.to_s] } end - def self.group field - end - - def self.order condition - end - - def self.where_not condition + NOT_IMPLEMENTED.each do |name| + define_singleton_method(name) do |*| + raise NotImplementedError, "Redcap::Record.#{name} is not implemented yet" + end end def self.select *fields - response = client.records fields: fields - response.map { |r| self.new r } + instantiate client.records(fields: fields) end def self.where condition @@ -81,19 +77,27 @@ def self.lte condition comparison condition, '<=' end + # Public API: the README documents `People.client.log = true`. + def self.client + @@client ||= Redcap.new + end + + # Drop the memoized client so a later call picks up new configuration. + def self.reset_client! + @@client = nil + end + def id record_id end def save if record_id - data = Hash[keys.zip(values)] - client.update [data] + client.update [to_data] else self.record_id = client.max_id + 1 - data = Hash[keys.zip(values)] - result = client.create [data] - result.first == record_id.to_s + result = client.create [to_data] + Array(result).first.to_s == record_id.to_s end end @@ -108,28 +112,47 @@ def client private - def self.client - @@client = Redcap.new unless @@client - @@client + def to_data + Hash[keys.zip(values)] end - def self.comparison condition, op - raise "method only accepts a Hash" unless condition.is_a? Hash - raise "method only accepts a Hash with one key/value pair" unless condition.size == 1 - key, val = condition.first - response = if(key == :id) - raise "method only accepts an Array of integers when searching by :id" unless val.is_a? Array - client.records records: val - elsif op == '=' - client.records filter: "[#{key}] = '#{val}'" - elsif %w( > < >= <= ).include? op - raise "method only accepts an integer or float for the value" unless val.is_a?(Integer) || val.is_a?(Float) - response = client.records filter: "[#{key}] #{op} #{val}" - else - [] - end - response.map { |r| self.new r } + def self.instantiate response + (response || []).map { |r| new r } end + def self.comparison condition, op + raise ArgumentError, 'method only accepts a Hash' unless condition.is_a? Hash + raise ArgumentError, 'method only accepts a Hash with one key/value pair' unless condition.size == 1 + + key, val = condition.first + response = + if key.to_s == 'id' + raise ArgumentError, 'method only accepts an Array of integers when searching by :id' unless val.is_a? Array + client.records records: val + elsif op == '=' + client.records filter: "[#{field_name! key}] = '#{escape val}'" + elsif %w( > < >= <= ).include? op + raise ArgumentError, 'method only accepts an integer or float for the value' unless val.is_a?(Integer) || val.is_a?(Float) + client.records filter: "[#{field_name! key}] #{op} #{val}" + else + [] + end + instantiate response + end + + # Values are interpolated into REDCap's filterLogic, where string literals + # are single-quoted. Without escaping, a value containing a quote closes the + # literal early and the rest is read as filter syntax. + def self.escape value + value.to_s.gsub(/[\\']/) { |char| "\\#{char}" } + end + + def self.field_name! key + name = key.to_s + raise ArgumentError, "#{key.inspect} is not a valid field name" unless name.match?(FIELD_NAME) + name + end + + private_class_method :instantiate, :comparison, :escape, :field_name! end end diff --git a/lib/redcap/version.rb b/lib/redcap/version.rb index b0695b8..9b867dd 100644 --- a/lib/redcap/version.rb +++ b/lib/redcap/version.rb @@ -1,3 +1,3 @@ module Redcap - VERSION = "0.3.0" + VERSION = "0.4.0" end diff --git a/redcap.gemspec b/redcap.gemspec index d0d0c9e..fd56cdc 100644 --- a/redcap.gemspec +++ b/redcap.gemspec @@ -14,18 +14,13 @@ Gem::Specification.new do |spec| spec.homepage = "https://github.com/peterclark/redcap" spec.license = "MIT" - # Prevent pushing this gem to RubyGems.org. To allow pushes either set the 'allowed_push_host' - # to allow pushing to a single host or delete this section to allow pushing to any host. - if spec.respond_to?(:metadata) - spec.metadata['allowed_push_host'] = "TODO: Set to 'http://mygemserver.com'" - else - raise "RubyGems 2.0 or newer is required to protect against " \ - "public gem pushes." - end + spec.required_ruby_version = ">= 3.0" spec.files = `git ls-files -z`.split("\x0").reject do |f| - f.match(%r{^(test|spec|features)/}) + f.match(%r{^(test|spec|features|\.github)/}) || f.match(%r{^(PLAN|CLAUDE)\.md$}) end + # bin/ holds development scripts (console, setup); exe/ would hold shipped + # executables. This gem ships none, so `executables` is intentionally empty. spec.bindir = "exe" spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } spec.require_paths = ["lib"] @@ -33,11 +28,11 @@ Gem::Specification.new do |spec| spec.add_dependency 'dotenv' spec.add_dependency 'rest-client' spec.add_dependency 'json' - spec.add_dependency 'hashie', "~> 3.4.6" + spec.add_dependency 'hashie', ">= 3.4", "< 6" spec.add_dependency 'memoist' - spec.add_development_dependency "bundler", "~> 1.13" - spec.add_development_dependency "rake", "~> 10.0" + spec.add_development_dependency "rake", "~> 13.0" spec.add_development_dependency "minitest", "~> 5.0" + spec.add_development_dependency "webmock", "~> 3.0" spec.add_development_dependency "awesome_print" end diff --git a/test/support/redcap_stub.rb b/test/support/redcap_stub.rb new file mode 100644 index 0000000..638efd4 --- /dev/null +++ b/test/support/redcap_stub.rb @@ -0,0 +1,81 @@ +require 'uri' +require 'json' + +# Helpers for stubbing the single REDCap endpoint. +# +# REDCap exposes one URL and selects the operation from the POST body, so every +# stub here targets the same address. Because the interesting part of most calls +# is *what the gem sent*, each stub records the decoded request body; assert on +# `last_request_body` as well as on the return value. +module RedcapStub + TEST_HOST = 'http://redcap.test/api/'.freeze + TEST_TOKEN = 'TESTTOKEN'.freeze + + # A client pointed at the stubbed endpoint. + def redcap_client(options = {}) + Redcap.new({ host: TEST_HOST, token: TEST_TOKEN }.merge(options)) + end + + # Point Record subclasses at the stubbed endpoint too. + def configure_redcap(options = {}) + Redcap.configure = { host: TEST_HOST, token: TEST_TOKEN }.merge(options) + end + + # `response` may be a Hash/Array (serialized to JSON) or a raw String, which + # is sent verbatim so tests can exercise malformed and non-JSON bodies. + def stub_redcap(response = [], status: 200, headers: {}) + body = response.is_a?(String) ? response : response.to_json + requests = (@redcap_requests ||= []) + + stub_request(:post, TEST_HOST).to_return do |request| + requests << decode_form(request.body) + { + status: status, + body: body, + headers: { 'Content-Type' => 'application/json' }.merge(headers) + } + end + end + + # Successive responses for calls that make more than one request (`save` on a + # new record fetches max_id before creating). Each argument is one complete + # response body; the last one repeats once the queue is exhausted. + def stub_redcap_sequence(*responses) + requests = (@redcap_requests ||= []) + queue = responses.dup + + stub_request(:post, TEST_HOST).to_return do |request| + requests << decode_form(request.body) + response = queue.size > 1 ? queue.shift : queue.first + body = response.is_a?(String) ? response : response.to_json + { status: 200, body: body, headers: { 'Content-Type' => 'application/json' } } + end + end + + # Temporarily set environment variables for the duration of the block. + def with_env(vars) + previous = {} + vars.each { |key, value| previous[key] = ENV[key]; ENV[key] = value } + yield + ensure + previous.each { |key, value| ENV[key] = value } + end + + # Every request body captured so far, decoded, oldest first. + def redcap_requests + @redcap_requests ||= [] + end + + def last_request_body + redcap_requests.last + end + + def request_count + redcap_requests.size + end + + # RestClient form-encodes a Hash payload; decode it back for assertions. + def decode_form(body) + URI.decode_www_form(body.to_s).to_h + end +end diff --git a/test/test_client.rb b/test/test_client.rb new file mode 100644 index 0000000..c80b375 --- /dev/null +++ b/test/test_client.rb @@ -0,0 +1,197 @@ +require 'test_helper' + +# Exercises the HTTP surface. Every assertion checks the request body as well as +# the return value: what the gem sends is the part REDCap actually sees. +class ClientTest < Minitest::Test + + def setup + super + @client = redcap_client + end + + # --- reads ------------------------------------------------------------- + + def test_records_posts_record_content + stub_redcap [{ 'record_id' => '1' }] + assert_equal [{ 'record_id' => '1' }], @client.records + assert_equal 'record', last_request_body['content'] + assert_equal RedcapStub::TEST_TOKEN, last_request_body['token'] + assert_equal 'json', last_request_body['format'] + end + + def test_records_sends_no_field_keys_when_none_requested + stub_redcap [] + @client.records + assert_empty last_request_body.keys.grep(/\Afields\[/) + end + + def test_records_sends_indexed_record_ids + stub_redcap [] + @client.records records: [4, 7] + assert_equal '4', last_request_body['records[0]'] + assert_equal '7', last_request_body['records[1]'] + end + + def test_records_sends_filter_logic + stub_redcap [] + @client.records filter: '[age] > 40' + assert_equal '[age] > 40', last_request_body['filterLogic'] + end + + def test_records_adds_record_id_to_a_field_subset + stub_redcap [] + @client.records fields: [:first_name] + assert_equal %w(first_name record_id), field_names + end + + # Regression: 'record_id' and :record_id both survived the union, so the + # payload carried the same field twice. + def test_records_does_not_duplicate_a_string_record_id + stub_redcap [] + @client.records fields: %w(record_id name) + assert_equal %w(record_id name), field_names + end + + def test_records_normalizes_symbol_fields_to_strings + stub_redcap [] + @client.records fields: [:age] + assert_equal %w(age record_id), field_names + end + + def test_metadata_posts_metadata_content + stub_redcap [{ 'field_name' => 'age' }] + assert_equal [{ 'field_name' => 'age' }], @client.metadata + assert_equal 'metadata', last_request_body['content'] + end + + def test_fields_maps_metadata_to_symbols + stub_redcap [{ 'field_name' => 'record_id' }, { 'field_name' => 'age' }] + assert_equal %i(record_id age), @client.fields + end + + def test_project_posts_project_content + stub_redcap({ 'project_title' => 'Study' }) + assert_equal({ 'project_title' => 'Study' }, @client.project) + assert_equal 'project', last_request_body['content'] + end + + def test_max_id_returns_the_highest_record_id + stub_redcap [{ 'record_id' => '2' }, { 'record_id' => '11' }, { 'record_id' => '7' }] + assert_equal 11, @client.max_id + end + + def test_max_id_is_zero_when_there_are_no_records + stub_redcap [] + assert_equal 0, @client.max_id + end + + # --- writes ------------------------------------------------------------ + + def test_create_posts_data_as_json_and_asks_for_ids + stub_redcap ['5'] + assert_equal ['5'], @client.create([{ record_id: 5, first_name: 'Joe' }]) + assert_equal 'ids', last_request_body['returnContent'] + assert_equal 'normal', last_request_body['overwriteBehavior'] + assert_equal 'flat', last_request_body['type'] + assert_equal [{ 'record_id' => 5, 'first_name' => 'Joe' }], JSON.parse(last_request_body['data']) + end + + def test_create_wraps_a_bare_hash + stub_redcap ['5'] + @client.create(record_id: 5) + assert_equal [{ 'record_id' => 5 }], JSON.parse(last_request_body['data']) + end + + def test_update_asks_for_a_count + stub_redcap({ 'count' => 1 }) + assert_equal true, @client.update([{ record_id: 1 }]) + assert_equal 'count', last_request_body['returnContent'] + end + + # Regression: the count was compared against a hard-coded 1, so a successful + # write of two records reported failure. + def test_update_of_two_records_succeeds_when_two_are_written + stub_redcap({ 'count' => 2 }) + assert_equal true, @client.update([{ record_id: 1 }, { record_id: 2 }]) + end + + def test_update_fails_when_the_count_falls_short + stub_redcap({ 'count' => 1 }) + assert_equal false, @client.update([{ record_id: 1 }, { record_id: 2 }]) + end + + def test_update_wraps_a_bare_hash + stub_redcap({ 'count' => 1 }) + assert_equal true, @client.update(record_id: 1) + end + + def test_delete_sends_the_delete_action + stub_redcap '2' + assert_equal 2, @client.delete([1, 2]) + assert_equal 'delete', last_request_body['action'] + assert_equal '1', last_request_body['records[0]'] + end + + def test_delete_ignores_a_non_array + stub_redcap '1' + assert_nil @client.delete(1) + assert_equal 0, request_count + end + + def test_delete_ignores_an_empty_array + stub_redcap '0' + assert_nil @client.delete([]) + assert_equal 0, request_count + end + + # --- logging ----------------------------------------------------------- + + def test_logging_is_off_by_default + stub_redcap [] + @client.logger.define_singleton_method(:debug) { |_| flunk 'logger should be silent' } + @client.records + end + + # Regression: the whole payload was interpolated into the log line, and the + # token is a payload key on every request. + def test_the_token_is_redacted_from_the_log + stub_redcap [] + lines = capture_log { @client.records } + refute lines.any? { |line| line.to_s.include?(RedcapStub::TEST_TOKEN) }, 'token leaked into the log' + assert lines.any? { |line| line.to_s.include?('[REDACTED]') } + end + + def test_the_log_still_reports_the_host_and_payload + stub_redcap [] + lines = capture_log { @client.records } + assert lines.first.to_s.include?(RedcapStub::TEST_HOST) + assert lines.first.to_s.include?('record') + end + + # --- caching ----------------------------------------------------------- + + # Regression: flush_cache was a Memoist artifact that only existed when + # REDCAP_CACHE was set at require time, so the documented call raised. + # Holds whether or not REDCAP_CACHE was set at require time; the return value + # differs between the Memoist and no-op implementations, so only the call + # itself is asserted. + def test_flush_cache_is_always_defined + assert_respond_to @client, :flush_cache + @client.flush_cache + end + + private + + def field_names + last_request_body.select { |k, _| k.start_with?('fields[') }.values + end + + def capture_log + lines = [] + @client.log = true + @client.logger.define_singleton_method(:debug) { |message| lines << message } + yield + lines + end + +end diff --git a/test/test_errors.rb b/test/test_errors.rb new file mode 100644 index 0000000..7845978 --- /dev/null +++ b/test/test_errors.rb @@ -0,0 +1,82 @@ +require 'test_helper' + +# Error paths had no defined behavior before: RestClient and JSON exceptions +# leaked straight through the abstraction. These pin the contract. +class ErrorsTest < Minitest::Test + + def test_a_missing_host_raises_configuration_error + client = Redcap.new host: nil, token: 'TOKEN' + error = assert_raises(Redcap::ConfigurationError) { client.records } + assert_match(/host/i, error.message) + end + + def test_a_missing_token_raises_configuration_error + client = Redcap.new host: RedcapStub::TEST_HOST, token: nil + error = assert_raises(Redcap::ConfigurationError) { client.records } + assert_match(/token/i, error.message) + end + + def test_a_blank_host_raises_configuration_error + client = Redcap.new host: ' ', token: 'TOKEN' + assert_raises(Redcap::ConfigurationError) { client.records } + end + + def test_configuration_errors_are_raised_before_any_request + stub_redcap [] + client = Redcap.new host: nil, token: nil + assert_raises(Redcap::ConfigurationError) { client.records } + assert_equal 0, request_count + end + + def test_a_server_error_raises_response_error_carrying_the_status + stub_redcap 'upstream exploded', status: 500 + error = assert_raises(Redcap::ResponseError) { redcap_client.records } + assert_equal 500, error.status + assert_equal 'upstream exploded', error.body + end + + def test_a_not_found_raises_response_error + stub_redcap 'nope', status: 404 + assert_raises(Redcap::ResponseError) { redcap_client.records } + end + + def test_a_forbidden_response_raises_response_error + stub_redcap 'bad token', status: 403 + error = assert_raises(Redcap::ResponseError) { redcap_client.records } + assert_equal 403, error.status + end + + # REDCap answers some failures with 200 and an error-shaped body. + def test_an_error_shaped_body_raises_response_error + stub_redcap({ 'error' => 'You do not have permission' }) + error = assert_raises(Redcap::ResponseError) { redcap_client.records } + assert_match(/permission/, error.message) + end + + def test_a_non_json_body_raises_parse_error + stub_redcap 'gateway timeout' + assert_raises(Redcap::ParseError) { redcap_client.records } + end + + def test_truncated_json_raises_parse_error + stub_redcap '[{"record_id":' + assert_raises(Redcap::ParseError) { redcap_client.records } + end + + def test_an_empty_body_returns_nil + stub_redcap '' + assert_nil redcap_client.records + end + + def test_a_bare_numeric_body_is_parsed + stub_redcap '2' + assert_equal 2, redcap_client.delete([1, 2]) + end + + def test_every_error_descends_from_redcap_error + [Redcap::ConfigurationError, Redcap::ResponseError, Redcap::ParseError].each do |klass| + assert_operator klass, :<, Redcap::Error + end + end + +end diff --git a/test/test_helper.rb b/test/test_helper.rb index e0a118e..3aa0710 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -4,3 +4,30 @@ gem 'minitest' require 'minitest/autorun' +require 'webmock/minitest' + +# Nothing in this suite should reach the network. An unstubbed request fails +# loudly rather than escaping to a real REDCap instance. +WebMock.disable_net_connect! + +require 'support/redcap_stub' + +class Minitest::Test + include RedcapStub + + # Both the configuration and Record's client are process-global, and Minitest + # randomizes order, so every test starts from a clean slate rather than + # inheriting whatever ran before it. + def setup + reset_redcap! + end + + def teardown + reset_redcap! + end + + def reset_redcap! + Redcap.configure = nil + Redcap::Record.reset_client! + end +end diff --git a/test/test_payload.rb b/test/test_payload.rb index 75d4d1e..989701f 100644 --- a/test/test_payload.rb +++ b/test/test_payload.rb @@ -1,10 +1,17 @@ require 'test_helper' +# The payload builder assembles REDCap's wire format directly; these assertions +# pin the indexed-key encoding that the rest of the gem depends on. class PayloadTest < Minitest::Test def setup - @redcap = Redcap.new - @payload = @redcap.send(:build_payload, content: :record, records: [1,2], fields: %w(name age), filter: '[age] > 40') + super + @redcap = redcap_client + @payload = build content: :record, records: [1, 2], fields: %w(name age), filter: '[age] > 40' + end + + def build(**kwargs) + @redcap.send(:build_payload, **kwargs) end def test_payload_is_hash @@ -12,29 +19,51 @@ def test_payload_is_hash end def test_payload_has_token - assert_equal @payload[:token], @redcap.configuration.token + assert_equal RedcapStub::TEST_TOKEN, @payload[:token] end def test_payload_has_format - assert_equal @payload[:format], @redcap.configuration.format + assert_equal :json, @payload[:format] end def test_payload_has_content - assert_equal @payload[:content], :record + assert_equal :record, @payload[:content] end def test_payload_has_records - assert_equal @payload['records[0]'], 1 - assert_equal @payload['records[1]'], 2 + assert_equal 1, @payload['records[0]'] + assert_equal 2, @payload['records[1]'] end def test_payload_has_fields - assert_equal @payload['fields[0]'], 'name' - assert_equal @payload['fields[1]'], 'age' + assert_equal 'name', @payload['fields[0]'] + assert_equal 'age', @payload['fields[1]'] end def test_payload_has_filter - assert_equal @payload[:filterLogic], '[age] > 40' + assert_equal '[age] > 40', @payload[:filterLogic] + end + + def test_payload_omits_filter_when_absent + refute build(content: :record).key?(:filterLogic) + end + + def test_payload_omits_action_when_absent + refute build(content: :record).key?(:action) + end + + def test_payload_includes_action_when_given + assert_equal :delete, build(content: :record, action: :delete)[:action] + end + + def test_payload_has_no_indexed_keys_when_records_and_fields_are_empty + payload = build(content: :project) + assert_empty payload.keys.grep(/\A(records|fields)\[/) + end + + def test_payload_tolerates_nil_records_and_fields + payload = build(content: :record, records: nil, fields: nil) + assert_empty payload.keys.grep(/\A(records|fields)\[/) end end diff --git a/test/test_record.rb b/test/test_record.rb index da321d8..56cffa8 100644 --- a/test/test_record.rb +++ b/test/test_record.rb @@ -6,22 +6,233 @@ class Person < Redcap::Record class RecordTest < Minitest::Test def setup + super + configure_redcap @person = Person.new end + # --- client wiring ----------------------------------------------------- + def test_client_is_redcap_client assert_instance_of Redcap::Client, Redcap::Record.client end def test_client_is_reused - p2 = Person.new - assert_equal @person.client.object_id, p2.client.object_id + assert_same Person.client, Person.new.client + end + + def test_reset_client_drops_the_memoized_client + first = Person.client + Person.reset_client! + refute_same first, Person.client end + # --- find -------------------------------------------------------------- + ['string', Object, [1], { hash: true }, 9.8].each do |type| define_method "test_that_find_rejects_a_#{type.class}" do - assert_nil Person.find( type ) + assert_nil Person.find(type) + end + end + + def test_find_returns_a_record + stub_redcap [{ 'record_id' => '3', 'first_name' => 'Bob' }] + person = Person.find(3) + assert_instance_of Person, person + assert_equal 'Bob', person.first_name + assert_equal '3', last_request_body['records[0]'] + end + + # Regression: Mash.new(nil) is a truthy empty record, so `if Person.find(id)` + # never guarded anything. + def test_find_returns_nil_when_nothing_matches + stub_redcap [] + assert_nil Person.find(999) + end + + # --- collections ------------------------------------------------------- + + def test_all_instantiates_every_record + stub_redcap [{ 'record_id' => '1' }, { 'record_id' => '2' }] + people = Person.all + assert_equal 2, people.size + assert(people.all? { |p| p.is_a?(Person) }) + end + + def test_ids_returns_integers + stub_redcap [{ 'record_id' => '1' }, { 'record_id' => '10' }] + assert_equal [1, 10], Person.ids + end + + def test_count_counts_ids + stub_redcap [{ 'record_id' => '1' }, { 'record_id' => '2' }, { 'record_id' => '3' }] + assert_equal 3, Person.count + end + + def test_pluck_returns_bare_values + stub_redcap [{ 'first_name' => 'Joe' }, { 'first_name' => 'Sal' }] + assert_equal %w(Joe Sal), Person.pluck(:first_name) + end + + def test_pluck_without_a_field_returns_empty + stub_redcap [] + assert_equal [], Person.pluck(nil) + assert_equal 0, request_count + end + + def test_select_requests_a_field_subset + stub_redcap [{ 'record_id' => '1', 'age' => '40' }] + people = Person.select(:first_name, :age) + assert_instance_of Person, people.first + assert_includes last_request_body.values, 'first_name' + end + + def test_id_reads_record_id + assert_equal 7, Person.new('record_id' => 7).id + end + + # --- queries ----------------------------------------------------------- + + def test_where_builds_an_equality_filter + stub_redcap [] + Person.where first_name: 'Bob' + assert_equal "[first_name] = 'Bob'", last_request_body['filterLogic'] + end + + # Regression: an unescaped quote closed the string literal early and the rest + # of the value was read as filter syntax. + def test_where_escapes_single_quotes_in_the_value + stub_redcap [] + Person.where name: "x' or '1'='1" + assert_equal "[name] = 'x\\' or \\'1\\'=\\'1'", last_request_body['filterLogic'] + end + + def test_where_escapes_backslashes_in_the_value + stub_redcap [] + Person.where name: 'back\\slash' + assert_equal "[name] = 'back\\\\slash'", last_request_body['filterLogic'] + end + + def test_where_rejects_a_field_name_that_is_not_an_identifier + stub_redcap [] + assert_raises(ArgumentError) { Person.where "age] = 1 or [1" => 2 } + assert_equal 0, request_count + end + + def test_where_by_id_fetches_records_directly + stub_redcap [{ 'record_id' => '1' }, { 'record_id' => '4' }] + Person.where id: [1, 4] + assert_equal '1', last_request_body['records[0]'] + assert_equal '4', last_request_body['records[1]'] + refute last_request_body.key?('filterLogic') + end + + def test_where_by_id_requires_an_array + assert_raises(ArgumentError) { Person.where id: 1 } + end + + { gt: '>', lt: '<', gte: '>=', lte: '<=' }.each do |method, operator| + define_method "test_#{method}_builds_a_#{method}_filter" do + stub_redcap [] + Person.public_send(method, age: 40) + assert_equal "[age] #{operator} 40", last_request_body['filterLogic'] end + + define_method "test_#{method}_rejects_a_non_numeric_value" do + assert_raises(ArgumentError) { Person.public_send(method, age: 'forty') } + end + end + + def test_comparisons_accept_a_float + stub_redcap [] + Person.gt age: 40.5 + assert_equal '[age] > 40.5', last_request_body['filterLogic'] + end + + def test_comparison_requires_a_hash + assert_raises(ArgumentError) { Person.where 'first_name' } + end + + def test_comparison_requires_exactly_one_pair + assert_raises(ArgumentError) { Person.where first_name: 'Bob', age: 40 } + end + + # --- persistence ------------------------------------------------------- + + def test_save_updates_an_existing_record + stub_redcap({ 'count' => 1 }) + person = Person.new('record_id' => 3, 'first_name' => 'Bob') + assert_equal true, person.save + assert_equal 'count', last_request_body['returnContent'] + assert_equal [{ 'record_id' => 3, 'first_name' => 'Bob' }], JSON.parse(last_request_body['data']) + end + + def test_save_creates_a_new_record_with_the_next_id + stub_redcap_sequence [{ 'record_id' => '7' }], ['8'] + person = Person.new('first_name' => 'Joe') + assert_equal true, person.save + assert_equal 8, person.record_id + assert_equal 'ids', last_request_body['returnContent'] + end + + def test_save_reports_failure_when_the_created_id_does_not_match + stub_redcap_sequence [{ 'record_id' => '7' }], ['99'] + assert_equal false, Person.new('first_name' => 'Joe').save + end + + def test_destroy_deletes_by_record_id + stub_redcap '1' + assert_equal 1, Person.new('record_id' => 3).destroy + assert_equal 'delete', last_request_body['action'] + assert_equal '3', last_request_body['records[0]'] + end + + def test_destroy_without_a_record_id_does_nothing + stub_redcap '1' + assert_nil Person.new.destroy + assert_equal 0, request_count + end + + def test_delete_all_deletes_the_given_ids + stub_redcap '2' + assert_equal 2, Person.delete_all([1, 2]) + assert_equal 'delete', last_request_body['action'] + end + + # --- metadata ---------------------------------------------------------- + + def test_metadata_delegates_to_the_client + stub_redcap [{ 'field_name' => 'age' }] + assert_equal [{ 'field_name' => 'age' }], Person.metadata + end + + def test_fields_delegates_to_the_client + stub_redcap [{ 'field_name' => 'age' }] + assert_equal [:age], Person.fields + end + + # --- unimplemented ----------------------------------------------------- + + # Regression: these returned nil silently, so `People.order(:age)` looked + # like it worked. + Redcap::Record::NOT_IMPLEMENTED.each do |name| + define_method "test_#{name}_raises_not_implemented" do + assert_raises(NotImplementedError) { Person.public_send(name, age: 1) } + end + end + + # --- visibility -------------------------------------------------------- + + def test_client_is_public + assert_respond_to Redcap::Record, :client + end + + # Regression: `private` does not apply to `def self.` methods, so the query + # internals were public despite the apparent intent. + def test_query_internals_are_private + refute_respond_to Redcap::Record, :comparison + refute_respond_to Redcap::Record, :escape + assert_raises(NoMethodError) { Redcap::Record.comparison({ a: 1 }, '=') } end end diff --git a/test/test_redcap.rb b/test/test_redcap.rb index 17ad807..043aeb2 100644 --- a/test/test_redcap.rb +++ b/test/test_redcap.rb @@ -2,28 +2,28 @@ class RedcapTest < Minitest::Test - def setup - @redcap = Redcap.new - end - def test_that_it_has_a_version_number refute_nil ::Redcap::VERSION end def test_that_it_has_a_configuration - assert_instance_of Redcap::Configuration, @redcap.configuration + assert_instance_of Redcap::Configuration, Redcap.new.configuration end def test_that_host_initializes_from_env - assert_equal @redcap.configuration.host, ENV['REDCAP_HOST'] + with_env('REDCAP_HOST' => 'http://from-env.test') do + assert_equal 'http://from-env.test', Redcap.new.configuration.host + end end def test_that_token_initializes_from_env - assert_equal @redcap.configuration.token, ENV['REDCAP_TOKEN'] + with_env('REDCAP_TOKEN' => 'ENVTOKEN') do + assert_equal 'ENVTOKEN', Redcap.new.configuration.token + end end def test_that_format_defaults_to_json - assert_equal @redcap.configuration.format, :json + assert_equal :json, Redcap.new.configuration.format end def test_it_accepts_a_block @@ -31,27 +31,72 @@ def test_it_accepts_a_block c.host = 'http://www.google.com' c.token = 1234 end - assert_equal Redcap.configuration.host, 'http://www.google.com' - assert_equal Redcap.configuration.token, 1234 + assert_equal 'http://www.google.com', Redcap.configuration.host + assert_equal 1234, Redcap.configuration.token + end + + # Regression: `Redcap.new` used to rebuild the configuration from ENV, + # silently discarding everything the block had just set. + def test_block_configuration_survives_a_bare_new + Redcap.configure do |c| + c.host = 'http://example.com' + c.token = 'SECRET' + end + client = Redcap.new + assert_equal 'http://example.com', client.configuration.host + assert_equal 'SECRET', client.configuration.token + end + + def test_bare_new_does_not_clobber_configuration_with_empty_env + with_env('REDCAP_HOST' => nil, 'REDCAP_TOKEN' => nil) do + Redcap.new host: 'http://first.test', token: 'FIRST' + assert_equal 'http://first.test', Redcap.new.configuration.host + end end def test_it_accepts_a_hash redcap = Redcap.new host: 'http://www.yahoo.com', token: 5678 - assert_equal redcap.configuration.host, 'http://www.yahoo.com' - assert_equal redcap.configuration.token, 5678 + assert_equal 'http://www.yahoo.com', redcap.configuration.host + assert_equal 5678, redcap.configuration.token + end + + def test_explicit_options_win_over_env + with_env('REDCAP_HOST' => 'http://from-env.test') do + assert_equal 'http://explicit.test', Redcap.new(host: 'http://explicit.test').configuration.host + end + end + + def test_explicit_options_replace_earlier_configuration + Redcap.new host: 'http://first.test', token: 'FIRST' + assert_equal 'http://second.test', Redcap.new(host: 'http://second.test').configuration.host + end + + # Regression: reading the configuration via `configure` raised LocalJumpError. + def test_configure_without_a_block_returns_the_configuration + assert_instance_of Redcap::Configuration, Redcap.configure + assert_same Redcap.configuration, Redcap.configure + end + + def test_assigning_nil_resets_the_configuration + Redcap.new host: 'http://first.test', token: 'FIRST' + Redcap.configure = nil + with_env('REDCAP_HOST' => nil) do + assert_nil Redcap.configuration.host + end end def test_it_has_a_logger - assert_instance_of Logger, @redcap.logger + assert_instance_of Logger, Redcap.new.logger end def test_log_is_off - assert_equal @redcap.log?, false + assert_equal false, Redcap.new.log? end def test_log_can_turn_on - @redcap.log = true - assert_equal @redcap.log?, true + redcap = Redcap.new + redcap.log = true + assert_equal true, redcap.log? end end