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

[bugfix] return ErrNotFound correctly #952

Merged
merged 3 commits into from
Nov 24, 2020
Merged

[bugfix] return ErrNotFound correctly #952

merged 3 commits into from
Nov 24, 2020

Conversation

arielshaqed
Copy link
Contributor

Package scany depends on a different version of pgx than the rest of lakeFS. So
errors.Is(err, pgx.ErrNoRows) fails. Luckily it (sort-of) knows of this issue and
wraps this call inside it as pgxscan.NotFound.

Also make ErrNotFound wrap pgx.ErrNoRows rather than a new error.

@arielshaqed arielshaqed requested a review from nopcoder November 24, 2020 09:39
@arielshaqed arielshaqed added pr/merge-if-approved Reviewer: please feel free to merge if no major comments pr/high-priority Pull requests that should be reviewed first labels Nov 24, 2020
@arielshaqed
Copy link
Contributor Author

I think this bug can be seen in various places, where the pgx error may become visible instead of a nice readable error.

@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #952 (dfeba0f) into master (5cea104) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #952      +/-   ##
==========================================
- Coverage   44.45%   44.43%   -0.02%     
==========================================
  Files         142      142              
  Lines       11441    11441              
==========================================
- Hits         5086     5084       -2     
- Misses       5725     5726       +1     
- Partials      630      631       +1     
Impacted Files Coverage Δ
db/tx.go 0.00% <0.00%> (ø)
catalog/mvcc/cataloger_create_repository.go 59.25% <0.00%> (-3.71%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cea104...dfeba0f. Read the comment docs.

Package `scany` depends on a different version of `pgx` than the rest of lakeFS.  So
`errors.Is(err, pgx.ErrNoRows)` fails.  Luckily it (sort-of) knows of this issue and
wraps this call inside it as `pgxscan.NotFound`.

Also make `ErrNotFound` wrap `pgx.ErrNoRows` rather than a new error.
@arielshaqed
Copy link
Contributor Author

Not quite right :-( @nopcoder please hold off from reviewing for now.

@arielshaqed arielshaqed marked this pull request as draft November 24, 2020 10:13
@arielshaqed arielshaqed marked this pull request as ready for review November 24, 2020 10:28
@nopcoder nopcoder merged commit 634e85a into master Nov 24, 2020
@nopcoder nopcoder deleted the bugfix/not-found branch November 24, 2020 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/high-priority Pull requests that should be reviewed first pr/merge-if-approved Reviewer: please feel free to merge if no major comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants