Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 12, 2025

Historical objects were incorrectly using their valid_to timestamp to query for associations, which could return records that didn't exist during the object's validity period.

Problem

When querying associations from historical records, ChronoModel was using the as_of_time method which was aliased to valid_to (the upper bound of the validity range). Since PostgreSQL ranges are left-inclusive and right-exclusive [start, end), querying at exactly valid_to could find records that became valid at that boundary time, even though they didn't exist during the historical record's validity period.

Consider this example:

country = Country.create name: 'Country'
city = country.cities.create name: 'City'

# Update both records in a transaction
city.transaction do
  country.update_column :name, 'Country 2'
  city.update_column :name, 'City 2'
end

# This should return "City" but was returning "City 2"
country.history.first.cities.first.name

The historical country record was querying for cities at its valid_to timestamp, which coincided with when the new city version became valid, resulting in the wrong association being returned.

Solution

Modified the as_of_time method to handle boundary timestamp issues:

  • For Time objects: Subtract 1 unit at precision 6 (microsecond) to query "just before" the boundary
  • For other values: Return as-is for backward compatibility

The implementation uses valid_to.is_a?(Time) to detect finite timestamps and leverages the existing ChronoModel::VALIDITY_TSRANGE_PRECISION constant for consistent precision handling.

This ensures association queries happen at timestamps that existed during the historical record's validity period, not at boundary times where the wrong version might be selected due to PostgreSQL's half-open range semantics.

Fixes #283.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Historical objects should not use valid_to to get associations Fix historical objects using valid_to for association queries Aug 12, 2025
Copilot AI requested a review from tagliala August 12, 2025 08:00
Copilot AI changed the title Fix historical objects using valid_to for association queries Fix historical objects using microsecond precision for association queries Aug 12, 2025
Copilot AI requested a review from tagliala August 12, 2025 08:45
Copilot AI changed the title Fix historical objects using microsecond precision for association queries Fix historical objects using dynamic precision detection for association queries Aug 15, 2025
@tagliala
Copy link
Member

@copilot please ignore all the previous attempt

Let's start again with this knowledge (from your colleague Claude AI)

I do not trust its solution, but please attempt to understand what it is saying:


ChronoModel Issue #283: Summary of Findings

The Problem

When objects are updated in the same transaction, they get the same valid_to timestamp, creating an edge case where association queries return the wrong version.

Clear Example

# Transaction at timestamp T = 2025-08-16 16:05:17
country = Country.create name: 'Country'
city = country.cities.create name: 'City'

Country.transaction do
  country.update_column :name, 'Country 2'  
  city.update_column :name, 'City 2'
end

Resulting ranges:

  • Historical country: [T1, T) with name "Country"
  • Historical city: [T2, T) with name "City"
  • Current country: [T, ∞) with name "Country 2"
  • Current city: [T, ∞) with name "City 2"

The Bug

When historical country queries for cities using its valid_to timestamp T:

-- ChronoModel's query:
T <@ history.cities.validity

-- Results:
T <@ [T2, T)  = false  ✅ (excludes historical "City") 
T <@ [T, ∞)   = true   ❌ (includes current "City 2" - WRONG!)

Expected: Get "City" (historical)
Actual: Get "City 2" (current)

Why This Happens

PostgreSQL ranges are half-open by default: [start, end)

  • The <@ operator correctly excludes the upper bound T from [T2, T)
  • But it correctly includes the lower bound T in [T, ∞)
  • So we get the new version instead of the old version

The Solution: Subtract 1 Microsecond

Instead of querying at the exact valid_to timestamp, query just before it:

# Instead of:
time = historical_country.validity.end  # T

# Use:
safe_time = historical_country.validity.end - 0.000001  # T - 1μs

# Query becomes:
(T - 1μs) <@ [T2, T)  = true    (includes historical "City")
(T - 1μs) <@ [T, )   = false   (excludes current "City 2")

Why Excluding valid_to is Correct (Finite Ranges)

When valid_to is finite, the object ended at that moment:

  • At timestamp T, "Country" becomes "Country 2"
  • At timestamp T, "City" becomes "City 2"
  • At moment T, the old versions are no longer valid
  • So we should query for what was valid just before T

