From d067d91b9dbc2de33246f890a68b831034a1599c Mon Sep 17 00:00:00 2001
From: Alp Kose <alperenkose@gmail.com>
Date: Thu, 10 Oct 2024 13:41:34 +0300
Subject: [PATCH 1/2] fix(panos_static_route): invalid nexthop for none value

This change also requires an update on pan-os-python to accept 'none'
value for the nexthop type.
---
 plugins/modules/panos_static_route.py | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/plugins/modules/panos_static_route.py b/plugins/modules/panos_static_route.py
index e329f99ff..0ff0b2ccd 100644
--- a/plugins/modules/panos_static_route.py
+++ b/plugins/modules/panos_static_route.py
@@ -157,6 +157,23 @@
 
 class Helper(ConnectionHelper):
     def spec_handling(self, spec, module):
+        if module.params["state"] == "present" and spec["nexthop_type"] is None:
+            # need this because we dont have the default assignment in sdk-params and
+            # `None` value params are being removed in ParamPath.element method (called via VersionedPanObject.element)
+            spec["nexthop_type"] = "ip-address"
+
+        # default to ip-address when nexthop is set in merged state
+        # we dont know if object exists or not in merged state, and we dont set default values in module invocation
+        # in order to avoid unintended updates to non-provided params, but if nexthop is given, type must be ip-address
+        if module.params["state"] == "merged" and spec["nexthop_type"] is None and spec["nexthop"] is not None:
+            spec["nexthop_type"] = "ip-address"
+
+        # NOTE merged state have a lot of API issues for updating nexthop we will let the API return it..
+        # from None to IP address - "Failed update nexthop_type: Edit breaks config validity"
+        # from IP address to next-vr - "Failed update nexthop_type: Edit breaks config validity"
+
+        # applies for updating existing routes from IP/next-vr/discard to none
+        # however it works for new objects, we ignore this as this is the existing implementation
         if module.params["state"] == "merged" and spec["nexthop_type"] == "none":
             msg = [
                 "Nexthop cannot be set to None with state='merged'.",
@@ -164,8 +181,6 @@ def spec_handling(self, spec, module):
             ]
             module.fail_json(msg=" ".join(msg))
 
-        if spec["nexthop_type"] == "none":
-            spec["nexthop_type"] = None
 
 
 def main():
@@ -182,7 +197,6 @@ def main():
             name=dict(required=True),
             destination=dict(),
             nexthop_type=dict(
-                default="ip-address",
                 choices=["ip-address", "discard", "none", "next-vr"],
             ),
             nexthop=dict(),
@@ -193,6 +207,9 @@ def main():
             failure_condition=dict(choices=["any", "all"]),
             preemptive_hold_time=dict(type="int"),
         ),
+        default_values=dict(
+            nexthop_type="ip-address",
+        ),
     )
 
     module = AnsibleModule(

From 253b44d804e36d08c6a7067d5f65e6a674d4abdc Mon Sep 17 00:00:00 2001
From: Alp Kose <alperenkose@gmail.com>
Date: Tue, 5 Nov 2024 12:23:27 +0300
Subject: [PATCH 2/2] fix: object_handling method for overriding defaults

Allows overriding values for newly created/replaced objects
for mitigating issues on passing `None` value to sdk for params
with default values.

With this change pan-os-python does not need to accept 'none' value
for the nexthop type in panos_static_route.
---
 plugins/module_utils/panos.py         | 31 ++++++++++++++++-----------
 plugins/modules/panos_static_route.py | 13 ++++++++---
 plugins/modules/panos_template.py     |  8 +++++++
 3 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/plugins/module_utils/panos.py b/plugins/module_utils/panos.py
index 8bce488b0..c3b0d231c 100644
--- a/plugins/module_utils/panos.py
+++ b/plugins/module_utils/panos.py
@@ -555,6 +555,20 @@ def spec_handling(self, spec, module):
         """
         pass
 
+    def object_handling(self, obj, module):
+        """Override to provide custom functionality for newly created/replaced objects.
+
+        This method is run for newly created objects with merged state or
+        created/replaced objects with present state.
+
+        By default it will handle default values for objects.
+        It's advised to call `super().object_handling(obj, module)` if overriden
+        in the modules.
+        """
+        for key, obj_value in obj.about().items():
+            if obj_value is None:
+                setattr(obj, key, self._get_default_value(obj, key))
+
     def pre_state_handling(self, obj, result, module):
         """Override to provide custom pre-state handling functionality."""
         pass
@@ -694,6 +708,8 @@ def apply_state(
                         continue
                     other_children.append(x)
                     item.remove(x)
+                # object_handling need to be before equal comparison for evaluating defaults
+                self.object_handling(obj, module)
                 if not item.equal(obj, compare_children=True):
                     result["changed"] = True
                     obj.extend(other_children)
@@ -703,10 +719,6 @@ def apply_state(
                                 # NOTE checking defaults for with_update_in_apply_state doesnot have
                                 # a use for now as template, stack and device group dont have
                                 # defaults in the SDK
-                                # it also breaks panos_template as SDK has `mode` attribute set
-                                # to "normal" by default, but there is no xpath for this.
-                                # if obj_value is None:
-                                #     setattr(obj, key, self._get_default_value(obj, key))
                                 if getattr(item, key) != getattr(obj, key):
                                     try:
                                         obj.update(key)
@@ -717,9 +729,6 @@ def apply_state(
                             result["after"] = self.describe(obj)
                             result["diff"]["after"] = eltostr(obj)
                         else:
-                            for key, obj_value in obj.about().items():
-                                if obj_value is None:
-                                    setattr(obj, key, self._get_default_value(obj, key))
                             result["after"] = self.describe(obj)
                             result["diff"]["after"] = eltostr(obj)
                             try:
@@ -728,9 +737,7 @@ def apply_state(
                                 module.fail_json(msg="Failed apply: {0}".format(e))
                 break
             else:
-                for key, obj_value in obj.about().items():
-                    if obj_value is None:
-                        setattr(obj, key, self._get_default_value(obj, key))
+                self.object_handling(obj, module)
                 result["changed"] = True
                 result["before"] = None
                 result["after"] = self.describe(obj)
@@ -889,9 +896,7 @@ def apply_state(
                                 )
                 break
             else:  # create new record with merge
-                for key, obj_value in obj.about().items():
-                    if obj_value is None:
-                        setattr(obj, key, self._get_default_value(obj, key))
+                self.object_handling(obj, module)
                 result["before"] = None
                 result["after"] = self.describe(obj)
                 result["diff"] = {
diff --git a/plugins/modules/panos_static_route.py b/plugins/modules/panos_static_route.py
index 0ff0b2ccd..ac249979e 100644
--- a/plugins/modules/panos_static_route.py
+++ b/plugins/modules/panos_static_route.py
@@ -52,14 +52,13 @@
         type: str
     nexthop_type:
         description:
-            - Type of next hop.
+            - Type of next hop. Defaults to I("ip-address").
         type: str
         choices:
             - ip-address
             - discard
             - none
             - next-vr
-        default: 'ip-address'
     nexthop:
         description:
             - Next hop IP address.  Required if I(state=present).
@@ -165,7 +164,11 @@ def spec_handling(self, spec, module):
         # default to ip-address when nexthop is set in merged state
         # we dont know if object exists or not in merged state, and we dont set default values in module invocation
         # in order to avoid unintended updates to non-provided params, but if nexthop is given, type must be ip-address
-        if module.params["state"] == "merged" and spec["nexthop_type"] is None and spec["nexthop"] is not None:
+        if (
+            module.params["state"] == "merged"
+            and spec["nexthop_type"] is None
+            and spec["nexthop"] is not None
+        ):
             spec["nexthop_type"] = "ip-address"
 
         # NOTE merged state have a lot of API issues for updating nexthop we will let the API return it..
@@ -181,6 +184,10 @@ def spec_handling(self, spec, module):
             ]
             module.fail_json(msg=" ".join(msg))
 
+    def object_handling(self, obj, module):
+        super().object_handling(obj, module)
+        if module.params.get("nexthop_type") == "none":
+            setattr(obj, "nexthop_type", None)
 
 
 def main():
diff --git a/plugins/modules/panos_template.py b/plugins/modules/panos_template.py
index e93a34103..8061744d2 100644
--- a/plugins/modules/panos_template.py
+++ b/plugins/modules/panos_template.py
@@ -105,6 +105,14 @@ def pre_state_handling(self, obj, result, module):
         vsys = to_sdk_cls("device", "Vsys")(module.params["default_vsys"])
         obj.add(vsys)
 
+    def object_handling(self, obj, module):
+        super().object_handling(obj, module)
+        # override 'mode' param sdk default to None if it's not set explicitly in invocation.
+        # SDK has `mode` attribute set to "normal" by default, but there is no xpath for this
+        # resulting in xpath schema error if default is used.
+        if module.params.get("mode") is None:
+            setattr(obj, "mode", None)
+
 
 def main():
     helper = get_connection(