Skip to content

feat!: precedence rules at four levels, and rename priority to precedence - #15

Merged
njakobsen merged 19 commits into
docs/usage-examplesfrom
feat/precedence-rules
Oct 26, 2026
Merged

njakobsen merged 19 commits into
docs/usage-examplesfrom
feat/precedence-rules

Conversation

@njakobsen

@njakobsen njakobsen commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #14. Breaking: callers rename priority to precedence, existing installs run the generated migration, and removing an entry now falls back to the next source instead of blanking the attribute. The stack above is released together as 1.0.

Changes

  • Precedence rules table: all precedence moves into laforge_precedence_rules, at four levels from most to least specific: one attribute of one record, one attribute of a model, every attribute of a model, and each source's global precedence. A per-record precedence can now be set before the source reports the attribute, and survives the source's entries being destroyed.
  • Schema: "any" is stored as '' or 0 so one unique index on scope and source covers every level and serves lookups by record type, id and attribute. A stored generated global_precedence column with a unique index keeps the global order free of ties. Rules reference their source with ON DELETE CASCADE.
  • Rename: priority becomes precedence throughout: DataSource.create!(name:, precedence:), record_data_entries(…, precedence:), update_attribute_source_precedence (records and models), update_source_precedence (models), source_precedences, DataSource.by_precedence, DataSource.maximum_precedence, and DataEntry#effective_precedence in place of priority_with_fallback. find_or_create_by(name:, priority:) becomes find_or_create_by!(name:) { |source| source.precedence = n }.
  • Upgrade migration: rails generate laforge:precedence_rules moves existing source and entry priorities into rules and drops both columns. It is reversible; up and down were checked against sample data. up first checks that every source has a priority, that no two share one, that entries with a priority belong to sources that exist, and that entries for the same record, attribute and source agree on theirs, raising and naming them before it changes anything.
  • Removal falls back: a forge leaves out entries marked by remove_data_entries, so each attribute falls back to the next source, or keeps its value where no other source has one, as the next forge would. Recording nil blanks an attribute.
  • Upgrading: the README has an Upgrading section, Upgrading from 0.0.1.alpha to 1.0, with the Rails minimum, the migration, the renames and the removal change; each PR above adds its own steps to it. The version is now 1.0.0.
  • Cache: LaForge.with_precedence_cache { … } reads the global and model-wide rules once for an import run instead of once per record. Rules changed through LaForge inside the block apply immediately; rules changed by SQL are seen once the block ends. A record also keeps the rules it read for its last forge until it is forged again or reloaded.
  • record.laforge: LaForge's methods move onto LaForge::Record. Each laforged model has its own subclass, Model.laforge, holding the model-level methods, and each record an instance, record.laforge, holding its methods, its forge's pending writes and its precedence resolver. The concerns AttributeGroups, Recording, Forging, Precedence and Inspection make it up, and the model delegates the documented API to it, so place.forge! is place.laforge.forge!.
  • Inactive sources: a source's active flag (rails generate laforge:active_sources) leaves its entries out of every forge while keeping them, and forge_attributes(except_sources:) answers what would be forged without a source.
  • Ranking by level: sources rank by level first, a tie at the sources' most specific level falling to their global precedence, and update_precedence_order sets the order of sources for one record's attribute, a model's attribute, a whole model, or globally.
  • Attribute groups: laforged attribute_groups: keeps attributes such as an address's lines from one source together.
  • Change events: data_entry_changed.laforge is published for each entry saved with a changed value or destroyed, once the write is committed, and not for a write that rolls back, with the values as they were saved. The gem now requires Rails 7.2, which the events need.

UAT Steps

  1. bundle exec rspec: 157 examples, 0 failures, 2 pending.
  2. In an app with existing data, run rails generate laforge:precedence_rules and rails db:migrate, then confirm forged values are unchanged; roll back to confirm the columns are restored.

Closes

Unreported

Special Handling

  • This PR requires user testing
  • Include this PR in the changelog

🤖 Generated with Claude Code

