search

google / go-jsonnet

Apache License 2.0
1.63k stars 234 forks source link

Import path should be opaque #190

Closed petr-k closed 4 years ago

petr-k commented 6 years ago

When importing jsonnet documents using the import statement, the current implementation assumes that the resource returned from importer's FoundHere is a filesystem path. On subsequent imports, the base dir of that path is derived via a call to path.Dir() (see e.g. https://github.com/google/go-jsonnet/blob/master/interpreter.go#L415). This function always performs a path cleanup.

This is a problem when implementing custom importers that do not use filesystem paths, but for example URLs. E.g. when an importer returns https://raw.githubusercontent.com/ksonnet/ksonnet-lib/master/ksonnet.beta.2/k.libsonnet in FoundHere, upon resolving relative imports based on that URL, the importDir comes back to the importer cleaned up as https:/raw.githubusercontent.com/ksonnet/ksonnet-lib/master/ksonnet.beta.2 (notice the single forward slash in https:/).

go-jsonnet should not assume filesystem paths, in fact the resource "path" should be opaque.

sbarzowski commented 6 years ago

You're right. Thanks for pointing this out.

This was done for compatibility with C++ jsonnet.

IMO the right way to do it would be to have canonicalizePath(codePath, importedPath string) function in Importer interface. This way interpreter would assume nothing about the structure of the path. That would also help with caching (caching once per code directory is not ideal). Having a way to canonicalize paths would let us cache every file exactly once.

@sparkprime What do you think?

sparkprime commented 6 years ago

Yes I was thinking the same. I also think that was proposed in https://github.com/google/jsonnet/issues/239

sparkprime commented 6 years ago

In other words, if the importer is responsible for deriving the new path from the current file and the possibly relative import string, then there is no need for it to expose a splitting function.

petr-k commented 6 years ago

Just to provide some context, I stumbled upon this when implementing an importer that would support URLs in import statements and search paths for the [kubecfg] tool: (https://github.com/ksonnet/kubecfg) https://github.com/ksonnet/kubecfg/issues/182

sparkprime commented 6 years ago

C++ implementation did not canonicalize paths: https://github.com/google/jsonnet/blob/master/core/vm.cpp#L37

sbarzowski commented 4 years ago

This has been solved a while back.