Jump to conversation
Unresolved conversations (10)
@lidel lidel Aug 10, 2022
nit: if you switch `path` from `string` to `contentPath ipath.Path`, then you will be able to read namespace via `contentPath.Namespace()` and simplify this
Outdated
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
What is the purpose of this check?
Outdated
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
nit: same as above, pass `contentPath` as an arg to `getRedirectsFile`, don't read `r.URL.Path` directly
Outdated
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
```suggestion return nil, fmt.Errorf("could not parse _redirects: %v", err) ```
Outdated
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
We already do a lot of redirects unrelated to `_redirects`, let's rename to make it clear what happens here: ```suggestion func (i *gatewayHandler) handleRedirectsFileRules(w http.ResponseWriter, r *http.Request, redirectRules []redirects.Rule) (bool, string, error) { ```
Outdated
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
nit: avoid using low-level `r.URL.Path`, we read it once in `gateway_handler.go`: ``` contentPath := ipath.New(r.URL.Path) ``` Instead, pass already validated `contentPath ipath.Path` as an argument all the way to `handleRedirect`. This way, we are nor surprised when things are moved around during the future refactor.
Outdated
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
nit: add second match that confirms the body includes `my index` (string present in index.html)
Outdated
.../sharness/t0109-gateway-web-_redirects.sh
@lidel lidel Aug 10, 2022
nit: add second match that confirms the body includes `my 404` (string present in expected pretty 404)
Outdated
.../sharness/t0109-gateway-web-_redirects.sh
@lidel lidel Aug 10, 2022
nit: add second match that confirms the body includes `my 404` (string present in expected pretty 404)
Outdated
.../sharness/t0109-gateway-web-_redirects.sh
@lidel lidel Aug 10, 2022
nit: confirm `302` redirects return valid destination in `Location` header (add one more `test_should_contain`)
Outdated
.../sharness/t0109-gateway-web-_redirects.sh
Resolved conversations (15)
@lidel lidel Aug 10, 2022
I think it is fine keeping it here – will measure time to first logical block, no matter if any additional logic like `_redirects` was applied or not.
Outdated
core/corehttp/gateway_handler.go
@lidel lidel Aug 10, 2022
nit: We try to avoid adding new dependencies if we can avoid it. This is mostly for security reasons. This one is small enough that we could remove this dependency and add necessary `urlpath.Match` logic to a subpackage in https://github.com/ipfs-shipyard/go-ipfs-redirects, so no third-party libraries are needed.
Outdated
go.mod
@justincjohnson
@lidel lidel Aug 10, 2022
nit: switch to https://github.com/ipfs-shipyard/go-ipfs-redirects
Outdated
go.mod
@lidel lidel Aug 10, 2022
nit: unsure about duplicating all this – the only difference between NonUnixfs and Unixfs versions is special handling in "default" blocks in the Unixfs one. Could we have a single `handlePathResolution` that takes additional arg `responseFormat` and do the conditional `isUnixfsResponseFormat(responseFormat)` in the single, shared `default` block instead?
Outdated
core/corehttp/gateway_handler.go
@justincjohnson
@lidel lidel Aug 10, 2022
We need to be extra careful here and protect `_redirects` functionality against regressions caused by future refactors (we are [planning to extract gateway code into standalone lib](https://github.com/ipfs/kubo/issues/8524#issuecomment-1195788188), so we know it is imminent). To guard against shipping unsupported behaviors, this PR needs tests for negative behaviors: - [ ] expect HTTP 500 when `_redirects` is triggered but contains a status code that is not supported (see my note on the https://github.com/ipfs/specs/pull/290 that we should be explicit about which codes are supported) - [ ] expect HTTP 500 when `_redirects` is invalid (just put `hello` in a text file and write test that confirms it produces human-readable error) - [ ] expect HTTP 500 when `_redirects` is invalid (includes `!` "forced" directive → should produce a human-readable error informing that forced redirects (aka shadowing) are not supported)
.../sharness/t0109-gateway-web-_redirects.sh
@justincjohnson
@lidel lidel Aug 10, 2022
nit: include `Location: ` header name in the match
.../sharness/t0109-gateway-web-_redirects.sh
@lidel lidel Aug 10, 2022
nit: include `Location: ` header name in the match
.../sharness/t0109-gateway-web-_redirects.sh
@lidel lidel Aug 10, 2022
nit: confirm `301` redirects return valid destination in `Location` header (add one more `test_should_contain`)
.../sharness/t0109-gateway-web-_redirects.sh
@lidel lidel Aug 10, 2022
nit: I think you need to re-do the CAR to fix `:splat` line in `_redirects` fixture – details in my comment at https://github.com/ipfs/specs/pull/290#discussion_r942868437
Outdated
.../sharness/t0109-gateway-web-_redirects.sh
@lidel lidel Aug 10, 2022
nit: add similar checks for `410` and `451`, (to return correct error code, call `webError` func with `http.StatusGone` and `http.StatusUnavailableForLegalReasons`)
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
This loop should error when unsupported code is used in `_redirects` file.
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Aug 10, 2022
nit: check if `rule.Status >= 301` and `rule.Status <= 308` and return redirect only if in this range of codes
...http/gateway_handler_unixfs__redirects.go
@lidel lidel Jul 7, 2022
@justincjohnson mind making me an Admin (Owner?) in that repo? Seems only that level can move it between orgs. (sadly under the water and I'll be reviewing things after Iceland, but we could move the repo before, so we have one thing less on our plate)
Outdated
...http/gateway_handler_unixfs__redirects.go
@justincjohnson @lidel
@justincjohnson justincjohnson Jun 16, 2022
Addressing new linting errors in CI
core/coreapi/name.go
@lidel lidel Jun 14, 2022
Mind creating a CAR that has the test dir, and using it instead of constructing it on the fly? We have some prior art in `test/sharness/t0118-gateway-car/carv1-basic.car` That way we could use its root CID in the ipfs/specs RFC under "Test Fixtures" section, ensuring other implementation of IPFS test against things we want.
Outdated
.../sharness/t0109-gateway-web-_redirects.sh
@justincjohnson