Why Excluding valid_to is Wrong (Infinite Ranges)

For current objects with infinite valid_to:

  • Range: [T, ∞) (still valid now)
  • If we subtract 1μs: (T - 1μs) <@ [T, ∞) = false
  • Result: No associations found = nil
  • But the object is still valid and should have associations

The Complete Fix

def build_temporal_condition(table_name, range, time)
  col = "#{table_name}.#{range.name}"
  
  # For historical objects (finite valid_to), subtract 1μs to avoid edge case
  # For current objects (infinite valid_to), use the time as-is
  safe_time = if time.respond_to?(:infinite?) && time.infinite?
    time  # Keep infinite time as-is
  else
    # Subtract 1 microsecond to get "just before" the boundary
    adjusted_time = time - 0.000001
    "'#{adjusted_time.strftime('%Y-%m-%d %H:%M:%S.%6N')}'::timestamp"
  end
  
  Arel.sql("#{safe_time} <@ #{col}")
end

Result:

  • Historical country gets historical "City" ✅
  • Current country gets current "City 2" ✅
  • No nil results ✅

Copy link
Contributor Author

Copilot AI commented Aug 21, 2025

@copilot please ignore all the previous attempt

Let's start again with this knowledge (from your colleague Claude AI)
...

I've completely rewritten the as_of_time method following Claude AI's analysis. The new implementation:

Key changes:

  • Detects infinite timestamps using respond_to?(:infinite?) && time.infinite?
  • For current records (infinite valid_to): uses time as-is to avoid nil results
  • For historical records (finite valid_to): subtracts 1μs to query within validity period
  • Handles nil case for backward compatibility

Why this works:

  • PostgreSQL ranges are half-open [start, end)
  • Querying at exact valid_to boundary returns wrong version due to edge case
  • Subtracting 1μs ensures we get "just before" the boundary = correct historical version
  • Infinite timestamps remain infinite = current associations work correctly

Fixed in commit 4e9b44f.

Copilot AI changed the title Fix historical objects using dynamic precision detection for association queries Fix historical association queries using proper infinite vs finite timestamp detection Aug 21, 2025
Copy link
Member

@tagliala tagliala left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Copilot AI commented Aug 21, 2025

@copilot please ignore all the previous attempt

Let's start again with this knowledge (from your colleague Claude AI)
...

I've implemented Claude AI's solution. The fix now properly distinguishes between infinite and finite timestamps:

  • Current records (infinite valid_to): Returns Time.current to maintain associations
  • Historical records (finite valid_to): Subtracts 1μs to query within validity period
  • Uses respond_to?(:infinite?) && end_time.infinite? to detect PostgreSQL infinity values

All time machine tests pass (285 examples, 0 failures). Fixed in commit 5fba3dc.

@tagliala
Copy link
Member

@copilot revert the last commit 5fba3dc

Copy link
Contributor Author

Copilot AI commented Aug 21, 2025

@copilot revert the last commit 5fba3dc

Reverted commit 5fba3dc as requested. The as_of_time method is now back to the simple implementation that checks valid_to.is_a?(Time) and subtracts precision accordingly, without complex infinite/finite timestamp detection. Fixed in commit 6dbabf5.

Copilot AI changed the title Fix historical association queries using proper infinite vs finite timestamp detection Fix historical association queries using proper timestamp boundary handling Aug 21, 2025
@tagliala
Copy link
Member

@copilot add an empty commit (no file changes) with a good commit message for this change, according to contributing guidelines and you instructions, and 7 rules for a good commit, and the issue it fixes in order to automatically mark the issue as "closed" with a human-understandable of what the issue was and why you did change what you changed

Copy link
Contributor Author

Copilot AI commented Aug 21, 2025

@copilot add an empty commit (no file changes) with a good commit message for this change, according to contributing guidelines and you instructions, and 7 rules for a good commit, and the issue it fi...

Created empty commit ac6c017 with comprehensive message that follows the 7 rules for good commits and references "Fixes #283" to automatically close the issue. The commit message explains the PostgreSQL tsrange boundary issue and how the fix ensures historical association queries work correctly by subtracting 1 microsecond from finite valid_to timestamps.