njakobsen and others added 3 commits October 16, 2026 16:28
…o precedence

Issue: precedence lived in two columns, `laforge_data_sources.priority` and `laforge_data_entries.priority`. A per-record priority could only be set on an entry that already existed, so it could not be set before a source reported the attribute, and destroying a source's entries (an orphan sweep) discarded it. There was no way to rank a source for one attribute across every record of a model, and wholesale reordering meant editing two tables. "Priority" also read as time-sensitive, while the ranking is about which source wins.

All precedence now lives in a `laforge_precedence_rules` table with four levels, most specific first: one attribute of one record, one attribute of a model, every attribute of a model, and each source's global precedence. "Any" is stored as `''` or `0` so one unique index on scope and source covers every level and serves lookups by record type, id and attribute. A stored generated `global_precedence` column with a unique index keeps the global order free of ties. Rules reference their source with `ON DELETE CASCADE`.

Instead of `priority` columns and keywords, the API uses `precedence`: `DataSource.create!(name:, precedence:)`, `record_data_entries(…, precedence:)`, `update_attribute_source_precedence` on records (now working without an existing entry) and on models, `update_source_precedence` for a whole model, `source_precedences`, `DataSource.by_precedence` and `DataSource.maximum_precedence`, and `DataEntry#effective_precedence` in place of `priority_with_fallback`. `DataEntry.in_precedence_order` ranks by the record's effective precedence, then the source's global precedence, then recency.

`rails generate laforge:precedence_rules` creates a migration that moves existing source and entry priorities into rules and drops both columns; it is reversible.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: every forge read the model-wide and global precedence rules again, two queries per record, so an import of many rows repeated the same reads for each one. The Rails query cache cannot be limited to these tables and is cleared by any write, which an import makes on nearly every row.

`LaForge.with_precedence_cache { … }` reads the global and model-wide rules once per model for the duration of the block, per thread and fiber, with nested blocks sharing the outer cache. A record's own rules still load with the record. Rules changed through LaForge inside the block clear the cache immediately, including mid-transaction and on rollback; rules changed by SQL or another process are seen once the block ends.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Root cause: a record memoizes the model-wide and global precedence rules it reads for a forge, and only the next forge reads them again. `reload` kept the memo, so ranking a reloaded record's entries outside a forge, such as with `DataEntry.in_precedence_order`, used rules from before the reload and missed sources or rules created since.

`reload` now forgets the memoized rules, so the next ranking reads them afresh, from the precedence cache inside `LaForge.with_precedence_cache`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@njakobsen
njakobsen force-pushed the feat/precedence-rules branch from 337003e to ca956bf Compare October 16, 2026 23:29
…les migration changes anything

Root cause: the `laforge:precedence_rules` migration created `laforge_precedence_rules` and then copied each source's `priority` into a global rule. A source with a NULL `priority`, which raw SQL or `update_column` can leave since only the model validated it, was skipped without a word and left without a global precedence. Two sources sharing a `priority` failed the insert on the unique `global_precedence` index after MySQL had already committed `create_table`, leaving an empty table and both `priority` columns, so a re-run failed on the existing table.

`up` now checks the sources first: a source with no `priority` raises naming the sources, and a shared `priority` raises naming each shared value and its sources, before any table or column is changed.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 23, 2026 07:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The upgrade can change existing winning values, and caching introduces failures on supported Rails versions and stale rankings after rollback.

3 open issues
What changed in this PR

This PR replaces LaForge’s source and entry priorities with four-level precedence rules, extending how applications control which source wins when records are forged.

Changes:

  • Renames the public priority APIs to precedence and adds record, model-attribute, model-wide, and global rules.
  • Adds installation and upgrade migrations for the rules table.
  • Introduces import-scoped caching and updates documentation and specs.
