Skip to content

Commit 7f3a99e

Browse files
committed
Correctly detect and handle installation path collisions
`compilesketches.CompileSketches.install_from_path()` has code for detecting a pre-existing file/folder at the installation destination path and either removing it or else failing with a helpful error message depending on the setting of the `force` parameter. The previous detection code did not work correctly in the event the pre-existing item was a broken symlink. The previous removal code did not work correctly in the event the pre-existing item was a symlink or file.
1 parent 3deecbf commit 7f3a99e

File tree

2 files changed

+49
-29
lines changed

2 files changed

+49
-29
lines changed

Diff for: compilesketches/compilesketches.py

+6-3
Original file line numberDiff line numberDiff line change
@@ -559,11 +559,14 @@ def install_from_path(self, source_path, destination_parent_path, destination_na
559559

560560
destination_path = destination_parent_path.joinpath(destination_name)
561561

562-
if destination_path.exists():
562+
if destination_path.exists() or destination_path.is_symlink():
563563
if force:
564-
# Clear existing folder
564+
# Clear existing item
565565
self.verbose_print("Overwriting installation at:", destination_path)
566-
shutil.rmtree(path=destination_path)
566+
if destination_path.is_symlink() or destination_path.is_file():
567+
destination_path.unlink()
568+
else:
569+
shutil.rmtree(path=destination_path)
567570
else:
568571
print("::error::Installation already exists:", destination_path)
569572
sys.exit(1)

Diff for: compilesketches/tests/test_compilesketches.py

+43-26
Original file line numberDiff line numberDiff line change
@@ -1245,38 +1245,60 @@ def test_get_list_from_multiformat_input(input_value, expected_list, expected_wa
12451245

12461246
# noinspection PyUnresolvedReferences
12471247
@pytest.mark.parametrize(
1248-
"source_path, destination_parent_path, destination_name, expected_destination_path",
1249-
[(pathlib.Path("foo/source-path"),
1250-
pathlib.Path("bar/destination-parent-path"),
1248+
"source_sub_path, destination_parent_sub_path, destination_name, expected_destination_sub_path",
1249+
[("foo/source-path",
1250+
"bar/destination-parent-path",
12511251
None,
1252-
pathlib.Path("bar/destination-parent-path/source-path")),
1253-
(pathlib.Path("foo/source-path"),
1254-
pathlib.Path("bar/destination-parent-path"),
1252+
"bar/destination-parent-path/source-path"),
1253+
("foo/source-path",
1254+
"bar/destination-parent-path",
12551255
"destination-name",
1256-
pathlib.Path("bar/destination-parent-path/destination-name"))])
1257-
@pytest.mark.parametrize("exists", [True, False])
1256+
"bar/destination-parent-path/destination-name")])
1257+
@pytest.mark.parametrize("exists", ["no", "yes", "symlink", "broken"])
12581258
@pytest.mark.parametrize("force", [True, False])
12591259
@pytest.mark.parametrize("is_dir", [True, False])
12601260
def test_install_from_path(capsys,
1261-
mocker,
1262-
source_path,
1263-
destination_parent_path,
1261+
tmp_path,
1262+
source_sub_path,
1263+
destination_parent_sub_path,
12641264
destination_name,
1265-
expected_destination_path,
1265+
expected_destination_sub_path,
12661266
exists,
12671267
force,
12681268
is_dir):
1269-
is_dir = unittest.mock.sentinel.is_dir
1269+
source_path = tmp_path.joinpath(source_sub_path)
12701270

1271-
compile_sketches = get_compilesketches_object()
1271+
# Generate source path
1272+
if is_dir:
1273+
source_path.mkdir(parents=True)
1274+
else:
1275+
source_path.parent.mkdir(parents=True, exist_ok=True)
1276+
source_path.touch()
12721277

1273-
mocker.patch.object(pathlib.Path, "exists", autospec=True, return_value=exists)
1274-
mocker.patch("shutil.rmtree", autospec=True)
1275-
mocker.patch.object(pathlib.Path, "mkdir", autospec=True)
1276-
mocker.patch.object(pathlib.Path, "is_dir", autospec=True, return_value=is_dir)
1277-
mocker.patch.object(pathlib.Path, "symlink_to", autospec=True)
1278+
destination_parent_path = tmp_path.joinpath(destination_parent_sub_path)
1279+
if destination_name is None:
1280+
destination_path = destination_parent_path.joinpath(source_path.name)
1281+
else:
1282+
destination_path = destination_parent_path.joinpath(destination_name)
1283+
destination_path.parent.mkdir(parents=True, exist_ok=True)
12781284

1279-
if exists and not force:
1285+
# Generate pre-existing destination path
1286+
if exists == "yes":
1287+
if is_dir:
1288+
destination_path.mkdir(parents=True)
1289+
else:
1290+
# source_path.parent.mkdir(parents=True)
1291+
destination_path.touch()
1292+
elif exists == "symlink":
1293+
destination_path.symlink_to(target=source_path, target_is_directory=source_path.is_dir())
1294+
elif exists == "broken":
1295+
destination_path.symlink_to(target=tmp_path.joinpath("nonexistent"), target_is_directory=is_dir)
1296+
1297+
expected_destination_path = tmp_path.joinpath(expected_destination_sub_path)
1298+
1299+
compile_sketches = get_compilesketches_object()
1300+
1301+
if exists != "no" and not force:
12801302
with pytest.raises(expected_exception=SystemExit, match="1"):
12811303
compile_sketches.install_from_path(source_path=source_path,
12821304
destination_parent_path=destination_parent_path,
@@ -1291,13 +1313,8 @@ def test_install_from_path(capsys,
12911313
destination_parent_path=destination_parent_path,
12921314
destination_name=destination_name,
12931315
force=force)
1294-
if exists and force:
1295-
shutil.rmtree.assert_called_once_with(path=expected_destination_path)
1296-
else:
1297-
shutil.rmtree.assert_not_called()
12981316

1299-
pathlib.Path.symlink_to.assert_called_once_with(expected_destination_path, target=source_path,
1300-
target_is_directory=is_dir)
1317+
assert expected_destination_path.resolve() == source_path
13011318

13021319

13031320
def test_install_from_path_functional(tmp_path):

0 commit comments

Comments
 (0)