caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
57.84k stars 4.02k forks source link

`celFileMatcherMacroExpander()` does not accept "try_files" map entry with a `Comprehension` value #6623

Open effleurager opened 1 week ago

effleurager commented 1 week ago

Looking at the documentation for the file matcher, the example CEL expression uses an array of strings to specify a list of files to search for. Seeming like the only alternative to globbing for Windows users, I was surprised to find building a list failed to compile with the following error:

provision http.matchers.expression: compiling CEL program: ERROR: <input>:1:6: matcher requires either a map or string literal argument

As a toy example, this seems like it should be a valid Caddyfile:

domain.tld {
  root * C:/path/to/folder
  @autosave `file({
    "try_files": [0,1,2,3,4,5,7,8,9].map(i, {path} + "_autosave_" + string(i) + ".sav"),
    "try_policy": "most_recently_modified"
  })`
  rewrite @autosave {file_match.relative}
  file_server
}

Given the severe restriction of globbing being unavailable, this seems like an important fallback to have?

francislavoie commented 1 week ago

Due to how matchers are provisioned (once at config load time, not on every request), they need to have static values as input (except for placeholders). I'm not sure it would be possible to implement this. From a type system standpoint I don't see how the MatchFile struct could take a dynamic try_files input fed by CEL.

In your case, can't you just list out all 10 permutations you want to try? That should work, no? The config is verbose yes, but it should work.

effleurager commented 1 week ago

While testing, I discovered a bug with the try_policy value not being assigned: https://github.com/caddyserver/caddy/blob/48ce47f1d44da485fbbf6be536a0e3822763f313/modules/caddyhttp/fileserver/matcher.go#L192-L195

I'm preparing a PR, but I'm wondering if I should add a test for CELLibrary specifically or just add an addition to expressionTests? https://github.com/caddyserver/caddy/blob/48ce47f1d44da485fbbf6be536a0e3822763f313/modules/caddyhttp/fileserver/matcher_test.go#L283-L292

francislavoie commented 1 week ago

Oops! Nice spotting. Yeah any kind of tests you can add are appreciated 😅