Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow navigation on history records without an at_value #33

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

evman182
Copy link
Collaborator

@evman182 evman182 commented Jan 9, 2024

No description provided.

@evman182 evman182 requested a review from bkroeker January 9, 2024 15:04
@evman182
Copy link
Collaborator Author

evman182 commented Jan 9, 2024

@Zyndoras thoughts on this solution?

@Zyndoras
Copy link

Zyndoras commented Jan 9, 2024

That would work, I guess.

Out of curiosity, what would then be the "encouraged" way of looking at records / associations at their end-of-life?

Wart.history.where(id: my_id).at(some_parent.eff_to - TemporalTables::ONE_MICROSECOND).first

=> Well defined timestamp, but rather clunky to use

Wart.history.where(id: my_id).last

=> As you already mentioned in the other PR, this does not really define the timestamp we are looking at (especially when navigating the associations), but is much easier to use.


Regarding your rearrangement of my test case: Most of the sanity checks / assertions don't make any sense if you move them all the way to the end of the spec...

@evman182
Copy link
Collaborator Author

evman182 commented Jan 9, 2024

@Zyndoras I'm just not sure there's a good answer to your question. If the query wasn't scoped to a certain point in time, and you're just querying the last history record, that record is not associated with a specific point in time. The history records describe the value of the record during a time range. That makes saying what history record an association should map to sorta impossible, since a parent record's association (child) could have been changed multiple times (i.e. have multiple child history records) during the lifespan of that parent history record.

Honestly, if I was writing the library from scratch, I prob would have had associations throw an exception if the parent history record wasn't fetched with an "at value". You can see me and the original author go back and forth about that in the comments on my earlier PR. For sure, the changes were breaking though, and that's why it was a major version bump.

All that being said, as an improvement, if you think it would help, we could look into adding some sort of functionality that returns an array of the association history records that were valid during the same time as the parent. So something like this

parent_h = Parent.history.where(id: 1).last

# would have to work on how naming/pluralization would work, and also learn about missing methods, lol
children_h = parent_h.effective_childs 

last_child = children_h.last

@Zyndoras
Copy link

Zyndoras commented Jan 10, 2024

@evman182 I'd say let's go ahead and merge this.

It restores the previous default behavior and resolves my issues with 2.0 (which mostly came from using raw SQL anyway, but I might just go ahead and manually set some at_values just to not rely too much on the fallback).

Maybe consider restoring the spec's order in case it ever breaks.

Cheers

@evman182 evman182 force-pushed the allowNonScopedNavigation branch from 31c976b to df944f5 Compare January 10, 2024 16:28
@evman182
Copy link
Collaborator Author

@bkroeker if you have a change to review this. Otherwise I'll merge it in a couple days, and tag 3.0

@@ -10,8 +10,8 @@ def target_scope
# Using responds_to? results in an infinite loop stack overflow.
if @owner.public_methods.include?(:at_value)
# If this is a history record but no at time was given,
# assume the record's effective to date
super.at(@owner.at_value || @owner.eff_to)
# assume the record's effective to date minus 1 microsecond
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe a brief explanation here why the microsecond is necessary?

@evman182 evman182 merged commit 9a38459 into bkroeker:master Jan 11, 2024
19 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