Repository: labstack/echo
Pull request #2116 (2022-03-08 02:45:35)

remove some unused code
Hi,
thank you for this amazing repository , its really efficient, and i noticed some code is pretty old and have never been used
so i think that maybe we may could remove them ?


Patch:
diff --git a/echo_fs_go1.16.go b/echo_fs_go1.16.go
index 435459d..eb17768 100644
--- a/echo_fs_go1.16.go
+++ b/echo_fs_go1.16.go
@@ -10,6 +10,7 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 )
 
@@ -94,10 +95,12 @@ func StaticFileHandler(file string, filesystem fs.FS) HandlerFunc {
 	}
 }
 
-// defaultFS emulates os.Open behaviour with filesystem opened by `os.DirFs`. Difference between `os.Open` and `fs.Open`
-// is that FS does not allow to open path that start with `..` or `/` etc. For example previously you could have `../images`
-// in your application but `fs := os.DirFS("./")` would not allow you to use `fs.Open("../images")` and this would break
-// all old applications that rely on being able to traverse up from current executable run path.
+// defaultFS exists to preserve pre v4.7.0 behaviour where files were open by `os.Open`.
+// v4.7 introduced `echo.Filesystem` field which is Go1.16+ `fs.Fs` interface.
+// Difference between `os.Open` and `fs.Open` is that FS does not allow opening path that start with `.`, `..` or `/`
+// etc. For example previously you could have `../images` in your application but `fs := os.DirFS("./")` would not
+// allow you to use `fs.Open("../images")` and this would break all old applications that rely on being able to
+// traverse up from current executable run path.
 // NB: private because you really should use fs.FS implementation instances
 type defaultFS struct {
 	prefix string
@@ -108,20 +111,26 @@ func newDefaultFS() *defaultFS {
 	dir, _ := os.Getwd()
 	return &defaultFS{
 		prefix: dir,
-		fs:     os.DirFS(dir),
+		fs:     nil,
 	}
 }
 
 func (fs defaultFS) Open(name string) (fs.File, error) {
+	if fs.fs == nil {
+		return os.Open(name)
+	}
 	return fs.fs.Open(name)
 }
 
 func subFS(currentFs fs.FS, root string) (fs.FS, error) {
 	root = filepath.ToSlash(filepath.Clean(root)) // note: fs.FS operates only with slashes. `ToSlash` is necessary for Windows
 	if dFS, ok := currentFs.(*defaultFS); ok {
-		// we need to make exception for `defaultFS` instances as it interprets root prefix differently from fs.FS to
-		// allow cases when root is given as `../somepath` which is not valid for fs.FS
-		root = filepath.Join(dFS.prefix, root)
+		// we need to make exception for `defaultFS` instances as it interprets root prefix differently from fs.FS.
+		// fs.Fs.Open does not like relative paths ("./", "../") and absolute paths at all but prior echo.Filesystem we
+		// were able to use paths like `./myfile.log`, `/etc/hosts` and these would work fine with `os.Open` but not with fs.Fs
+		if isRelativePath(root) {
+			root = filepath.Join(dFS.prefix, root)
+		}
 		return &defaultFS{
 			prefix: root,
 			fs:     os.DirFS(root),
@@ -130,6 +139,21 @@ func subFS(currentFs fs.FS, root string) (fs.FS, error) {
 	return fs.Sub(currentFs, root)
 }
 
+func isRelativePath(path string) bool {
+	if path == "" {
+		return true
+	}
+	if path[0] == '/' {
+		return false
+	}
+	if runtime.GOOS == "windows" && strings.IndexByte(path, ':') != -1 {
+		// https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#file_and_directory_names
+		// https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats
+		return false
+	}
+	return true
+}
+
 // MustSubFS creates sub FS from current filesystem or panic on failure.
 // Panic happens when `fsRoot` contains invalid path according to `fs.ValidPath` rules.
 //


Test patch:
diff --git a/binder_test.go b/binder_test.go
index 946906a..0349677 100644
--- a/binder_test.go
+++ b/binder_test.go
@@ -54,7 +54,7 @@ func TestBindingError_Error(t *testing.T) {
 func TestBindingError_ErrorJSON(t *testing.T) {
 	err := NewBindingError("id", []string{"1", "nope"}, "bind failed", errors.New("internal error"))
 
-	resp, err := json.Marshal(err)
+	resp, _ := json.Marshal(err)
 
 	assert.Equal(t, `{"field":"id","message":"bind failed"}`, string(resp))
 }
diff --git a/echo_test.go b/echo_test.go
index d31e7b6..0e1e42b 100644
--- a/echo_test.go
+++ b/echo_test.go
@@ -84,6 +84,14 @@ func TestEchoStatic(t *testing.T) {
 			expectStatus:         http.StatusOK,
 			expectBodyStartsWith: string([]byte{0x89, 0x50, 0x4e, 0x47}),
 		},
+		{
+			name:                 "ok with relative path for root points to directory",
+			givenPrefix:          "/images",
+			givenRoot:            "./_fixture/images",
+			whenURL:              "/images/walle.png",
+			expectStatus:         http.StatusOK,
+			expectBodyStartsWith: string([]byte{0x89, 0x50, 0x4e, 0x47}),
+		},
 		{
 			name:                 "No file",
 			givenPrefix:          "/images",
@@ -246,11 +254,54 @@ func TestEchoStaticRedirectIndex(t *testing.T) {
 }
 
 func TestEchoFile(t *testing.T) {
-	e := New()
-	e.File("/walle", "_fixture/images/walle.png")
-	c, b := request(http.MethodGet, "/walle", e)
-	assert.Equal(t, http.StatusOK, c)
-	assert.NotEmpty(t, b)
+	var testCases = []struct {
+		name             string
+		givenPath        string
+		givenFile        string
+		whenPath         string
+		expectCode       int
+		expectStartsWith string
+	}{
+		{
+			name:             "ok",
+			givenPath:        "/walle",
+			givenFile:        "_fixture/images/walle.png",
+			whenPath:         "/walle",
+			expectCode:       http.StatusOK,
+			expectStartsWith: string([]byte{0x89, 0x50, 0x4e}),
+		},
+		{
+			name:             "ok with relative path",
+			givenPath:        "/",
+			givenFile:        "./go.mod",
+			whenPath:         "/",
+			expectCode:       http.StatusOK,
+			expectStartsWith: "module github.com/labstack/echo/v",
+		},
+		{
+			name:             "nok file does not exist",
+			givenPath:        "/",
+			givenFile:        "./this-file-does-not-exist",
+			whenPath:         "/",
+			expectCode:       http.StatusNotFound,
+			expectStartsWith: "{\"message\":\"Not Found\"}\n",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			e := New() // we are using echo.defaultFS instance
+			e.File(tc.givenPath, tc.givenFile)
+
+			c, b := request(http.MethodGet, tc.whenPath, e)
+			assert.Equal(t, tc.expectCode, c)
+
+			if len(b) > len(tc.expectStartsWith) {
+				b = b[:len(tc.expectStartsWith)]
+			}
+			assert.Equal(t, tc.expectStartsWith, b)
+		})
+	}
 }
 
 func TestEchoMiddleware(t *testing.T) {
diff --git a/router_test.go b/router_test.go
index 5cbb8d9..457566b 100644
--- a/router_test.go
+++ b/router_test.go
@@ -1,7 +1,6 @@
 package echo
 
 import (
-	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -2446,33 +2445,3 @@ func BenchmarkRouterGooglePlusAPIMisses(b *testing.B) {
 func BenchmarkRouterParamsAndAnyAPI(b *testing.B) {
 	benchmarkRouterRoutes(b, paramAndAnyAPI, paramAndAnyAPIToFind)
 }
-
-func (n *node) printTree(pfx string, tail bool) {
-	p := prefix(tail, pfx, "└── ", "├── ")
-	fmt.Printf("%s%s, %p: type=%d, parent=%p, handler=%v, pnames=%v\n", p, n.prefix, n, n.kind, n.parent, n.methodHandler, n.pnames)
-
-	p = prefix(tail, pfx, "    ", "│   ")
-
-	children := n.staticChildren
-	l := len(children)
-
-	if n.paramChild != nil {
-		n.paramChild.printTree(p, n.anyChild == nil && l == 0)
-	}
-	if n.anyChild != nil {
-		n.anyChild.printTree(p, l == 0)
-	}
-	for i := 0; i < l-1; i++ {
-		children[i].printTree(p, false)
-	}
-	if l > 0 {
-		children[l-1].printTree(p, true)
-	}
-}
-
-func prefix(tail bool, p, on, off string) string {
-	if tail {
-		return fmt.Sprintf("%s%s", p, on)
-	}
-	return fmt.Sprintf("%s%s", p, off)
-}