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.

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

Fix: static file routing path rewrite. #1538

Merged
merged 4 commits into from Oct 1, 2021

Conversation

fufuok
Copy link
Contributor

@fufuok fufuok commented Sep 24, 2021

Please provide enough information so that others can review your pull request:

Fixes: #1537

@welcome
Copy link

welcome bot commented Sep 24, 2021

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

Comment on lines +331 to +334
path = path[prefixLen:]
if len(path) == 0 || path[len(path)-1] != '/' {
path = append(path, '/')
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go test -v -run=^Test_Route_Static
=== RUN   Test_Route_Static_Root
--- PASS: Test_Route_Static_Root (0.01s)
=== RUN   Test_Route_Static_HasPrefix
--- PASS: Test_Route_Static_HasPrefix (0.00s)
PASS
ok      github.com/gofiber/fiber/v2     0.077s

Copy link
Member

@renanbastos93 renanbastos93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats, you made a good job.

@ReneWerner87
Copy link
Member

@fufuok please correct the imports

@fufuok
Copy link
Contributor Author

fufuok commented Oct 1, 2021

@fufuok please correct the imports

Done.

ioutil.TempFile instead of os.CreateTemp
ioutil.ReadAll instead of io.ReadAll

Tested in go1.14 and go1.17.

# go version
go version go1.14.15 windows/amd64

# go test -v -run=^Test_Route_Static
=== RUN   Test_Route_Static_Root
--- PASS: Test_Route_Static_Root (0.22s)
=== RUN   Test_Route_Static_HasPrefix
--- PASS: Test_Route_Static_HasPrefix (0.01s)
PASS
ok      github.com/gofiber/fiber/v2     1.633s
# go version
go version go1.17 linux/amd64

# go test -v -run=^Test_Route_Static
=== RUN   Test_Route_Static_Root
--- PASS: Test_Route_Static_Root (0.02s)
=== RUN   Test_Route_Static_HasPrefix
--- PASS: Test_Route_Static_HasPrefix (0.01s)
PASS
ok      github.com/gofiber/fiber/v2     0.088s

@ReneWerner87
Copy link
Member

ReneWerner87 commented Oct 1, 2021

@fufuok please check
image

the existing tests should actually still work as they were without error

@fufuok
Copy link
Contributor Author

fufuok commented Oct 1, 2021

https://github.com/gofiber/fiber/blob/master/app_test.go#L738
Why does the trailing slash here expect the result to be 404.

Please view this question gif and code: #1537

@ReneWerner87
Copy link
Member

I think you are right, you can change the expectations

@fufuok
Copy link
Contributor Author

fufuok commented Oct 1, 2021

image

Sorry, I rechecked the test case, because there is the default index.html in the directory, so it should be 200. I want to add another directory test case without index.html.

Unlike #1537 , it has option Browse: true

If possible, I would like to submit the above code, please approve it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Static file routing returns parent directory error
3 participants