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

fix(gateway): return HTTP 500 on namesys.ErrResolveFailed #150

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Feb 1, 2023

Fixes ipfs/kubo#9514.

There's a catch here. AFAIK, there is no good way of distinguishing between a non-existing DNSLink entry and an IPNS key that cannot be resolved. With this PR, both return 500 Internal Server Error, as both return namesys.ErrResolveFailed as error. What do you think @lidel? If we want to treat both errors the same way, I think it's good to go. Otherwise, we may need further changes in the namesys package.

Sharness tests will fail here. See ipfs/kubo#9589.

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #150 (77fb194) into main (c394290) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   29.19%   29.41%   +0.21%     
==========================================
  Files          96       99       +3     
  Lines       11018    11093      +75     
==========================================
+ Hits         3217     3263      +46     
- Misses       7448     7472      +24     
- Partials      353      358       +5     
Impacted Files Coverage Δ
gateway/handler.go 62.75% <100.00%> (+0.71%) ⬆️
gateway/handler_unixfs__redirects.go 38.79% <0.00%> (-0.55%) ⬇️
files/is_hidden_windows.go 82.35% <0.00%> (ø)
tar/sanitize_windows.go 41.66% <0.00%> (ø)
files/filewriter_windows.go 100.00% <0.00%> (ø)

@hacdias hacdias self-assigned this Feb 1, 2023
@hacdias hacdias marked this pull request as ready for review February 1, 2023 09:40
@hacdias hacdias requested a review from lidel as a code owner February 1, 2023 09:40
@hacdias
Copy link
Member Author

hacdias commented Feb 8, 2023

@lidel this would be easier to rebase once #156 is merged.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

#156 Is merged, feel free to rebase and move tests from ipfs/kubo#9589 to this PR

@lidel lidel changed the title fix(gateway): return http.StatusInternalServerError by default if cannot resolve fix(gateway): return http.StatusInternalServerError on namesys.ErrResolveFailed Feb 15, 2023
@lidel lidel changed the title fix(gateway): return http.StatusInternalServerError on namesys.ErrResolveFailed fix(gateway): return HTTP 500 on namesys.ErrResolveFailed Feb 15, 2023
@hacdias hacdias force-pushed the fix/kubo-9514 branch 2 times, most recently from 3c9b71d to 87e8bc6 Compare February 15, 2023 07:28
@hacdias
Copy link
Member Author

hacdias commented Feb 15, 2023

@lidel sharness will also fail here, as there is a single sharness test that checks the status code for this cases. See ipfs/kubo#9589 for Kubo PR.

@lidel lidel merged commit 7b20141 into main Feb 22, 2023
@lidel lidel deleted the fix/kubo-9514 branch February 22, 2023 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

gateway: IPNS key resolution timeout should return HTTP 5xx
2 participants