fix(datasource-active-record): normalize demodulized polymorphic types & reuse shared joins

[bexchauveto]

Summary

Two related fixes in the ActiveRecord datasource:

• Normalize demodulized polymorphic types on read — when a record's stored polymorphic *_type is demodulized (e.g. Account instead of Module::Account), resolve it through polymorphic_class_for so the serialized value matches the fully-qualified class name Forest expects.
• Reuse a shared join instead of preloading a has_one :through — track joins by table signature (foreign key → target) rather than by table name only. Joins that share a table with a compatible signature are reused; only genuinely conflicting joins fall back to preload. Filter/sort joins are still never reused.

Test plan

• join_to_one_optimization_spec.rb updated and passing

Note

Normalize demodulized polymorphic types and reuse shared JOINs in ActiveRecord datasource

• Adds normalize_polymorphic_types to ActiveRecordSerializer to resolve demodulized polymorphic type values (e.g. from models with store_full_class_name = false) to fully qualified class names via polymorphic_class_for, for both direct and JOIN-hydrated relations.
• Reworks JOIN planning in Query#split_relations to track joins by signature (source_table.foreign_key->target_table.primary_key) instead of table name, allowing identical JOINs to be reused rather than duplicated or fallen back to preload unnecessarily.
• Introduces joinable_joins, conflicting?, join_signatures, and signature helpers to encapsulate the new conflict-detection logic; joinable_target no longer rejects joins based solely on table reuse.
• Adds dummy models (Api::Note, Api::Topic) and migrations to support polymorphic type normalization tests.
• Behavioral Change: serialized hashes for polymorphic belongs_to type columns now return the resolved class name rather than the raw DB value; JOIN reuse may change query structure for associations sharing an intermediate table.

^{Macroscope summarized c9caa98.}

[qltysh]

4 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with high complexity (count = 6): normalize_polymorphic_types	2
qlty	Structure	Function with many parameters (count = 4): joinable_joins	1
qlty	Structure	Function with many returns (count = 6): joinable_target	1

[qltysh]

Function with high complexity (count = 9): normalize_polymorphic_types [qlty:function-complexity]

[qltysh]

Function with many parameters (count = 4): joinable_joins [qlty:function-parameters]

[qltysh]

Function with high complexity (count = 5): joinable_joins [qlty:function-complexity]

[qltysh]

Function with many returns (count = 6): joinable_target [qlty:return-statements]

[matthv]

Reviewed both fixes (join-signature reuse + polymorphic type normalization). The overall design is sound — source.fk → target captures the common belongs_to ON clause, the :root/:filter sentinels correctly keep self-joins and filter/sort joins out of reuse, and the conflict logic is nicely unit-tested. A few things to address before merge, one of them a confirmed correctness bug.

🔴 Blocking — join signature omits the target join key → wrong record served, silently

utils/query.rb (signature, ~L334) builds "<source_table>.<fk> -> <target_table>", but a belongs_to ON clause is <target>.<join_key> = <source>.<fk>, where <join_key> is the :primary_key option (defaults to the target PK). Two unscoped belongs_to on the same source, same FK, same target table but a different :primary_key produce identical signatures with different ON clauses. conflicting? then treats the second as reusable; AR aliases the second join (distinct reflection), but collect_joined_selects reads the target by its raw table name — so the second relation is served the first join's row. No error, no log.

Reproduced end-to-end on the dummy app (two belongs_to on account_history_id, one primary_key: 'id', one primary_key: 'order_id'): both signatures equal, AR aliases the 2nd join, and the serialized result returns the wrong history id (9 instead of 10).

Suggested fix — fold the target join key into the signature so a differing ON yields a differing signature (→ conflict → safe preload, matching the old behavior):

def signature(reflection)
  join_key = Array(reflection.respond_to?(:join_primary_key) ? reflection.join_primary_key : reflection.association_primary_key)
  "#{reflection.active_record.table_name}.#{Array(reflection.foreign_key).join(',')}" \
    "->#{reflection.klass.table_name}.#{join_key.join(',')}"
end

(The same-FK/same-target case without a primary_key: difference is benign — identical ON, AR collapses to one join. The signature is the only thing standing between "collapse" and "silent alias", so it must encode everything the ON depends on.)

🔴 Blocking — polymorphic normalization is untested and not reproducible with current fixtures

