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

caddyhttp: Support single-line not matcher shortcut #3228

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Apr 5, 2020

Simple addition to make it possible to have not negate a matcher on the same line instead of requiring it to be contained in a block.

E.g. before:

@notStatic {
    not {
        path /static*
    }
}

after:

@notStatic {
    not path /static*
}

easy

@francislavoie francislavoie requested a review from mholt April 5, 2020 17:21
@sarge
Copy link
Collaborator

sarge commented Apr 5, 2020

Nice, I wonder what a good testing approach is for this?

Perhaps a usecase in here

func TestServerType(t *testing.T) {
for i, tc := range []struct {
input string
expectWarn bool
expectError bool
}{
{
input: `http://localhost
@debug {
query showdebug=1
}
`,
expectWarn: false,
expectError: false,
},
{
input: `http://localhost
@debug {
query bad format
}
`,
expectWarn: false,
expectError: true,
},
} {

@francislavoie
Copy link
Member Author

I don't think TestServerType is the right place for testing matchers. We don't have any tests for Caddyfile matchers currently as far as I can tell, so I didn't bother making any - my experience in Go is extremely limited. I can read the code, but barely write anything 😛

@sarge
Copy link
Collaborator

sarge commented Apr 5, 2020

Understood, I think that test name is bit misleading because it is actually testing the parsing of a matcher.
[edit]
It isn't a great test (I wrote it) as it doesn't test the actual outcome, but it does validate if is valid syntax.

ps: I think you are underating yourself :)

@francislavoie
Copy link
Member Author

I'll rename TestServerType to TestMatcherSyntax and add some tests 👍

@francislavoie
Copy link
Member Author

Added a couple test cases, but I don't think there's a huge amount of value here because it didn't return a warning or error before with not path /somepath*, it just generated bad JSON.

"match": [
        {
                "not": [
                        {},
                        {},
                        {}
                ]
        }
],

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.

Looks good, thanks @francislavoie ! Congrats on your first Go-centric PR. 😃

@@ -559,7 +559,7 @@ func (m *MatchNot) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
for d.Next() {
var mp matcherPair
matcherMap := make(map[string]RequestMatcher)
for d.NextBlock(0) {
for d.NextArg() || d.NextBlock(0) {
Copy link
Member

Choose a reason for hiding this comment

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

This is clever, but I do admit I think it's kinda weird. We can go with it for now and fix it later if people complain.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about it do you find weird?

Copy link
Member

Choose a reason for hiding this comment

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

It's just an unusual condition for a parsing loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Idk, reads perfectly fine to me. 🤷‍♂

"While there's still an arg or a block ahead, loop to read for additional matchers"

@mholt mholt added this to the v2.0.0 milestone Apr 6, 2020
@mholt mholt merged commit a3cfe43 into caddyserver:master Apr 6, 2020
@francislavoie francislavoie deleted the not-shortcut branch April 6, 2020 19:06
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