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.

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

Make local files default for fs commands #506

Merged
merged 12 commits into from
Jun 23, 2023
55 changes: 16 additions & 39 deletions cmd/fs/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,55 +2,32 @@ package fs

import (
"context"
"fmt"
"runtime"
"strings"

"github.com/databricks/cli/cmd/root"
"github.com/databricks/cli/libs/filer"
)

type Scheme string

const (
DbfsScheme = Scheme("dbfs")
LocalScheme = Scheme("file")
NoScheme = Scheme("")
)

func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) {
parts := strings.SplitN(fullPath, ":/", 2)
if len(parts) < 2 {
return nil, "", fmt.Errorf(`no scheme specified for path %s. Please specify scheme "dbfs" or "file". Example: file:/foo/bar or file:/c:/foo/bar`, fullPath)
// Split path at : to detect any file schemes
parts := strings.SplitN(fullPath, ":", 2)

// If dbfs file scheme is not specified, then it's a local path
if len(parts) < 2 || parts[0] != "dbfs" {
f, err := filer.NewLocalClient("")
return f, fullPath, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct though; if folks specify foobar:/bla` it shouldn't be interpreted as a local path.

If there is a : in the path they can always use ./foo:bar to specify it and it should work.

As long as the parts[0] is all \w characters, it should be "dbfs". If it isn't, we should return an error.

Otherwise typos on the scheme can cause issues.

scheme := Scheme(parts[0])

path := parts[1]
switch scheme {
case DbfsScheme:
w := root.WorkspaceClient(ctx)
// If the specified path has the "Volumes" prefix, use the Files API.
if strings.HasPrefix(path, "Volumes/") {
f, err := filer.NewFilesClient(w, "/")
return f, path, err
}
f, err := filer.NewDbfsClient(w, "/")
return f, path, err
w := root.WorkspaceClient(ctx)

case LocalScheme:
if runtime.GOOS == "windows" {
parts := strings.SplitN(path, ":", 2)
if len(parts) < 2 {
return nil, "", fmt.Errorf("no volume specfied for path: %s", path)
}
volume := parts[0] + ":"
relPath := parts[1]
f, err := filer.NewLocalClient(volume)
return f, relPath, err
}
f, err := filer.NewLocalClient("/")
// If the specified path has the "Volumes" prefix, use the Files API.
if strings.HasPrefix(path, "/Volumes/") {
f, err := filer.NewFilesClient(w, "/")
return f, path, err

default:
return nil, "", fmt.Errorf(`unsupported scheme %s specified for path %s. Please specify scheme "dbfs" or "file". Example: file:/foo/bar or file:/c:/foo/bar`, scheme, fullPath)
}

// The file is a dbfs file, and uses the DBFS APIs
f, err := filer.NewDbfsClient(w, "/")
return f, path, err
}
4 changes: 2 additions & 2 deletions libs/filer/dbfs_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,14 @@ type DbfsClient struct {
workspaceClient *databricks.WorkspaceClient

// File operations will be relative to this path.
root RootPath
root WorkspaceRootPath
}

func NewDbfsClient(w *databricks.WorkspaceClient, root string) (Filer, error) {
return &DbfsClient{
workspaceClient: w,

root: NewRootPath(root),
root: NewWorkspaceRootPath(root),
}, nil
}

Expand Down
4 changes: 2 additions & 2 deletions libs/filer/files_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ type FilesClient struct {
apiClient *client.DatabricksClient

// File operations will be relative to this path.
root RootPath
root WorkspaceRootPath
}

func filesNotImplementedError(fn string) error {
Expand All @@ -77,7 +77,7 @@ func NewFilesClient(w *databricks.WorkspaceClient, root string) (Filer, error) {
workspaceClient: w,
apiClient: apiClient,

root: NewRootPath(root),
root: NewWorkspaceRootPath(root),
}, nil
}

Expand Down
4 changes: 2 additions & 2 deletions libs/filer/local_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ import (
// LocalClient implements the [Filer] interface for the local filesystem.
type LocalClient struct {
// File operations will be relative to this path.
root RootPath
root localRootPath
}

func NewLocalClient(root string) (Filer, error) {
return &LocalClient{
root: NewRootPath(root),
root: NewLocalRootPath(root),
}, nil
}

Expand Down
27 changes: 27 additions & 0 deletions libs/filer/local_root_path.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package filer

import (
"fmt"
"path/filepath"
"strings"
)

type localRootPath struct {
rootPath string
}

func NewLocalRootPath(root string) localRootPath {
if root == "" {
return localRootPath{""}
}
return localRootPath{filepath.Clean(root)}
}

func (rp *localRootPath) Join(name string) (string, error) {
absPath := filepath.Join(rp.rootPath, name)

if !strings.HasPrefix(absPath, rp.rootPath) {
return "", fmt.Errorf("relative path escapes root: %s", name)
}
return absPath, nil
}
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions libs/filer/workspace_files_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ type WorkspaceFilesClient struct {
apiClient *client.DatabricksClient

// File operations will be relative to this path.
root RootPath
root WorkspaceRootPath
}

func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, error) {
Expand All @@ -92,7 +92,7 @@ func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer,
workspaceClient: w,
apiClient: apiClient,

root: NewRootPath(root),
root: NewWorkspaceRootPath(root),
}, nil
}

Expand Down
12 changes: 6 additions & 6 deletions libs/filer/root_path.go → libs/filer/workspace_root_path.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,23 @@ import (
"strings"
)

// RootPath can be joined with a relative path and ensures that
// WorkspaceRootPath can be joined with a relative path and ensures that
// the returned path is always a strict child of the root path.
type RootPath struct {
type WorkspaceRootPath struct {
rootPath string
}

// NewRootPath constructs and returns [RootPath].
// NewWorkspaceRootPath constructs and returns [RootPath].
// The named path is cleaned on construction.
func NewRootPath(name string) RootPath {
return RootPath{
func NewWorkspaceRootPath(name string) WorkspaceRootPath {
return WorkspaceRootPath{
rootPath: path.Clean(name),
}
}

// Join returns the specified path name joined to the root.
// It returns an error if the resulting path is not a strict child of the root path.
func (p *RootPath) Join(name string) (string, error) {
func (p *WorkspaceRootPath) Join(name string) (string, error) {
absPath := path.Join(p.rootPath, name)

// Don't allow escaping the specified root using relative paths.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (

func testRootPath(t *testing.T, uncleanRoot string) {
cleanRoot := path.Clean(uncleanRoot)
rp := NewRootPath(uncleanRoot)
rp := NewWorkspaceRootPath(uncleanRoot)

remotePath, err := rp.Join("a/b/c")
assert.NoError(t, err)
Expand Down
Loading