go-chi / chi

lightweight, idiomatic and composable router for building Go HTTP services
https://go-chi.io
MIT License
18.42k stars 985 forks source link

Matching parameters containing the delimiter #781

Open KN4CK3R opened 1 year ago

KN4CK3R commented 1 year ago

Example:

r.Get("/{name}.json", ...)

// Request: GET /1.0.json

This fails because of (how I think) the routing algorithm works. For {name}.json the first character after the placeholder is saved as "tail delimiter". The route search now resolves the placeholder by searching for the delimiter in the string. For /test.json this searches for . and the result is test. However if the request is /1.0.json this logic fails to resolve the correct value because the delimiter . is contained in the value. The result is 1 instead of the expected 1.0.

A possible fix would be to search for the whole non-placeholder-suffix .json. That would still fail for something like {name}. or {name}.{extension}. Another fix would be to make the search greedy and read the value until the last occurence of the delimiter. As this works now fine for my case it fails for {name}.{other}.json even without the delimiter inside the values. Still I think the greedy matching is more often what you would expect.

Greedy example:

            // serially loop through each node grouped by the tail delimiter
            for idx := 0; idx < len(nds); idx++ {
                xn = nds[idx]

+               segmentlen := strings.IndexByte(xsearch, '/')
+               if segmentlen == -1 {
+                   segmentlen = len(xsearch)
+               } else {
+                   segmentlen += 1 // just keep the old code working
+               }

                // label for param nodes is the delimiter byte
-               p := strings.IndexByte(xsearch, xn.tail)
+               p := strings.LastIndexByte(xsearch[:segmentlen], xn.tail)

                if p < 0 {
                    if xn.tail == '/' {
VojtechVitek commented 1 year ago

As this works now fine for my case it fails for {name}.{other}.json

It looks like the closest test cases are https://github.com/go-chi/chi/blob/master/tree_test.go#L223-L225

tr.InsertRoute(mGET, "/articles/files/{file}.{ext}", hStub12)
{m: mGET, r: "/articles/files/file.zip", h: hStub12, k: []string{"file", "ext"}, v: []string{"file", "zip"}},
{m: mGET, r: "/articles/files/photos.tar.gz", h: hStub12, k: []string{"file", "ext"}, v: []string{"photos", "tar.gz"}},
{m: mGET, r: "/articles/files/photos.tar.gz", h: hStub12, k: []string{"file", "ext"}, v: []string{"photos", "tar.gz"}},

where the {ext} match looks to be greedy.

VojtechVitek commented 1 year ago

Out of curiosity, have you tried using regex matcher or match the whole filename /{filenameWithExt} instead?

KN4CK3R commented 1 year ago

Thank you for the test case. It's an example which doesn't work anymore if the first parameter is greedy.


Matching the whole filename works but requires manual routing. In my usecase I need the routes

/{version}
/{version}.json
/{version}.zip

My current workaround can be seen here: https://github.com/go-gitea/gitea/pull/22404/files#diff-0a90ef0e0fa95e2969bf6f0dc0aad6245eee6b078e1fd98ea7d201defb7f5e12R305-R309

r.Get("/{version}", func(ctx *context.Context) {
    version := ctx.Params("version")
    if strings.HasSuffix(version, ".zip") {
        ...
    } else if strings.HasSuffix(version, ".json") {
        ...
    } else {
        ...
    }
})
js-everts commented 1 year ago

This is also the root cause behind #758.

Most other routers seem to only allow a single param per path segment (without static prefixes or suffixes), probably to avoid pitfalls like this (i haven't looked at them all).

@VojtechVitek

where the {ext} match looks to be greedy.

I think he wants {file} to be greedy not {ext}. Its a weird situation because if i see /{file}.json, I think its intuitive to expect {file} to be greedy, but if i see /{file}.{ext} then its intuitive NOT to expect greedy matching anywhere.

It may be beneficial to add some documentation regarding the non greedy nature of param matching.

VojtechVitek commented 1 year ago

Regex returns the longest leftmost match. So if I put two greedy groups next to each other, I expect the left group to be "more" greedy than the right group. See https://regex101.com/r/6MfS1E/1.

So in this case, I'd expect the same. For photos.tar.gz I'd expect {file}.{ext} to match file = "photos" and ext = "json".

Go's filepath.Ext("photos.tar.gz") also isn't greedy - it returns ".gz". https://go.dev/play/p/klM4s7qAlFU

KN4CK3R commented 1 year ago

Maybe a little bit more matching logic could help:

  1. /{file} matches everything
  2. /{file}.ext does not use . as delimiter but .ext. Could be implemented as strings.HasSuffix(segment, part.suffix)
  3. /some_{file}.ext could be implemented as strings.HasPrefix(segment, part.prefix) && strings.HasSuffix(segment, part.suffix)
  4. /{file}.{ext} is documented as {file}/{ext} is greedy (whatever should be the default, currently {ext} would be greedy)

So the tree must know if there is a single placeholder in a segment and if there is prefix and/or suffix or if there are multiple placeholders.

6543 commented 1 year ago

well I did a workaround for now: https://github.com/go-gitea/gitea/pull/23874/files but it's ugly

6543 commented 1 year ago

testcase:


r.Group("/{stringParam}", func() {
    r.Get("", func(ctx *context.Context) {
        p := ctx.Params("stringParam")
        // on `curl http://localhost/test.it` expect it to be "test.it"
    })
    r.Get(".png", func(ctx *context.Context) {
        p := ctx.Params("stringParam")
        // on `curl http://localhost/test.it.png` expect it to be "test.it"
    })
})
6543 commented 1 year ago

well I did open #811 ... let's see