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 #1787: Add support for optional filesystem to the static middleware #1797

Merged
merged 5 commits into from
May 8, 2021
Merged

Fix #1787: Add support for optional filesystem to the static middleware #1797

merged 5 commits into from
May 8, 2021

Conversation

lukasdietrich
Copy link
Contributor

@lukasdietrich lukasdietrich commented Mar 2, 2021

This PR is related to #1787.

By supporting http.Filesystem we also provide support for the new fs.FS while maintaining compatibility with versions prior to go1.16.

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #1797 (eb51091) into master (6f9b71c) will decrease coverage by 0.02%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1797      +/-   ##
==========================================
- Coverage   89.56%   89.54%   -0.03%     
==========================================
  Files          32       32              
  Lines        2646     2659      +13     
==========================================
+ Hits         2370     2381      +11     
  Misses        178      178              
- Partials       98      100       +2     
Impacted Files Coverage Δ
middleware/static.go 70.00% <81.25%> (+2.83%) ⬆️

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 6f9b71c...eb51091. Read the comment docs.

@aldas
Copy link
Contributor

aldas commented Mar 2, 2021

NB: you might want to try static tests with config.Filesystem = http.FS(os.DirFS(".")) and probably add tests for embed fs (with build tag // +build go1.16)

I think the change may not be that simple as Open in os/fs packages does some extra file name validation and messes up some logic we rely on static middleware.

middleware/static.go Outdated Show resolved Hide resolved
@lukasdietrich
Copy link
Contributor Author

@aldas Thank you for your super quick codereview!

NB: you might want to try static tests with config.Filesystem = http.FS(os.DirFS(".")) and probably add tests for embed fs (with build tag // +build go1.16)

Great idea to use build tags! Go did not let me use embed.FS, because go1.15 is specified in go.mod, I did however add some testcases using testing/testfs and os.DirFS(...).

I think the change may not be that simple as Open in os/fs packages does some extra file name validation and messes up some logic we rely on static middleware.

Which validation exactly do you mean? What I saw so far is that fs.FS as well as http.Filesystem expects forward slashes instead of the system file separator.

regarding to filepath.Clean vs path.Clean there is subtle difference in their behavior #1718 (comment)

Well I am glad you caught that one! I reverted back to filepath.Clean and then convert to filepath.ToSlash to comply with the http.FileSystem. Do you think that is a sufficient solution?

@aldas
Copy link
Contributor

aldas commented Mar 3, 2021

well I was just tinkering around and saw your PR. I tried solve same thing ( couple of days ago ) by going fs.FS route and did not switch from c.File() to http.ServeContent and saw that changes just pile up with some of them being "different" path validation in Open. Using http.Filesystem is nice move.
That go.mod problem I think will be fixed in next Go patch version but this does not need to block accepting this PR.

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

Looks good to me

@lammel lammel modified the milestones: v4.2.1, v4.3 Mar 3, 2021
@aldas aldas merged commit b643e68 into labstack:master May 8, 2021
@aldas aldas mentioned this pull request May 8, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants