Warn on usage of open that will break in the future (#4030)

This code is similar to the one made for `require` but for `open`. Part of #3857 Closes #3960 Co-authored-by: Joan López de la Franca Beltran <[email protected]>
grafana · Nov 6, 2024 · c77cb75 · c77cb75
1 parent 1d6d66e
commit c77cb75
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 37 deletions.
diff --git a/js/bundle.go b/js/bundle.go
@@ -9,6 +9,7 @@ import (
 	"net/url"
 	"path/filepath"
 	"runtime"
+	"strings"
 
 	"github.com/grafana/sobek"
 	"github.com/sirupsen/logrus"
@@ -455,6 +456,22 @@ func (b *Bundle) setInitGlobals(rt *sobek.Runtime, vu *moduleVUImpl, modSys *mod
 		if err != nil {
 			return nil, err
 		}
+		if !(strings.HasPrefix(filename, "file://") || filepath.IsAbs(filename)) {
+			otherPath, shouldWarn := modSys.ShouldWarnOnParentDirNotMatchingCurrentModuleParentDir(vu, pwd)
+			logger := b.preInitState.Logger
+			if shouldWarn {
+				logger.Warningf("open() was used and is currently relative to '%s', but in the future "+
+					"it will be aligned with how `require` and imports work and will be relative to '%s'. This means "+
+					"that in the future open will open relative path relative to the module/file it is written in. "+
+					"You can future proof this by using `import.meta.resolve()` to get relative paths to the file it "+
+					"is written in the current k6 version.", pwd, otherPath)
+				err = b.preInitState.Usage.Uint64("deprecations/openRelativity", 1)
+				if err != nil {
+					logger.WithError(err).Warn("failed reporting usage of deprecated relativity of open()")
+				}
+			}
+		}
+
 		return openImpl(rt, b.filesystems["file"], pwd, filename, args...)
 	})
 	warnAboutModuleMixing := func(name string) {

diff --git a/js/modules/require_impl.go b/js/modules/require_impl.go
@@ -136,6 +136,32 @@ func (ms *ModuleSystem) CurrentlyRequiredModule() (*url.URL, error) {
 	return loader.Dir(u), nil
 }
 
+// ShouldWarnOnParentDirNotMatchingCurrentModuleParentDir is a helper function to figure out if the provided url
+// is the same folder that an import will be to.
+// It also checks if the modulesystem is locked which means we are past the first init context.
+// If false is returned means that we are not in the init context or the path is matching - so no warning should be done
+// If true, the returned path is the module path that it should be relative to - the one that is checked against.
+func (ms *ModuleSystem) ShouldWarnOnParentDirNotMatchingCurrentModuleParentDir(vu VU, parentModulePwd *url.URL,
+) (string, bool) {
+	if ms.resolver.locked {
+		return "", false
+	}
+	normalizePathToURL := func(path string) string {
+		u, err := url.Parse(path)
+		if err != nil {
+			return path
+		}
+		return loader.Dir(u).String()
+	}
+	parentModuleDir := parentModulePwd.String()
+	parentModuleStr2 := getCurrentModuleScript(vu)
+	parentModuleStr2Dir := normalizePathToURL(parentModuleStr2)
+	if parentModuleDir != parentModuleStr2Dir {
+		return parentModuleStr2, true
+	}
+	return "", false
+}
+
 func toESModuleExports(exp Exports) interface{} {
 	if exp.Named == nil {
 		return exp.Default

diff --git a/js/path_resolution_test.go b/js/path_resolution_test.go
@@ -7,18 +7,21 @@ import (
 
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
+
 	"go.k6.io/k6/lib/fsext"
 	"go.k6.io/k6/lib/testutils"
 )
 
 // This whole file is about tests around https://github.com/grafana/k6/issues/2674
 
-func TestOpenPathResolution(t *testing.T) {
+func TestPathResolution(t *testing.T) {
 	t.Parallel()
 	testCases := map[string]struct {
-		fsMap map[string]any
+		fsMap         map[string]any
+		expectedLogs  []string
+		expectedError string
 	}{
-		"simple": {
+		"open simple": {
 			fsMap: map[string]any{
 				"/A/B/data.txt": "data file",
 				"/A/A/A/A/script.js": `
@@ -30,7 +33,7 @@ func TestOpenPathResolution(t *testing.T) {
 				`,
 			},
 		},
-		"intermediate": {
+		"open intermediate": {
 			fsMap: map[string]any{
 				"/A/B/data.txt": "data file",
 				"/A/C/B/script.js": `
@@ -45,7 +48,7 @@ func TestOpenPathResolution(t *testing.T) {
 				`,
 			},
 		},
-		"complex": {
+		"open complex": {
 			fsMap: map[string]any{
 				"/A/B/data.txt": "data file",
 				"/A/C/B/script.js": `
@@ -63,8 +66,11 @@ func TestOpenPathResolution(t *testing.T) {
 					export default function() {}
 				`,
 			},
+			expectedLogs: []string{
+				`open() was used and is currently relative to 'file:///A/B/B/', but in the future it will be aligned with how`,
+			},
 		},
-		"space in path": {
+		"open space in path": {
 			fsMap: map[string]any{
 				"/A/B D/data.txt": "data file",
 				"/A/C D/B/script.js": `
@@ -82,35 +88,11 @@ func TestOpenPathResolution(t *testing.T) {
 					export default function() {}
 				`,
 			},
+			expectedLogs: []string{
+				`open() was used and is currently relative to 'file:///A/B%20D/B/', but in the future it will be aligned with how `,
+			},
 		},
-	}
-
-	for name, testCase := range testCases {
-		name, testCase := name, testCase
-
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-			fs := fsext.NewMemMapFs()
-			err := writeToFs(fs, testCase.fsMap)
-			fs = fsext.NewCacheOnReadFs(fs, fsext.NewMemMapFs(), 0)
-			require.NoError(t, err)
-			b, err := getSimpleBundle(t, "/main.js", `export { default } from "/A/A/A/A/script.js"`, fs)
-			require.NoError(t, err)
-
-			_, err = b.Instantiate(context.Background(), 0)
-			require.NoError(t, err)
-		})
-	}
-}
-
-func TestRequirePathResolution(t *testing.T) {
-	t.Parallel()
-	testCases := map[string]struct {
-		fsMap         map[string]any
-		expectedLogs  []string
-		expectedError string
-	}{
-		"simple": {
+		"require simple": {
 			fsMap: map[string]any{
 				"/A/B/data.js": "module.exports='export content'",
 				"/A/A/A/A/script.js": `
@@ -122,7 +104,7 @@ func TestRequirePathResolution(t *testing.T) {
 				`,
 			},
 		},
-		"intermediate": {
+		"require intermediate": {
 			fsMap: map[string]any{
 				"/A/B/data.js": "module.exports='export content'",
 				"/A/C/B/script.js": `
@@ -137,7 +119,7 @@ func TestRequirePathResolution(t *testing.T) {
 				`,
 			},
 		},
-		"complex": {
+		"require complex": {
 			fsMap: map[string]any{
 				"/A/B/data.js": "module.exports='export content'",
 				"/A/C/B/script.js": `
@@ -155,7 +137,7 @@ func TestRequirePathResolution(t *testing.T) {
 				`,
 			},
 		},
-		"complex wrong": {
+		"require complex wrong": {
 			fsMap: map[string]any{
 				"/A/B/data.js": "module.exports='export content'",
 				"/A/C/B/script.js": `