File Description
spec/​spec_helper.rb Sets up the precedence rules test schema.
spec/​lib/​usage_spec.rb Covers rule levels, persistence, caching, and renamed APIs.
spec/​lib/​precedence_rules_migration_spec.rb Tests source-priority migration preconditions.
spec/​lib/​data_source_spec.rb Updates source setup to use precedence.
spec/​lib/​activerecord_spec.rb Updates forging and recording specs for precedence rules.
Readme.md Documents installation, precedence rules, caching, and usage.
lib/​laforge/​precedence_rule.rb Defines rule validation and precedence resolution.
lib/​laforge/​precedence_cache.rb Adds block-scoped caching of shared rules.
lib/​laforge/​data_source.rb Stores global precedence through an associated rule.
lib/​laforge/​data_entry.rb Resolves and sorts entries using precedence rules.
lib/​laforge/​activerecord.rb Integrates rule management and resolution into models.
lib/​laforge.rb Loads the new rule and cache components.
lib/​generators/​laforge/​templates/​precedence_rules_migration.rb.tt Migrates legacy priorities into rules and provides rollback.
lib/​generators/​laforge/​templates/​migration.rb.tt Creates the rules schema for new installations.
lib/​generators/​laforge/​precedence_rules_generator.rb Generates the upgrade migration.

Review effort: Balanced

Comment thread lib/laforge/precedence_cache.rb Resolved
Comment thread lib/laforge/data_source.rb Resolved
njakobsen and others added 2 commits October 23, 2026 00:54
… their priority before migrating

Root cause: the `laforge:precedence_rules` migration gave each record, attribute and source one rule with the `MAX(priority)` of its entries. Duplicate entries are allowed, so where an older one had priority 100 and a newer one 10, the older value won before the upgrade, and afterwards both shared the rule of 100 and the newer value won by recency; rolling back gave both 100.

`up` now also checks the entries before changing anything: entries for the same record, attribute and source with different priorities, or with a priority on only some of them, raise, naming up to ten of them and saying to give them the same priority or delete the duplicates first.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Root cause: `PrecedenceCache` keeps its store in `ActiveSupport::IsolatedExecutionState`, which Rails 7.0 introduced, but the gemspec still allowed any Rails above 5.2, so on Rails 6 forging, and saving a source, raised `NameError` even without caching.

The gemspec now requires Rails 7.0 or later.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 23, 2026 08:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved migration safety, precedence refresh, and missing-source ranking issues can leave upgrades incomplete or produce incorrect forged values.

1 open issue
3 previously missed issues

In code that hasn't changed since last review

Medium severity Refresh persisted record rules before each forge

lib/​laforge/​activerecord.rb:85

This resets the global/model resolver but leaves the loaded precedence_rules association unchanged. After one forge, updating this record's rule through another instance and forging the original instance again still uses the old per-record precedence. That does not provide the refresh-on-next-forge behavior described in the PR. Refresh persisted per-record rules for a new forge while preserving pending in-memory additions, edits, and removals; an unconditional association reload would lose those pending changes.

Medium severity Avoid cache invalidation for record-specific rule saves

lib/​laforge/​precedence_rule.rb:31

Saving a record-specific rule clears the shared global/model cache even though that rule is read directly from the record association. An import that records precedence: on each row therefore reloads global and model rules after every saved row, defeating with_precedence_cache. Limit save/destroy invalidation to global/model rules and persisted rules moving out of those scopes. A new record-specific rule's previous record_id can be the default 0, so creation must not be mistaken for a scope transition. Keep rollback invalidation conservative.

Medium severity Rank missing source precedence below all numeric values

lib/​laforge/​precedence_rule.rb:72

A missing source now receives precedence 0 rather than ranking below every numeric precedence. Deleting a source removes its rules but leaves its data entries, so an orphaned entry can outrank a live source with a valid negative precedence and overwrite that source's value during forging. Use negative infinity for a missing global rule to preserve the previous missing-source ordering.

3 resolved since last review

Review effort: Balanced

…the precedence rules migration

Root cause: `laforge_data_entries` has no foreign key to its source, so deleting a source leaves its entries. An orphaned entry with a priority passed the migration's checks, then its rule failed the new table's foreign key after `create_table` had been committed on MySQL, so a re-run failed on the existing table.

