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

PythonMutator: Fix relative path error #2253

Merged
merged 6 commits into from
Jan 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion bundle/config/mutator/python/python_locations.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"io"
pathlib "path"
"path/filepath"

"github.com/databricks/cli/libs/dyn"
Expand Down Expand Up @@ -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()

Expand All @@ -116,6 +117,12 @@ func parsePythonLocations(input io.Reader) (*pythonLocations, error) {
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) {
kanterov marked this conversation as resolved.
Show resolved Hide resolved
entry.File = filepath.Join(bundleRoot, entry.File)
}

location := dyn.Location{
File: entry.File,
Line: entry.Line,
Expand Down
22 changes: 19 additions & 3 deletions bundle/config/mutator/python/python_locations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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: "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: "/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(reader)
locations, err := parsePythonLocations(filepath.Clean("/tmp/my_project"), reader)

assert.NoError(t, err)

Expand Down
6 changes: 3 additions & 3 deletions bundle/config/mutator/python/python_mutator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand Down
7 changes: 6 additions & 1 deletion bundle/config/mutator/python/python_mutator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ func TestPythonMutator_Name_applyMutators(t *testing.T) {
func TestPythonMutator_loadResources(t *testing.T) {
withFakeVEnv(t, ".venv")

rootPath := filepath.Join(t.TempDir(), "my_project")

b := loadYaml("databricks.yml", `
experimental:
python:
Expand All @@ -64,6 +66,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{
Expand Down Expand Up @@ -120,7 +125,7 @@ func TestPythonMutator_loadResources(t *testing.T) {

assert.Equal(t, []dyn.Location{
{
File: "src/examples/job1.py",
File: filepath.Join(rootPath, "src/examples/job1.py"),
Line: 5,
Column: 7,
},
Expand Down