normalize_polymorphic_types (the whole point of fix #1) has no behavioral coverage — the new active_record_serializer_spec.rb only exercises the target_model helper, never asserts a normalized *_type value. More importantly, both polymorphic dummy models (Member, Address) resolve to top-level classes, so polymorphic_class_for(x).name is an identity transform — deleting normalize_polymorphic_types today keeps the suite green.

Please add a namespaced polymorphic fixture (e.g. Api::Foo stored as "Foo" with store_full_class_name = false) and assert the normalized value on both the preloaded (hash_object) and JOINed (hash_joined_relation) paths — the JOINed path is a separate commit and shares no code beyond this method.

🟠 Silent-failure direction — new rescues fail toward "join anyway / keep raw value" instead of the conservative path, and none log

• active_record_serializer.rb (normalize_polymorphic_types, ~L87): rescue StandardError; next keeps the raw, un-normalized type — which is exactly the namespaced case this fix targets (a NameError from constantize gets swallowed → fix silently no-ops). Narrow the rescue to NameError and log (see the [ForestAdmin] Unable to process association … convention in datasource_active_record/…/collection.rb).
• query.rb (join_signatures, ~L330): rescue => {} returns an empty hash, which makes the relation joinable while contributing nothing to conflict detection → can re-open the wrong-data path above. Prefer returning nil → fall back to preload.
• active_record_serializer.rb (target_model, ~L93): rescue => nil silently skips normalization on the JOINed path.

🟠 Test — conflict/preload-fallback case only asserts SQL shape

join_to_one_optimization_spec.rb (~L185) checks includes_values/JOIN count but never runs collection.list, and the fixture Account has no secondary_history, so there's no scenario where the two same-table relations point at different rows. That's exactly the scenario a wrong merge would corrupt. Suggest: two distinct AccountHistory rows, then assert result.first['secondary_history']['id'] is the secondary row (and order:reference still correct).

🟡 Minor

• query.rb:278 — comment still says "Set of tables"; it now returns a Hash{table => signature}. Also worth restoring the deleted rationale (why signature reuse is safe: same table via a different FK→target path is a distinct join AR would alias).
• normalize_polymorphic_types calls reflect_on_all_associations(:belongs_to) per record even for models with no polymorphic belongs_to — worth memoizing per model_class given the perf focus.
• Commit 14ef31b9 carries a trailing [local] marker in its message.

Happy to pair on the signature fix / the namespaced fixture if useful.

[bexchauveto]

Thanks for the thorough review @matthv — all addressed in c9caa98.

🔴 Signature omits the target join key — confirmed and fixed. signature now folds in the target join key (join_primary_key, falling back to association_primary_key), so source.fk → target.join_key. Same source/FK/target but a differing :primary_key now yields a differing signature → conflict → safe preload, exactly as before the optimization. Added a unit assertion on the new format.

🔴 Polymorphic normalization untested / not reproducible — added a namespaced fixture: Api::Note (store_full_class_name = false, polymorphic notable) + Api::Topic, with the *_type column stored demodulized as "Topic". active_record_serializer_spec now asserts the normalized "Api::Topic" on both paths — the preloaded hash_object and the JOINed hash_joined_relation (reached end-to-end via Account belongs_to :note). Confirmed both fail (read raw "Topic") if normalize_polymorphic_types is removed.

🟠 Silent-failure direction —

• normalize_polymorphic_types: rescue narrowed to NameError + logs via the in-package [ForestAdmin] Unable to … convention, keeping the raw value.
• join_signatures: now returns nil (→ preload) instead of {}, so an unknown ON clause can no longer re-open the wrong-data path.
• target_model: rescue narrowed to NameError.

🟠 Conflict/preload-fallback test only asserted SQL shape — it now creates a distinct AccountHistory, sets it as secondary_history, runs collection.list, and asserts secondary_history.id is the secondary row while order:reference stays correct — i.e. the exact wrong-merge scenario.

🟡 Minor — fixed the stale "Set of tables" comment (now Hash{table => signature}) and restored the reuse rationale; memoized the polymorphic-belongs_to lookup per model_class.

One I left as-is: the [local] marker in 14ef31b9's message — not worth rewriting pushed history over. Full suite green (179 examples, 0 failures), Rubocop clean.

[forest-bot]

🎉 This PR is included in version 1.35.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