Add HeaderMatchMode.NotExists, enable matchers that only match if the… #1806
Conversation
… configured header does not exist
|
|
There was a problem hiding this comment.
Thank you for your contribution!
Particularly the sortorder and "_AppliesScenarios" tests were not added to as I couldn't figure out what the intent of them was.
No worries, your change does not impact these tests.
They only care about the number of matchers, not their type.
| @@ -70,21 +70,31 @@ public Task ApplyAsync(HttpContext httpContext, CandidateSet candidates) | |||
|
|
|||
| foreach (var matcher in matchers) | |||
| { | |||
| if (headers.TryGetValue(matcher.Name, out var requestHeaderValues) && | |||
| !StringValues.IsNullOrEmpty(requestHeaderValues)) | |||
| var headerExistsInRequest = headers.TryGetValue(matcher.Name, out var requestHeaderValues); | |||
There was a problem hiding this comment.
headerExistsInRequest should include !StringValues.IsNullOrEmpty. An empty header has no semantic meaning in almost all cases. I don't know if any servers would actually return an empty header, but you already have that check on the next line so you might as well incorporate it to be consistent.
There was a problem hiding this comment.
I would be surprised if I set NotExists and it still matched on a request that does have the header (albeit empty).
There was a problem hiding this comment.
I guess that the difference between literally empty vs semantically empty. I doubt it will make a difference in practice. Nevermind.
There was a problem hiding this comment.
I would be surprised if I set NotExists and it still matched on a request that does have the header (albeit empty).
That was my thinking in breaking the two portions apart. Also the hope of using this to help glue together weird legacy software that does things like send no Accept header to a service with an incompatible default.
There was a problem hiding this comment.
Thinking about it a little further, I don't see a way to express "match only if the specified header is present, but empty". My use-case didn't need that, and I assumed that if I did at some point I could just use ExactHeader or PrefixHeader to an empty string. Looks like that existing IsNullOrEmpty check would prevent those from matching in that scenario.
I doubt it matters, just a note.
|
@MihaZupan @Tratcher The docs needs to be updated. |
Addresses #1697 by adding HeaderMatchMode.NotExists. Inverse of HeaderMatchMode.Exists, being that it only matches if the header is not present at all.
Have added a few tests, but had trouble deciphering the intent behind some of the tests in HeaderMatcherPolicyTests.cs and so may have missed some cases. Particularly the sortorder and "_AppliesScenarios" tests were not added to as I couldn't figure out what the intent of them was.
One failing test Yarp.ReverseProxy.FunctionalTests.HttpProxyCookieTests_Http2::ProxyAsync_RequestWithCookieHeaders is also failing in main, so I don't think I broke it.
Thanks for a great piece of software.