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

Add a basic Url rewrite modifier. #82

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

favalos
Copy link

@favalos favalos commented Sep 26, 2024

Wrote a very simple and basic Url Rewrite functionality.

With the test configuration I tested doing:

http://localhost:8080/rewrite/blog/

Hope this could be the beginning of a contribution to this project, any feedback or improvements please let me know.

Thanks!

@jamesmunns
Copy link
Collaborator

Hey there! I think this looks reasonable, from a quick first glance:

One perf note is that it might be faster to do the replace, then figure out if we replaced anything, instead of looking for the match and then replacing. The docs say:

If no match is found, then the haystack is returned unchanged. In that case, this implementation will likely return a Cow::Borrowed value such that no allocation is performed.

When a Cow::Borrowed is returned, the value returned is guaranteed to be equivalent to the haystack given.

That being said: I'm just guessing (and that's not a good basis for requesting a perf change!) that it'd be faster to do a replace and then check if the result is Borrowed or Owned, instead of doing a match and then conditionally a replace, so maybe just a comment that we could consider benchmarking this in the future would be fine.

Basically suggesting:

// instead of this:
if is_match() {
    replace()
}

// this
let res = replace()
if !res.is_borrowed() {
    // set the new URI here
}

Copy link
Collaborator

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Made some nits, the one blocker is that I'd like to see the docs updated, then we can merge this!

@Szpadel
Copy link

Szpadel commented Oct 12, 2024

My 2 cents about performance
Instead of

filter kind="url-rewrite" regex="^/rewrite/(.*)$" rewrite="/${1}"

Doing

filter kind="url-rewrite" regex="^/rewrite/" rewrite="/"

So removing capture group would switch replacen , with replace is wrapping, to fast path with significally better performance.
(I think that this could be documented)

if !res.is_borrowed() {
    // set the new URI here
}

I feel here more readable would be res.is_owned() instead. Or match if we want to avoid nightly only feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants