Merge pull request #86 from wshlavacek/modelapi-centralize-action-validation

Centralize action-argument validation in modelapi

Author: jrfaeder

bionetgen/core/utils/utils.py

@@ -2,7 +2,7 @@
 import shutil
 import subprocess
 
-from bionetgen.core.exc import BNGPerlError
+from bionetgen.core.exc import BNGParseError, BNGPerlError
 from bionetgen.core.utils.logging import BNGLogger
 
 
@@ -460,11 +460,101 @@ def __init__(self):
         self.irregular_args["blocks"] = "list"
         self.irregular_args["opts"] = "list"
 
+        # Expected positional arity (min, max) for actions whose arguments
+        # are positional rather than `name=>value` keyword pairs. `max=None`
+        # means unbounded. Actions absent from this table are treated as
+        # variable-arity (`(0, None)`).
+        self.positional_arity = {
+            # no_setter_syntax
+            "quit": (0, 0),
+            "setModelName": (1, 1),
+            "substanceUnits": (0, 1),
+            "version": (0, 1),
+            "setOption": (2, 2),
+            "setConcentration": (2, 2),
+            "addConcentration": (2, 2),
+            "setParameter": (2, 2),
+            # square_braces — list of zero or more entries
+            "saveConcentrations": (0, None),
+            "resetConcentrations": (0, None),
+            "saveParameters": (0, None),
+            "resetParameters": (0, None),
+        }
+
     def is_before_model(self, action_name):
         if action_name in self.before_model:
             return True
         return False
 
+    def validate_action(self, action_type, action_args):
+        """
+        Centralized schema check shared by parse-time construction
+        (BNGParser) and direct construction (Action.__init__). Raises
+        BNGParseError on any inconsistency.
+
+        Positional actions (no_setter_syntax / square_braces) store their
+        arguments as a dict whose keys are the literal positional values
+        and whose values are None — this canonical shape matches what the
+        parser emits and is what gen_string serializes back out.
+        """
+        if action_type not in self.possible_types:
+            raise BNGParseError(message=f"Action type {action_type} not recognized!")
+
+        if not isinstance(action_args, dict):
+            raise BNGParseError(
+                message=(
+                    f"Action {action_type} arguments must be a dict, "
+                    f"got {type(action_args).__name__}"
+                )
+            )
+
+        if action_type in self.normal_types:
+            valid = self.arg_dict.get(action_type)
+            if valid is None:
+                if len(action_args) > 0:
+                    raise BNGParseError(
+                        message=(f"Action {action_type} does not take arguments")
+                    )
+                return
+            if len(valid) > 0:
+                for arg_name in action_args:
+                    if arg_name not in valid:
+                        raise BNGParseError(
+                            message=(
+                                f"Action argument {arg_name} not recognized "
+                                f"for action {action_type}!"
+                            )
+                        )
+            return
+
+        # Positional path covers no_setter_syntax and square_braces.
+        for arg_name, arg_value in action_args.items():
+            if arg_value is not None:
+                raise BNGParseError(
+                    message=(
+                        f"Action {action_type} is positional; pass each "
+                        f"value as a dict key mapped to None (got "
+                        f"{arg_name!r}={arg_value!r}). For example: "
+                        f"{{'\"A()\"': None, '100': None}}."
+                    )
+                )
+
+        mn, mx = self.positional_arity.get(action_type, (0, None))
+        n = len(action_args)
+        if n < mn or (mx is not None and n > mx):
+            if mn == mx:
+                expected = f"exactly {mn}"
+            elif mx is None:
+                expected = f"at least {mn}"
+            else:
+                expected = f"between {mn} and {mx}"
+            raise BNGParseError(
+                message=(
+                    f"Action {action_type} expects {expected} positional "
+                    f"argument(s); got {n}."
+                )
+            )
+
     def define_parser(self):
         ## Define action grammar
         import pyparsing as pp

bionetgen/modelapi/structs.py

@@ -302,36 +302,21 @@ class Action(ModelObj):
         action arguments as keys and their values as values
     """
 
-    def __init__(self, action_type=None, action_args={}) -> None:
+    def __init__(self, action_type=None, action_args=None) -> None:
         super().__init__()
+        if action_args is None:
+            action_args = {}
         AList = ActionList()
         self.normal_types = AList.normal_types
         self.no_setter_syntax = AList.no_setter_syntax
         self.square_braces = AList.square_braces
         self.possible_types = AList.possible_types
-        # Set initial values
         self.name = action_type
         self.type = action_type
         self.args = action_args
-        # check type
-        if self.type not in self.possible_types:
-            raise BNGParseError(message=f"Action type {self.type} not recognized!")
+        AList.validate_action(action_type, action_args)
         seen_args = []
-        for arg in action_args:
-            arg_name, arg_value = arg, action_args[arg]
-            valid_arg_list = AList.arg_dict[self.type]
-            # TODO: actions that don't take argument names should be parsed separately to check validity of arg-val tuples
-            # TODO: currently not type checking arguments
-            if valid_arg_list is None:
-                raise BNGParseError(
-                    message=f"Argument {arg_name} is given, but action {self.type} does not take arguments"
-                )
-            if len(valid_arg_list) > 0:
-                if arg_name not in AList.arg_dict[self.type]:
-                    raise BNGParseError(
-                        message=f"Action argument {arg_name} not recognized!\nCheck to make sure action is correctly formatted"
-                    )
-                # TODO: If arg_value is the correct type
+        for arg_name, arg_value in action_args.items():
             if arg_name in seen_args:
                 print(
                     f"Warning: argument {arg_name} already given, using latter value {arg_value}"