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

api: Convert timezones before txn query #1543

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

Eric-Warehime
Copy link
Contributor

Summary

Fixes #458

Test Plan

Added unit tests.

@Eric-Warehime Eric-Warehime requested a review from a team June 13, 2023 20:03
@shiqizng
Copy link
Contributor

also add a test in handler to validate this works.
https://github.com/algorand/indexer/blob/develop/api/handlers_test.go#L80-L82

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #1543 (b3d50f8) into develop (5c1a24c) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1543      +/-   ##
===========================================
+ Coverage    68.20%   68.34%   +0.14%     
===========================================
  Files           38       38              
  Lines         7438     7440       +2     
===========================================
+ Hits          5073     5085      +12     
+ Misses        1934     1926       -8     
+ Partials       431      429       -2     
Impacted Files Coverage Δ
idb/postgres/postgres.go 67.28% <100.00%> (+0.48%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tzaffi
Copy link
Contributor

tzaffi commented Jun 13, 2023

also add a test in handler to validate this works. https://github.com/algorand/indexer/blob/develop/api/handlers_test.go#L80-L82

I echo this sentiment. We can see that previously the coverage was lacking as all the Before/AfterTimes were UTC. So we ought to be able to add a test case that failed previously and is now fixed thanks to this PR.

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

LGTM. Optionally, it would be great to change or add one of the cases in handlers_test.go to be non-UTC

@Eric-Warehime
Copy link
Contributor Author

@shiqizng @tzaffi
With this implementation we can't add a handlers test because I haven't changed the idb.TransactionFilter. It seems like at the handlers level we would assume that the idb implementation would properly handle any time.Time objects regardless of their timezone. So instead of converting all idb filter objects to have UTC timezone, I've changed it in the postgres implementation itself.

So the handlers output would be unchanged, but when you get the resulting sql query for postgres it would go from idb.TransactionFilter{BeforeTime: Non-UTC-TIME} to something like SELECT...ON...WHERE h.realtime > UTC-Converted-Time

@tzaffi
Copy link
Contributor

tzaffi commented Jun 13, 2023

@shiqizng @tzaffi With this implementation we can't add a handlers test because I haven't changed the idb.TransactionFilter. It seems like at the handlers level we would assume that the idb implementation would properly handle any time.Time objects regardless of their timezone. So instead of converting all idb filter objects to have UTC timezone, I've changed it in the postgres implementation itself.

So the handlers output would be unchanged, but when you get the resulting sql query for postgres it would go from idb.TransactionFilter{BeforeTime: Non-UTC-TIME} to something like SELECT...ON...WHERE h.realtime > UTC-Converted-Time

I wonder if the fixtures test is a good place to add this. I'm not proposing to add it to this PR because the transactions/ endpoint isn't yet implemented for the fixtures (though it's pretty easy to add it). I can create a small issue to add such a test case if others agree.

@shiqizng
Copy link
Contributor

the existing tests are missing cases for idb.TransactionFilter{BeforeTime: Non-UTC-TIME} though. The handler output should change, right? we want prove query provided in 458 now returns the correct results?


round := uint64(1)
///////////
// Given // A block at with 8 transactions at ts 1671036853.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo.

@Eric-Warehime Eric-Warehime merged commit 9e03fca into algorand:develop Jun 14, 2023
@Eric-Warehime Eric-Warehime deleted the convert-query-timezones branch June 14, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants