fix: windows support for loading files (#71)

On windows, there are quite a few specifics when it comes to resolving
a local file. This complexity stemmed from the desire to support
common, albeit invalid URI constructs for windows.

Overall, the original sin of this function has been to ignore the host
part of an URI, but now we can't really break this behavior.

* fixes #72
* refactored the local path pre-processing, so the windows-specifics
  appear more clearly
* fixed a few inconsistencies on windows file URIs and guarded against a panic

* test: refactored tests for the local file strategy, now table-driven and
  more systematic
* doc: added detailed documentation to LoadStrategy()
* ci: re-enacted tests on windows
* ci: temporarily muted past linting issues: relinting is deferred to another PR

Signed-off-by: Frederic BIDON <[email protected]>

.github/workflows/ci.yaml

@@ -12,9 +12,11 @@ jobs:
 
     steps:
     - uses: actions/checkout@v2
-    - uses: golangci/golangci-lint-action@v2
+    - uses: golangci/golangci-lint-action@v3
       with:
         args: --timeout=5m
+        version: latest
+        only-new-issues: true
 
   build:
     runs-on: ${{ matrix.os }}
@@ -24,7 +26,7 @@ jobs:
     strategy:
       matrix:
         # No Windows this time. Some tests expect Unix-style paths.
-        os: [ ubuntu-latest, macos-latest ]
+        os: [ ubuntu-latest, macos-latest, windows-latest]
       fail-fast: false
 
     steps:

.golangci.yml

@@ -11,7 +11,7 @@ linters-settings:
     threshold: 100
   goconst:
     min-len: 3
-    min-occurrences: 2
+    min-occurrences: 3
 
 linters:
   enable-all: true
@@ -52,3 +52,7 @@ linters:
     - nilnil
     - nonamedreturns
     - nosnakecase
+    - depguard
+    # temporarily disabled
+    - goconst
+    - testifylint

loading.go

@@ -21,6 +21,7 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"path"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -40,43 +41,97 @@ var LoadHTTPBasicAuthPassword = ""
 var LoadHTTPCustomHeaders = map[string]string{}
 
 // LoadFromFileOrHTTP loads the bytes from a file or a remote http server based on the path passed in
-func LoadFromFileOrHTTP(path string) ([]byte, error) {
-	return LoadStrategy(path, os.ReadFile, loadHTTPBytes(LoadHTTPTimeout))(path)
+func LoadFromFileOrHTTP(pth string) ([]byte, error) {
+	return LoadStrategy(pth, os.ReadFile, loadHTTPBytes(LoadHTTPTimeout))(pth)
 }
 
 // LoadFromFileOrHTTPWithTimeout loads the bytes from a file or a remote http server based on the path passed in
 // timeout arg allows for per request overriding of the request timeout
-func LoadFromFileOrHTTPWithTimeout(path string, timeout time.Duration) ([]byte, error) {
-	return LoadStrategy(path, os.ReadFile, loadHTTPBytes(timeout))(path)
+func LoadFromFileOrHTTPWithTimeout(pth string, timeout time.Duration) ([]byte, error) {
+	return LoadStrategy(pth, os.ReadFile, loadHTTPBytes(timeout))(pth)
 }
 
-// LoadStrategy returns a loader function for a given path or uri
-func LoadStrategy(path string, local, remote func(string) ([]byte, error)) func(string) ([]byte, error) {
-	if strings.HasPrefix(path, "http") {
+// LoadStrategy returns a loader function for a given path or URI.
+//
+// The load strategy returns the remote load for any path starting with `http`.
+// So this works for any URI with a scheme `http` or `https`.
+//
+// The fallback strategy is to call the local loader.
+//
+// The local loader takes a local file system path (absolute or relative) as argument,
+// or alternatively a `file://...` URI, **without host** (see also below for windows).
+//
+// There are a few liberalities, initially intended to be tolerant regarding the URI syntax,
+// especially on windows.
+//
+// Before the local loader is called, the given path is transformed:
+//   - percent-encoded characters are unescaped
+//   - simple paths (e.g. `./folder/file`) are passed as-is
+//   - on windows, occurrences of `/` are replaced by `\`, so providing a relative path such a `folder/file` works too.
+//
+// For paths provided as URIs with the "file" scheme, please note that:
+//   - `file://` is simply stripped.
+//     This means that the host part of the URI is not parsed at all.
+//     For example, `file:///folder/file" becomes "/folder/file`,
+//     but `file://localhost/folder/file` becomes `localhost/folder/file` on unix systems.
+//     Similarly, `file://./folder/file` yields `./folder/file`.
+//   - on windows, `file://...` can take a host so as to specify an UNC share location.
+//
+// Reminder about windows-specifics:
+// - `file://host/folder/file` becomes an UNC path like `\\host\folder\file` (no port specification is supported)
+// - `file:///c:/folder/file` becomes `C:\folder\file`
+// - `file://c:/folder/file` is tolerated (without leading `/`) and becomes `c:\folder\file`
+func LoadStrategy(pth string, local, remote func(string) ([]byte, error)) func(string) ([]byte, error) {
+	if strings.HasPrefix(pth, "http") {
 		return remote
 	}
-	return func(pth string) ([]byte, error) {
-		upth, err := pathUnescape(pth)
+
+	return func(p string) ([]byte, error) {
+		upth, err := pathUnescape(p)
 		if err != nil {
 			return nil, err
 		}
 
-		if strings.HasPrefix(pth, `file://`) {
-			if runtime.GOOS == "windows" {
-				// support for canonical file URIs on windows.
-				// Zero tolerance here for dodgy URIs.
-				u, _ := url.Parse(upth)
-				if u.Host != "" {
-					// assume UNC name (volume share)
-					// file://host/share/folder\... ==> \\host\share\path\folder
-					// NOTE: UNC port not yet supported
-					upth = strings.Join([]string{`\`, u.Host, u.Path}, `\`)
-				} else {
-					// file:///c:/folder/... ==> just remove the leading slash
-					upth = strings.TrimPrefix(upth, `file:///`)
-				}
-			} else {
-				upth = strings.TrimPrefix(upth, `file://`)
+		if !strings.HasPrefix(p, `file://`) {
+			// regular file path provided: just normalize slashes
+			return local(filepath.FromSlash(upth))
+		}
+
+		if runtime.GOOS != "windows" {
+			// crude processing: this leaves full URIs with a host with a (mostly) unexpected result
+			upth = strings.TrimPrefix(upth, `file://`)
+
+			return local(filepath.FromSlash(upth))
+		}
+
+		// windows-only pre-processing of file://... URIs
+
+		// support for canonical file URIs on windows.
+		u, err := url.Parse(filepath.ToSlash(upth))
+		if err != nil {
+			return nil, err
+		}
+
+		if u.Host != "" {
+			// assume UNC name (volume share)
+			// NOTE: UNC port not yet supported
+
+			// when the "host" segment is a drive letter:
+			// file://C:/folder/... => C:\folder
+			upth = path.Clean(strings.Join([]string{u.Host, u.Path}, `/`))
+			if !strings.HasSuffix(u.Host, ":") && u.Host[0] != '.' {
+				// tolerance: if we have a leading dot, this can't be a host
+				// file://host/share/folder\... ==> \\host\share\path\folder
+				upth = "//" + upth
+			}
+		} else {
+			// no host, let's figure out if this is a drive letter
+			upth = strings.TrimPrefix(upth, `file://`)
+			first, _, _ := strings.Cut(strings.TrimPrefix(u.Path, "/"), "/")
+			if strings.HasSuffix(first, ":") {
+				// drive letter in the first segment:
+				// file:///c:/folder/... ==> strip the leading slash
+				upth = strings.TrimPrefix(upth, `/`)
 			}
 		}