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

httpcaddyfile: Add shorthands for parameterized placeholders #3305

Merged

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Apr 24, 2020

Adds the following new placeholder shorthands to the Caddyfile (plus one non-* placeholder):

{port}      ->  {http.request.port}
{query.*}   ->  {http.request.uri.query.*}
{labels.*}  ->  {http.request.host.labels.*}
{header.*}  ->  {http.request.header.*}
{path.*}    ->  {http.request.uri.path.*}
{re.*.*}    ->  {http.regexp.*.*}

These are quite commonly used placeholders. Because they're relatively long, it's hard to remember the exact syntax on the fly, requiring frequent referencing of the docs to verify.

Currently I implemented this via just a quick prefix replacement, it's unlikely users would use these string patterns on their own. Probably the most concerning would be {r. because it's such a short sequence.

I chose the very short {r.*.*} as the syntax for the regexp placeholder, because when users provide a name argument, it's pretty obvious what r is referencing, since the name will be associated with a path_regexp or header_regexp matcher. It's easy to trace in the Caddyfile.

Edit: Switched to {re.*.*} for some added clarity.

As an improvement to this, in second iteration I think we could implement this via regexp replacements instead, it would probably be safer as well because we'd verify the final } is actually present. Just wanted to get out a quick proof of concept first.

Edit: Done, implemented using regexp. Pretty easy.

@francislavoie francislavoie added this to the 2.1 milestone Apr 24, 2020
@francislavoie francislavoie requested a review from mholt April 24, 2020 06:05
@mholt mholt added the under review 🧐 Review is pending before merging label Apr 25, 2020
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 was simpler than I thought it'd be!

For the record, it is also possible to use the existing list above for pretty much all these replacements, i.e.: {"query.", "{http.request.uri.query." without using regexp, but as we discussed on Slack, performance doesn't really matter here, and with the regex we get the benefit of ensuring there's a closing curly brace } rather than assuming. Probably not a big deal; if we wanted to simplify the code, that's one easy way to do it. (ref. this commit: 09075d8)

I would suggest that the regexp be compiled only once though, rather than in a loop.

@francislavoie francislavoie force-pushed the caddyfile-placeholder-shorthands branch from 16f4562 to c44babd Compare April 25, 2020 17:52
mholt
mholt previously approved these changes Apr 25, 2020
@mholt mholt removed the under review 🧐 Review is pending before merging label Apr 25, 2020
@mholt mholt added the under review 🧐 Review is pending before merging label Apr 27, 2020
@mholt
Copy link
Member

mholt commented May 5, 2020

@francislavoie When you have a chance could you help with the merge conflict? I'll do another review then.

@francislavoie francislavoie force-pushed the caddyfile-placeholder-shorthands branch from fdf9f9c to 2aba6da Compare May 5, 2020 19:11
@francislavoie
Copy link
Member Author

Rebased! Btw, easier to review with whitespace changes hidden.

@francislavoie francislavoie force-pushed the caddyfile-placeholder-shorthands branch 2 times, most recently from c3055d8 to e236cf6 Compare May 11, 2020 21:07
@mholt
Copy link
Member

mholt commented May 11, 2020

Thanks, looks good!

My only remaining question is about the use of r. I like short, but I almost never see r used to mean regular expression. I usually see regex or regexp or at least re.

What do you think of re?

@francislavoie
Copy link
Member Author

re is okay 👍 does make me think of like Re: (regarding) in emails, but I don't find it particularly unclear.

@francislavoie francislavoie force-pushed the caddyfile-placeholder-shorthands branch from e236cf6 to ffc3b89 Compare May 11, 2020 21:15
httpcaddyfile: Now with regexp instead


httpcaddyfile: Allow dashes, gofmt


httpcaddyfile: Compile regexp only once


httpcaddyfile: Cleanup struct


httpcaddyfile: Optimize the replacers, pull out of the loop


httpcaddyfile: Add `{port}` shorthand
@francislavoie francislavoie force-pushed the caddyfile-placeholder-shorthands branch from ffc3b89 to 1de9abf Compare May 11, 2020 22:45
@francislavoie francislavoie requested a review from mholt May 11, 2020 22:46
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.

Great, LGTM! Thanks Francis.

@mholt mholt removed the under review 🧐 Review is pending before merging label May 11, 2020
@mholt mholt merged commit ea7e4b4 into caddyserver:master May 11, 2020
@francislavoie francislavoie deleted the caddyfile-placeholder-shorthands branch May 11, 2020 23:08
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.

2 participants