Revert #1674 - failing tests

[aldas]

reverts #1674
reverts #1671

NB: in future please use squash commits when merging - it is cleaner for history and reverting merged PRs is easier

[codecov]

Codecov Report

Merging #1722 (547ca5c) into master (0482cb3) will increase coverage by 0.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1722      +/-   ##
==========================================
+ Coverage   85.31%   85.77%   +0.45%     
==========================================
  Files          29       29              
  Lines        2002     2017      +15     
==========================================
+ Hits         1708     1730      +22     
+ Misses        186      181       -5     
+ Partials      108      106       -2     

Impacted Files	Coverage Δ
echo.go	86.39% <ø> (-0.05%)	⬇️
bind.go	90.00% <100.00%> (+5.03%)	⬆️
context.go	87.19% <100.00%> (+0.59%)	⬆️

---

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 ea31edb...547ca5c. Read the comment docs.

[aldas]

#1674 seems to cause problems that are detected with #1671 . Maybe #1674 is correct ... who knows ¯\_(ツ)_/¯

f72eaa4#diff-996f64128c7fffe6cc8254ea4e1ac3c5693956f86ab5c8952e525083cf14ca27L26

NB: #1671 is also strange. get() is now executed twice. error from first call is ignored
but

echo/echo.go

Line 503 in ea31edb

get(prefix, h)

[pafuent]

@aldas Maybe I'm missing something but, Why you reverted #1671? The issue is related to #1674, in particular the first call to g.Any()
That one fixes #1569. Without that get (which registers the static route in Echo's Router) a GET to 127.0.0.1:8080/static returns a 404. If you want to get the correct result you need to add an slash at the end (127.0.0.1:8080/static/), and that routes works because it matches 127.0.0.1:8080/static/*

[pafuent]

BTW, thanks for the advice on squashing commits 👍

[aldas]

its easy - I did not understand why it would do it twice - always twice. it looked more like a quick fix - not a real solution. what if it already ended with slash?

[lammel]

@aldas Maybe I'm missing something but, Why you reverted #1671? The issue is related to #1674, in particular the first call to g.Any()
That one fixes #1569. Without that get (which registers the static route in Echo's Router) a GET to 127.0.0.1:8080/static returns a 404. If you want to get the correct result you need to add an slash at the end (127.0.0.1:8080/static/), and that routes works because it matches 127.0.0.1:8080/static/*

Additional note: The behaviour of treating a /path not the same as /path/ is currently an issue of the router implementation for any routes. It might make sense to fix this behaviour too. For static file delivery additionally most people would expect "/path/" to automatically server "/path/index.html" when available.

It might make sense to take a deeper look into the topic first, which should be done in a new PR based as replacement for #1671
But tests do not fail anymore after reverting only #1674 only.

Currently I'm in favour of only reverting #1674 and opening a new PR to discuss better alternatives for #1671.

Comments welcome.

[pafuent]

If the prefix already end with a slash, the slash at the end is required.
Besides of that, Nice Catch!!!! If prefix already ends with a slash, without the extra get() the only way to get it working is calling it in this way 127.0.0.1/static//

[aldas]

anyway i'm not saying it must be reverted. I assumed everyone guilty - both PRs seemed to pass tests before both of them were merged so - revert everything, make master to ok again and check later.

As far as I can remember trailing slash is a thing in Nginx and/or Apache location configuration directive. So you would add location sometimes as /mypath* or /mypath as it would match trailing slash and not slash

[lammel]

We will revert both and see what to do with the old PRs.
Thanks @aldas .