`up` now also raises before changing anything when entries with a priority belong to sources that no longer exist, naming the missing source ids and saying to delete those entries or clear their priority first.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 23, 2026 08:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The missing-source fallback can promote orphaned entries over live sources and change forged values after deletion or upgrade.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Preserve negative-infinity fallback for orphaned source precedence

lib/​laforge/​precedence_rule.rb:72

A missing global rule now ranks as 0 instead of below every numeric precedence. Deleting a source removes its rules but leaves its entries, and the upgrade permits orphaned entries whose old priority is NULL. An orphan can therefore beat a live source with negative precedence, or tie a live source at zero and win by recency, changing forged values after deletion or upgrade. Preserve the previous -Float::INFINITY fallback for missing global rules.

1 resolved since last review

Review effort: Balanced

Comment thread Readme.md Outdated
rails db:migrate
```

Upgrading from a version that stored `priority` on sources and entries? `rails generate laforge:precedence_rules` creates a migration that moves both into the precedence rules table (see [Precedence](#precedence)).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should have a dedicated "Upgrading" section with subheadings for each transition that explain what changed and anything that needs to be done to upgrade. The subheadings would be along the lines of "Upgrading from 1.0 to 1.1", etc.

@njakobsen njakobsen Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0d23865: a new ## Upgrading section has ### Upgrading from 0.0.1.alpha to 1.0, with one release for the whole stack, and d56fb70 bumps the version to 1.0.0. Setup now just points to it. Each PR above adds its own steps to the same section: #16 (c89ed9b) adds the Rails 7.2 minimum, the recorded_at and active_sources generators, level-first ranking, values JSON cannot hold, and a form's *_attributes/*_ids. #18 adds the expiry generator and #19 the rulesets one.

Comment thread Readme.md Outdated
```

