feat!: Matches required query parameters #21
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jphastings
added a commit
to jphastings/boxo
that referenced
this pull request
Jul 9, 2023
Draft commit for using ipfs/go-ipfs-redirects-file#21 to allow query parameters in redirects. Cannot be completed/will not be ready until that PR is merged, and a new version released (so go.mod/go.sum can be updated here)
jphastings
added a commit
to jphastings/boxo
that referenced
this pull request
Jul 9, 2023
Draft commit for using ipfs/go-ipfs-redirects-file#21 to allow query parameters in redirects. Cannot be completed/will not be ready until that PR is merged, and a new version released (so go.mod/go.sum can be updated here)
jphastings
force-pushed
the
query-params
branch
2 times, most recently
from
efc53cb to
17de3f8
Compare
jphastings
commented
Jul 9, 2023
| "/fixed-val val=val /to 200\n/dynamic-val val=:val /to/:val 301\n/empty-val val= /to 404\n/any-val val /to 302\n", | ||
| "/multi-query val1=val1 val2=:val2 val3= val4 /to/:val2\n/multi-query2 val1=val1 val2=:val2 val3= val4 /to/:val2 302\n", | ||
| "/bad-syntax1 val=a&val=b /to\n", "/bad-syntax2 val=a&val2=b /to 302\n", "/a ^¬params /b\n", "/bad-status type=:type /to 3oo\n", "/bad-chars :type=whatever /to\n", "/bad-chars type=what:ever /to\n", | ||
| } |
There was a problem hiding this comment.
I wasn't 100% clear on the fuzz testing strategy here, I'd appreciate a closer look at whether I've chosen suitable testcases here.
Code now parses and `MatchAndExpandPlaceholders` for query parameters as well as paths. Placeholders are shared between the two. This commit codifies some of the previously implicit edgecases, particularly around duplicate placeholders. Closes ipfs#20
|
(turning to a draft until we have an IPIP mentioned in #20) |
|
Closing as the IPIP did not happen + we went with simpler support – details in #20 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Code now
Parses andMatchAndExpandPlaceholdersfor query parameters as well as paths. I ended up implementing #20 without waiting for a discussion on the issue, for which I apologise! I'll be happy to rewrite without a review if there are concerns with the proposal.Placeholders are shared between the two.
This is a breaking change for the package's API (though not for the
_redirectsfile format), as the query params need to be passed intoMatchAndExpandPlaceholders. I'll be un-drafting ipfs/boxo#406 (which adapts syntax to include this work), once (if!) this PR is merged, sogo.mod/go.sumcan be corrected.This commit also codifies in tests some of the previously implicit edge cases, particularly around duplicate placeholders.
Closes #20
If you build a copy of
ipfswith this in (via ipfs/boxo#406) then you can visit this CID locally and see this in action.