Fix routing for parameters with dots

[nono]

See issue #1108

[codecov]

Codecov Report

Merging #1109 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1109      +/-   ##
==========================================
+ Coverage   81.14%   81.22%   +0.07%     
==========================================
  Files          25       25              
  Lines        1899     1907       +8     
==========================================
+ Hits         1541     1549       +8     
  Misses        250      250              
  Partials      108      108

Impacted Files	Coverage Δ
router.go	93.51% <100%> (+0.2%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf7b555...eca0d61. Read the comment docs.

[vishr]

@nono Are you getting this error since #1106?

[nono]

@vishr The error was introduced by #1101, and I still have it after #1106

[vishr]

/cc @Carpetsmoker @ansel1 There are couple of issues report since #1101, I don't want to overload router.go for every case like this. What are your thoughts now? I am in favor of reverting that change.

[arp242]

Personally I think using dots in variable values never makes sense, as it's near-universally used for file extensions, even more so than / (one of the few things Windows, Unix, and macOS all agree on!).

Without my previous PR supporting /path/:id.json is hard; you need to add a middleware which modifies the parameters where you need to be careful to not introduce race conditions, so I think some change somewhere would be a good idea.

I don't know if this PR really improves things. It seems rather complex (and potentially surprising) to me. what will happen with /download/:filename.tar.gz for example?

How do other routers solve this problem?

[arp242]

If I understand the issue correct, then I think the problem is that people have this:

/path/:param

with a param value of:

/path/something.with.dots

I think that is the "real bug" that needs to be fixed; we only want to drop on dots in the param name, so we don't end up with param.json as the parameter name.

[nono]

My thoughts:

• I'd definitively expect the route /versions/:version to match /versions/1.2.3, and it's really hard to work-around if echo does not support it (for example, a route to download a file has to declined on /files/:filename for a filename without extension like README, /files/:filename.:ext for a filename with a single extension like index.html, /files/:filename.:ext.:ext for a tarball.tar.gz, etc.)
• It'd be nice to have two routes, /users/:id and /users/:id.json, with their own handlers, but if it is not supported by echo, a middleware can do the trick
• It looks like /users/:{id}.json and /things/:{param.name.with.dots} is overkill.

[s32x]

I think it's also worth noting that #1101 conflicts with rfc3986 section 2.3

2.3.  Unreserved Characters

   Characters that are allowed in a URI but do not have a reserved
   purpose are called unreserved.  These include uppercase and lowercase
   letters, decimal digits, hyphen, period, underscore, and tilde.

      unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"

Unless I'm interpreting the spec wrong it seems a . character should be treated as any other letter/number and should not have a special purpose (as reserved characters do).

[arp242]

RFC says nothing about routing, much less how variables in paths are treated.

[s32x]

Sorry, I guess I did interpret it wrong then. Just thought it might be relevant and worth mentioning. :) Either way, I'm in favor of having the change reverted as well.

[vishr]

Closing as #1101 is reverted