**Note:** calling `remove_data_entries` marks the Data Entries for destruction, but they are not destroyed and the removed attributes not nullified until `forge!` is called. A marked entry still takes part in the forge as a `nil` value, so the attribute is blanked rather than falling back to a lower priority source. See [Falling back to the next source](#falling-back-to-the-next-source).
**Note:** calling `remove_data_entries` marks the Data Entries for destruction, but they are not destroyed and the removed attributes not nullified until `forge!` is called. A marked entry still takes part in the forge as a `nil` value, so the attribute is blanked rather than falling back to a lower precedence source. See [Falling back to the next source](#falling-back-to-the-next-source).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to review this. It seems to go against the concepts we've set out with adding associations in the later PRs. The name suggests the entry is being removed and so I would expect it not to have any effect on lower precedence sources. If the desire was to blank out the data, the user should be recording a nil or other blank value.

    foo.record_data_entries({ height: 19, name: 'El Capitan' }, source2)
    foo.record_data_entries({ height: nil, width: 5 }, source1)

Otherwise I would imagine we end up with a blank value after forging, but since the data entry is destroyed, the next forge call would apply the value from the lower precedence source.

@njakobsen njakobsen Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 30367da (feat!): a forge now leaves out entries marked by remove_data_entries. Each attribute falls back to the next source, or keeps its value where no other source has one, which is what the next forge does and matches deactivating, withdrawing and expiry. To blank an attribute, record nil, as in your example. The README examples for removal, falling back and a source that stops reporting are rewritten, and Upgrading has a before/after step for code that relied on the blanking. #16 needed one scenario spec changed to record nil (01e26bb).

Comment thread Readme.md Outdated

A custom priority stays with the entry when the same attribute is recorded again from the same source without one. Pass
`priority: nil` to clear it so the entry falls back to its source's priority.
A record's precedence for an attribute is kept when the source records the attribute again without passing one, and removed by passing `precedence: nil`. It does not depend on the source's entries: it can be set before the source has recorded the attribute, and it survives the source's entries being destroyed. Pass `nil` to any of the setters above to remove a rule.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid colons in sentences unless used to introduce a list.

@njakobsen njakobsen Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0d23865: "It does not depend on the source's entries, so it can be set…". I swept the rest of the README the same way, keeping colons only where they introduce a list, on #15 here and on the text each later PR adds (c89ed9b on #16, and amended into #18 and #19).

Comment thread Readme.md Resolved
Comment thread Readme.md Outdated
end
```

Equal precedences are broken by the sources' own precedence. To reorder sources, update their precedence; `LaForge::DataSource.by_precedence` lists them highest first and `LaForge::DataSource.maximum_precedence` returns the highest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have a unique index that prevents equal precedence?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for the global level: global_precedence has a unique index, so sources never tie there. Ties can only happen at a more specific level, such as two model-wide rules of 3 for the same attribute, and those are ranked by the sources' own precedence. Fixed the sentence in 0d23865 to say exactly that.

Comment thread Readme.md Outdated
```

### Entry priority
### Authoritative sources

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't introduce a concept of "Authoritative". LaForge allows such a concept to be implemented by the host app, but no such flag or concept exists in LaForge itself. Keep this about per-entry precedence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0d23865: the section is now "Per-entry precedence", and covers only precedence: when recording, for one attribute of one record. The model-wide example is gone (the Precedence table already covers that level). On #16 its precedence-order example uses the place's own order instead of a model-wide one.

njakobsen and others added 3 commits October 23, 2026 11:36
Issue: `remove_data_entries` marks entries for destruction, and a forge took a marked entry as a `nil` value at its source's precedence, so the attribute was blanked even where a lower source had a value. Once the entries were destroyed, the next forge fell back to the lower source, so the same entries forged two different results, and blanking depended on removing inside `forge!` rather than on what the sources say.

Instead of forging a marked entry as `nil`, a forge now leaves marked entries out, as the next forge will not find them. Each removed attribute falls back to the next source, and an attribute no other source has an entry for keeps its value, as when the entries are destroyed. To blank an attribute, record `nil` for it, which keeps outranking lower sources at every later forge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… ties at more specific levels

Issue: upgrade notes were scattered through Setup as questions, with no version to say which upgrade they belong to and nothing about the renamed API or the change to removal. "Authoritative sources" named a concept LaForge does not have, the tie-break sentence suggested global precedences could tie although a unique index prevents it, and the precedence cache section did not mention a controller action.

`Upgrading from 0.0.1.alpha to 1.0` lists the Rails minimum, the precedence rules migration, the renames from `priority` to `precedence`, and recording `nil` where a removal was meant to blank. Setup points to it. The section on raising a source for some attributes is now "Per-entry precedence", the tie-break sentence covers only the more specific levels, the cache section shows an `around_action`, and sentences no longer use colons except to introduce a list.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 23, 2026 18:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The missing-rule fallback can promote orphaned entries above valid sources and change forged values after upgrading.

1 open issue

Review effort: Balanced

Comment thread lib/laforge/precedence_rule.rb Resolved
Comment thread Readme.md Outdated
foo.record_data_entries({ height: 18 }, source2) # Still priority 3
foo.record_data_entries({ height: 18 }, source2, priority: nil) # Back to source2's priority
class PlacesController < ApplicationController
around_action(only: :update) { |_controller, action| LaForge.with_precedence_cache { action.call } }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow the exact example in https://guides.rubyonrails.org/action_controller_overview.html#other-ways-to-use-callbacks.

Suggested change
around_action(only: :update) { |_controller, action| LaForge.with_precedence_cache { action.call } }
around_action(only: :update) { |_controller, action| LaForge.with_precedence_cache(&action) }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 57757d5: the example now reads LaForge.with_precedence_cache(&action).

…source

Root cause: `PrecedenceRule::Resolver#global_precedence` returned `0` for a source with no global rule. The precedence rules migration keeps entries without a priority whose source no longer exists, which an install without foreign keys can have, so after upgrading such an entry outranked a live source with a negative precedence, and tied with one at `0`, where the newer entry won. Before the migration, a missing source priority ranked below every number.

Instead of `0`, a source without a global rule now ranks as `-Float::INFINITY`, below every source that has one, as before the migration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ntroller example

Issue: the `around_action` example wrapped `action.call` in a block of its own, where the Rails guide passes the action straight through, as in https://guides.rubyonrails.org/action_controller_overview.html#other-ways-to-use-callbacks.

The example now reads `LaForge.with_precedence_cache(&action)`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 23, 2026 19:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Test cleanup can leave a shared source unranked, and the destructive migration lacks database-backed round-trip coverage.

1 open issue
3 previously missed issues

In code that hasn't changed since last review

Medium severity Add database-backed migration round-trip coverage

lib/​generators/​laforge/​templates/​precedence_rules_migration.rb.tt:25

Please add a database-backed round-trip spec for the data transfer and rollback. The migration specs stub all preflight query results, and the success-path example deliberately raises at create_table, so no example exercises these INSERTs, the removal of the priority columns, or down. Seed legacy sources and entries with both custom and inherited priorities, run up, verify the resulting rules and forged values, then run down and verify the restored priorities. This would protect the destructive upgrade path rather than only its error reporting.

Low severity Document name-only source lookup with rule creation

Readme.md:52

The upgrade instructions also need the special case for source lookups. Simply renaming find_or_create_by(name:, priority:) to use precedence: generates a query against a nonexistent column, because precedence now lives in an associated rule. Document the name-only lookup with a creation block, as used in the updated specs.

Low severity Reset data_entries association after scoped override deletion

Readme.md:244

The scoped destroy_all does not clear an already-loaded place.data_entries association, such as the one loaded by a previous forge. Since forging reads that association with to_a, the next forge can still apply the deleted override. Retain the earlier association-reset instruction (or require a reload) before promising the same fallback behavior.

1 resolved since last review

Review effort: Balanced

Comment thread spec/lib/activerecord_spec.rb Resolved
Issue: the comments added for precedence rules leaned on terms defined nowhere ("model-wide", "effective precedence"), described classes as things that act, joined separate statements with colons and semicolons, put the reason a caller ranks by global precedence inside the callee, and left branches undocumented, such as the `-Float::INFINITY` fallback for a source without a global rule, the inner join in `DataSource.by_precedence`, and what `write_precedence_rule` returns. One claim about index prefix use for lookups could not be verified from the code.

The `PrecedenceRule` overview now defines a record's own rules, model-wide rules and effective precedence, and the other comments use those terms. Method comments open with what they return and cover their nil and fallback cases, `DataEntry.in_precedence_order` lists its ordering keys as a list, class comments say what the class is used for, and a few spec descriptions and inline spec comments are reworded to describe the code as it stands. The generator templates' comments list the precedence rules migration's checks, the entries-from-deleted-sources check among them, and point the install migration to `PrecedenceRule` for the levels, the `remove_data_entries` comment names its `attribute_names:` keyword, and the shared-priorities error is two sentences.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 23, 2026 20:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Saving record-specific precedence rules unnecessarily invalidates shared caches, defeating import-wide caching when each row records a precedence.

1 open issue
2 previously missed issues

In code that hasn't changed since last review

Medium severity Limit cache invalidation to global and model-wide rules

lib/​laforge/​precedence_rule.rb:35

Saving or removing a record-specific rule clears the global/model-wide cache, even though record-specific rules are never stored there. An import that records each new row with precedence: therefore invalidates the cache after every forge!, making the next row reload both shared rule sets and defeating with_precedence_cache. Limit save/destroy invalidation to global or model-wide rules, including updates that move a rule out of those scopes, while retaining rollback invalidation.

Low severity Avoid clearing required DataSource precedence

Readme.md:127

The setters listed above include source2.update!(precedence: ...), but passing nil there raises ActiveRecord::RecordInvalid: DataSource requires a precedence, and its writer assigns nil rather than removing the global rule. Restrict this instruction to the record/model setters and state that a source must retain a global precedence.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 26, 2026 18:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved correctness issues affect deletion event payloads, source validation after reordering, and precedence values near the database limit.

5 open issues

Review effort: Balanced

Comment thread lib/laforge/data_entry.rb
CHANGED = 'data_entry_changed.laforge'.freeze

after_save { DataEntry.publish_change(self, value_was: value_before_last_save, value: value) if previously_new_record? || saved_change_to_value? }
after_destroy { DataEntry.publish_change(self, value_was: value, value: nil, destroyed: true) }
private

def precedence_is_unique
return if precedence.nil?
Comment on lines +89 to +90
clear = [PrecedenceRule.global.maximum(:precedence) || 0, order.size].max + 1
rules.each_with_index { |rule, index| rule.update!(precedence: clear + index) } # clear of the unique index first
Comment on lines +40 to +41
after_save { PrecedenceCache.clear }
after_destroy { PrecedenceCache.clear }
njakobsen and others added 6 commits October 26, 2026 12:06
…ge`, which the model delegates to

Issue: `laforged` mixed every LaForge method into the model through `InstanceMethods` and `ClassMethods`, so a model carried LaForge's internals beside the API the application calls. `laforge_precedence_of` and `laforge_global_precedence` were public on every record, the precedence resolver lived on the model as `@laforge_precedence_resolver`, and LaForge could not be called apart from the model's own methods.

`laforged` now gives each model its own subclass of the new `LaForge::Record`, reached through `Model.laforge`, and each record an instance of it, reached through `record.laforge`. `LaForge::Record` includes the concerns `Recording`, `Forging`, `Precedence` and `Inspection`, whose instance methods act on the record and whose `class_methods` act on the model. Each concern lists its API in `API`, and `CLASS_API` where it has one, and `LaForge::ActiveRecord::Model` delegates them, so `place.forge!` is `place.laforge.forge!` and `Place.update_source_precedence` is `Place.laforge.update_source_precedence`. Forging and ranking are unchanged.

- A subclass of a laforged model gets a subclass of its parent's `laforge`, with `model` set to itself, so it keeps what the parent declared.
- The delegators live in `Model`, a module the model includes, so a method the model defines after `laforged` can wrap LaForge's and call `super`.
- `reload` and `initialize_dup` drop `@laforge`, so a reloaded record reads the rules again and a duplicate gets its own `laforge` rather than the original's.
- The low-level methods stay public on `record.laforge`, marked `@api private`, so LaForge's own objects call them without `send`.
- `record_data_entry` records through `record_data_entries`, so a single value and several go through one pipeline.
- `LaForge::ActiveRecord.set_marked_for_destruction` is the one place that clears a record's destruction mark, since Active Record has no method for it.
- `Model.laforge` reads as "Place.laforge" in `inspect` and error messages, and leaves Ruby's `name` alone.
- The README notes the `laforge` form.
- `forge!` forges through the model's `forge`, so a `forge` the model defines after `laforged` wraps `forge!` too.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd forge without given sources

Issue: a source that was no longer used could only be stopped by deleting its entries, which lost what it said, and there was no way to ask what would be forged without one source's values, such as the import value an edit replaced.

A data source now has an `active` flag. A forge leaves out an inactive source's entries as if it had none, so each attribute falls back to the next source, or keeps its value where no other source has one, while the entries are kept and recording still writes them, so activating the source again restores what it says at the next forge. `forge_attributes(except_sources:)` leaves out the given sources' entries in the same way, through the new `record.laforge.forging_entries`.

- Nothing changes until a record is forged again, as for any change to precedence.
- `DataSource.inactive_ids` reads through `PrecedenceCache`, and saving `active`, or rolling such a save back, clears the cache, so a forge inside `with_precedence_cache` sees the change and never an undone one.
- The install migration creates the column, and `rails generate laforge:active_sources` adds it to an existing install with every source active, which the Upgrading section lists.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ach level

Issue: a settings screen that orders sources per record, per field, per model and globally had to choose precedence numbers itself, and a number at one level was compared with numbers at other levels, so a source added later with a high global precedence jumped above an order someone had set for one record's field.

Instead of comparing each source's most specific rule by its number, sources now rank by level first. A source with a rule at a more specific level outranks every source whose most specific rule is less specific, and precedences only compare sources with rules at the same level, so a single rule, including `precedence:` given while recording, puts its source above every source without one at that level. `PrecedenceRule::Resolver#rank` returns the comparable rank, and `in_precedence_order` and forging use it.

`record.update_precedence_order(:attribute, [sources])`, `Model.update_precedence_order([sources], attribute: nil)` and `LaForge.update_precedence_order([sources])` set the order at each level and choose the numbers. Sources not listed rank below the listed ones, in the next level's order, and nil or `[]` removes an order. `record.precedence_order(:attribute)` and `Model.precedence_order(attribute:)` list the active sources in rank order with the level that ranks each, and `PrecedenceRule.for_records(Model)` lists records' own rules.

- A tie at the sources' most specific level falls to their global precedence, not to a less specific rule only one of them has.
- With no attribute, a model-wide rule is labelled `:model`, as it is when an attribute is given.
- A source with no rule at any level, as entries of a deleted source can have, is listed last with a nil level.
- `precedence_order` reads the rules afresh, so rules changed since the last forge count.
- The global order renumbers every source, moving them clear of the unique index first.
- The README documents the orders and an optional index for counting across records, and the Upgrading section says to check per-record precedences.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… groups

Issue: attributes that only make sense from one source together, such as the lines of an address, were resolved one by one, so a forge could take one source's street and another's postcode.

`laforged attribute_groups: { postal_address: %i[street postcode city] }` declares groups, which the new `AttributeGroups` concern reads and checks. Forging takes every attribute of a group from the source whose entry for any of them ranks highest, and an attribute that source has no entry for is nil. Recording part of a group raises, naming what is missing, precedence rules are set on the group, a rule on one of its attributes raises, and removing or forging one attribute covers the whole group.

- An attribute the winning source has no entry for is nil rather than taken from another source, which would split the group.
- `laforged` raises for a group of fewer than two attributes, a group named like an attribute, and an attribute in two groups.
- The resolver reads a grouped attribute's rules under its group's name, and `precedence:` given while recording goes to the group.
- A group's attributes are assigned together, where the first of them would be.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue: an application that follows what sources say, such as to notice that an import changed the value an editor's entry replaced, had to read every entry to find what changed.

Each data entry saved new or with a changed value, and each one destroyed, publishes `LaForge::DataEntry::CHANGED` (`'data_entry_changed.laforge'`) with the entry, its record, attribute and source, `value_was`, `value` and `destroyed`. Every event goes through `DataEntry.publish_change`.

- The event fires from the current transaction's `after_commit`, so a write that rolls back is never announced, and a write outside an explicit transaction is published as it is saved.
- The payload is taken when the write is made, its values copied, so `value_was` is right when one transaction saves an entry twice, and a value changed in place before the commit reaches subscribers as it was saved.
- `ActiveRecord::Base.current_transaction` needs Rails 7.2, so the gemspec and the Upgrading section require it.
- Writes that skip callbacks, such as rows removed by database cascades, and entries deleted with their record, publish nothing.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…deleted source's precedence from a reloaded record

- `spec/spec_helper.rb` connects to the database `LAFORGE_TEST_DB` names, and to `laforge_test` without it, so several checkouts can run the suite at once.
- The example that deletes `bbc`'s global rule reloads the source before restoring its precedence. The source's cached `global_precedence_rule` is the deleted row, so saving it unreloaded leaves `bbc` with no global precedence for the examples after it.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@njakobsen
njakobsen force-pushed the feat/precedence-rules branch from 7ca29f2 to 3d7a8ca Compare October 26, 2026 22:11
Copilot AI balanced review requested due to automatic review settings October 26, 2026 22:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +39 to +40
clash = groups.keys & seen.keys
raise ArgumentError, "#{model.name} attribute group #{clash.first} is also an attribute of group #{seen[clash.first]}; give the group its own name" if clash.any?
@njakobsen
njakobsen merged commit bb41c7e into master Oct 26, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants