From 3ea5acd9627774e50c787f542d68416d085f394b Mon Sep 17 00:00:00 2001 From: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com> Date: Wed, 20 Nov 2024 08:54:09 -0500 Subject: [PATCH] Log exceptions and fix erroneous handling (#46) --- src/wxflow/configuration.py | 2 +- src/wxflow/file_utils.py | 4 ++-- src/wxflow/fsutils.py | 7 ++++--- src/wxflow/schema.py | 3 ++- src/wxflow/sqlitedb.py | 13 ++++++------ tests/test_file_utils.py | 40 +++++++++++++++++++++++++++++-------- tests/test_sqlitedb.py | 2 +- 7 files changed, 49 insertions(+), 22 deletions(-) diff --git a/src/wxflow/configuration.py b/src/wxflow/configuration.py index 3f9319b..64c134b 100644 --- a/src/wxflow/configuration.py +++ b/src/wxflow/configuration.py @@ -188,7 +188,7 @@ def _true_or_not(string: str): try: return to_datetime(string) # Try as a datetime - except Exception as exc: + except Exception: if string in BOOLS: # Likely a boolean, convert to True/False return _true_or_not(string) elif '.' in string: # Likely a number and that too a float diff --git a/src/wxflow/file_utils.py b/src/wxflow/file_utils.py index 967a023..f5adf51 100644 --- a/src/wxflow/file_utils.py +++ b/src/wxflow/file_utils.py @@ -86,7 +86,7 @@ def _copy_files(filelist, required=True): logger.info(f'Copied {src} to {dest}') except Exception as ee: logger.exception(f"Error copying {src} to {dest}") - raise ee(f"Error copying {src} to {dest}") + raise ee else: if required: logger.exception(f"Source file '{src}' does not exist and is required, ABORT!") @@ -109,4 +109,4 @@ def _make_dirs(dirlist): logger.info(f'Created {dd}') except Exception as ee: logger.exception(f"Error creating directory {dd}") - raise ee(f"Error creating directory {dd}") + raise ee diff --git a/src/wxflow/fsutils.py b/src/wxflow/fsutils.py index 70466d7..7fa6f4b 100644 --- a/src/wxflow/fsutils.py +++ b/src/wxflow/fsutils.py @@ -108,9 +108,10 @@ def cp(source: str, target: str) -> None: try: shutil.copy2(source, target) except OSError: - raise OSError(f"unable to copy {source} to {target}") - except Exception as exc: - raise Exception(exc) + raise OSError(f"Unable to copy {source} to {target}") + except Exception as ee: + logger.exception(f"An unknown error occurred while copying {source} to {target}") + raise ee # Group ID number for a given group name diff --git a/src/wxflow/schema.py b/src/wxflow/schema.py index 82c13ba..431e8da 100644 --- a/src/wxflow/schema.py +++ b/src/wxflow/schema.py @@ -661,7 +661,8 @@ def _get_key_name(key): sub_schema, is_main_schema=False, description=_get_key_description(key) ) if isinstance(key, Optional) and hasattr(key, "default"): - expanded_schema[key_name]["default"] = _to_json_type(_invoke_with_optional_kwargs(key.default, **kwargs) if callable(key.default) else key.default) # nopep8 + expanded_schema[key_name]["default"] = (_to_json_type(_invoke_with_optional_kwargs(key.default, **kwargs) + if callable(key.default) else key.default)) elif isinstance(key_name, Or): # JSON schema does not support having a key named one name or another, so we just add both options # This is less strict because we cannot enforce that one or the other is required diff --git a/src/wxflow/sqlitedb.py b/src/wxflow/sqlitedb.py index 9a43a98..0c4bf0b 100644 --- a/src/wxflow/sqlitedb.py +++ b/src/wxflow/sqlitedb.py @@ -1,8 +1,11 @@ import sqlite3 +from logging import getLogger from typing import Any, List, Optional, Tuple __all__ = ["SQLiteDB"] +logger = getLogger(__name__.split('.')[-1]) + class SQLiteDBError(Exception): """ @@ -35,10 +38,7 @@ def connect(self) -> None: """ - try: - self.connection = sqlite3.connect(self.db_name) - except sqlite3.OperationalError as exc: - raise sqlite3.OperationalError(exc) + self.connection = sqlite3.connect(self.db_name) def disconnect(self) -> None: """ @@ -110,13 +110,14 @@ def remove_column(self, table_name: str, column_name: str) -> None: try: query = f"ALTER TABLE {table_name} DROP COLUMN {column_name}" self.execute_query(query) - except sqlite3.OperationalError as exc: + except sqlite3.OperationalError as ee: query = f"PRAGMA table_info({table_name})" cursor = self.execute_query(query) columns = [column[1] for column in cursor.fetchall()] if column_name not in columns: raise ValueError(f"Column '{column_name}' does not exist in table '{table_name}'") - raise sqlite3.OperationalError(exc) + logger.exception(f"Failed to remove '{column_name}' from '{table_name}' due to an unknown error.") + raise ee def update_data( self, diff --git a/tests/test_file_utils.py b/tests/test_file_utils.py index 6236621..acea145 100644 --- a/tests/test_file_utils.py +++ b/tests/test_file_utils.py @@ -1,5 +1,7 @@ import os +import pytest + from wxflow import FileHandler @@ -27,6 +29,12 @@ def test_mkdir(tmp_path): assert os.path.exists(dd) +def test_bad_mkdir(): + # Attempt to create a directory in an unwritable parent directory + with pytest.raises(OSError): + FileHandler({'mkdir': ["/dev/null/foo"]}).sync() + + def test_copy(tmp_path): """ Test for copying files: @@ -35,6 +43,7 @@ def test_copy(tmp_path): tmp_path - pytest fixture """ + # Test 1 (nominal operation) - Creating a directory and copying files to it input_dir_path = tmp_path / 'my_input_dir' # Create the input directory @@ -56,7 +65,7 @@ def test_copy(tmp_path): for src, dest in zip(src_files, dest_files): copy_list.append([src, dest]) - # Create config object for FileHandler + # Create config dictionary for FileHandler config = {'copy': copy_list} # Copy input files to output files @@ -66,17 +75,32 @@ def test_copy(tmp_path): for ff in dest_files: assert os.path.isfile(ff) - # Create a config object for copying optional non-existent files (c.txt does not exist) + # Test 2 - Attempt to copy files to a non-writable directory + # Create a list of bad targets (/dev/null is unwritable) + bad_dest_files = ["/dev/null/a.txt", "/dev/null/bb.txt"] + + bad_copy_list = [] + for src, dest in zip(src_files, bad_dest_files): + bad_copy_list.append([src, dest]) + + # Create a config dictionary for FileHandler + bad_config = {'copy': bad_copy_list} + + # Attempt to copy + with pytest.raises(OSError): + FileHandler(bad_config).sync() + + # Test 3 - Attempt to copy missing, optional files to a writable directory + # Create a config dictionary (c.txt does not exist) copy_list.append([input_dir_path / 'c.txt', output_dir_path / 'c.txt']) config = {'copy_opt': copy_list} - # Copy input files to output files + # Copy input files to output files (should not raise an error) FileHandler(config).sync() - # Create a config object for copying required non-existent files (c.txt does not exist) + # Test 4 - Attempt to copy missing, required files to a writable directory + # Create a config dictionary (c.txt does not exist) config = {'copy_req': copy_list} - try: + c_file = input_dir_path / 'c.txt' + with pytest.raises(FileNotFoundError, match=f"Source file '{c_file}' does not exist"): FileHandler(config).sync() - except FileNotFoundError as e: - c_file = input_dir_path / 'c.txt' - assert f"Source file '{c_file}' does not exist" in str(e) diff --git a/tests/test_sqlitedb.py b/tests/test_sqlitedb.py index 2909987..035da90 100644 --- a/tests/test_sqlitedb.py +++ b/tests/test_sqlitedb.py @@ -56,7 +56,7 @@ def test_remove_column(db): column_name = "address" db.remove_column("test_table", column_name) - # Verify that the column exists in the test table + # Verify that the column no longer exists in the test table assert not column_exists(db, "test_table", column_name)