Skip to content

Conversation

tagliala
Copy link
Member

@tagliala tagliala commented Sep 19, 2025

All supported Rails versions consistently respond to #name and
#table_alias, so branching on #table_name is unnecessary.

This change standardizes on the stable API by resolving models via
join.left.name and, in JoinNode, computing the virtual table name as
join_node.table_alias || join_node.name.

It also retains the early return in chrono_join_history when the join
has already been wrapped and removes the emulation of Arel::Table-like
readers from the SqlLiteral JoinNode.

respond_to? guards in JoinNode have been removed because
Relation#chrono_join_history only instantiates it for nodes that
respond to :name (and Arel::Table provides :table_alias across
all supported Rails versions), so this invariant is enforced at the call
site.

The generated SQL semantics are unchanged; the refactor simply aligns
our code with the methods that are available across Rails supported
rails versions and reduces maintenance

Close #388

@tagliala
Copy link
Member Author

data:
- url: "https://github.com/ifad/chronomodel/pull/389"
  state: "open"
  draft: false
  title: "[WIP] [ci-skip] Test"
  number: 389
  created_at: "2 minutes"
  closed_at: ""
  merged_at: ""
  labels: []
  author: "tagliala"
  comments: 0
data:
- url: "https://github.com/ifad/chronomodel/issues/388"
  state: "open"
  title: "Refactor historical Join Node"
  number: 388
  created_at: "20 minutes"
  updated_at: "5 minutes"
  closed_at: ""
  labels: []
  author: "tagliala"
  comments: 0

Reasoning about the PR and the table in #388

  • What changed:

    • In lib/chrono_model/patches/relation.rb, chrono_join_history was simplified to:
      • Early-return if join.left already has as_of_time (avoids double wrapping).
      • Look up the model only via join.left.name (no more table_name fallback).
      • Replace the left node using JoinNode with @_as_of_time.
    • In lib/chrono_model/patches/join_node.rb:
      • JoinNode now only exposes as_of_time (removed name, table_name, table_alias readers).
      • The virtual table name is resolved as join_node.table_alias || join_node.name.
      • All branching for join_node.table_name vs join_node.name was removed.
  • Why this matches the compatibility matrix in Refactor historical Join Node #388:

    • Per the table, Arel::Table responds to name and table_alias across Rails 7.0 → 8.1, but table_name is only present in 7.0 (it was an alias and disappeared in 7.1+).
    • Arel::Nodes::SqlLiteral does not respond to any of name/table_name/table_alias in all versions, which is fine because JoinNode itself is a SqlLiteral and we’re now relying on its content, not on emulating Arel::Table methods after replacement.
    • Using join_left.name (and table_alias where present) normalizes behavior across all listed Rails versions and removes the brittle dependence on table_name.
  • Potential risk/consideration:

    • Older code comments suggested we previously exposed name/table_name/table_alias on JoinNode to accommodate AR < 7.1 expectations. Removing those readers assumes Rails (even 7.0) won’t call those methods once the left side is a SqlLiteral. The matrix plus the current refactor suggest the plan is to rely on the literal being treated as opaque SQL, which is typically how Arel handles SqlLiteral nodes.
    • As long as Rails 7.0 paths don’t require those emulated readers on the replaced node, this change should be safe and simplifies maintenance. If 7.0 remains in the support matrix, a quick validation on 7.0 would be prudent to ensure no remaining calls rely on JoinNode.name/table_alias. The approach is consistent with the matrix and should be good in practice.

Is this a good change?

  • Yes. It:
    • Removes branching on a method that’s gone in Rails ≥ 7.1 (table_name).
    • Leans on name and table_alias, which are consistently available for Arel::Table across 7.0–8.1 per Refactor historical Join Node #388.
    • Simplifies JoinNode and relation code, reducing compatibility shims and potential NoMethodError paths.
    • Keeps the defensive early-return for nested includes (as_of_time check), preserving existing behavior.

Suggested commit message
Refactor: resolve historical join table via name/table_alias across Rails 7.0–8.1

Problem

  • Arel::Table#table_name was removed in Rails ≥ 7.1. Our join replacement logic branched on #table_name vs #name, leading to brittle paths and potential NoMethodError on newer Rails.

Changes

  • In chrono_join_history, rely on join.left.name to resolve the model and keep the early-return when join.left already responds to as_of_time.
  • In JoinNode, build the virtual table using (join_node.table_alias || join_node.name) and drop the emulation of name/table_name/table_alias on the SqlLiteral node.
  • Remove table_name handling entirely.

Why

  • According to the compatibility table in Refactor historical Join Node #388:
    • Arel::Table responds to name and table_alias across Rails 7.0 → 8.1.
    • table_name only exists in 7.0 and is not present in 7.1+.
  • Using name/table_alias provides a stable, cross-version API surface and simplifies the code while preserving the intended SQL.

Impact

  • No change to the generated SQL semantics; joins still target history via the computed virtual table at @_as_of_time.
  • Avoids relying on deprecated/removed methods and reduces maintenance.

Ref: #388

@tagliala tagliala force-pushed the chore/refactor-join-node branch from daa237c to a17edcb Compare September 19, 2025 10:13
@tagliala tagliala changed the title [WIP] [ci-skip] Test Resolve historical join table via name/table_alias Sep 19, 2025
@tagliala tagliala requested a review from Copilot September 19, 2025 10:14
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This refactor standardizes the historical join table resolution to use consistent Rails API methods across all supported versions. The PR simplifies code by removing branching logic for table name resolution and relies on the stable #name and #table_alias methods available in all supported Rails versions.

Key changes:

  • Eliminates unnecessary branching on #table_name vs #name methods
  • Standardizes on join.left.name for model resolution
  • Simplifies JoinNode implementation by removing duplicate attribute storage

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/chrono_model/patches/relation.rb Removes conditional table name resolution logic, standardizes on join_left.name
lib/chrono_model/patches/join_node.rb Simplifies JoinNode by removing attribute emulation and complex table name resolution

@tagliala tagliala force-pushed the chore/refactor-join-node branch from a17edcb to 238e6fc Compare September 19, 2025 10:20
@tagliala tagliala requested a review from Copilot September 19, 2025 10:23
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

@tagliala tagliala force-pushed the chore/refactor-join-node branch from 238e6fc to 13c89c5 Compare September 19, 2025 10:25
All supported Rails versions consistently respond to `#name` and
`#table_alias`, so branching on `#table_name` is unnecessary.

This change standardizes on the stable API by resolving models via
`join.left.name` and, in JoinNode, computing the virtual table name as
`join_node.table_alias || join_node.name`.

It also retains the early return in chrono_join_history when the join
has already been wrapped and removes the emulation of `Arel::Table`-like
readers from the SqlLiteral JoinNode.

`respond_to?` guards in JoinNode have been removed because
`Relation#chrono_join_history` only instantiates it for nodes that
respond to `:name` (and `Arel::Table` provides `:table_alias` across
all supported Rails versions), so this invariant is enforced at the call
site.

The generated SQL semantics are unchanged; the refactor simply aligns
our code with the methods that are available across Rails supported
rails versions and reduces maintenance

Close #388
@tagliala tagliala force-pushed the chore/refactor-join-node branch from 13c89c5 to d00c3ad Compare September 19, 2025 10:25
@tagliala tagliala merged commit 0b0546c into master Sep 19, 2025
25 checks passed
@tagliala tagliala deleted the chore/refactor-join-node branch September 19, 2025 12:12
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.

Refactor historical Join Node
1 participant