-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
matchers: support null
value in expression matcher
#6320
base: master
Are you sure you want to change the base?
Conversation
But breaks other stuff 🤔 https://github.com/caddyserver/caddy/actions/runs/9120010471/job/25076559154?pr=6320#step:9:568 |
I think this is making all strings nullable? I think that's probably not what we want 🤔 We might need to make a specific function to handle null but only for @TristonianJones FYI in case you have any suggestions. The gist of it is we're trying to allow |
@@ -704,7 +708,7 @@ var ( | |||
placeholderRegexp = regexp.MustCompile(`{([a-zA-Z][\w.-]+)}`) | |||
placeholderExpansion = `caddyPlaceholder(request, "${1}")` | |||
|
|||
CELTypeJSON = cel.MapType(cel.StringType, cel.DynType) | |||
CELTypeJSON = cel.MapType(cel.StringType, cel.AnyType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't modify this line. The DynType
is really more accurate here.
Hi @francislavoie, I think part of this issue is that the internal types are |
The plain
header
match (non-CEL) supports checking for the absence of a header. The corresponding CEL matcher couldn't achieve the same because it expects a string a slice in the value of header->value map. In the plainheader
matcher, the absence of a value is represented by a nil in-place of zero-slice as the value of the header key in the map. Thenil
only equalsnil
when iterating through the map. We needed an analogous value, i.e.null
.My attempt here is to add support for
null
. I'm stumbling in the dark as I'm not familiar with the cel-go module, but something works, so am I close?Here are the checks I've run:
So, works?