Deep review: correctness fixes, architecture deepening, slimmer deps & real CI matrix by mike927 · Pull Request #11 · nomtek/rspec-json_api

mike927 · 2026-06-05T15:46:49Z

Deep review of the codebase applying correctness fixes, architecture improvements, dependency hygiene, and real backwards-compatibility verification. Each finding is a separate, self-contained commit; the suite is green after every one.

Verification: 61 examples, 0 failures · rubocop clean (29 files) · gem build succeeds. Run locally on Ruby 4.0.1 / Bundler 4.0.4.

Bug fixes

Load failure on Ruby 4.0 — types/email.rb / types/uri.rb reference the stdlib URI constant but nothing required it; on Ruby 4.0 the gem failed to load. Added require "uri".
UUID multiline bypass — the UUID regex used ^…$ (line anchors); a valid UUID followed by \n + arbitrary content matched. Switched to \A…\z. (+regression test)
Unrescued JSON.parse — non-JSON input raised JSON::ParserError instead of a clean mismatch. Now rescued. (+test)
dig TypeError — a nested object replaced by a scalar raised TypeError instead of failing the match (reachable via nested interface arrays). Added a Hash-guarded dig_path. (+test)
compare_regexp return type — returned str =~ re (Integer/nil); now re.match?(str) (Boolean).

Architecture (deepening)

Constraints module — the proc-options DSL (type/value/min/max/inclusion/regex/lambda/allow_blank) is extracted from compare_proc into its own testable module, dropping the sort_by ordering hack and the sanitize! mutation. Unsupported options now raise instead of being silently dropped. (+unit spec)
Unified comparator — CompareArray and CompareHash are merged into one SchemaMatch with a single shape-dispatching match(actual, expected); the matcher no longer owns dispatch knowledge. Duplicate array logic deleted.
No more core monkey-patching — deep_keys / deep_key_paths / deep_sort / sanitize! were forced onto core Hash/Array for every consumer; moved into an internal Traversal module. (+guard test)
Lazy failure diff — Diffy::Diff was built on every assertion, including passing ones; now built only for failure_message. (+red→green test)

Dependencies & backwards compatibility

rails → railties — the gem only uses ActiveSupport's blank?/present? and Rails::Generators; depending on the rails meta-gem forced the entire framework on consumers. Switched to railties, keeping the >= 6.1.4.1 floor. Dependency graph: 87 → 65 gems. Generators verified to still load.
Real CI matrix — CI ran a single Ruby/Rails combo. Added per-Rails-line gemfiles + a GitHub Actions matrix covering the floor (Ruby 3.2 / Rails 6.1) through the ceiling (Ruby 3.4 / Rails 8.1), fail-fast: false, plus a separate rubocop lint job. Added x86_64-linux to the lockfile platforms.

Housekeeping

Removed the dead/broken interface.erb generator template; generator output now emits # frozen_string_literal: true and freezes the interface hash.
Dropped the four rubocop complexity suppressions (MethodLength / AbcSize / CyclomaticComplexity / PerceivedComplexity) that the refactors made unnecessary.

Notes / follow-ups

BUNDLED WITH 4.0.4 is legitimate (Ruby 4.0 ships Bundler 4.x).
Only Ruby 4.0.1 was available locally, so the older matrix cells (Rails 6.1 / 7.x on Ruby 3.2–3.3) are wired correctly but will be proven on first CI run — the floor cell + Bundler 4 is the most likely to need a tweak.
A CHANGELOG entry / version bump before release is left to the maintainer.

🤖 Generated with Claude Code

types/email.rb and types/uri.rb reference the stdlib URI constant (URI::MailTo::EMAIL_REGEXP, URI::DEFAULT_PARSER), but nothing required "uri". On older Ruby it was loaded as a side effect of other gems; on Ruby 4.0 it is not, so requiring the gem raised NameError: uninitialized constant URI and the whole suite failed to load. Require "uri" explicitly at load time.

The UUID regex used ^...$ anchors, which in Ruby match line boundaries, not string boundaries. A string whose first line is a valid UUID followed by a newline and arbitrary content matched, letting crafted multiline values pass validation. Switch to \A...\z so the whole string must be a UUID. Adds a regression test.

compare_regexp returned `actual_value.to_s =~ expected_value`, an Integer match index or nil rather than a Boolean. It worked only by truthiness in the surrounding all? chains. Use expected_value.match?(actual_value.to_s) to return a proper Boolean. Behaviour is unchanged; existing regex specs stay green.

