From 0016b89b7b3c3c00f2f666b4df404d7eebac7a93 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Wed, 7 Aug 2024 19:37:26 -0600 Subject: [PATCH 1/7] Extend test coverage of policy check client script --- tests/MCPClient/conftest.py | 8 + tests/MCPClient/test_policy_check.py | 602 +++++++++++++++++++++++++++ 2 files changed, 610 insertions(+) create mode 100644 tests/MCPClient/test_policy_check.py diff --git a/tests/MCPClient/conftest.py b/tests/MCPClient/conftest.py index 5751888596..323e71b64f 100644 --- a/tests/MCPClient/conftest.py +++ b/tests/MCPClient/conftest.py @@ -143,6 +143,14 @@ def fprule_preservation(fprule: fprmodels.FPRule) -> fprmodels.FPRule: return fprule +@pytest.fixture +def fprule_policy_check(fprule: fprmodels.FPRule) -> fprmodels.FPRule: + fprule.purpose = fprmodels.FPRule.POLICY + fprule.save() + + return fprule + + @pytest.fixture def fprule_thumbnail(fprule: fprmodels.FPRule) -> fprmodels.FPRule: fprule.purpose = fprmodels.FPRule.THUMBNAIL diff --git a/tests/MCPClient/test_policy_check.py b/tests/MCPClient/test_policy_check.py new file mode 100644 index 0000000000..9d8906f43c --- /dev/null +++ b/tests/MCPClient/test_policy_check.py @@ -0,0 +1,602 @@ +import json +import pathlib +import uuid +from unittest import mock + +import policy_check +import pytest +from client.job import Job +from main import models + + +@pytest.fixture() +def sip(sip): + # ReplacementDict expands SIP paths based on the shared directory. + sip.currentpath = "%sharedPath%" + sip.save() + return sip + + +@pytest.fixture() +def preservation_file(preservation_file): + preservation_file.filegrpuse = "preservation" + preservation_file.save() + return preservation_file + + +@pytest.fixture +def event(sip_file): + return models.Event.objects.create(file_uuid=sip_file, event_type="normalization") + + +@pytest.fixture +def derivation_for_preservation(sip_file, preservation_file, event): + return models.Derivation.objects.create( + source_file=sip_file, derived_file=preservation_file, event=event + ) + + +@pytest.fixture +def preservation_file_format_version(preservation_file, format_version): + return models.FileFormatVersion.objects.create( + file_uuid=preservation_file, format_version=format_version + ) + + +@pytest.fixture +def access_file(sip: models.SIP, transfer: models.Transfer) -> models.File: + location = b"%SIPDirectory%objects/file.wav" + return models.File.objects.create( + transfer=transfer, + sip=sip, + filegrpuse="access", + originallocation=location, + currentlocation=location, + ) + + +@pytest.fixture +def derivation_for_access(sip_file, access_file): + return models.Derivation.objects.create( + source_file=sip_file, derived_file=access_file + ) + + +@pytest.fixture +def access_file_format_version(access_file, format_version): + return models.FileFormatVersion.objects.create( + file_uuid=access_file, format_version=format_version + ) + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_check_if_rules_exist( + execute_or_run, + sip_file, + sip, + fprule_policy_check, + sip_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + sip_file.currentlocation.decode(), + str(sip_file.uuid), + str(sip.uuid), + str(shared_directory_path), + sip_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + assert ( + models.Event.objects.filter( + file_uuid_id=sip_file.uuid, + event_type="validation", + event_detail=( + f'program="{fprule_policy_check.command.tool.description}";' + f' version="{fprule_policy_check.command.tool.version}"' + ), + event_outcome="pass", + event_outcome_detail=json.loads(expected_stdout)["eventOutcomeDetailNote"], + ).count() + == 1 + ) + assert job.pyprint.mock_calls == [ + mock.call("Running", fprule_policy_check.command.description), + mock.call( + f"Command {fprule_policy_check.command.description} completed with output {expected_stdout}" + ), + mock.call( + f"Creating policy checking event for {sip_file.currentlocation.decode()} ({sip_file.uuid})" + ), + ] + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_checker_check_if_no_rules_exist( + execute_or_run, + sip_file, + sip, + sip_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + sip_file.currentlocation.decode(), + str(sip_file.uuid), + str(sip.uuid), + str(shared_directory_path), + sip_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + + job.pyprint.assert_called_once_with( + "Not performing a policy check because there are no relevant FPR rules" + ) + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_checker_check_if_file_does_not_exist( + execute_or_run, + sip_file, + sip, + sip_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + "", + "", + str(sip.uuid), + str(shared_directory_path), + sip_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + + job.pyprint.assert_called_once_with( + "Not performing a policy check because there is no file with UUID ." + ) + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_checker_execute_rule_command_returns_failed( + execute_or_run, + sip_file, + sip, + fprule_policy_check, + sip_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + sip_file.currentlocation.decode(), + str(sip_file.uuid), + str(sip.uuid), + str(shared_directory_path), + sip_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (1, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(1) + assert job.pyprint.mock_calls == [ + mock.call("Running", fprule_policy_check.command.description) + ] + job.print_error.assert_called_once_with( + f"Command {fprule_policy_check.command.description} failed with exit status 1; stderr:", + "", + ) + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_checker_execute_rule_command_returns_failed_if_event_outcome_information_other_than_pass( + execute_or_run, + sip_file, + sip, + fprule_policy_check, + sip_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + sip_file.currentlocation.decode(), + str(sip_file.uuid), + str(sip.uuid), + str(shared_directory_path), + sip_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "", + "eventOutcomeDetailNote": "", + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(1) + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_check_verifies_file_type_is_preservation( + execute_or_run, + derivation_for_preservation, + preservation_file, + sip, + fprule_policy_check, + preservation_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + preservation_file.currentlocation.decode(), + str(preservation_file.uuid), + str(sip.uuid), + str(shared_directory_path), + preservation_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + assert job.pyprint.mock_calls == [ + mock.call("Running", ""), + mock.call( + f"Command {fprule_policy_check.command.description} completed with output {expected_stdout}" + ), + mock.call( + f"Creating policy checking event for {preservation_file.currentlocation.decode()} ({preservation_file.uuid})" + ), + ] + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_check_fails_if_file_is_not_preservation_derivative( + execute_or_run, + preservation_file, + sip, + fprule_policy_check, + preservation_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + preservation_file.currentlocation.decode(), + str(preservation_file.uuid), + str(sip.uuid), + str(shared_directory_path), + preservation_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + assert job.pyprint.mock_calls == [ + mock.call( + f"File {preservation_file.uuid} is not a preservation derivative; not performing a policy check." + ) + ] + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_check_verifies_file_type_is_access( + execute_or_run, + derivation_for_access, + access_file, + sip, + fprule_policy_check, + access_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + access_file.currentlocation.decode(), + str(access_file.uuid), + str(sip.uuid), + str(shared_directory_path), + "access", + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + assert job.pyprint.mock_calls == [ + mock.call("Running", ""), + mock.call( + f"Command {fprule_policy_check.command.description} completed with output {expected_stdout}" + ), + mock.call( + f"Creating policy checking event for {access_file.currentlocation.decode()} ({access_file.uuid})" + ), + ] + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_check_fails_if_file_is_not_access_derivative( + execute_or_run, + access_file, + sip, + fprule_policy_check, + access_file_format_version, + format, + format_version, + shared_directory_path, +): + job = mock.Mock( + args=[ + "policy_check", + access_file.currentlocation.decode(), + str(access_file.uuid), + str(sip.uuid), + str(shared_directory_path), + "access", + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + + assert job.pyprint.mock_calls == [ + mock.call( + f"File {access_file.uuid} is not an access derivative; not performing a policy check." + ), + mock.call(f"File {access_file.uuid} is not a derivative."), + ] + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_checker_saves_policy_check_result_into_logs_directory( + execute_or_run, + sip_file, + sip, + fprule_policy_check, + sip_file_format_version, + format, + format_version, + shared_directory_path, +): + log_directory = shared_directory_path / "logs" + log_directory.mkdir() + job = mock.Mock( + args=[ + "policy_check", + sip_file.currentlocation.decode(), + str(sip_file.uuid), + str(sip.uuid), + str(shared_directory_path), + "preservation", + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": "MediaConch policy check result against policy file MP3 has duration: All policy checks passed: Does the audio duration exist?; MP3 has duration", + "policy": '\n\n Rudimentary test to check for an MP3 having a duration value.\n mp3\n', + "policyFileName": "MP3 has duration", + "stdout": "foobar", + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + + +@pytest.mark.django_db +@mock.patch("policy_check.executeOrRun") +def test_policy_checker_saves_policy_check_result_into_submission_documentation_directory( + execute_or_run, + sip_file, + sip, + fprule_policy_check, + sip_file_format_version, + format, + format_version, + shared_directory_path, +): + submission_documentation_directory = ( + shared_directory_path / "metadata" / "submissionDocumentation" + ) + submission_documentation_directory.mkdir(parents=True) + job = mock.Mock( + args=[ + "policy_check", + sip_file.currentlocation.decode(), + str(sip_file.uuid), + str(sip.uuid), + str(shared_directory_path), + sip_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + expected_stdout = json.dumps( + { + "eventOutcomeInformation": "pass", + "eventOutcomeDetailNote": "MediaConch policy check result against policy file MP3 has duration: All policy checks passed: Does the audio duration exist?; MP3 has duration", + "policy": '\n\n Rudimentary test to check for an MP3 having a duration value.\n mp3\n', + "policyFileName": "MP3 has duration", + "stdout": "foobar", + } + ) + execute_or_run.return_value = (0, expected_stdout, "") + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) + + +@pytest.mark.django_db +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 0, + json.dumps({"eventOutcomeInformation": "pass"}), + "", + ), +) +def test_policy_checker_checks_manually_normalized_access_derivative_file( + execute_or_run, + transfer, + sip_file, + sip, + fprule_policy_check, + format, + format_version, + shared_directory_path, +): + sip_file_name = pathlib.Path(sip_file.currentlocation.decode()).name + manually_access_derivative_file = models.File.objects.create( + transfer=transfer, + sip=sip, + filegrpuse="original", + originallocation=f"%transferDirectory%objects/manualNormalization/access/{sip_file_name}".encode(), + ) + models.FileFormatVersion.objects.create( + file_uuid=manually_access_derivative_file, format_version=format_version + ) + job = mock.Mock( + args=[ + "policy_check", + f"{shared_directory_path}/DIP/objects/{uuid.uuid4()}-{sip_file_name}", + "None", + str(sip.uuid), + str(shared_directory_path), + sip_file.filegrpuse, + ], + JobContext=mock.MagicMock(), + spec=Job, + ) + + policy_check.call([job]) + + job.set_status.assert_called_once_with(0) From b06948f0c940bbc576bc81526f490e27bacba959 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Tue, 13 Aug 2024 12:27:30 -0600 Subject: [PATCH 2/7] Add typing annotations in policy_check script --- .../lib/clientScripts/policy_check.py | 67 ++++++++++++------- 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/src/MCPClient/lib/clientScripts/policy_check.py b/src/MCPClient/lib/clientScripts/policy_check.py index 7e1414b730..be6da962fe 100755 --- a/src/MCPClient/lib/clientScripts/policy_check.py +++ b/src/MCPClient/lib/clientScripts/policy_check.py @@ -18,7 +18,13 @@ from custom_handlers import get_script_logger django.setup() +from typing import Dict +from typing import List +from typing import Optional +from typing import Tuple + import databaseFunctions +from client.job import Job from dicts import replace_string_values from django.conf import settings as mcpclient_settings from django.core.exceptions import ValidationError @@ -39,7 +45,14 @@ FAIL_CODE = 1 -def main(job, file_path, file_uuid, sip_uuid, shared_path, file_type): +def main( + job: Job, + file_path: str, + file_uuid: str, + sip_uuid: str, + shared_path: str, + file_type: str, +) -> int: """Entry point for policy checker.""" setup_dicts(mcpclient_settings) @@ -61,7 +74,15 @@ class PolicyChecker: ``check`` method. """ - def __init__(self, job, file_path, file_uuid, sip_uuid, shared_path, file_type): + def __init__( + self, + job: Job, + file_path: str, + file_uuid: str, + sip_uuid: str, + shared_path: str, + file_type: str, + ) -> None: """Initiate a new policy check.""" self.job = job self.file_path = file_path @@ -73,11 +94,11 @@ def __init__(self, job, file_path, file_uuid, sip_uuid, shared_path, file_type): self.is_manually_normalized_access_derivative = ( self._get_is_manually_normalized_access_derivative() ) - self._sip_logs_dir = None - self._sip_subm_doc_dir = None - self._sip_policy_checks_dir = None + self._sip_logs_dir: Optional[str] = None + self._sip_subm_doc_dir: Optional[str] = None + self._sip_policy_checks_dir: Optional[str] = None - def check(self): + def check(self) -> int: """Check the file identified by ``self.file_uuid`` against any policy-check FPR commands that are applicable. If any fail, return a non-zero exit code; otherwise return ``0``. @@ -110,7 +131,7 @@ def check(self): purpose = "policy_check" - def _we_check_this_type_of_file(self): + def _we_check_this_type_of_file(self) -> bool: """Return ``True`` if this policy checker should perform a check on this file; ``False`` otherwise. This will depend on ``self.file_type`` and on whether the file is an original or a preservation/access @@ -143,7 +164,7 @@ def _we_check_this_type_of_file(self): else: return True - def _file_is_derivative(self, for_access=False): + def _file_is_derivative(self, for_access=False) -> bool: """Return ``True`` if the target file is a derivative; ``False`` otherwise. """ @@ -151,7 +172,7 @@ def _file_is_derivative(self, for_access=False): return True # Access derivatives have Derivation rows with NULL event types (cf. # normalize.py client script). - event_type = "normalization" + event_type: Optional[str] = "normalization" if for_access: event_type = None try: @@ -162,12 +183,12 @@ def _file_is_derivative(self, for_access=False): except (Derivation.DoesNotExist, ValidationError): return False - def _get_policies_dir(self): + def _get_policies_dir(self) -> str: return os.path.join( self.shared_path, "sharedMicroServiceTasksConfigs", "policies" ) - def _get_is_manually_normalized_access_derivative(self): + def _get_is_manually_normalized_access_derivative(self) -> bool: """Manually normalized access derivatives are never given UUIDs. Therefore, we need this heuristic for determining if that is what we are dealing with. TODO/QUESTION: will this return false positives? @@ -178,7 +199,7 @@ def _get_is_manually_normalized_access_derivative(self): return True return False - def _file_is_for_access(self): + def _file_is_for_access(self) -> bool: """Return ``True`` if the file with UUID ``self.file_uuid`` is "for" access. """ @@ -189,7 +210,7 @@ def _file_is_for_access(self): return True return False - def _get_manually_normalized_access_derivative_file_uuid(self): + def _get_manually_normalized_access_derivative_file_uuid(self) -> File: """If the file-to-be-policy-checked is a manually normalized access derivative it will have no file UUID in the database. We therefore have to retrieve the UUID of the original file that was format-identified, @@ -209,7 +230,7 @@ def _get_manually_normalized_access_derivative_file_uuid(self): except (File.DoesNotExist, File.MultipleObjectsReturned, ValidationError): return None - def _get_rules(self): + def _get_rules(self) -> FPRule: """Return the FPR rules with purpose ``self.purpose`` and that apply to the type/format of file given as input. """ @@ -227,7 +248,7 @@ def _get_rules(self): rules = FPRule.active.filter(purpose=f"default_{self.purpose}") return rules - def _execute_rule_command(self, rule): + def _execute_rule_command(self, rule: FPRule) -> str: """Execute the FPR command of FPR rule ``rule`` against the file passed in to this client script. The output of that command determines what we print to stdout and stderr, and the nature of the validation event that @@ -296,7 +317,7 @@ def _execute_rule_command(self, rule): ) return result - def _get_command_to_execute(self, rule): + def _get_command_to_execute(self, rule: FPRule) -> Tuple[str, List[str]]: """Return a 2-tuple consisting of a) the FPR rule ``rule``'s command and b) a list of arguments to pass to it. """ @@ -313,14 +334,14 @@ def _get_command_to_execute(self, rule): else: return (rule.command.command, [self.file_path, self.policies_dir]) - def _save_to_logs_dir(self, output): + def _save_to_logs_dir(self, output: Dict[str, str]) -> None: """Save the MediaConch policy file as well as the raw MediaConch stdout for the target file to the logs/ directory of the SIP. """ self._save_stdout_to_logs_dir(output) self._save_policy_to_subm_doc_dir(output) - def _save_stdout_to_logs_dir(self, output): + def _save_stdout_to_logs_dir(self, output: Dict[str, str]) -> None: """Save the output of running MediaConch's policy checker against the input file to logs/policyChecks//.xml in the SIP. @@ -337,7 +358,7 @@ def _save_stdout_to_logs_dir(self, output): with open(stdout_path, "w") as f: f.write(mc_stdout) - def _save_policy_to_subm_doc_dir(self, output): + def _save_policy_to_subm_doc_dir(self, output: Dict[str, str]) -> None: """Save the policy file text in ``output['policy']`` to a file named ``output['policyFileName']`` in metadata/submissionDocumentation/policies/ in the SIP, if it is not @@ -355,7 +376,7 @@ def _save_policy_to_subm_doc_dir(self, output): fileo.write(policy) @property - def sip_logs_dir(self): + def sip_logs_dir(self) -> Optional[str]: """Return the absolute path the logs/ directory of the SIP (or Transfer) that the target file is a part of. """ @@ -398,7 +419,7 @@ def sip_logs_dir(self): return None @property - def sip_subm_doc_dir(self): + def sip_subm_doc_dir(self) -> Optional[str]: """Return the absolute path the metadata/submissionDocumentation/ directory of the SIP that the target file is a part of. """ @@ -428,7 +449,7 @@ def sip_subm_doc_dir(self): return None @property - def sip_policy_checks_dir(self): + def sip_policy_checks_dir(self) -> Optional[str]: if self._sip_policy_checks_dir: return self._sip_policy_checks_dir if self.sip_logs_dir: @@ -459,7 +480,7 @@ def _get_file_type(argv): return "original" -def call(jobs): +def call(jobs: List[Job]) -> None: with transaction.atomic(): for job in jobs: with job.JobContext(logger=logger): From 9f1a521e0c1926c3723ddc248fde2fe1f169280a Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Tue, 13 Aug 2024 14:12:39 -0600 Subject: [PATCH 3/7] Add type hints to policy check tests --- tests/MCPClient/test_policy_check.py | 227 ++++++++++++++------------- 1 file changed, 118 insertions(+), 109 deletions(-) diff --git a/tests/MCPClient/test_policy_check.py b/tests/MCPClient/test_policy_check.py index 9d8906f43c..320c5c0a46 100644 --- a/tests/MCPClient/test_policy_check.py +++ b/tests/MCPClient/test_policy_check.py @@ -6,11 +6,12 @@ import policy_check import pytest from client.job import Job +from fpr import models as fprmodels from main import models @pytest.fixture() -def sip(sip): +def sip(sip: models.SIP) -> models.SIP: # ReplacementDict expands SIP paths based on the shared directory. sip.currentpath = "%sharedPath%" sip.save() @@ -18,26 +19,30 @@ def sip(sip): @pytest.fixture() -def preservation_file(preservation_file): +def preservation_file(preservation_file: models.File) -> models.File: preservation_file.filegrpuse = "preservation" preservation_file.save() return preservation_file @pytest.fixture -def event(sip_file): +def event(sip_file: models.SIP) -> models.Event: return models.Event.objects.create(file_uuid=sip_file, event_type="normalization") @pytest.fixture -def derivation_for_preservation(sip_file, preservation_file, event): +def derivation_for_preservation( + sip_file: models.File, preservation_file: models.File, event: models.Event +) -> models.Derivation: return models.Derivation.objects.create( source_file=sip_file, derived_file=preservation_file, event=event ) @pytest.fixture -def preservation_file_format_version(preservation_file, format_version): +def preservation_file_format_version( + preservation_file: models.File, format_version: fprmodels.FormatVersion +) -> models.FileFormatVersion: return models.FileFormatVersion.objects.create( file_uuid=preservation_file, format_version=format_version ) @@ -56,14 +61,18 @@ def access_file(sip: models.SIP, transfer: models.Transfer) -> models.File: @pytest.fixture -def derivation_for_access(sip_file, access_file): +def derivation_for_access( + sip_file: models.File, access_file: models.File +) -> models.Derivation: return models.Derivation.objects.create( source_file=sip_file, derived_file=access_file ) @pytest.fixture -def access_file_format_version(access_file, format_version): +def access_file_format_version( + access_file: models.File, format_version: fprmodels.FormatVersion +) -> models.FileFormatVersion: return models.FileFormatVersion.objects.create( file_uuid=access_file, format_version=format_version ) @@ -72,15 +81,15 @@ def access_file_format_version(access_file, format_version): @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_check_if_rules_exist( - execute_or_run, - sip_file, - sip, - fprule_policy_check, - sip_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + sip_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + sip_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -131,14 +140,14 @@ def test_policy_check_if_rules_exist( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_checker_check_if_no_rules_exist( - execute_or_run, - sip_file, - sip, - sip_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + sip_file: models.File, + sip: models.SIP, + sip_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -171,14 +180,14 @@ def test_policy_checker_check_if_no_rules_exist( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_checker_check_if_file_does_not_exist( - execute_or_run, - sip_file, - sip, - sip_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + sip_file: models.File, + sip: models.SIP, + sip_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -211,15 +220,15 @@ def test_policy_checker_check_if_file_does_not_exist( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_checker_execute_rule_command_returns_failed( - execute_or_run, - sip_file, - sip, - fprule_policy_check, - sip_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + sip_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + sip_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -255,15 +264,15 @@ def test_policy_checker_execute_rule_command_returns_failed( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_checker_execute_rule_command_returns_failed_if_event_outcome_information_other_than_pass( - execute_or_run, - sip_file, - sip, - fprule_policy_check, - sip_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + sip_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + sip_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -292,11 +301,11 @@ def test_policy_checker_execute_rule_command_returns_failed_if_event_outcome_inf @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_check_verifies_file_type_is_preservation( - execute_or_run, - derivation_for_preservation, - preservation_file, - sip, - fprule_policy_check, + execute_or_run: mock.Mock, + derivation_for_preservation: models.Derivation, + preservation_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, preservation_file_format_version, format, format_version, @@ -339,15 +348,15 @@ def test_policy_check_verifies_file_type_is_preservation( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_check_fails_if_file_is_not_preservation_derivative( - execute_or_run, - preservation_file, - sip, - fprule_policy_check, - preservation_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + preservation_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + preservation_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -381,16 +390,16 @@ def test_policy_check_fails_if_file_is_not_preservation_derivative( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_check_verifies_file_type_is_access( - execute_or_run, - derivation_for_access, - access_file, - sip, - fprule_policy_check, - access_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + derivation_for_access: models.Derivation, + access_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + access_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -428,15 +437,15 @@ def test_policy_check_verifies_file_type_is_access( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_check_fails_if_file_is_not_access_derivative( - execute_or_run, - access_file, - sip, - fprule_policy_check, - access_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + access_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + access_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -472,15 +481,15 @@ def test_policy_check_fails_if_file_is_not_access_derivative( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_checker_saves_policy_check_result_into_logs_directory( - execute_or_run, - sip_file, - sip, - fprule_policy_check, - sip_file_format_version, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + sip_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + sip_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: pathlib.Path, +) -> None: log_directory = shared_directory_path / "logs" log_directory.mkdir() job = mock.Mock( @@ -514,14 +523,14 @@ def test_policy_checker_saves_policy_check_result_into_logs_directory( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") def test_policy_checker_saves_policy_check_result_into_submission_documentation_directory( - execute_or_run, - sip_file, - sip, - fprule_policy_check, - sip_file_format_version, - format, - format_version, - shared_directory_path, + execute_or_run: mock.Mock, + sip_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + sip_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: pathlib.Path, ): submission_documentation_directory = ( shared_directory_path / "metadata" / "submissionDocumentation" @@ -565,15 +574,15 @@ def test_policy_checker_saves_policy_check_result_into_submission_documentation_ ), ) def test_policy_checker_checks_manually_normalized_access_derivative_file( - execute_or_run, - transfer, - sip_file, - sip, - fprule_policy_check, - format, - format_version, - shared_directory_path, -): + execute_or_run: mock.Mock, + transfer: models.Transfer, + sip_file: models.File, + sip: models.SIP, + fprule_policy_check: fprmodels.FPRule, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: sip_file_name = pathlib.Path(sip_file.currentlocation.decode()).name manually_access_derivative_file = models.File.objects.create( transfer=transfer, From d56425d020389ded7015488b022ac3b2f792f254 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Tue, 13 Aug 2024 15:10:02 -0600 Subject: [PATCH 4/7] Add policy check modules into mypy.ini --- mypy.ini | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mypy.ini b/mypy.ini index fbeebf5d56..b23454ab2a 100644 --- a/mypy.ini +++ b/mypy.ini @@ -4,7 +4,8 @@ explicit_package_bases = True warn_redundant_casts = True warn_unused_configs = True -[mypy-src.MCPClient.lib.client.*,src.MCPClient.*.normalize,src.MCPClient.*.validate_file] + +[mypy-src.MCPClient.lib.client.*,src.MCPClient.*.normalize,src.MCPClient.*.validate_file, src.MCPClient.*.policy_check] check_untyped_defs = True disallow_any_generics = True disallow_incomplete_defs = True @@ -18,7 +19,8 @@ strict_equality = True warn_return_any = True warn_unused_ignores = True -[mypy-tests.MCPClient.conftest,tests.MCPClient.test_normalize,tests.MCPClient.test_validate_file] + +[mypy-tests.MCPClient.conftest,tests.MCPClient.test_normalize,tests.MCPClient.test_validate_file,tests.MCPClient.test_policy_check] check_untyped_defs = True disallow_any_generics = True disallow_incomplete_defs = True From 00d91eeb9eb80552543801385ef6d50f738ebe1e Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Wed, 14 Aug 2024 10:38:49 -0600 Subject: [PATCH 5/7] Add type hints to policy_check script and its tests workflow.json always passes the shared path and file type arguments, so they can become required arguments. --- .../lib/clientScripts/policy_check.py | 20 +-- tests/MCPClient/test_policy_check.py | 155 ++++++++---------- 2 files changed, 71 insertions(+), 104 deletions(-) diff --git a/src/MCPClient/lib/clientScripts/policy_check.py b/src/MCPClient/lib/clientScripts/policy_check.py index be6da962fe..dfe2e4891d 100755 --- a/src/MCPClient/lib/clientScripts/policy_check.py +++ b/src/MCPClient/lib/clientScripts/policy_check.py @@ -164,7 +164,7 @@ def _we_check_this_type_of_file(self) -> bool: else: return True - def _file_is_derivative(self, for_access=False) -> bool: + def _file_is_derivative(self, for_access: bool = False) -> bool: """Return ``True`` if the target file is a derivative; ``False`` otherwise. """ @@ -466,20 +466,6 @@ def sip_policy_checks_dir(self) -> Optional[str]: return self._sip_policy_checks_dir -def _get_shared_path(argv): - try: - return argv[4] - except IndexError: - return None - - -def _get_file_type(argv): - try: - return argv[5] - except IndexError: - return "original" - - def call(jobs: List[Job]) -> None: with transaction.atomic(): for job in jobs: @@ -487,8 +473,8 @@ def call(jobs: List[Job]) -> None: file_path = job.args[1] file_uuid = job.args[2] sip_uuid = job.args[3] - shared_path = _get_shared_path(job.args) - file_type = _get_file_type(job.args) + shared_path = job.args[4] + file_type = job.args[5] try: job.set_status( diff --git a/tests/MCPClient/test_policy_check.py b/tests/MCPClient/test_policy_check.py index 320c5c0a46..7bf4a77bb1 100644 --- a/tests/MCPClient/test_policy_check.py +++ b/tests/MCPClient/test_policy_check.py @@ -1,6 +1,7 @@ import json import pathlib import uuid +from typing import Any from unittest import mock import policy_check @@ -78,9 +79,27 @@ def access_file_format_version( ) +STDOUT = json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} +) + + +@pytest.fixture +def EXECUTE_OR_RUN() -> Any: + with mock.patch( + "policy_check.executeOrRun", + return_value=( + 0, + STDOUT, + "", + ), + ): + yield + + @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") -def test_policy_check_if_rules_exist( +def test_policy_check_succeeds_if_rules_exist( execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, @@ -138,8 +157,15 @@ def test_policy_check_if_rules_exist( @pytest.mark.django_db -@mock.patch("policy_check.executeOrRun") -def test_policy_checker_check_if_no_rules_exist( +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 1, + STDOUT, + "", + ), +) +def test_policy_checker_warns_if_rules_do_not_exist( execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, @@ -160,13 +186,6 @@ def test_policy_checker_check_if_no_rules_exist( JobContext=mock.MagicMock(), spec=Job, ) - expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', - } - ) - execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) @@ -178,8 +197,15 @@ def test_policy_checker_check_if_no_rules_exist( @pytest.mark.django_db -@mock.patch("policy_check.executeOrRun") -def test_policy_checker_check_if_file_does_not_exist( +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 1, + STDOUT, + "", + ), +) +def test_policy_checker_warns_if_file_does_not_exist( execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, @@ -200,13 +226,6 @@ def test_policy_checker_check_if_file_does_not_exist( JobContext=mock.MagicMock(), spec=Job, ) - expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', - } - ) - execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) @@ -218,8 +237,15 @@ def test_policy_checker_check_if_file_does_not_exist( @pytest.mark.django_db -@mock.patch("policy_check.executeOrRun") -def test_policy_checker_execute_rule_command_returns_failed( +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 1, + STDOUT, + "", + ), +) +def test_policy_checker_fails_if_execute_rule_command_fails( execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, @@ -241,13 +267,6 @@ def test_policy_checker_execute_rule_command_returns_failed( JobContext=mock.MagicMock(), spec=Job, ) - expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', - } - ) - execute_or_run.return_value = (1, expected_stdout, "") policy_check.call([job]) @@ -262,8 +281,15 @@ def test_policy_checker_execute_rule_command_returns_failed( @pytest.mark.django_db -@mock.patch("policy_check.executeOrRun") -def test_policy_checker_execute_rule_command_returns_failed_if_event_outcome_information_other_than_pass( +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 1, + STDOUT, + "", + ), +) +def test_policy_checker_fails_if_event_outcome_information_in_output_is_not_pass( execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, @@ -285,13 +311,6 @@ def test_policy_checker_execute_rule_command_returns_failed_if_event_outcome_inf JobContext=mock.MagicMock(), spec=Job, ) - expected_stdout = json.dumps( - { - "eventOutcomeInformation": "", - "eventOutcomeDetailNote": "", - } - ) - execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) @@ -306,11 +325,11 @@ def test_policy_check_verifies_file_type_is_preservation( preservation_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, - preservation_file_format_version, - format, - format_version, - shared_directory_path, -): + preservation_file_format_version: models.FileFormatVersion, + format: fprmodels.Format, + format_version: fprmodels.FormatVersion, + shared_directory_path: str, +) -> None: job = mock.Mock( args=[ "policy_check", @@ -326,7 +345,7 @@ def test_policy_check_verifies_file_type_is_preservation( expected_stdout = json.dumps( { "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + "eventOutcomeDetailNote": "a note", } ) execute_or_run.return_value = (0, expected_stdout, "") @@ -346,9 +365,8 @@ def test_policy_check_verifies_file_type_is_preservation( @pytest.mark.django_db -@mock.patch("policy_check.executeOrRun") def test_policy_check_fails_if_file_is_not_preservation_derivative( - execute_or_run: mock.Mock, + EXECUTE_OR_RUN: mock.Mock, preservation_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, @@ -369,13 +387,6 @@ def test_policy_check_fails_if_file_is_not_preservation_derivative( JobContext=mock.MagicMock(), spec=Job, ) - expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', - } - ) - execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) @@ -415,7 +426,7 @@ def test_policy_check_verifies_file_type_is_access( expected_stdout = json.dumps( { "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": f'format="{format.description}"; version="{format_version.version}"; result="Well-Formed and valid"', + "eventOutcomeDetailNote": "a note", } ) execute_or_run.return_value = (0, expected_stdout, "") @@ -479,9 +490,8 @@ def test_policy_check_fails_if_file_is_not_access_derivative( @pytest.mark.django_db -@mock.patch("policy_check.executeOrRun") def test_policy_checker_saves_policy_check_result_into_logs_directory( - execute_or_run: mock.Mock, + EXECUTE_OR_RUN: mock.Mock, sip_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, @@ -504,16 +514,6 @@ def test_policy_checker_saves_policy_check_result_into_logs_directory( JobContext=mock.MagicMock(), spec=Job, ) - expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": "MediaConch policy check result against policy file MP3 has duration: All policy checks passed: Does the audio duration exist?; MP3 has duration", - "policy": '\n\n Rudimentary test to check for an MP3 having a duration value.\n mp3\n', - "policyFileName": "MP3 has duration", - "stdout": "foobar", - } - ) - execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) @@ -521,9 +521,8 @@ def test_policy_checker_saves_policy_check_result_into_logs_directory( @pytest.mark.django_db -@mock.patch("policy_check.executeOrRun") def test_policy_checker_saves_policy_check_result_into_submission_documentation_directory( - execute_or_run: mock.Mock, + EXECUTE_OR_RUN: mock.Mock, sip_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, @@ -531,7 +530,7 @@ def test_policy_checker_saves_policy_check_result_into_submission_documentation_ format: fprmodels.Format, format_version: fprmodels.FormatVersion, shared_directory_path: pathlib.Path, -): +) -> None: submission_documentation_directory = ( shared_directory_path / "metadata" / "submissionDocumentation" ) @@ -548,16 +547,6 @@ def test_policy_checker_saves_policy_check_result_into_submission_documentation_ JobContext=mock.MagicMock(), spec=Job, ) - expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": "MediaConch policy check result against policy file MP3 has duration: All policy checks passed: Does the audio duration exist?; MP3 has duration", - "policy": '\n\n Rudimentary test to check for an MP3 having a duration value.\n mp3\n', - "policyFileName": "MP3 has duration", - "stdout": "foobar", - } - ) - execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) @@ -565,16 +554,8 @@ def test_policy_checker_saves_policy_check_result_into_submission_documentation_ @pytest.mark.django_db -@mock.patch( - "policy_check.executeOrRun", - return_value=( - 0, - json.dumps({"eventOutcomeInformation": "pass"}), - "", - ), -) def test_policy_checker_checks_manually_normalized_access_derivative_file( - execute_or_run: mock.Mock, + EXECUTE_OR_RUN: mock.Mock, transfer: models.Transfer, sip_file: models.File, sip: models.SIP, From e802f991c65ebee2e51886a7c1c502e8492b3fc8 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Thu, 15 Aug 2024 12:46:40 -0600 Subject: [PATCH 6/7] Add suggested changes --- .../lib/clientScripts/policy_check.py | 8 +- tests/MCPClient/test_policy_check.py | 169 ++++++++++-------- 2 files changed, 98 insertions(+), 79 deletions(-) diff --git a/src/MCPClient/lib/clientScripts/policy_check.py b/src/MCPClient/lib/clientScripts/policy_check.py index dfe2e4891d..b3a6374276 100755 --- a/src/MCPClient/lib/clientScripts/policy_check.py +++ b/src/MCPClient/lib/clientScripts/policy_check.py @@ -78,7 +78,7 @@ def __init__( self, job: Job, file_path: str, - file_uuid: str, + file_uuid: Optional[str], sip_uuid: str, shared_path: str, file_type: str, @@ -172,9 +172,7 @@ def _file_is_derivative(self, for_access: bool = False) -> bool: return True # Access derivatives have Derivation rows with NULL event types (cf. # normalize.py client script). - event_type: Optional[str] = "normalization" - if for_access: - event_type = None + event_type = None if for_access else "normalization" try: Derivation.objects.get( derived_file__uuid=self.file_uuid, event__event_type=event_type @@ -210,7 +208,7 @@ def _file_is_for_access(self) -> bool: return True return False - def _get_manually_normalized_access_derivative_file_uuid(self) -> File: + def _get_manually_normalized_access_derivative_file_uuid(self) -> Optional[File]: """If the file-to-be-policy-checked is a manually normalized access derivative it will have no file UUID in the database. We therefore have to retrieve the UUID of the original file that was format-identified, diff --git a/tests/MCPClient/test_policy_check.py b/tests/MCPClient/test_policy_check.py index 7bf4a77bb1..e37a1d6847 100644 --- a/tests/MCPClient/test_policy_check.py +++ b/tests/MCPClient/test_policy_check.py @@ -1,7 +1,6 @@ import json import pathlib import uuid -from typing import Any from unittest import mock import policy_check @@ -79,27 +78,9 @@ def access_file_format_version( ) -STDOUT = json.dumps( - {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} -) - - -@pytest.fixture -def EXECUTE_OR_RUN() -> Any: - with mock.patch( - "policy_check.executeOrRun", - return_value=( - 0, - STDOUT, - "", - ), - ): - yield - - @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") -def test_policy_check_succeeds_if_rules_exist( +def test_policy_checker_succeeds_if_rules_exist( execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, @@ -107,7 +88,7 @@ def test_policy_check_succeeds_if_rules_exist( sip_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -131,7 +112,7 @@ def test_policy_check_succeeds_if_rules_exist( policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.SUCCESS_CODE) assert ( models.Event.objects.filter( file_uuid_id=sip_file.uuid, @@ -161,7 +142,9 @@ def test_policy_check_succeeds_if_rules_exist( "policy_check.executeOrRun", return_value=( 1, - STDOUT, + json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ), "", ), ) @@ -189,7 +172,7 @@ def test_policy_checker_warns_if_rules_do_not_exist( policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.NOT_APPLICABLE_CODE) job.pyprint.assert_called_once_with( "Not performing a policy check because there are no relevant FPR rules" @@ -201,7 +184,9 @@ def test_policy_checker_warns_if_rules_do_not_exist( "policy_check.executeOrRun", return_value=( 1, - STDOUT, + json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ), "", ), ) @@ -212,13 +197,14 @@ def test_policy_checker_warns_if_file_does_not_exist( sip_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: + file_uuid = str(uuid.uuid4()) job = mock.Mock( args=[ "policy_check", "", - "", + file_uuid, str(sip.uuid), str(shared_directory_path), sip_file.filegrpuse, @@ -229,23 +215,16 @@ def test_policy_checker_warns_if_file_does_not_exist( policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.NOT_APPLICABLE_CODE) job.pyprint.assert_called_once_with( - "Not performing a policy check because there is no file with UUID ." + f"Not performing a policy check because there is no file with UUID {file_uuid}." ) @pytest.mark.django_db -@mock.patch( - "policy_check.executeOrRun", - return_value=( - 1, - STDOUT, - "", - ), -) -def test_policy_checker_fails_if_execute_rule_command_fails( +@mock.patch("policy_check.executeOrRun") +def test_policy_checker_fails_if_rule_command_fails( execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, @@ -253,7 +232,7 @@ def test_policy_checker_fails_if_execute_rule_command_fails( sip_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -267,16 +246,22 @@ def test_policy_checker_fails_if_execute_rule_command_fails( JobContext=mock.MagicMock(), spec=Job, ) + status = 1 + expected_stdout = json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ) + stderr = "" + execute_or_run.return_value = (status, expected_stdout, stderr) policy_check.call([job]) - job.set_status.assert_called_once_with(1) + job.set_status.assert_called_once_with(policy_check.FAIL_CODE) assert job.pyprint.mock_calls == [ mock.call("Running", fprule_policy_check.command.description) ] job.print_error.assert_called_once_with( - f"Command {fprule_policy_check.command.description} failed with exit status 1; stderr:", - "", + f"Command {fprule_policy_check.command.description} failed with exit status {status}; stderr:", + stderr, ) @@ -285,7 +270,9 @@ def test_policy_checker_fails_if_execute_rule_command_fails( "policy_check.executeOrRun", return_value=( 1, - STDOUT, + json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ), "", ), ) @@ -297,7 +284,7 @@ def test_policy_checker_fails_if_event_outcome_information_in_output_is_not_pass sip_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -314,12 +301,12 @@ def test_policy_checker_fails_if_event_outcome_information_in_output_is_not_pass policy_check.call([job]) - job.set_status.assert_called_once_with(1) + job.set_status.assert_called_once_with(policy_check.FAIL_CODE) @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") -def test_policy_check_verifies_file_type_is_preservation( +def test_policy_checker_verifies_file_type_is_preservation( execute_or_run: mock.Mock, derivation_for_preservation: models.Derivation, preservation_file: models.File, @@ -328,7 +315,7 @@ def test_policy_check_verifies_file_type_is_preservation( preservation_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -343,16 +330,13 @@ def test_policy_check_verifies_file_type_is_preservation( spec=Job, ) expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": "a note", - } + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} ) execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.SUCCESS_CODE) assert job.pyprint.mock_calls == [ mock.call("Running", ""), mock.call( @@ -365,15 +349,25 @@ def test_policy_check_verifies_file_type_is_preservation( @pytest.mark.django_db -def test_policy_check_fails_if_file_is_not_preservation_derivative( - EXECUTE_OR_RUN: mock.Mock, +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 0, + json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ), + "", + ), +) +def test_policy_checker_fails_if_file_is_not_preservation_derivative( + execute_or_run: mock.Mock, preservation_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, preservation_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -390,7 +384,7 @@ def test_policy_check_fails_if_file_is_not_preservation_derivative( policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.NOT_APPLICABLE_CODE) assert job.pyprint.mock_calls == [ mock.call( f"File {preservation_file.uuid} is not a preservation derivative; not performing a policy check." @@ -400,7 +394,7 @@ def test_policy_check_fails_if_file_is_not_preservation_derivative( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") -def test_policy_check_verifies_file_type_is_access( +def test_policy_checker_verifies_file_type_is_access( execute_or_run: mock.Mock, derivation_for_access: models.Derivation, access_file: models.File, @@ -409,7 +403,7 @@ def test_policy_check_verifies_file_type_is_access( access_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -424,16 +418,13 @@ def test_policy_check_verifies_file_type_is_access( spec=Job, ) expected_stdout = json.dumps( - { - "eventOutcomeInformation": "pass", - "eventOutcomeDetailNote": "a note", - } + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} ) execute_or_run.return_value = (0, expected_stdout, "") policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.SUCCESS_CODE) assert job.pyprint.mock_calls == [ mock.call("Running", ""), mock.call( @@ -447,7 +438,7 @@ def test_policy_check_verifies_file_type_is_access( @pytest.mark.django_db @mock.patch("policy_check.executeOrRun") -def test_policy_check_fails_if_file_is_not_access_derivative( +def test_policy_checker_fails_if_file_is_not_access_derivative( execute_or_run: mock.Mock, access_file: models.File, sip: models.SIP, @@ -455,7 +446,7 @@ def test_policy_check_fails_if_file_is_not_access_derivative( access_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -464,7 +455,7 @@ def test_policy_check_fails_if_file_is_not_access_derivative( str(access_file.uuid), str(sip.uuid), str(shared_directory_path), - "access", + access_file.filegrpuse, ], JobContext=mock.MagicMock(), spec=Job, @@ -479,7 +470,7 @@ def test_policy_check_fails_if_file_is_not_access_derivative( policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.NOT_APPLICABLE_CODE) assert job.pyprint.mock_calls == [ mock.call( @@ -490,8 +481,18 @@ def test_policy_check_fails_if_file_is_not_access_derivative( @pytest.mark.django_db +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 0, + json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ), + "", + ), +) def test_policy_checker_saves_policy_check_result_into_logs_directory( - EXECUTE_OR_RUN: mock.Mock, + execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, @@ -517,12 +518,22 @@ def test_policy_checker_saves_policy_check_result_into_logs_directory( policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.SUCCESS_CODE) @pytest.mark.django_db +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 0, + json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ), + "", + ), +) def test_policy_checker_saves_policy_check_result_into_submission_documentation_directory( - EXECUTE_OR_RUN: mock.Mock, + execute_or_run: mock.Mock, sip_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, @@ -550,19 +561,29 @@ def test_policy_checker_saves_policy_check_result_into_submission_documentation_ policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.SUCCESS_CODE) @pytest.mark.django_db +@mock.patch( + "policy_check.executeOrRun", + return_value=( + 0, + json.dumps( + {"eventOutcomeInformation": "pass", "eventOutcomeDetailNote": "a note"} + ), + "", + ), +) def test_policy_checker_checks_manually_normalized_access_derivative_file( - EXECUTE_OR_RUN: mock.Mock, + execute_or_run: mock.Mock, transfer: models.Transfer, sip_file: models.File, sip: models.SIP, fprule_policy_check: fprmodels.FPRule, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: sip_file_name = pathlib.Path(sip_file.currentlocation.decode()).name manually_access_derivative_file = models.File.objects.create( @@ -589,4 +610,4 @@ def test_policy_checker_checks_manually_normalized_access_derivative_file( policy_check.call([job]) - job.set_status.assert_called_once_with(0) + job.set_status.assert_called_once_with(policy_check.SUCCESS_CODE) From e29bda3b4e8c96df7b3ceb36d9678624d9c2ad76 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Thu, 15 Aug 2024 14:23:42 -0600 Subject: [PATCH 7/7] Add some more changes as suggested --- tests/MCPClient/test_policy_check.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/MCPClient/test_policy_check.py b/tests/MCPClient/test_policy_check.py index e37a1d6847..94dd9bdee6 100644 --- a/tests/MCPClient/test_policy_check.py +++ b/tests/MCPClient/test_policy_check.py @@ -155,7 +155,7 @@ def test_policy_checker_warns_if_rules_do_not_exist( sip_file_format_version: models.FileFormatVersion, format: fprmodels.Format, format_version: fprmodels.FormatVersion, - shared_directory_path: str, + shared_directory_path: pathlib.Path, ) -> None: job = mock.Mock( args=[ @@ -412,7 +412,7 @@ def test_policy_checker_verifies_file_type_is_access( str(access_file.uuid), str(sip.uuid), str(shared_directory_path), - "access", + access_file.filegrpuse, ], JobContext=mock.MagicMock(), spec=Job,