@tagliala tagliala marked this pull request as ready for review September 8, 2025 14:20
@tagliala tagliala requested a review from Copilot September 8, 2025 19:43

This comment was marked as outdated.

@tagliala tagliala force-pushed the copilot/fix-283 branch 3 times, most recently from 5f36ef8 to a3beb9f Compare September 9, 2025 09:24
@tagliala tagliala requested a review from Copilot September 9, 2025 09:28

This comment was marked as outdated.

Historical objects were incorrectly using their `valid_to` timestamp to query
for associations, which could return records that didn't exist during the
object's validity period.

PostgreSQL ranges are left-inclusive and right-exclusive (`[start, end)`). When
querying associations from historical records at exactly `valid_to`, the query
could find records that became valid at that boundary time, returning the
wrong version due to half-open range semantics.

The fix subtracts 1 unit at precision 6 (1 microsecond) from finite `valid_to`
timestamps when querying associations, ensuring queries happen within the
historical record's validity period rather than at boundary times.

Fixes #283
@tagliala
Copy link
Member

tagliala commented Sep 9, 2025

Claude 4:

I'll take a look at the pull request to understand the problem and evaluate the proposed fix.Looking at this pull request, I think both the problem identification and the proposed fix are excellent. Here's my analysis:

The Problem is Real and Significant

The issue with historical association queries is a classic edge case that stems from PostgreSQL's half-open range semantics [start, end). The problem where country.history.first.cities.first.name was returning "City 2" instead of "City" demonstrates a fundamental flaw in how temporal boundaries were being handled. This could lead to serious data integrity issues in applications relying on historical accuracy.

The Solution is Elegant and Well-Thought-Out

The fix shows sophisticated understanding of the underlying issue:

  1. Precise boundary handling: Subtracting 1 microsecond from valid_to timestamps ensures queries happen "just before" the boundary, avoiding the edge case where new records become valid exactly at the boundary time.

  2. Maintains backward compatibility: The valid_to.is_a?(Time) check ensures only finite timestamps are adjusted, preserving existing behavior for other value types.

  3. Leverages existing constants: Using ChronoModel::VALIDITY_TSRANGE_PRECISION shows good architectural consistency.

Code Quality Observations

The PR demonstrates excellent practices:

Potential Considerations

The microsecond precision adjustment is clever, but it assumes that business logic granularity doesn't require microsecond-level precision for temporal queries. For most applications, this is a reasonable assumption, but it's worth documenting this limitation.

Overall, this is a high-quality fix that addresses a subtle but important bug in temporal data handling. The solution shows deep understanding of both the ChronoModel architecture and PostgreSQL's temporal semantics.

@tagliala tagliala requested a review from Copilot September 9, 2025 09:39
Copy link

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 PR fixes a boundary timestamp issue in historical association queries where records were returning incorrect associations due to PostgreSQL's half-open range semantics. The fix ensures that when querying associations from historical records, the timestamp used is strictly within the record's validity period rather than at the boundary where wrong versions might be selected.

  • Replaces simple valid_to alias with a boundary-aware as_of_time method that subtracts 1 microsecond from finite timestamps
  • Introduces a new constant VALIDITY_TSRANGE_PRECISION for consistent timestamp precision handling
  • Adds comprehensive test coverage for the historical association boundary case

Reviewed Changes

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

File Description
lib/chrono_model.rb Adds VALIDITY_TSRANGE_PRECISION constant for microsecond-level timestamp precision
lib/chrono_model/time_machine/history_model.rb Replaces as_of_time alias with boundary-aware method that handles timestamp precision
spec/chrono_model/time_machine/historical_association_spec.rb Adds new test file covering the historical association boundary case
spec/chrono_model/history_models_spec.rb Updates expected history models to include test models from the new spec

@tagliala tagliala merged commit b4b43af into master Sep 9, 2025
25 checks passed
@tagliala tagliala deleted the copilot/fix-283 branch September 9, 2025 09:45
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.

Historical objects should not use valid_to to get associations

2 participants