matches? called JSON.parse without rescuing, so a non-JSON actual (e.g. a plain error string) raised JSON::ParserError and errored the example instead of reporting a clean mismatch. Rescue JSON::ParserError and return false, keeping the raw input for the failure message. Extracts the comparison dispatch into a private schema_match? to keep matches? small. Adds a regression test.

compare_key_paths_and_values used Hash#dig(*key_path), which raises TypeError ("String does not have #dig method") when the schema expects a nested object but the actual value at an intermediate key is a scalar. This is reachable through nested interface arrays, where the top-level deep_keys guard does not apply, so a mismatch crashed instead of failing. Add a Hash-guarded dig_path that returns nil on a non-Hash intermediate. Adds a regression test.

The schema Proc options (type/value/min/max/inclusion/regex/lambda/ allow_blank) were evaluated inline in CompareHash#compare_proc, which: - relied on a sort_by hack to force allow_blank to run first inside an all? block (with a bare `return` to short-circuit), - mutated the option hash via the Hash#sanitize! monkey-patch, and - silently dropped unsupported option keys. Move the DSL into RSpec::JsonApi::Constraints with a small interface, Constraints.match(value, options). allow_blank is handled explicitly before the other options (no sort hack, no inner return), and an unsupported option now raises ArgumentError instead of being ignored. compare_proc delegates to it. Behaviour for supported options is unchanged; adds a unit spec covering the new module.

Array comparison existed twice: CompareArray handled a top-level array schema, while CompareHash#compare_array handled nested arrays, each with its own interface? check and divergent rules (e.g. CompareArray skipped the element-count check). The matcher also owned the dispatch knowledge (class equality + key-set guard + which module to call). Introduce RSpec::JsonApi::SchemaMatch with one public entry, match(actual, expected), that dispatches on shape internally and applies the top-level guards. Top-level arrays now route through the same compare_array used for nested arrays, so there is a single array implementation. Delete CompareArray and CompareHash; the matcher just calls SchemaMatch.match. Behaviour is unchanged (59 examples green).

lib/extensions/hash.rb and lib/extensions/array.rb opened core Hash and Array to add deep_keys, deep_key_paths, deep_sort and sanitize!, forcing those methods onto every object in the host application - the very monkey-patching the gem's own spec_helper disables. sanitize! also shadowed Rails vocabulary. Move the structural helpers into RSpec::JsonApi::Traversal as module functions taking the data as an argument, called only from SchemaMatch. sanitize! is dropped (unused since the Constraints extraction). Core classes are no longer mutated; adds a guard test asserting so.

matches? built Diffy::Diff on every assertion, including passing ones, then threw it away. Diffy is only needed to render failure_message. Move diff construction into a memoized private method called from failure_message, so the happy path does no diff work. Adds a test asserting Diffy::Diff is not instantiated when the schema matches.

interface.erb was never used - InterfaceGenerator writes its file from an inline heredoc via create_file, and the template itself was broken (`<%= module RSpec %>` would render the return value of the module definition). Remove it. Also emit `# frozen_string_literal: true` from both generators and freeze the generated interface hash, matching the hand-written example_interface and the rest of the codebase.

The gem only uses ActiveSupport core extensions (blank?/present?) and Rails::Generators, which lives in railties. Depending on the "rails" meta-gem forced the entire framework - ActiveRecord, ActionCable, ActionMailer, ActionMailbox, ActiveStorage, ActionText - onto every consumer of a JSON-matcher gem. Replace rails with railties, keeping the >= 6.1.4.1 floor (backwards compatible). Regenerating the lock drops the dependency graph from 87 to 65 gems. Generators verified to load against railties alone; suite green.

CI previously ran a single Ruby (3.2.2) against whatever Rails resolved (the latest), so the gemspec promise of Ruby >= 3.2 and Rails >= 6.1 was never actually exercised. Add per-Rails-line bundle gemfiles under gemfiles/ (each inherits the root dev dependencies via eval_gemfile and pins the Rails line) and a GitHub Actions matrix covering the floor (Ruby 3.2 / Rails 6.1) through the ceiling (Ruby 3.4 / Rails 8.1), with fail-fast disabled. Rubocop runs once in a separate lint job. Also add the x86_64-linux platform to the lockfile so the lint job installs cleanly on CI.

