fix(import): tolerate null Rule names and orphaned rejected transfers#2775
fix(import): tolerate null Rule names and orphaned rejected transfers#2775pro3958 wants to merge 3 commits into
Conversation
|
Root cause is the importer being too strict about two legitimate export shapes: Rule.name is nullable in the model (allow_nil: true) yet preflight required it, and RejectedTransfer rows can reference transactions that were deleted after the rejection. The fix requires Rule |
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe import preflight now accepts nullable ChangesRejected transfer import handling
Database schema dump
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ImportAPI
participant ImportPreflight
participant SureImportPreflight
participant FamilyDataImporter
participant Database
ImportAPI->>ImportPreflight: submit NDJSON for preflight
ImportPreflight->>SureImportPreflight: validate Rule and RejectedTransfer references
SureImportPreflight-->>ImportPreflight: return errors and skipped-reference warnings
ImportAPI->>FamilyDataImporter: run strict import
FamilyDataImporter->>Database: resolve referenced transactions optionally
FamilyDataImporter->>Database: create valid RejectedTransfer
FamilyDataImporter-->>ImportAPI: created and skipped summary
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ffa380db7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/sure_import/preflight.rb`:
- Around line 273-278: Update the warning construction in the preflight import
logic around SOFT_REFERENCE_TYPES to use an i18n translation for the complete
user-facing message, including the existing dynamic line, type, field, and value
placeholders and the skipped-during-import text; preserve the existing warning
key and error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba3f2a2f-ccf8-4056-b3ae-89148264b9ba
📒 Files selected for processing (4)
app/models/family/data_importer.rbapp/models/sure_import/preflight.rbtest/models/family/data_importer_test.rbtest/models/sure_import_test.rb
|
Good catch — fixed. |
|
Localized — the missing-reference warning and error templates now live in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/schema.rb`:
- Around line 13-16: Regenerate db/schema.rb with the Rails 8.1.3 toolchain so
the schema declaration uses ActiveRecord::Schema[8.1] and the PostgreSQL
extension is represented as enable_extension "pg_catalog.plpgsql". Preserve the
existing schema contents while updating the generated Rails/pg-specific format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d571650-e537-401b-8df7-3c35973999ee
📒 Files selected for processing (5)
app/models/import/preflight.rbapp/models/sure_import/preflight.rbconfig/locales/models/sure_import/preflight/en.ymldb/schema.rbtest/models/sure_import_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/sure_import/preflight.rb
| ActiveRecord::Schema[7.2].define(version: 2026_07_14_120000) do | ||
| # These are extensions that must be enabled in order to support this database | ||
| enable_extension "pg_catalog.plpgsql" | ||
| enable_extension "pgcrypto" | ||
| enable_extension "plpgsql" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the declared Rails/AR version the schema should target.
fd -H -t f 'Gemfile.lock' | xargs -r rg -n '^\s+(rails|activerecord) \('
fd -H -t f '.ruby-version|.tool-versions' | xargs -r cat
rg -n 'ActiveRecord::Schema\[' db/schema.rb
rg -n 'enable_extension' db/schema.rbRepository: we-promise/sure
Length of output: 652
🏁 Script executed:
#!/bin/bash
set -eu
echo "== Rails/ActiveRecord lock entries =="
grep -E '^\s+(rails|activerecord) \(' Gemfile.lock || true
echo
echo "== .ruby-version / .tool-versions =="
for f in .ruby-version .tool-versions Gemfile docker-compose.yml Dockerfile; do
[ -e "$f" ] && { echo "--- $f"; cat -n "$f"; }
done
echo
echo "== schema header and extensions =="
sed -n '1,30p' db/schema.rb
rg -n 'enable_extension' db/schema.rb
echo
echo "== migrations / schema.rb tracked status =="
git status --short db schema.rb migrations 2>/dev/null || true
git diff -- db/schema.rb 2>/dev/null | sed -n '1,120p' || trueRepository: we-promise/sure
Length of output: 8996
🌐 Web query:
ActiveRecord::Schema[7.2] Rails 8.1 db:schema:load schema format
💡 Result:
In Rails 8.1, ActiveRecord::Schema uses version-specific definitions to maintain compatibility [1][2]. A generated db/schema.rb file typically identifies the current version of the framework, such as ActiveRecord::Schema[8.1] [3][4]. This versioning mechanism allows the schema dumper to correctly use the DSL features appropriate for that specific Rails release [1][5]. When you run bin/rails db:schema:load, Rails loads the schema based on the configured format [6][7]. The format is controlled by config.active_record.schema_format, which defaults to :ruby (generating db/schema.rb) or can be set to :sql (generating db/structure.sql) [8]. Recent updates to Rails have improved the db:schema:load task to better respect the SCHEMA_FORMAT environment variable, ensuring that the loading process correctly interprets the file format when an override is provided [9][10]. When db/schema.rb is used, Rails executes the Ruby code within the ActiveRecord::Schema[version].define block to recreate the database tables and indexes [3][4][1]. If db/structure.sql is used instead, the task loads the schema by executing the raw SQL statements contained in that file [3][4].
Citations:
- 1: https://rubyrubyrubyruby.dev/rails/8.1/ActiveRecord/Schema.html
- 2: http://rubyrubyrubyruby.dev/rails/7.2/ActiveRecord/Schema.html
- 3: https://guides.rubyonrails.org/v8.1.0/active_record_migrations.html
- 4: https://guides.rubyonrails.org/active_record_migrations.html
- 5: https://rubyrubyrubyruby.dev/rails/8.1/ActiveRecord/Schema/Definition/ClassMethods.html
- 6: https://api.rubyonrails.org/v8.1.2/classes/ActiveRecord/Tasks/DatabaseTasks.html
- 7: https://api.rubyonrails.org/v8.1.0/classes/ActiveRecord/Tasks/DatabaseTasks.html
- 8: https://guides.rubyonrails.org/v8.1/configuring.html
- 9: Respect SCHEMA_FORMAT in db:schema:load rails/rails#55059
- 10: rails/rails@ea77ec1
🏁 Script executed:
#!/bin/bash
set -eu
echo "== local Gemfile.lock excerpt around app rails pin and activerecord =="
sed -n '920,935p' Gemfile.lock
sed -n '560,580p' Gemfile.lock
echo
echo "== app configuration/schema format pins =="
rg -n 'ActiveRecord::Schema|schema_format|SCHEMA_FORMAT|db:schema:load|ActiveRecord::Base\.schema_create|schema_creation' app config lib db -g '*.rb' || trueRepository: we-promise/sure
Length of output: 1362
🌐 Web query:
"ActiveRecord::Schema[8.1]" Rails 8.1 schema.rb dump
💡 Result:
In Rails 8.1, ActiveRecord::Schema[8.1].define is the standard method used within the db/schema.rb file to define the current structure of a database [1][2]. Key aspects of this implementation include: Versioned Compatibility: The [8.1] argument specifies the Rails version compatibility layer [3]. This allows Active Record to ensure that the schema is loaded and interpreted using the behavior expected by that specific version of Rails, maintaining stability even as underlying framework internals evolve [3]. Role in Schema Dumping: When you run rails db:schema:dump, Rails introspects your database and generates the db/schema.rb file, which acts as a database-agnostic, Ruby-based representation of your schema [1][4]. The call to ActiveRecord::Schema[8.1].define wraps this generated schema information [1][3]. New Features in Rails 8.1: A notable change introduced in Rails 8.1 is that table columns are now sorted alphabetically within the schema dump [4]. This behavior provides a consistent, deterministic order for columns, which helps reduce unnecessary diffs in version control when multiple developers modify the same tables [5][4]. When upgrading to Rails 8.1, running a schema dump will trigger this reordering, which may appear as a significant change in your initial commit [4].
Citations:
- 1: https://guides.rubyonrails.org/v8.1.0/active_record_migrations.html
- 2: https://guides.rubyonrails.org/active_record_migrations.html
- 3: https://api.rubyonrails.org/classes/ActiveRecord/Schema.html
- 4: https://blog.saeloun.com/2025/12/29/rails-sorts-table-columns-by-name-when-dumping-schema/
- 5: https://fossies.org/linux/www/rails-8.1.3.tar.gz/rails-8.1.3/activerecord/CHANGELOG.md
Keep db/schema.rb in the Rails 8.1 schema format.
The project is pinned to Rails 8.1.3, but this file declares ActiveRecord::Schema[7.2] and uses enable_extension "plpgsql" instead of the Rails 8.1/pg 1.5 enable_extension "pg_catalog.plpgsql" form. Regenerate db/schema.rb under the 8.1 toolchain; db:schema:load on 8.1 may execute the mismatched DSL and future dumps can churn.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db/schema.rb` around lines 13 - 16, Regenerate db/schema.rb with the Rails
8.1.3 toolchain so the schema declaration uses ActiveRecord::Schema[8.1] and the
PostgreSQL extension is represented as enable_extension "pg_catalog.plpgsql".
Preserve the existing schema contents while updating the generated
Rails/pg-specific format.
Importing a full all.ndjson export aborted on data that is actually valid. The preflight listed name as a required field for Rule, but rules.name is nullable and the model allows it, so a single rule with "name": null blocked the entire import. Separately, a RejectedTransfer whose referenced transaction had been deleted raised a hard missing_reference error in preflight and a MissingReferenceError in strict mode, even though the importer already had a skip path for it. Require Rule.id instead of Rule.name in preflight, matching the field the importer actually needs. Treat RejectedTransfer references as advisory: a missing referenced transaction becomes a warning, and the importer resolves the references with required: false so the orphaned row is skipped and counted instead of raising. Fixes we-promise#2721
… boundary
The orphaned-RejectedTransfer path emits warnings as {code, message} hashes via
add_warning, but the published OpenAPI contract documents /api/v1/imports/preflight
warnings as strings. sure_import_preflight_payload copied them through unchanged, so
the endpoint returned a heterogeneous array once that path was exercised and
contract-generated clients could fail to deserialize.
Map warnings to their human-readable message at the API boundary so the array stays
homogeneous strings, matching the documented schema. The internal {code, message}
shape is unchanged. Adds a regression test.
The missing-reference warning and error are user-facing (returned in Result#payload[:warnings]/[:errors]) but were hard-coded. Move the full templates to config/locales/models/sure_import/preflight/en.yml and interpolate line, type, field, and value, per the i18n coding guideline. Warning key and error behavior are unchanged, and the rendered text is identical.
ReviewWhat it does: Two related preflight/import fixes for the full NDJSON (
Correctness: Verified by tracing the code paths — Tests: Both new tests trace back to a real failure on unfixed code (confirmed by manual trace, not by running Rails locally — Ruby version mismatch in this sandbox prevented execution): the preflight test fails pre-fix because Minor (already disclosed by author): an empty-string No other call sites assume non-null Generated by Claude Code |
2 participants
Fixes #2721
Problem
Importing a full
all.ndjsonexport through the Raw Data / Full.ndjsonimportflow aborted on exports that are perfectly valid data, for two reasons:
Rule.namewas treated as required. The Sure preflight listednameas arequired field for
Rule, butrules.nameis nullable (db/schema.rb) and themodel explicitly allows it (
validates :name, length: { minimum: 1 }, allow_nil: true).A single rule with
"name": nullfailed preflight and blocked the entire import.Dangling
RejectedTransferrows aborted the import. ARejectedTransferisan advisory hint that stops two transactions from being re-matched. When one of
the referenced transactions is gone (e.g. deleted after the rejection), the
preflight raised a hard
missing_referenceerror, and — even past preflight — theimporter raised
MissingReferenceErrorin strict (session) mode instead of takingthe
next unless inflow && outflowskip path that was already written for it.Fix
SureImport::Preflight: requireRule.id(the field the importer actually needs,consistent with every other record type) instead of the optional
Rule.name.SureImport::Preflight: treatRejectedTransferreferences as advisory — a missingreferenced transaction is now a warning (
skipped_missing_reference) rather thana blocking error, matching the importer's skip behaviour.
Family::DataImporter#import_rejected_transfers: resolve the transaction referenceswith
required: falseso an orphaned rejected transfer is skipped (and counted asskipped) instead of raising under strict imports.The exporter already filters rejected transfers to family transactions and the DB
enforces
on_delete: :cascade, so new exports cannot contain these rows; this changemakes the importer resilient to existing exports that predate those guarantees.
Tests
test/models/sure_import_test.rb— preflight accepts a nullRule.nameand reportsan orphaned
RejectedTransferas a warning rather than an error.test/models/family/data_importer_test.rb— a strict (session) import skips arejected transfer whose transaction is missing while still importing the valid one.
Migration notes
None.
Notes I made while reviewing this
app/models/sure_import/preflight.rb:28— Droppingnamefrom Rule required fields means an empty-string ("") name — as opposed to null — now passes preflight but would raise ActiveRecord::RecordInvalid during import (Rule validates name length minimum:1 for non-nil values), aborting the whole import transaction instead of surfacing a clean preflight error. Real exports serialize null rather than empty string, so low likelihood.app/models/sure_import/preflight.rb:274— A RejectedTransfer that references a transaction which SHOULD exist but is genuinely missing now degrades silently to a warning + skip rather than surfacing loudly. This is the intended advisory semantics and matches the importer's skip behavior, but it does slightly reduce the diagnostic signal for a malformed export.app/models/family/data_importer.rb:736— In non-strict mode (import_session nil) the old code skipped orphaned rejected transfers silently; now they increment the 'skipped' summary. This is a slight behavior change to the summary counts, but it is more accurate and harmless.app/models/import/preflight.rb:240— sure_import_preflight_payload appends plain-string warnings (lines 241-242) into the same array that now receives {code:, message:} hash warnings from SureImport::Preflight. Previously preflight warnings were always empty, so this heterogeneous mix is newly created and is exposed in the /api/v1/imports/preflight JSON response. No crash, but inconsistent shape for any consumer.app/models/sure_import/preflight.rb:275— The new warning message string is hardcoded English (not t()), consistent with every other message in this preflight file. Not a new violation, but the repo's i18n rule technically covers user-facing strings and these warnings surface via the import UI/API.Summary by CodeRabbit
Bug Fixes
Tests