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.

Sign up for GitHub

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

Jump to bottom

breaking change: Don't try to prepend https to module specifiers #3451

Merged
merged 1 commit into from
Nov 10, 2023
Merged

breaking change: Don't try to prepend https to module specifiers #3451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 21 additions & 21 deletions js/tc39/breaking_test_errors.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/tc39/tc39_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ func (ctx *tc39TestCtx) runTC39Module(name, src string, includes []string, vm *g
return fs.ReadFile(currentFS, specifier.Path[1:])
},
comp)
u := &url.URL{Path: path.Join(ctx.base, name)}
u := &url.URL{Scheme: "file", Path: path.Join(ctx.base, name)}

base := u.JoinPath("..")
ms := modules.NewModuleSystem(mr, moduleRuntime.VU)
Expand Down
16 changes: 5 additions & 11 deletions lib/old_archive_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func TestOldArchive(t *testing.T) {
testCases := map[string]string{
// map of filename to data for each main file tested
"github.com/k6io/k6/samples/example.js": `github file`,
"cdnjs.com/packages/Faker": `faker file`,
"cdnjs.com/libraries/Faker": `faker file`,
"C:/something/path2": `windows script`,
"/absolulte/path2": `unix script`,
}
Expand All @@ -75,14 +75,14 @@ func TestOldArchive(t *testing.T) {
fs := makeMemMapFs(t, map[string][]byte{
// files
"/files/github.com/k6io/k6/samples/example.js": []byte(`github file`),
"/files/cdnjs.com/packages/Faker": []byte(`faker file`),
"/files/cdnjs.com/libraries/Faker": []byte(`faker file`),
"/files/example.com/path/to.js": []byte(`example.com file`),
"/files/_/C/something/path": []byte(`windows file`),
"/files/_/absolulte/path": []byte(`unix file`),

// scripts
"/scripts/github.com/k6io/k6/samples/example.js2": []byte(`github script`),
"/scripts/cdnjs.com/packages/Faker2": []byte(`faker script`),
"/scripts/cdnjs.com/libraries/Faker2": []byte(`faker script`),
"/scripts/example.com/path/too.js": []byte(`example.com script`),
"/scripts/_/C/something/path2": []byte(`windows script`),
"/scripts/_/absolulte/path2": []byte(`unix script`),
Expand All @@ -104,9 +104,9 @@ func TestOldArchive(t *testing.T) {
"/example.com/path/to.js": []byte(`example.com file`),
"/example.com/path/too.js": []byte(`example.com script`),
"/github.com/k6io/k6/samples/example.js": []byte(`github file`),
"/cdnjs.com/packages/Faker": []byte(`faker file`),
"/cdnjs.com/libraries/Faker": []byte(`faker file`),
"/github.com/k6io/k6/samples/example.js2": []byte(`github script`),
"/cdnjs.com/packages/Faker2": []byte(`faker script`),
"/cdnjs.com/libraries/Faker2": []byte(`faker script`),
}),
}

Expand Down Expand Up @@ -157,12 +157,6 @@ func TestFilenamePwdResolve(t *testing.T) {
expectedFilenameURL: &url.URL{Opaque: "cdnjs.com/libraries/Faker"},
expectedPwdURL: &url.URL{Scheme: "file", Path: "/home/nobody"},
},
{
Filename: "example.com/something/dot.js",
Pwd: "example.com/something/",
expectedFilenameURL: &url.URL{Host: "example.com", Scheme: "", Path: "/something/dot.js"},
expectedPwdURL: &url.URL{Host: "example.com", Scheme: "", Path: "/something"},
},
{
Filename: "https://example.com/something/dot.js",
Pwd: "https://example.com/something",
Expand Down
102 changes: 39 additions & 63 deletions loader/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ var (
{"cdnjs", cdnjs, regexp.MustCompile(`^cdnjs\.com/libraries/([^/]+)(?:/([(\d\.)]+-?[^/]*))?(?:/(.*))?$`)},
{"github", github, regexp.MustCompile(`^github\.com/([^/]+)/([^/]+)/(.*)$`)},
}
errNoLoaderMatched = errors.New("no loader matched")
)

const (
httpsSchemeCouldntBeLoadedMsg = `The moduleSpecifier "%s" couldn't be retrieved from` +
` the resolved url "%s". Error : "%s"`
fileSchemeCouldntBeLoadedMsg = `The moduleSpecifier "%s" couldn't be found on ` +
Expand All @@ -46,30 +50,13 @@ var (
`your script and modules so that they're accessible by k6 from ` +
`inside of the container, see ` +
`https://k6.io/docs/using-k6/modules#using-local-modules-with-docker.`
nothingWorkedLoadedMsg = fileSchemeCouldntBeLoadedMsg +
` Additionally it was tried to be loaded as remote module by prepending "https://" to it, ` +
`which also didn't work. Remote resolution error: "%s"`
errNoLoaderMatched = errors.New("no loader matched")
)

// noSchemeRemoteModuleResolutionError is returned when a url with no scheme was tried to be
// resolved and errored out
type noSchemeRemoteModuleResolutionError struct {
err error // original error
moduleSpecifier string
}
type unresolvableURLError string

func (n noSchemeRemoteModuleResolutionError) Error() string {
return fmt.Sprintf(
`Module specifier "%s" was tried to be loaded as remote module by prepending "https://" to it, `+
`which didn't work. If you are trying to import a nodejs module, this is not supported `+
`as k6 is _not_ nodejs based. Please read https://k6.io/docs/using-k6/modules for more information. `+
`Remote resolution error: "%s"`, n.moduleSpecifier, n.err)
}

// Unwrap returns the wrapped error.
func (n noSchemeRemoteModuleResolutionError) Unwrap() error {
return n.err
func (u unresolvableURLError) Error() string {
// TODO potentially add more things about what k6 supports if users report being confused.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably a good idea to provide the list of the supported moduleSpecifiers to give a user an understanding of how they look like

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really don't like our messages being super huge. Which is why I prefer to err on the side of them being short then giving a lot of information that potentially won't be useful.

In this case explaining the modules can be absolute file paths, with or with file://, https:// urls, or relative paths (and what they will be relative to) + the k6/* stuff is way too much text. That arguably won't even help you enough.

I am somewhat okay with linking to the docs https://grafana.com/docs/k6/latest/using-k6/modules/, but even that seems a bit longish.

Especially as my expectation is that the most common case will be buffer or something similar from nodejs or npm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @oleiade ^

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we discussing this message?

The moduleSpecifier \"example.com/html\" couldn't be recognised as something k6 supports

How about something like 🤔

The module specifier \"example.com/html\" is unsupported. Supported module modifiers are `file://`, `https://`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or even less, gaining from the context:

The module specifier \"example.com/html\" is unsupported. Supported are file:// and https://.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we discussing this message?

Isn't this what you wanted? Or did I misunderstood?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was trying to address this:

I really don't like our messages being super huge. Which is why I prefer to err on the side of them being short then giving a lot of information that potentially won't be useful.

I mean, the message that we return right now (got it from tests):

The moduleSpecifier \"example.com/html\" couldn't be recognised as something k6 supports

And suggesting that we consider modifying it to something that contains examples:

The module specifier \"example.com/html\" is unsupported. Supported are file:// and https://.

Anyway, it was a non-blocking suggestion, so feel free to ignore it and merge as it is 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not mention both relative module specifier and neither does k6/net/grpc for example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've merged it for now - I would prefer to get some user feedback on this before doing something.

If you (or anyone else) disagree - please open an issue.

We can change the message fairly easy - even though it will need to be checked around in tests

return fmt.Sprintf(`The moduleSpecifier %q couldn't be recognised as something k6 supports.`, (string)(u))
}

// Resolve a relative path to an absolute one.
Expand Down Expand Up @@ -97,18 +84,14 @@ func Resolve(pwd *url.URL, moduleSpecifier string) (*url.URL, error) {
}
return u, err
}
// here we only care if a loader is pickable, if it is and later there is an error in the loading
// from it we don't want to try another resolve
_, loader, _ := pickLoader(moduleSpecifier)
if loader == nil {
u, err := url.Parse("https://" + moduleSpecifier)
if err != nil {
return nil, noSchemeRemoteModuleResolutionError{err: err, moduleSpecifier: moduleSpecifier}
}
u.Scheme = ""
return u, nil
if loader != nil {
// here we only care if a loader is pickable, if it is and later there is an error in the loading
// from it we don't want to try another resolve
return &url.URL{Opaque: moduleSpecifier}, nil
}
return &url.URL{Opaque: moduleSpecifier}, nil

return nil, unresolvableURLError(moduleSpecifier)
}

func resolveFilePath(pwd *url.URL, moduleSpecifier string) (*url.URL, error) {
Expand Down Expand Up @@ -172,6 +155,10 @@ func Load(
}
scheme := moduleSpecifier.Scheme
if scheme == "" {
if moduleSpecifier.Opaque == "" {
//nolint:stylecheck
return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, originalModuleSpecifier)
}
scheme = "https"
}

Expand All @@ -188,43 +175,32 @@ func Load(
if !errors.Is(err, fs.ErrNotExist) {
return nil, err
}
if scheme == "https" {
finalModuleSpecifierURL := &url.URL{}

switch {
case moduleSpecifier.Opaque != "": // This is loader
finalModuleSpecifierURL, err = resolveUsingLoaders(logger, moduleSpecifier.Opaque)
if err != nil {
return nil, err
}
case moduleSpecifier.Scheme == "":
logger.Warningf(`The moduleSpecifier "%s" has no scheme but we will try to resolve it as remote module. `+
`This is deprecated and will be removed in v0.48.0 - all remote modules will `+
`need to explicitly be prepended with "https://".`, originalModuleSpecifier)
*finalModuleSpecifierURL = *moduleSpecifier
finalModuleSpecifierURL.Scheme = scheme
default:
finalModuleSpecifierURL = moduleSpecifier
}
var result *SourceData
result, err = loadRemoteURL(logger, finalModuleSpecifierURL)
if err == nil {
result.URL = moduleSpecifier
// TODO maybe make an fsext.Fs which makes request directly and than use CacheOnReadFs
// on top of as with the `file` scheme fs
_ = fsext.WriteFile(filesystems[scheme], pathOnFs, result.Data, 0o644)
return result, nil
}
if scheme != "https" {
//nolint:stylecheck
return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, originalModuleSpecifier)
}

finalModuleSpecifierURL := moduleSpecifier

if moduleSpecifier.Scheme == "" && moduleSpecifier.Opaque == "" {
// we have an error and we did remote module resolution without a scheme
// let's write the coolest error message to try to help the lost soul who got to here
return nil, noSchemeRemoteModuleResolutionError{err: err, moduleSpecifier: originalModuleSpecifier}
if moduleSpecifier.Opaque != "" { // This is a loader
finalModuleSpecifierURL, err = resolveUsingLoaders(logger, moduleSpecifier.Opaque)
if err != nil {
return nil, err
}
}

var result *SourceData
result, err = loadRemoteURL(logger, finalModuleSpecifierURL)
if err != nil {
//nolint:stylecheck
return nil, fmt.Errorf(httpsSchemeCouldntBeLoadedMsg, originalModuleSpecifier, finalModuleSpecifierURL, err)
}
result.URL = moduleSpecifier
// TODO maybe make an fsext.Fs which makes request directly and than use CacheOnReadFs
// on top of as with the `file` scheme fs
_ = fsext.WriteFile(filesystems[scheme], pathOnFs, result.Data, 0o644)

return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, originalModuleSpecifier)
return result, nil
}

func resolveUsingLoaders(logger logrus.FieldLogger, name string) (*url.URL, error) {
Expand Down
14 changes: 3 additions & 11 deletions loader/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ func TestResolve(t *testing.T) {
t.Run("Missing", func(t *testing.T) {
t.Parallel()
u, err := loader.Resolve(root, "example.com/html")
require.NoError(t, err)
assert.Equal(t, u.String(), "//example.com/html")
// TODO: check that warning will be emitted if Loaded
require.ErrorContains(t, err, "The moduleSpecifier \"example.com/html\" couldn't be recognised as something k6 supports")
require.Nil(t, u)
})
t.Run("WS", func(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -245,18 +244,11 @@ func TestLoad(t *testing.T) {
root, err := url.Parse("file:///")
require.NoError(t, err)

t.Run("IP URL", func(t *testing.T) {
t.Parallel()
_, err := loader.Resolve(root, "192.168.0.%31")
require.Error(t, err)
require.Contains(t, err.Error(), `invalid URL escape "%31"`)
})

testData := [...]struct {
name, moduleSpecifier string
}{
{"URL", sr("HTTPSBIN_URL/invalid")},
{"HOST", "some-path-that-doesnt-exist.js"},
{"HOST", "https://some-path-that-doesnt-exist.js"},
}

filesystems := map[string]fsext.Fs{"https": fsext.NewMemMapFs()}
Expand Down
17 changes: 5 additions & 12 deletions loader/readsource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,12 @@ func ReadSource(
pwdURL := &url.URL{Scheme: "file", Path: filepath.ToSlash(filepath.Clean(pwd)) + "/"}
srcURL, err := Resolve(pwdURL, filepath.ToSlash(src))
if err != nil {
var noSchemeError noSchemeRemoteModuleResolutionError
if errors.As(err, &noSchemeError) {
// TODO maybe try to wrap the original error here as well, without butchering the message
return nil, fmt.Errorf(nothingWorkedLoadedMsg, noSchemeError.moduleSpecifier, noSchemeError.err)
var unresolvedError unresolvableURLError
if errors.As(err, &unresolvedError) {
//nolint:stylecheck
return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, (string)(unresolvedError))
}
return nil, err
}
result, err := Load(logger, filesystems, srcURL, src)
var noSchemeError noSchemeRemoteModuleResolutionError
if errors.As(err, &noSchemeError) {
// TODO maybe try to wrap the original error here as well, without butchering the message
return nil, fmt.Errorf(nothingWorkedLoadedMsg, noSchemeError.moduleSpecifier, noSchemeError.err)
}

return result, err
return Load(logger, filesystems, srcURL, src)
}