…tors compare_hash.rb (now schema_match.rb) was excluded from MethodLength, AbcSize, CyclomaticComplexity and PerceivedComplexity, and the MethodLength list still referenced the deleted extensions/hash.rb. After extracting Constraints, unifying the comparators and splitting compare_array into compare_typed_array / compare_interface_array / compare_exact_array (plus a one-line compare_values), none of those suppressions are needed - rubocop is clean without them. Remove all four complexity excludes and the AbcSize exclude for match_json_schema.rb. The remaining Naming/* excludes are deliberate naming choices, not complexity debt, so they stay.

The lint job uses bundler-cache, which installs gems into vendor/bundle inside the checkout. The project .rubocop.yml set AllCops/Exclude, which *replaces* RuboCop default excludes (including vendor/**/*), so rubocop started linting third-party gems and failed. Merge with the defaults via inherit_mode and exclude vendor explicitly. Did not surface locally because gems install outside the repo there.

mike927

Reviewed the PR and left inline notes on matcher behavior changes.

mike927 · 2026-06-05T15:52:58Z

+      def compare_interface_array(actual_value, expected_value)
+        interface = expected_value[0]
+
+        actual_value.all? { |elem| compare(elem, interface) }


Top-level interface-array schemas used to enforce each element key structure before recursing (actual_elem.deep_keys == expected[0].deep_keys). Now SchemaMatch.match sends top-level arrays here, but compare(elem, interface) does not call same_key_structure?. That lets extra or missing null-valued keys match, e.g. [{ id: "1", extra: nil }] satisfies [{ id: String }] because the extra path compares nil == nil. Please preserve the key-structure guard for hash elements, or route those elements through match before comparing values.

Fixed in a79e487. Confirmed the regression: [{ id: "1", extra: nil }] matched [{ id: String }] (the extra path compared nil == nil). Interface-array elements now go through match, so each element is held to the same same_key_structure? guard as a top-level object — this also covers nested interface arrays consistently and rejects non-hash elements instead of raising. Added a regression test (when an element has an extra null-valued key).

mike927 · 2026-06-05T15:52:58Z

+        # The diff is only needed to render a failure message, so it is built
+        # lazily and memoized rather than on every matches? call.
+        def diff
+          @diff ||= Diffy::Diff.new(expected, actual, context: 5)


@diff is memoized across matches? calls, but @actual is replaced on every call. If a matcher instance is reused after a failed match, the second failure message can show the first diff. Clearing @diff at the start of matches? keeps the lazy allocation while making the failure output reflect the current actual value.

Fixed in 6043179. @diff is now reset at the start of matches?, so the lazy build is preserved (still skipped on a passing match) but always reflects the current @actual. Added a regression test that reuses a matcher instance across two failing matches and asserts the second message does not contain the first actual value.

@mike927

Unifying the comparators routed top-level interface arrays through compare(elem, interface), dropping the per-element key-structure guard that CompareArray.compare used to apply (actual_elem.deep_keys == expected[0].deep_keys). An element with an extra null-valued key then matched, e.g. [{ id: "1", extra: nil }] satisfied [{ id: String }] because the extra path compared nil == nil. Route interface-array elements through match instead of compare, so each element is held to the same key-structure guard (and class check) as a top-level object. Adds a regression test. Raised by @mike927 in review of #11.

@Actual

The failure diff is memoized in @diff but @Actual is replaced on every matches? call. A reused matcher instance whose first match failed would render the first diff in a later failure message, even though got: showed the new actual value. Reset @diff at the start of matches? so the lazy build still happens but always reflects the current actual. Adds a regression test. Raised by @mike927 in review of #11.

Michal added 14 commits June 5, 2026 17:19

mike927 commented Jun 5, 2026

View reviewed changes

Michal added 3 commits June 5, 2026 17:57

Release: bump version to 1.5.0 and update CHANGELOG

cf1ec86

mike927 merged commit fcfb401 into master Jun 5, 2026
14 checks passed

mike927 deleted the feature/deep-review-improvements branch June 5, 2026 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deep review: correctness fixes, architecture deepening, slimmer deps & real CI matrix#11

Deep review: correctness fixes, architecture deepening, slimmer deps & real CI matrix#11
mike927 merged 17 commits into
masterfrom
feature/deep-review-improvements

mike927 commented Jun 5, 2026

Uh oh!

mike927 left a comment

Uh oh!

mike927 Jun 5, 2026

Uh oh!

mike927 Jun 5, 2026

Uh oh!

mike927 Jun 5, 2026

Uh oh!

mike927 Jun 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mike927 commented Jun 5, 2026

Bug fixes

Architecture (deepening)

Dependencies & backwards compatibility

Housekeeping

Notes / follow-ups

Uh oh!

mike927 left a comment

Choose a reason for hiding this comment

Uh oh!

mike927 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

mike927 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

mike927 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

mike927 Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant