From 16f557d7c7a87f5f87676168ede69a57ae5c3efd Mon Sep 17 00:00:00 2001 From: Laura King Date: Wed, 30 Aug 2023 17:33:10 -0700 Subject: [PATCH 1/8] ENH: json db tempfile cleanup --- happi/tests/test_backends.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/happi/tests/test_backends.py b/happi/tests/test_backends.py index d32e1bd8..59190688 100644 --- a/happi/tests/test_backends.py +++ b/happi/tests/test_backends.py @@ -202,6 +202,19 @@ def test_json_tempfile_uniqueness(): tempfiles.append(jb._temp_path()) assert len(set(tempfiles)) == len(tempfiles) +def test_json_tempfile_remove(monkeypatch): + # Force the function to fail so there will be a temp file to remove + monkeypatch.setattr('shutil.move', no_op) + def no_op(*args, **kwargs): + raise Exception + + # Ensure shutil.move raises an exception + with pytest.raises(Exception): + jb = JSONBackend("testing.json", initialize=True) + + # Ensure the temp file was removed correctly + assert os.path.exists(jb._temp_path) == False + @requires_questionnaire def test_qs_find(mockqsbackend): From d34af33f46f16147f7a87745f4dd749df8dc7715 Mon Sep 17 00:00:00 2001 From: Laura King Date: Thu, 31 Aug 2023 11:15:25 -0700 Subject: [PATCH 2/8] TST: Test json store bad temp file deletion --- happi/backends/json_db.py | 28 ++++++++++++++++------------ happi/tests/test_backends.py | 22 +++++++++++++--------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/happi/backends/json_db.py b/happi/backends/json_db.py index b2a6db42..ce161628 100644 --- a/happi/backends/json_db.py +++ b/happi/backends/json_db.py @@ -138,27 +138,31 @@ def store(self, db: dict[str, ItemMeta]) -> None: Dictionary to store in JSON. """ temp_path = self._temp_path() - - with open(temp_path, 'w') as fd: - json.dump(db, fd, sort_keys=True, indent=4) - - if os.path.exists(self.path): - shutil.copymode(self.path, temp_path) - shutil.move(temp_path, self.path) + try: + with open(temp_path, 'w') as fd: + json.dump(db, fd, sort_keys=True, indent=4) + + if os.path.exists(self.path): + shutil.copymode(self.path, temp_path) + shutil.move(temp_path, self.path) + except BaseException as ex: + + # log that the db move was not executed properly + logger.debug('JSON db move failed: %s', ex, exc_info=ex) + # remove temporary file + if os.path.exists(temp_path): + print(f"we are removing {temp_path}") + os.remove(temp_path) def _temp_path(self) -> str: """ Return a temporary path to write the json file to during "store". - Includes a username and a timestamp for sorting - (in the cases where the temp file isn't cleaned up) - and a hash for uniqueness + Includes a hash for uniqueness (in the cases where multiple temp files are written at once). """ directory = os.path.dirname(self.path) filename = ( - f".{getpass.getuser()}" - f"_{int(time.time())}" f"_{str(uuid.uuid4())[:8]}" f"_{os.path.basename(self.path)}" ) diff --git a/happi/tests/test_backends.py b/happi/tests/test_backends.py index 59190688..bfbdea3a 100644 --- a/happi/tests/test_backends.py +++ b/happi/tests/test_backends.py @@ -203,17 +203,21 @@ def test_json_tempfile_uniqueness(): assert len(set(tempfiles)) == len(tempfiles) def test_json_tempfile_remove(monkeypatch): - # Force the function to fail so there will be a temp file to remove - monkeypatch.setattr('shutil.move', no_op) - def no_op(*args, **kwargs): - raise Exception + # Set consistent temppath + jb = JSONBackend("testing.json", initialize=False) + temp_path = jb._temp_path() + jb._temp_path = lambda: temp_path + + # Ensure file is created, then throw error + def shutil_move_patch(*args, **kwargs): + assert os.path.isfile(os.path.join(os.getcwd(), temp_path)) + raise RuntimeError('Simulated error.') - # Ensure shutil.move raises an exception - with pytest.raises(Exception): - jb = JSONBackend("testing.json", initialize=True) + monkeypatch.setattr('shutil.move', shutil_move_patch) - # Ensure the temp file was removed correctly - assert os.path.exists(jb._temp_path) == False + # Test, and ensure file is deleted appropriately + jb.initialize() + assert os.path.exists(temp_path) == False @requires_questionnaire From 55e7da0c1d9c752e907eb427e33e1dcbad65f0bb Mon Sep 17 00:00:00 2001 From: Laura King Date: Thu, 31 Aug 2023 16:47:57 -0700 Subject: [PATCH 3/8] MNT: remove unneeded imports and print --- happi/backends/json_db.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/happi/backends/json_db.py b/happi/backends/json_db.py index ce161628..8493d1a0 100644 --- a/happi/backends/json_db.py +++ b/happi/backends/json_db.py @@ -2,14 +2,12 @@ Backend implemenation using the ``simplejson`` package. """ import contextlib -import getpass import logging import math import os import os.path import re import shutil -import time import uuid from typing import Any, Callable, Optional, Union @@ -151,7 +149,6 @@ def store(self, db: dict[str, ItemMeta]) -> None: logger.debug('JSON db move failed: %s', ex, exc_info=ex) # remove temporary file if os.path.exists(temp_path): - print(f"we are removing {temp_path}") os.remove(temp_path) def _temp_path(self) -> str: From 71c434e9e8f21f994d418355cc904dafb4a74587 Mon Sep 17 00:00:00 2001 From: Laura King Date: Thu, 31 Aug 2023 17:03:56 -0700 Subject: [PATCH 4/8] DOC: add pre-release notes --- .../304-remove_temporary_file.rst | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 docs/source/upcoming_release_notes/304-remove_temporary_file.rst diff --git a/docs/source/upcoming_release_notes/304-remove_temporary_file.rst b/docs/source/upcoming_release_notes/304-remove_temporary_file.rst new file mode 100644 index 00000000..8301657c --- /dev/null +++ b/docs/source/upcoming_release_notes/304-remove_temporary_file.rst @@ -0,0 +1,23 @@ +304 remove_temporar_file +################# + +API Changes +----------- +- N/A + +Features +-------- +- N/A + +Bugfixes +-------- +- N/A + +Maintenance +----------- +- Adds error handling for the temporary file created when initializing a json backend object. +- Changes format of temporary file name generation to contain only a unique hash. + +Contributors +------------ +- laura-king From 9b53571e61ee67017e3453e4e7df0570d4f19c2c Mon Sep 17 00:00:00 2001 From: Laura King Date: Thu, 31 Aug 2023 17:15:43 -0700 Subject: [PATCH 5/8] STY: change line spacing and = to is --- happi/tests/test_backends.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/happi/tests/test_backends.py b/happi/tests/test_backends.py index bfbdea3a..adaa2e8a 100644 --- a/happi/tests/test_backends.py +++ b/happi/tests/test_backends.py @@ -202,6 +202,7 @@ def test_json_tempfile_uniqueness(): tempfiles.append(jb._temp_path()) assert len(set(tempfiles)) == len(tempfiles) + def test_json_tempfile_remove(monkeypatch): # Set consistent temppath jb = JSONBackend("testing.json", initialize=False) @@ -217,7 +218,7 @@ def shutil_move_patch(*args, **kwargs): # Test, and ensure file is deleted appropriately jb.initialize() - assert os.path.exists(temp_path) == False + assert os.path.exists(temp_path) is False @requires_questionnaire From c91264265ec285aca7b96862681399d7bd95a1bf Mon Sep 17 00:00:00 2001 From: Laura King Date: Tue, 5 Sep 2023 10:08:34 -0700 Subject: [PATCH 6/8] MNT: add raise, and fix pre release name --- .../source/upcoming_release_notes/304-remove_temporary_file.rst | 2 +- happi/backends/json_db.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/source/upcoming_release_notes/304-remove_temporary_file.rst b/docs/source/upcoming_release_notes/304-remove_temporary_file.rst index 8301657c..50cffb82 100644 --- a/docs/source/upcoming_release_notes/304-remove_temporary_file.rst +++ b/docs/source/upcoming_release_notes/304-remove_temporary_file.rst @@ -1,4 +1,4 @@ -304 remove_temporar_file +304 remove_temporary_file ################# API Changes diff --git a/happi/backends/json_db.py b/happi/backends/json_db.py index 8493d1a0..fc613a2f 100644 --- a/happi/backends/json_db.py +++ b/happi/backends/json_db.py @@ -150,6 +150,7 @@ def store(self, db: dict[str, ItemMeta]) -> None: # remove temporary file if os.path.exists(temp_path): os.remove(temp_path) + raise def _temp_path(self) -> str: """ From 025c369fee4bdbe38f06f7bdd8bfcb5ffd7252e1 Mon Sep 17 00:00:00 2001 From: Laura King Date: Thu, 7 Sep 2023 11:46:55 -0700 Subject: [PATCH 7/8] MNT: raise exception within json db initialize, update test --- happi/backends/json_db.py | 2 -- happi/tests/test_backends.py | 5 +++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/happi/backends/json_db.py b/happi/backends/json_db.py index fc613a2f..afab6105 100644 --- a/happi/backends/json_db.py +++ b/happi/backends/json_db.py @@ -144,8 +144,6 @@ def store(self, db: dict[str, ItemMeta]) -> None: shutil.copymode(self.path, temp_path) shutil.move(temp_path, self.path) except BaseException as ex: - - # log that the db move was not executed properly logger.debug('JSON db move failed: %s', ex, exc_info=ex) # remove temporary file if os.path.exists(temp_path): diff --git a/happi/tests/test_backends.py b/happi/tests/test_backends.py index adaa2e8a..59987725 100644 --- a/happi/tests/test_backends.py +++ b/happi/tests/test_backends.py @@ -212,12 +212,13 @@ def test_json_tempfile_remove(monkeypatch): # Ensure file is created, then throw error def shutil_move_patch(*args, **kwargs): assert os.path.isfile(os.path.join(os.getcwd(), temp_path)) - raise RuntimeError('Simulated error.') + raise RuntimeError('Simulated testing error.') monkeypatch.setattr('shutil.move', shutil_move_patch) # Test, and ensure file is deleted appropriately - jb.initialize() + with pytest.raises(BaseException): + jb.initialize() assert os.path.exists(temp_path) is False From 8f084699b3f705934be9d7a60ef7b0e5cac0ccbf Mon Sep 17 00:00:00 2001 From: Laura King Date: Mon, 18 Sep 2023 13:47:16 -0700 Subject: [PATCH 8/8] TST: more clear assertion type in test --- happi/tests/test_backends.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/happi/tests/test_backends.py b/happi/tests/test_backends.py index 59987725..67a34261 100644 --- a/happi/tests/test_backends.py +++ b/happi/tests/test_backends.py @@ -209,7 +209,7 @@ def test_json_tempfile_remove(monkeypatch): temp_path = jb._temp_path() jb._temp_path = lambda: temp_path - # Ensure file is created, then throw error + # Ensure file is created, then throw error through patching def shutil_move_patch(*args, **kwargs): assert os.path.isfile(os.path.join(os.getcwd(), temp_path)) raise RuntimeError('Simulated testing error.') @@ -217,7 +217,7 @@ def shutil_move_patch(*args, **kwargs): monkeypatch.setattr('shutil.move', shutil_move_patch) # Test, and ensure file is deleted appropriately - with pytest.raises(BaseException): + with pytest.raises(RuntimeError): jb.initialize() assert os.path.exists(temp_path) is False