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

Improve filesystem support #2064

Merged
merged 5 commits into from Jan 24, 2022
Merged

Improve filesystem support #2064

merged 5 commits into from Jan 24, 2022

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Jan 8, 2022

Improve filesystem support. Relates to #2059 I have taken some code from v5 and made it v4 compatible

Usable if you have Go 1.16+, older Go versions will use old implementation

  • Add field echo.Filesystem, default filesystem that is set on echo.New() emulates how previously os.Open worked
  • Add methods:
    • echo.FileFS(path, file string, filesystem fs.FS, m ...MiddlewareFunc),
    • echo.StaticFS(pathPrefix string, fileSystem fs.FS),
    • group.FileFS(path, file string, filesystem fs.FS, m ...MiddlewareFunc),
    • group.StaticFS(pathPrefix string, fileSystem fs.FS).
    • MustSubFS(currentFs fs.FS, fsRoot string) fs.FS
  • Following methods will use internally echo.Filesystem to serve files:
    • echo.File,
    • echo.Static,
    • group.File,
    • group.Static,
    • Context.File

Note: embed.FS embedds files with full paths that include that same embedded directory name as prefix. In that case you can use helper function to create sub fs with that prefix echo.MustSubFS(embedded, rootDirectory). That "root prefix" was not added to the method signature as there could be fs.FS implementations that do not act that way.

Example with embed.FS:

//go:embed testdata
var embedded embed.FS

// assuming we have directory structure
// ./  <- execution dir
// ./testdata/  <- directory we are embedding
// ./testdata/index.html   <- on of the embedded files

func main() {
	e := echo.New()

	e.Use(middleware.Logger())
	e.Use(middleware.Recover())

	e.StaticFS("/assets", echo.MustSubFS(embedded, "testdata"))

	log.Fatal(e.Start(":8080"))
}

@aldas
Copy link
Contributor Author

aldas commented Jan 8, 2022

well well, I forgot already that fs.FS was introduced in Go 1.16. If echo.Filesystem part is removed and only *FS methods are left this could be made work with build tags for Go 1.16+.

@lammel
Copy link
Contributor

lammel commented Jan 9, 2022

well well, I forgot already that fs.FS was introduced in Go 1.16. If echo.Filesystem part is removed and only *FS methods are left this could be made work with build tags for Go 1.16+.

I'd suggest we tag this PR as v5 and merge on the v5 branch where Go < 1.16 is dropped, so we can use fs.FS directly from Go. What do you think @aldas ?

@aldas
Copy link
Contributor Author

aldas commented Jan 9, 2022

There is not point tag it as v5 as most of it is taken from v5 branch.

I can make it work with Go 1.16+ versions with build constraints. I would create additional struct to be embedded into echo.Echo struct in separate file along with those fs.FS related stuff and create version for pre and after 1.16+

Add it to echo.Echo along with common

echo/echo.go

Lines 68 to 70 in 296c313

Echo struct {
common
// startupMutex is mutex to lock Echo instance access during server configuration and startup. Useful for to get

and create version specific implementations in that versioned file

// +build go1.16

type versioned struct {
	Filesystem fs.FS
}

// ... methods requiring fs.FS interface
// +build !go1.16
type versioned struct {}

@lammel
Copy link
Contributor

lammel commented Jan 9, 2022

I can make it work with Go 1.16+ versions with build constraints. I would create additional struct to be embedded into echo.Echo struct in separate file along with those fs.FS related stuff and create version for pre and after 1.16+

If it is not too much effort, great. Than we can merge it for v4.6. Lovely!

…hods: echo.FileFS, echo.StaticFS, group.FileFS, group.StaticFS. Following methods will use echo.Filesystem to server files: echo.File, echo.Static, group.File, group.Static, Context.File
@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #2064 (2f39f09) into master (6f6befe) will increase coverage by 0.18%.
The diff coverage is 92.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2064      +/-   ##
==========================================
+ Coverage   91.57%   91.76%   +0.18%     
==========================================
  Files          33       36       +3     
  Lines        2921     2962      +41     
==========================================
+ Hits         2675     2718      +43     
+ Misses        157      156       -1     
+ Partials       89       88       -1     
Impacted Files Coverage Δ
context.go 88.04% <ø> (+1.10%) ⬆️
group.go 100.00% <ø> (ø)
context_fs_go1.16.go 80.00% <80.00%> (ø)
echo_fs_go1.16.go 95.83% <95.83%> (ø)
echo.go 95.14% <100.00%> (+0.93%) ⬆️
group_fs_go1.16.go 100.00% <100.00%> (ø)

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 6f6befe...2f39f09. Read the comment docs.

@aldas aldas requested a review from lammel January 11, 2022 18:57
@aldas aldas marked this pull request as draft January 12, 2022 23:01
context_fs_go1.16_test.go Outdated Show resolved Hide resolved
@aldas aldas marked this pull request as ready for review January 15, 2022 09:30
@aldas aldas requested a review from lammel January 15, 2022 09:30
@aldas
Copy link
Contributor Author

aldas commented Jan 15, 2022

Ok, this should be done now. Added some tweaks for easier usage with embed.FS. Note: there are no embed.FS in tests as then go.mod should be bumped to go1.16.

Please review @lammel

@aldas
Copy link
Contributor Author

aldas commented Jan 20, 2022

bump @lammel

@lammel
Copy link
Contributor

lammel commented Jan 23, 2022

bump @lammel

Bump noted ;-) Checking now...

Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

Reviewed all files, LGTM. Very nice stuff to support Go 1.16 too.

@aldas aldas merged commit feaa6ed into labstack:master Jan 24, 2022
@lammel lammel mentioned this pull request Jan 27, 2022
3 tasks
@jxsl13
Copy link

jxsl13 commented Mar 7, 2022

I think the behavior of e.Static() changed due to this update.
I did upgrade from the latest 4.6 version to 4.7 which broke my Static file downloads.

before

e.Static("/static", staticDir)

seems not to work the way it did in 4.6.x, as I do get HTTP 404 errors instead of the files above.

after

e.StaticFS("/static", os.DirFS(staticDir))

@aldas
Copy link
Contributor Author

aldas commented Mar 7, 2022

@jxsl13 are you using relative paths as staticDir? Are those out of CWD (current working directory) directory structure?

And let me clarify.

a) This line e.Static("/static", staticDir) does not work as it worked before?
b) or e.Static("/static", staticDir) and e.StaticFS("/static", os.DirFS(staticDir)) does not seem to be functionally equivalent?

@jxsl13
Copy link

jxsl13 commented Mar 14, 2022

@aldas
b) is functionally equivalent
a) does not work as it did work before 4.7

sorry for the late reply

I am constructing an absolute path from os.Getwd + some subdirectories. So staticDir is an absolute path

@aldas
Copy link
Contributor Author

aldas commented Mar 14, 2022

@jxsl13 please try v4.7.1 it should be better now - hopefully.

@jxsl13
Copy link

jxsl13 commented Mar 14, 2022

@aldas it does work again.

@jxsl13
Copy link

jxsl13 commented Mar 14, 2022

State in 4.7.1:
a) does work as it did work before 4.7
AND
b) is functionally equivalent

@aldas
Copy link
Contributor Author

aldas commented Mar 14, 2022

nice.

for history sake: os.DirFs.Open() considers paths that start with ./, ../ and / as invalid.

@aldas aldas deleted the filesystem_methods branch July 12, 2022 19:04
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.

None yet

3 participants