eli-579 safer handling of non-numeric values if passed to ADD_DAYS

Author: eddalmond1

src/eligibility_signposting_api/services/processors/derived_values/add_days_handler.py

@@ -118,12 +118,13 @@ def _get_days_to_add(self, context: DerivedValueContext) -> int:
         Returns:
             Number of days to add
         """
-        # Priority 1: Token argument
+        # Priority 1: Token argument (if non-empty)
         if context.function_args:
             try:
                 return int(context.function_args)
-            except ValueError:
-                pass
+            except ValueError as e:
+                message = f"Invalid days argument '{context.function_args}' for ADD_DAYS function. Expected an integer."
+                raise ValueError(message) from e
 
         # Priority 2: Vaccine-specific configuration
         if context.attribute_name in self.vaccine_type_days:

tests/unit/services/processors/test_derived_values.py

@@ -1,6 +1,6 @@
 from unittest.mock import MagicMock
 
-import pytest
+from hamcrest import assert_that, calling, equal_to, is_, raises, same_instance
 
 from eligibility_signposting_api.services.processors.derived_values import (
     AddDaysHandler,
@@ -26,7 +26,7 @@ def test_calculate_adds_default_days_to_date(self):
         result = handler.calculate(context)
 
         # 2025-01-01 + 91 days = 2025-04-02
-        assert result == "20250402"
+        assert_that(result, is_(equal_to("20250402")))
 
     def test_calculate_with_function_args_override(self):
         """Test that function args override default days."""
@@ -42,7 +42,7 @@ def test_calculate_with_function_args_override(self):
         result = handler.calculate(context)
 
         # 2025-01-01 + 30 days = 2025-01-31
-        assert result == "20250131"
+        assert_that(result, is_(equal_to("20250131")))
 
     def test_calculate_with_vaccine_specific_days(self):
         """Test that vaccine-specific days are used when configured."""
@@ -61,7 +61,7 @@ def test_calculate_with_vaccine_specific_days(self):
         result = handler.calculate(context)
 
         # 2025-01-01 + 365 days = 2026-01-01
-        assert result == "20260101"
+        assert_that(result, is_(equal_to("20260101")))
 
     def test_calculate_with_date_format(self):
         """Test that date format is applied to output."""
@@ -76,7 +76,7 @@ def test_calculate_with_date_format(self):
 
         result = handler.calculate(context)
 
-        assert result == "02 April 2025"
+        assert_that(result, is_(equal_to("02 April 2025")))
 
     def test_calculate_returns_empty_when_source_not_found(self):
         """Test that empty string is returned when source date not found."""
@@ -91,7 +91,7 @@ def test_calculate_returns_empty_when_source_not_found(self):
 
         result = handler.calculate(context)
 
-        assert result == ""
+        assert_that(result, is_(equal_to("")))
 
     def test_calculate_returns_empty_when_vaccine_not_found(self):
         """Test that empty string is returned when vaccine type not found."""
@@ -106,7 +106,7 @@ def test_calculate_returns_empty_when_vaccine_not_found(self):
 
         result = handler.calculate(context)
 
-        assert result == ""
+        assert_that(result, is_(equal_to("")))
 
     def test_calculate_with_invalid_date_raises_error(self):
         """Test that invalid date format raises ValueError."""
@@ -119,20 +119,38 @@ def test_calculate_with_invalid_date_raises_error(self):
             date_format=None,
         )
 
-        with pytest.raises(ValueError, match="Invalid date format"):
-            handler.calculate(context)
+        assert_that(
+            calling(handler.calculate).with_args(context),
+            raises(ValueError, pattern="Invalid date format"),
+        )
+
+    def test_calculate_with_invalid_function_args_raises_error(self):
+        """Test that non-integer function args raises ValueError."""
+        handler = AddDaysHandler(default_days=91)
+        context = DerivedValueContext(
+            person_data=[{"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20250101"}],
+            attribute_name="COVID",
+            source_attribute="LAST_SUCCESSFUL_DATE",
+            function_args="abc",  # Invalid: not an integer
+            date_format=None,
+        )
+
+        assert_that(
+            calling(handler.calculate).with_args(context),
+            raises(ValueError, pattern="Invalid days argument 'abc' for ADD_DAYS function"),
+        )
 
     def test_get_source_attribute_maps_derived_to_source(self):
         """Test that get_source_attribute maps derived attributes correctly."""
         handler = AddDaysHandler()
 
-        assert handler.get_source_attribute("NEXT_DOSE_DUE") == "LAST_SUCCESSFUL_DATE"
+        assert_that(handler.get_source_attribute("NEXT_DOSE_DUE"), is_(equal_to("LAST_SUCCESSFUL_DATE")))
 
     def test_get_source_attribute_returns_original_if_not_mapped(self):
         """Test that unmapped attributes return themselves."""
         handler = AddDaysHandler()
 
-        assert handler.get_source_attribute("UNKNOWN_ATTR") == "UNKNOWN_ATTR"
+        assert_that(handler.get_source_attribute("UNKNOWN_ATTR"), is_(equal_to("UNKNOWN_ATTR")))
 
     def test_function_args_priority_over_vaccine_config(self):
         """Test that function args take priority over vaccine-specific config."""
@@ -151,7 +169,7 @@ def test_function_args_priority_over_vaccine_config(self):
         result = handler.calculate(context)
 
         # 2025-01-01 + 30 days = 2025-01-31
-        assert result == "20250131"
+        assert_that(result, is_(equal_to("20250131")))
 
 
 class TestDerivedValueRegistry:
@@ -165,29 +183,29 @@ def test_register_and_get_handler(self):
 
         retrieved = registry.get_handler("ADD_DAYS")
 
-        assert retrieved is handler
+        assert_that(retrieved, same_instance(handler))  # type: ignore[call-overload]
 
     def test_get_handler_case_insensitive(self):
         """Test that handler lookup is case insensitive."""
         registry = DerivedValueRegistry()
         handler = AddDaysHandler()
         registry.register(handler)
 
-        assert registry.get_handler("add_days") is handler
-        assert registry.get_handler("Add_Days") is handler
+        assert_that(registry.get_handler("add_days"), same_instance(handler))  # type: ignore[call-overload]
+        assert_that(registry.get_handler("Add_Days"), same_instance(handler))  # type: ignore[call-overload]
 
     def test_has_handler_returns_true_when_exists(self):
         """Test has_handler returns True for registered handlers."""
         registry = DerivedValueRegistry()
         registry.register(AddDaysHandler())
 
-        assert registry.has_handler("ADD_DAYS") is True
+        assert_that(registry.has_handler("ADD_DAYS"), is_(True))
 
     def test_has_handler_returns_false_when_not_exists(self):
         """Test has_handler returns False for unregistered handlers."""
         registry = DerivedValueRegistry()
 
-        assert registry.has_handler("UNKNOWN") is False
+        assert_that(registry.has_handler("UNKNOWN"), is_(False))
 
     def test_calculate_delegates_to_correct_handler(self):
         """Test that calculate delegates to the correct handler."""
@@ -215,7 +233,7 @@ def test_calculate_delegates_to_correct_handler(self):
 
         # Verify the mock handler was called with the context
         mock_handler.calculate.assert_called_once_with(context)
-        assert result == "mock_result"
+        assert_that(result, is_(equal_to("mock_result")))
 
     def test_calculate_raises_for_unknown_function(self):
         """Test that calculate raises ValueError for unknown functions."""
@@ -229,32 +247,31 @@ def test_calculate_raises_for_unknown_function(self):
             date_format=None,
         )
 
-        with pytest.raises(ValueError, match="No handler registered"):
-            registry.calculate(
-                function_name="UNKNOWN",
-                context=context,
-            )
+        assert_that(
+            calling(registry.calculate).with_args(function_name="UNKNOWN", context=context),
+            raises(ValueError, pattern="No handler registered"),
+        )
 
     def test_is_derived_attribute_returns_true_for_derived(self):
         """Test is_derived_attribute for known derived attributes."""
         registry = DerivedValueRegistry()
         registry.register(AddDaysHandler())
 
-        assert registry.is_derived_attribute("NEXT_DOSE_DUE") is True
+        assert_that(registry.is_derived_attribute("NEXT_DOSE_DUE"), is_(True))
 
     def test_is_derived_attribute_returns_false_for_non_derived(self):
         """Test is_derived_attribute for non-derived attributes."""
         registry = DerivedValueRegistry()
         registry.register(AddDaysHandler())
 
-        assert registry.is_derived_attribute("LAST_SUCCESSFUL_DATE") is False
+        assert_that(registry.is_derived_attribute("LAST_SUCCESSFUL_DATE"), is_(False))
 
     def test_default_handlers_are_registered(self):
         """Test that default handlers from the module are registered."""
         registry = DerivedValueRegistry()
 
         # The default ADD_DAYS handler should be registered via __init__.py
-        assert registry.has_handler("ADD_DAYS")
+        assert_that(registry.has_handler("ADD_DAYS"), is_(True))
 
     def test_clear_defaults_removes_default_handlers(self):
         """Test that clear_defaults removes all default handlers."""
@@ -266,7 +283,7 @@ def test_clear_defaults_removes_default_handlers(self):
 
             # New registry should have no handlers
             registry = DerivedValueRegistry()
-            assert not registry.has_handler("ADD_DAYS")
+            assert_that(registry.has_handler("ADD_DAYS"), is_(False))
         finally:
             # Restore defaults for other tests using public method
             DerivedValueRegistry.set_default_handlers(saved_defaults)