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

Remove temporary file in JSON backend store function #330

Merged
merged 10 commits into from
Sep 18, 2023
23 changes: 23 additions & 0 deletions docs/source/upcoming_release_notes/304-remove_temporary_file.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
304 remove_temporary_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
28 changes: 14 additions & 14 deletions happi/backends/json_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -138,27 +136,29 @@ 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:
logger.debug('JSON db move failed: %s', ex, exc_info=ex)
# remove temporary file
if os.path.exists(temp_path):
os.remove(temp_path)
laura-king marked this conversation as resolved.
Show resolved Hide resolved
raise

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)}"
)
Expand Down
19 changes: 19 additions & 0 deletions happi/tests/test_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,25 @@ def test_json_tempfile_uniqueness():
assert len(set(tempfiles)) == len(tempfiles)


def test_json_tempfile_remove(monkeypatch):
# 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 through patching
def shutil_move_patch(*args, **kwargs):
assert os.path.isfile(os.path.join(os.getcwd(), temp_path))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be a bit careful here - this assertion might raise, and it'd be caught by with pytest.raises(BaseException) below. Assertions are AssertionError(Exception) which inherits from BaseException (standard library has a nice ASCII tree of this here)

The short of it is: I think you should switch the below clause on line 220 to be with pytest.raises(RuntimeError) to make sure we're catching line 215 and not line 214

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, and I'll update the raise.

I will look more into how assert works in pytest-for some reason I thought the test would just count as a fail and move on. Not sure why I thought of it as isolated

raise RuntimeError('Simulated testing error.')

monkeypatch.setattr('shutil.move', shutil_move_patch)

# Test, and ensure file is deleted appropriately
with pytest.raises(RuntimeError):
jb.initialize()
assert os.path.exists(temp_path) is False


@requires_questionnaire
def test_qs_find(mockqsbackend):
assert len(list(mockqsbackend.find(dict(beamline='TST')))) == 14
Expand Down