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: handle _redirects for If-None-Match headers #412

Merged
merged 6 commits into from
Jul 25, 2023

Conversation

aschmahmann
Copy link
Contributor

Fixes a bug where If-None-Match requests would return 404s when _redirects files should've been used.

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #412 (d9b38ab) into main (f6b448b) will increase coverage by 0.13%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   49.62%   49.76%   +0.13%     
==========================================
  Files         248      248              
  Lines       29912    29933      +21     
==========================================
+ Hits        14843    14895      +52     
+ Misses      13640    13610      -30     
+ Partials     1429     1428       -1     
Files Changed Coverage Δ
gateway/handler_defaults.go 37.65% <0.00%> (-2.62%) ⬇️
gateway/handler.go 78.91% <58.33%> (+0.98%) ⬆️

... and 10 files with indirect coverage changes

@aschmahmann
Copy link
Contributor Author

aschmahmann commented Jul 18, 2023

@lidel HEAD requests currently skip _redirects, I forget some of our prior conversation on if they should respect them as well.

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.

@aschmahmann according to HTTP specs the behavior for HEAD should be the same as GET,
but it has such niche utility, that we said it is ok to skip the _redirect logic for HEAD if it is too much work in MVP (#176).

iiuc next steps here:

@BigLep
Copy link
Contributor

BigLep commented Jul 20, 2023

2023-07-20 conversation: this will make it in Kubo 0.22. We're not planning to do a Kubo 0.21 patch release currently with this. This was reported by Fleek.

@aschmahmann will create an issue for this and notify #ipfs-operators in case anyone wants to block the header for the time being.

@hacdias hacdias requested a review from lidel July 25, 2023 11:29
@hacdias hacdias marked this pull request as ready for review July 25, 2023 11:33
@hacdias hacdias requested review from hacdias and a team as code owners July 25, 2023 11:33
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM, added a test. Pinging @lidel for a second look at the test.

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.

lgtm, thank you!

@lidel lidel merged commit 1f5df74 into main Jul 25, 2023
@lidel lidel deleted the fix/gateway-redirects-non-get branch July 25, 2023 13:54
Jorropo pushed a commit that referenced this pull request Aug 8, 2023
* fix: handle _redirects for If-None-Match headers
* fix: handle _redirects for If-None-Match headers
* fix(gateway): HEAD requests now respect _redirects
* feat: add _redirects regression test
* docs: add changelog entry

(cherry picked from commit 1f5df74)
@BigLep BigLep mentioned this pull request Aug 9, 2023
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.

4 participants