From 0dd8d21c8be35ef9a26b81efb48c15f03624be7f Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 28 Jan 2025 22:14:10 +0100 Subject: [PATCH 1/6] PythonMutator: Fix relative path error --- bundle/config/mutator/python/python_locations.go | 7 ++++++- bundle/config/mutator/python/python_locations_test.go | 4 ++-- bundle/config/mutator/python/python_mutator.go | 6 +++--- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/bundle/config/mutator/python/python_locations.go b/bundle/config/mutator/python/python_locations.go index 2fa86bea0c..97a3e489d6 100644 --- a/bundle/config/mutator/python/python_locations.go +++ b/bundle/config/mutator/python/python_locations.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io" + pathlib "path" "path/filepath" "github.com/databricks/cli/libs/dyn" @@ -99,7 +100,7 @@ func removeVirtualLocations(locations []dyn.Location) []dyn.Location { // parsePythonLocations parses locations.json from the Python mutator. // // locations file is newline-separated JSON objects with pythonLocationEntry structure. -func parsePythonLocations(input io.Reader) (*pythonLocations, error) { +func parsePythonLocations(bundleRoot string, input io.Reader) (*pythonLocations, error) { decoder := json.NewDecoder(input) locations := newPythonLocations() @@ -116,6 +117,10 @@ func parsePythonLocations(input io.Reader) (*pythonLocations, error) { return nil, fmt.Errorf("failed to parse python location: %s", err) } + if !pathlib.IsAbs(entry.File) { + entry.File = filepath.Join(bundleRoot, entry.File) + } + location := dyn.Location{ File: entry.File, Line: entry.Line, diff --git a/bundle/config/mutator/python/python_locations_test.go b/bundle/config/mutator/python/python_locations_test.go index 32afcc92bb..f9738b7e9d 100644 --- a/bundle/config/mutator/python/python_locations_test.go +++ b/bundle/config/mutator/python/python_locations_test.go @@ -166,11 +166,11 @@ func TestLoadOutput(t *testing.T) { } func TestParsePythonLocations(t *testing.T) { - expected := dyn.Location{File: "foo.py", Line: 1, Column: 2} + expected := dyn.Location{File: "/tmp/my_project/foo.py", Line: 1, Column: 2} input := `{"path": "foo", "file": "foo.py", "line": 1, "column": 2}` reader := bytes.NewReader([]byte(input)) - locations, err := parsePythonLocations(reader) + locations, err := parsePythonLocations("/tmp/my_project", reader) assert.NoError(t, err) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index cd2e286e59..f75f111cf6 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -331,7 +331,7 @@ func (m *pythonMutator) runPythonMutator(ctx context.Context, root dyn.Value, op return dyn.InvalidValue, diag.Errorf("failed to load diagnostics: %s", pythonDiagnosticsErr) } - locations, err := loadLocationsFile(locationsPath) + locations, err := loadLocationsFile(opts.bundleRootPath, locationsPath) if err != nil { return dyn.InvalidValue, diag.Errorf("failed to load locations: %s", err) } @@ -381,7 +381,7 @@ func writeInputFile(inputPath string, input dyn.Value) error { } // loadLocationsFile loads locations.json containing source locations for generated YAML. -func loadLocationsFile(locationsPath string) (*pythonLocations, error) { +func loadLocationsFile(bundleRoot, locationsPath string) (*pythonLocations, error) { locationsFile, err := os.Open(locationsPath) if errors.Is(err, fs.ErrNotExist) { return newPythonLocations(), nil @@ -391,7 +391,7 @@ func loadLocationsFile(locationsPath string) (*pythonLocations, error) { defer locationsFile.Close() - return parsePythonLocations(locationsFile) + return parsePythonLocations(bundleRoot, locationsFile) } func loadOutputFile(rootPath, outputPath string, locations *pythonLocations) (dyn.Value, diag.Diagnostics) { From 354d7dbb767b87a78c7f4ec499bbae4ed8f88376 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 29 Jan 2025 08:39:15 +0100 Subject: [PATCH 2/6] Fix tests --- bundle/config/mutator/python/python_mutator_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index 322fb79e80..04f21c2767 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "os/exec" + pathlib "path" "path/filepath" "runtime" "testing" @@ -54,6 +55,8 @@ func TestPythonMutator_Name_applyMutators(t *testing.T) { func TestPythonMutator_loadResources(t *testing.T) { withFakeVEnv(t, ".venv") + rootPath := pathlib.Join(t.TempDir(), "my_project") + b := loadYaml("databricks.yml", ` experimental: python: @@ -64,6 +67,9 @@ func TestPythonMutator_loadResources(t *testing.T) { job0: name: job_0`) + // set rootPath so that we can make absolute paths in dyn.Location + b.BundleRootPath = rootPath + ctx := withProcessStub( t, []string{ @@ -120,7 +126,7 @@ func TestPythonMutator_loadResources(t *testing.T) { assert.Equal(t, []dyn.Location{ { - File: "src/examples/job1.py", + File: pathlib.Join(rootPath, "src/examples/job1.py"), Line: 5, Column: 7, }, From e01862757c64fe8befc8e21c02224b380352d0de Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 29 Jan 2025 09:29:50 +0100 Subject: [PATCH 3/6] Fix tests --- bundle/config/mutator/python/python_mutator_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index 04f21c2767..9d957e797f 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "os/exec" - pathlib "path" "path/filepath" "runtime" "testing" @@ -55,7 +54,7 @@ func TestPythonMutator_Name_applyMutators(t *testing.T) { func TestPythonMutator_loadResources(t *testing.T) { withFakeVEnv(t, ".venv") - rootPath := pathlib.Join(t.TempDir(), "my_project") + rootPath := filepath.Join(t.TempDir(), "my_project") b := loadYaml("databricks.yml", ` experimental: @@ -126,7 +125,7 @@ func TestPythonMutator_loadResources(t *testing.T) { assert.Equal(t, []dyn.Location{ { - File: pathlib.Join(rootPath, "src/examples/job1.py"), + File: filepath.Join(rootPath, "src/examples/job1.py"), Line: 5, Column: 7, }, From 775c0cfcc7bcc4c27209495bfcea93c9148c3eaf Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 29 Jan 2025 11:22:35 +0100 Subject: [PATCH 4/6] Address CR comments --- .../config/mutator/python/python_locations.go | 2 ++ .../mutator/python/python_locations_test.go | 22 ++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/bundle/config/mutator/python/python_locations.go b/bundle/config/mutator/python/python_locations.go index 97a3e489d6..9cb65c302e 100644 --- a/bundle/config/mutator/python/python_locations.go +++ b/bundle/config/mutator/python/python_locations.go @@ -117,6 +117,8 @@ func parsePythonLocations(bundleRoot string, input io.Reader) (*pythonLocations, return nil, fmt.Errorf("failed to parse python location: %s", err) } + // Output can contain both relative paths and absolute paths outside of bundle root. + // Mutator pipeline expects all path to be absolute at this point, so make all paths absolute. if !pathlib.IsAbs(entry.File) { entry.File = filepath.Join(bundleRoot, entry.File) } diff --git a/bundle/config/mutator/python/python_locations_test.go b/bundle/config/mutator/python/python_locations_test.go index f9738b7e9d..80fe61b8ef 100644 --- a/bundle/config/mutator/python/python_locations_test.go +++ b/bundle/config/mutator/python/python_locations_test.go @@ -165,12 +165,28 @@ func TestLoadOutput(t *testing.T) { require.Equal(t, filepath.Join(bundleRoot, generatedFileName), notebookPath.Locations()[0].File) } -func TestParsePythonLocations(t *testing.T) { - expected := dyn.Location{File: "/tmp/my_project/foo.py", Line: 1, Column: 2} +func TestParsePythonLocations_absolutePath(t *testing.T) { + // output can contain absolute path that is outside of the bundle root + expected := dyn.Location{File: filepath.Clean("/Shared/foo.py"), Line: 1, Column: 2} + + input := `{"path": "foo", "file": "/Shared/foo.py", "line": 1, "column": 2}` + reader := bytes.NewReader([]byte(input)) + locations, err := parsePythonLocations("/tmp/", reader) + + assert.NoError(t, err) + + assert.True(t, locations.keys["foo"].exists) + assert.Equal(t, expected, locations.keys["foo"].location) +} + +func TestParsePythonLocations_relativePath(t *testing.T) { + // output can contain relative paths, we expect all locations to be absolute + // at this stage of mutator pipeline + expected := dyn.Location{File: filepath.Clean("/tmp/my_project/foo.py"), Line: 1, Column: 2} input := `{"path": "foo", "file": "foo.py", "line": 1, "column": 2}` reader := bytes.NewReader([]byte(input)) - locations, err := parsePythonLocations("/tmp/my_project", reader) + locations, err := parsePythonLocations(filepath.Clean("/tmp/my_project"), reader) assert.NoError(t, err) From f845329c5193e59d704cb94c12bed77821c1baa9 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 29 Jan 2025 11:31:46 +0100 Subject: [PATCH 5/6] Clean absolute paths --- bundle/config/mutator/python/python_locations.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bundle/config/mutator/python/python_locations.go b/bundle/config/mutator/python/python_locations.go index 9cb65c302e..7c3e2a5666 100644 --- a/bundle/config/mutator/python/python_locations.go +++ b/bundle/config/mutator/python/python_locations.go @@ -121,6 +121,8 @@ func parsePythonLocations(bundleRoot string, input io.Reader) (*pythonLocations, // Mutator pipeline expects all path to be absolute at this point, so make all paths absolute. if !pathlib.IsAbs(entry.File) { entry.File = filepath.Join(bundleRoot, entry.File) + } else { + entry.File = filepath.Clean(entry.File) } location := dyn.Location{ From d33ef358f17dbb38c4f0bc41d690b38c3b4011e7 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 29 Jan 2025 11:32:47 +0100 Subject: [PATCH 6/6] Fix tests --- bundle/config/mutator/python/python_locations.go | 2 -- bundle/config/mutator/python/python_locations_test.go | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/bundle/config/mutator/python/python_locations.go b/bundle/config/mutator/python/python_locations.go index 7c3e2a5666..9cb65c302e 100644 --- a/bundle/config/mutator/python/python_locations.go +++ b/bundle/config/mutator/python/python_locations.go @@ -121,8 +121,6 @@ func parsePythonLocations(bundleRoot string, input io.Reader) (*pythonLocations, // Mutator pipeline expects all path to be absolute at this point, so make all paths absolute. if !pathlib.IsAbs(entry.File) { entry.File = filepath.Join(bundleRoot, entry.File) - } else { - entry.File = filepath.Clean(entry.File) } location := dyn.Location{ diff --git a/bundle/config/mutator/python/python_locations_test.go b/bundle/config/mutator/python/python_locations_test.go index 80fe61b8ef..2860af8201 100644 --- a/bundle/config/mutator/python/python_locations_test.go +++ b/bundle/config/mutator/python/python_locations_test.go @@ -167,7 +167,7 @@ func TestLoadOutput(t *testing.T) { func TestParsePythonLocations_absolutePath(t *testing.T) { // output can contain absolute path that is outside of the bundle root - expected := dyn.Location{File: filepath.Clean("/Shared/foo.py"), Line: 1, Column: 2} + expected := dyn.Location{File: "/Shared/foo.py", Line: 1, Column: 2} input := `{"path": "foo", "file": "/Shared/foo.py", "line": 1, "column": 2}` reader := bytes.NewReader([]byte(input))