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

[Access] Do not return not found for tx result when collection not indexed #4454

Merged
merged 2 commits into from
Jun 9, 2023

Conversation

peterargue
Copy link
Contributor

@peterargue peterargue commented Jun 8, 2023

A change was added recently to support searching for tx results by block and collection. This had a bug where if transaction was included in a block, but the block had not yet been fully indexed, GetTransactionResult would return a NotFound error with the message not found: could not retrieve collection: key not found. Normally, the api would return a result, but the status would be pending.

This PR update the endpoint to only return NotFound in this situation if the request include the blockID or collectionID as search criteria.

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2023

Codecov Report

Merging #4454 (e8702a0) into master (9d79667) will decrease coverage by 0.02%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master    #4454      +/-   ##
==========================================
- Coverage   54.16%   54.15%   -0.02%     
==========================================
  Files         888      888              
  Lines       83743    83754      +11     
==========================================
- Hits        45358    45354       -4     
- Misses      34865    34884      +19     
+ Partials     3520     3516       -4     
Flag Coverage Δ
unittests 54.15% <61.11%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
engine/access/rpc/backend/backend_transactions.go 46.15% <61.11%> (-2.85%) ⬇️

... and 11 files with indirect coverage changes

@@ -273,16 +277,19 @@ func (b *backendTransactions) GetTransactionResult(

// access node may not have the block if it hasn't yet been finalized, hence block can be nil at this point
if block != nil {
blockID = block.ID()
Copy link
Member

Choose a reason for hiding this comment

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

we can keep this, so that only compute blockID once

// if the request included a blockID or collectionID in its the search criteria, not found
// should result in an error because it's not possible to guarantee that the result found
// is the correct one.
if err != nil && (blockID != flow.ZeroID || collectionID != flow.ZeroID) {
Copy link
Member

Choose a reason for hiding this comment

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

can we simply this condition? or break it into multiple if checks? it's too complex.

@koko1123 koko1123 merged commit d20d87b into master Jun 9, 2023
@koko1123 koko1123 deleted the petera/fix-access-tx-results branch June 9, 2023 01:21
koko1123 pushed a commit that referenced this pull request Jun 9, 2023
…dexed (#4454)

* [Access] Do not return not found for tx result when collection not indexed

* address review feedback
@koko1123 koko1123 mentioned this pull request Jun 9, 2023
koko1123 added a commit that referenced this pull request Jun 9, 2023
…dexed (#4454) (#4458)

* [Access] Do not return not found for tx result when collection not indexed

* address review feedback

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
This pull request was closed.
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.

4 participants