Skip to content

map: Replace placeholders with regex groups#3991

Merged
mholt merged 8 commits intocaddyserver:masterfrom
rajat315315:fix-3971
Mar 10, 2021
Merged

map: Replace placeholders with regex groups#3991
mholt merged 8 commits intocaddyserver:masterfrom
rajat315315:fix-3971

Conversation

@rajat315315
Copy link
Contributor

Fixes #3971

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2021

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie added the needs tests 💯 Requires automated tests label Jan 26, 2021
@francislavoie
Copy link
Member

I don't think this is quite right. The linked issue is asking to essentially do the same as the path_regexp matcher, where it sets regexp results in the replacer to be used later. Basically, this code needs to be ported to the map handler when a regexp matches:

https://github.com/caddyserver/caddy/blob/deedf8a/modules/caddyhttp/matchers.go#L932-L944

@rajat315315
Copy link
Contributor Author

rajat315315 commented Jan 28, 2021

@francislavoie it good? Or better?

@francislavoie
Copy link
Member

Hmm. I'm not sure that's still quite right. The typical regexp placeholder involves a name, because you could have more than one regexp in your handler chain (i.e. a path_regexp or header_regexp matchers, etc). Your implementation will clobber those since it's placing the results at the same placeholder depth as where the names would normally be.

I think we need to figure out how to tell the map handler the regexp name to be used. I'm not sure what's right there. @mholt do you have any ideas?

@mholt
Copy link
Member

mholt commented Mar 2, 2021

@francislavoie I'm just getting to this now, but I'm confused. What was wrong with the initial commit? The current commit looks way complicated. ReplaceAllString() is what allows you to use capture groups like \1 in replacements. Placeholders are a separate concept. Why does this have anything to do with matchers?

@mholt mholt added under review 🧐 Review is pending before merging and removed needs tests 💯 Requires automated tests labels Mar 2, 2021
@mholt
Copy link
Member

mholt commented Mar 2, 2021

Hmm are you sure? Linked issue says:

I want to refer to the captured parts of the regex in destination

@francislavoie
Copy link
Member

Oh, I see, my bad. I misunderstood how the first implementation was meant to be used because there was no explanation or associated test. I figured we would use the same regexp placeholders like {re.*.*} here but it makes sense to use \* for using the capture groups inside the map case.

@mholt
Copy link
Member

mholt commented Mar 2, 2021

I'm pretty sure we can restore the first commit @rajat315315 -- but keep the later commits.

@rajat315315
Copy link
Contributor Author

rajat315315 commented Mar 3, 2021

Sure
@mholt Should I revert later ones? and keep first one?

@mholt
Copy link
Member

mholt commented Mar 3, 2021

I believe we want to keep this one: acb5ff3

And any related test cases. :)

@rajat315315
Copy link
Contributor Author

-- but keep the later commits.

I'm not sure about this statement..

@francislavoie
Copy link
Member

By "later commits", Matt means the changes to the tests. Basically just revert the changes after the first commit, then add tests.

@francislavoie francislavoie changed the title Replace placeholders with regex groups map: Replace placeholders with regex groups Mar 5, 2021
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.

Cool, let's try this out. Hopefully nobody wants literal ${1} strings in their outputs. :)

@mholt mholt merged commit 802f80c into caddyserver:master Mar 10, 2021
mholt pushed a commit to caddyserver/website that referenced this pull request Apr 5, 2021
caddyserver/caddy#3991

The `outputs` bit was getting a bit long so I split it up into multiple paragraphs.

The capture group explanation is copied from the `path_regexp` matcher docs.

Added bullets in front of the example cases, because the newlines get collapsed in markdown otherwise, unless we either add bullets or double newlines. I think bullets work better here.
@rajat315315
Copy link
Contributor Author

Hello everyone,
I request you to please nominate me, if you think I have done some meaningful contribution to the community...
https://stars.github.com/nominate/

@francislavoie francislavoie added this to the v2.4.0 milestone May 1, 2021
@francislavoie francislavoie removed the under review 🧐 Review is pending before merging label Jul 7, 2021
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.

Feature request: map handler takes account of regex capture group

4 participants