[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

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

Open
effleurager opened this issue Oct 13, 2024 · 3 comments
Open

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

effleurager opened this issue Oct 13, 2024 · 3 comments

Comments

@effleurager
Copy link
Contributor

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
Copy link
Member
francislavoie commented Oct 13, 2024 •
edited
Loading

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
Copy link
Contributor Author
effleurager commented Oct 13, 2024 •
edited
Loading

While testing, I discovered a bug with the try_policy value not being assigned:

caddy/modules/caddyhttp/fileserver/matcher.go

Lines 192 to 195 in 48ce47f

var try_policy string
if len(values["try_policy"]) > 0 {
root = values["try_policy"][0]
}

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?

caddy/modules/caddyhttp/fileserver/matcher_test.go

Lines 283 to 292 in 48ce47f

var expressionTests = []struct {
name string
expression *caddyhttp.MatchExpression
urlTarget string
httpMethod string
httpHeader *http.Header
wantErr bool
wantResult bool
clientCertificate []byte
}{

@francislavoie
Copy link
Member

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

@effleurager effleurager mentioned this issue Oct 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@francislavoie @effleurager