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 new file mode 100644 index 0000000..b0c5f5d --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,89 @@ +# 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. + +`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 +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_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 +``` + +## 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 — 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. `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**, 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`). 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. + +**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 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`. + +## Testing conventions + +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). + +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`. + +Use `with_env` rather than assigning `ENV` directly — several older assertions compared config against +`ENV['REDCAP_*']` and passed vacuously as `nil == nil`. + +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 new file mode 100644 index 0000000..f1e21ab --- /dev/null +++ b/PLAN.md @@ -0,0 +1,377 @@ +# 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. + +--- + +> ## 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 +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. 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