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

rewrite: Implement uri query operations #6120

Merged

Conversation

armadi1809
Copy link
Contributor

@francislavoie This is my Initial take at #6096. This is still to be improved as only add, delete and set are implemented, but I wanted to get your initial feedback on the overall architecture of the solution before implementing the remaining operations.

@armadi1809 armadi1809 force-pushed the implement-query-rewrite-operations branch from 3050813 to fadd6c2 Compare February 21, 2024 05:43
@francislavoie
Copy link
Member

francislavoie commented Feb 21, 2024

Yeah, good start.Couple things I notice off the bat:

  • I think it should support a block (i.e. braces) to do multiple operations with a single handler, similar to the header directive. I think that's already covered at the JSON level, just need to add the block Caddyfile parsing.

  • We need to apply the replacer on the query values (and maybe the keys too? Probably doesn't hurt). Users are usually not going to use static values, more often than not it's going to be dynamic using placeholders.

@francislavoie francislavoie marked this pull request as draft February 23, 2024 20:25
@francislavoie francislavoie added the feature ⚙️ New feature or request label Feb 23, 2024
@francislavoie francislavoie added this to the 2.x milestone Feb 23, 2024
@armadi1809 armadi1809 force-pushed the implement-query-rewrite-operations branch from 95e372a to a10e256 Compare February 26, 2024 03:05
@armadi1809
Copy link
Contributor Author

@francislavoie, out of the list of operations in the issue, which ones you think we should add on top of what we have here at the moment?

@francislavoie
Copy link
Member

I think rename would be good to have. The defaulting operation isn't necessary. Value replacement would also be good to have.

We don't have to implement those right away though, we can add them in a follow-up.

@armadi1809
Copy link
Contributor Author

armadi1809 commented Feb 26, 2024

I just added rename. We can add replace later as it looked a little bit more involved (especially if we want to use regex like for the headers). Please let me know if any modifications are still necessary in this PR.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

This is looking really good! :) Nice work. Just a couple nits from my first pass. Probably ready to move out of draft state.

modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Show resolved Hide resolved
@francislavoie francislavoie changed the title Implement URI query operations rewrite: Implement uri query operations Mar 2, 2024
@armadi1809 armadi1809 force-pushed the implement-query-rewrite-operations branch from afbcbc2 to 5dffc2b Compare March 3, 2024 03:56
@armadi1809 armadi1809 marked this pull request as ready for review March 3, 2024 03:57
@armadi1809
Copy link
Contributor Author

@mholt Thanks for the round of review! All your feedback items should now be resolved. I also removed the draft status from the PR.

modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
modules/caddyhttp/rewrite/rewrite.go Outdated Show resolved Hide resolved
@armadi1809
Copy link
Contributor Author

All the discussions above (except for the order of operations) should now be resolved.

@mholt mholt enabled auto-merge (squash) March 5, 2024 23:59
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

I think this is good to go -- let's give it a shot! Nice work @armadi1809 :)

@mholt mholt linked an issue Mar 6, 2024 that may be closed by this pull request
@francislavoie francislavoie force-pushed the implement-query-rewrite-operations branch from 51c2d47 to a87408e Compare March 6, 2024 01:05
@francislavoie francislavoie force-pushed the implement-query-rewrite-operations branch from a87408e to 84abc77 Compare March 6, 2024 15:01
@mholt mholt merged commit 69290d2 into caddyserver:master Mar 6, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query rewrite operations
3 participants