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

GlobalID may have an issue with historical objects fetched via as_of #192

Open
tagliala opened this issue Feb 10, 2023 · 6 comments
Open
Labels
bug CRITICAL Critical issues

Comments

@tagliala
Copy link
Member

tagliala commented Feb 10, 2023

It is not clear to me what is the correct behavior, but it is worth to mention the following:

Vendor.last.history.last.model_name.name
"Vendor::History"

Vendor.history.last.to_gid
#<GlobalID:0x00007fd3ba2ed9f8 @uri=#<URI::GID gid://app/Vendor::History/123>>

Vendor.as_of(1.day.ago).last.model_name.name
"Vendor"

Vendor.as_of(1.day.ago).last.to_gid
#<GlobalID:0x00007fd3bac4ba18 @uri=#<URI::GID gid://app/Vendor/1>>
@tagliala tagliala added bug CRITICAL Critical issues labels Feb 10, 2023
@lleirborras
Copy link
Member

the 2nd one does not point to historical record

@tagliala
Copy link
Member Author

the 2nd one does not point to historical record

Yes, let me try to clarify

Vendor.last.history.last.model_name.name
"Vendor::History"

Vendor.history.last.to_gid
#<GlobalID:0x00007fd3ba2ed9f8 @uri=#<URI::GID gid://app/Vendor::History/123>>

Vendor.as_of(1.day.ago).last.model_name.name
"Vendor"

Vendor.as_of(1.day.ago).last.to_gid
#<GlobalID:0x00007fd3bac4ba18 @uri=#<URI::GID gid://app/Vendor/1>>

The second one is indeed a class of "Vendor" because it is how chronomodel works.

However, we can't support GlobalID properly because fetching gid://app/Vendor::History/123 will return a "Vendor::History", but Vendor.as_of instantiates a Vendor

@lleirborras
Copy link
Member

I understood that, what I mean is that Vendor.as_of(1.day.ago).last.model_name.name should return a History record but I assume in this case that record is "current" and so it returns Vendor

@tagliala
Copy link
Member Author

I understood that, what I mean is that Vendor.as_of(1.day.ago).last.model_name.name should return a History record but I assume in this case that record is "current" and so it returns Vendor

Correct. And it is correct to return a Vendor, so GlobalID should need a custom behavior for this kind of records 😞

I do not know GlobalID internals, so I will take note of this and implement a workaround for our specific use case

@lleirborras
Copy link
Member

lleirborras commented Feb 10, 2023

I do not think it's correct, there should be a convention that when you ask a historical query always get a historical record, even if it's the most recent one, while non-historical queries return normal models

@tagliala
Copy link
Member Author

tagliala commented Feb 10, 2023

I do not think it's correct,

I think that Chronomodel itself is not correct, but in the specific case, unfortunately this behavior is correct.

If I'm getting a Vendor.as_of(time), I would like to use that vendor like it is a vendor itself, not its historical reference

Example: if the Vendor is returned as its historical reference with an instance of Vendor::History, a lot of things will fail in a Rails application using chronomodel

  • Rails views (= render @vendor): Missing partial vendor/histories/_history
  • Policies: Vendor::HistoryPolicy for #<Vendor::History

because everything is expecting an instance of Vendor class

However, here it is a prototype workaround:

GlobalID::Locator.use :chronolocator do |gid|
  collection = gid.model_class
  collection = collection.as_of(Time.parse.utc(gid.params[:as_of])) if gid.params[:as_of] # safe parse here
  collection.find(gid.model_id)
end

Then:

vendor.to_gid(app: 'chronolocator', as_of: vendor.as_of_time&.iso8601) # chronolocator should reflect app name

I don't want to provide a custom locator for chronomodel and override to_gid, but at least we have a working approach for some specific use cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CRITICAL Critical issues
Projects
None yet
Development

No branches or pull requests

2 participants