fix: Resolve issues with nested explicit list and null value serialization (#574)

Author: lgarber-akamai

linodecli/api_request.py

@@ -7,15 +7,19 @@
 import sys
 import time
 from sys import version_info
-from typing import Iterable, List, Optional
+from typing import Any, Iterable, List, Optional
 
 import requests
 from packaging import version
 from requests import Response
 
 from linodecli.helpers import API_CA_PATH
 
-from .baked.operation import ExplicitNullValue, OpenAPIOperation
+from .baked.operation import (
+    ExplicitEmptyListValue,
+    ExplicitNullValue,
+    OpenAPIOperation,
+)
 from .helpers import handle_url_overrides
 
 
@@ -199,6 +203,47 @@ def _build_request_url(ctx, operation, parsed_args) -> str:
     return result
 
 
+def _traverse_request_body(o: Any) -> Any:
+    """
+    This function traverses is intended to be called immediately before
+    request body serialization and contains special handling for dropping
+    keys with null values and translating ExplicitNullValue instances into
+    serializable null values.
+    """
+    if isinstance(o, dict):
+        result = {}
+        for k, v in o.items():
+            # Implicit null values should be dropped from the request
+            if v is None:
+                continue
+
+            # Values that are expected to be serialized as empty lists
+            # and explicit None values are converted here.
+            # See: operation.py
+            # NOTE: These aren't handled at the top-level of this function
+            # because we don't want them filtered out in the step below.
+            if isinstance(v, ExplicitEmptyListValue):
+                result[k] = []
+                continue
+
+            if isinstance(v, ExplicitNullValue):
+                result[k] = None
+                continue
+
+            value = _traverse_request_body(v)
+
+            # We should exclude implicit empty lists
+            if not (isinstance(value, (dict, list)) and len(value) < 1):
+                result[k] = value
+
+        return result
+
+    if isinstance(o, list):
+        return [_traverse_request_body(v) for v in o]
+
+    return o
+
+
 def _build_request_body(ctx, operation, parsed_args) -> Optional[str]:
     if operation.method == "get":
         # Get operations don't have a body
@@ -208,32 +253,18 @@ def _build_request_body(ctx, operation, parsed_args) -> Optional[str]:
     if ctx.defaults:
         parsed_args = ctx.config.update(parsed_args, operation.allowed_defaults)
 
-    to_json = {}
-
-    for k, v in vars(parsed_args).items():
-        # Skip null values
-        if v is None:
-            continue
-
-        # Explicitly include ExplicitNullValues
-        if isinstance(v, ExplicitNullValue):
-            to_json[k] = None
-            continue
-
-        to_json[k] = v
-
     expanded_json = {}
 
     # expand paths
-    for k, v in to_json.items():
+    for k, v in vars(parsed_args).items():
         cur = expanded_json
         for part in k.split(".")[:-1]:
             if part not in cur:
                 cur[part] = {}
             cur = cur[part]
         cur[k.split(".")[-1]] = v
 
-    return json.dumps(expanded_json)
+    return json.dumps(_traverse_request_body(expanded_json))
 
 
 def _print_request_debug_info(method, url, headers, body):

linodecli/baked/operation.py

@@ -60,6 +60,12 @@ class ExplicitNullValue:
     """
 
 
+class ExplicitEmptyListValue:
+    """
+    A special type used to explicitly pass empty lists to the API.
+    """
+
+
 def wrap_parse_nullable_value(arg_type):
     """
     A helper function to parse `null` as None for nullable CLI args.
@@ -91,9 +97,16 @@ def __call__(self, parser, namespace, values, option_string=None):
 
         output_list = getattr(namespace, self.dest)
 
+        # If a user has already specified an [] but is specifying
+        # another value, assume "[]" was intended to be a literal.
+        if isinstance(output_list, ExplicitEmptyListValue):
+            setattr(namespace, self.dest, ["[]", values])
+            return
+
         # If the output list is empty and the user specifies a []
-        # argument, keep the list empty
+        # argument, set the list to an explicitly empty list.
         if values == "[]" and len(output_list) < 1:
+            setattr(namespace, self.dest, ExplicitEmptyListValue())
             return
 
         output_list.append(values)

tests/fixtures/api_request_test_foobar_post.yaml

@@ -120,3 +120,7 @@ components:
               field_int:
                 type: number
                 description: An arbitrary field.
+              nullable_string:
+                type: string
+                description: An arbitrary nullable string.
+                nullable: true

tests/unit/test_api_request.py

@@ -12,7 +12,7 @@
 import requests
 
 from linodecli import api_request
-from linodecli.baked.operation import ExplicitNullValue
+from linodecli.baked.operation import ExplicitEmptyListValue, ExplicitNullValue
 
 
 class TestAPIRequest:
@@ -102,6 +102,36 @@ def test_build_request_body_null_field(self, mock_cli, create_operation):
             == result
         )
 
+    def test_build_request_body_null_field_nested(
+        self, mock_cli, create_operation
+    ):
+        create_operation.allowed_defaults = ["region", "image"]
+        result = api_request._build_request_body(
+            mock_cli,
+            create_operation,
+            SimpleNamespace(
+                generic_arg="foo",
+                region=None,
+                image=None,
+                object_list=[{"nullable_string": ExplicitNullValue()}],
+            ),
+        )
+        assert (
+            json.dumps(
+                {
+                    "generic_arg": "foo",
+                    "region": "us-southeast",
+                    "image": "linode/ubuntu21.10",
+                    "object_list": [
+                        {
+                            "nullable_string": None,
+                        }
+                    ],
+                }
+            )
+            == result
+        )
+
     def test_build_request_body_non_null_field(
         self, mock_cli, create_operation
     ):
@@ -517,3 +547,41 @@ def test_get_retry_after(self):
         headers = {}
         output = api_request._get_retry_after(headers)
         assert output == 0
+
+    def test_traverse_request_body(self):
+        result = api_request._traverse_request_body(
+            {
+                "test_string": "sdf",
+                "test_obj_list": [
+                    {
+                        "populated_field": "cool",
+                        "foo": None,
+                        "bar": ExplicitNullValue(),
+                    }
+                ],
+                "test_dict": {
+                    "foo": "bar",
+                    "bar": None,
+                    "baz": ExplicitNullValue(),
+                },
+                "cool": [],
+                "cooler": ExplicitEmptyListValue(),
+                "coolest": ExplicitNullValue(),
+            }
+        )
+
+        assert result == {
+            "test_string": "sdf",
+            "test_obj_list": [
+                {
+                    "populated_field": "cool",
+                    "bar": None,
+                }
+            ],
+            "test_dict": {
+                "foo": "bar",
+                "baz": None,
+            },
+            "cooler": [],
+            "coolest": None,
+        }

tests/unit/test_operation.py

@@ -1,7 +1,7 @@
 import argparse
 
 from linodecli.baked import operation
-from linodecli.baked.operation import ExplicitNullValue
+from linodecli.baked.operation import ExplicitEmptyListValue, ExplicitNullValue
 
 
 class TestOperation:
@@ -178,11 +178,13 @@ def test_parse_args_object_list(self, create_operation):
                 "test3",
             ]
         )
+
         assert result.object_list == [
             {
                 "field_string": "test1",
                 "field_int": 123,
                 "field_dict": {"nested_string": "test2", "nested_int": 789},
+                "nullable_string": None,  # We expect this to be filtered out later
             },
             {"field_int": 456, "field_dict": {"nested_string": "test3"}},
         ]
@@ -209,7 +211,7 @@ def test_array_arg_action_basic(self):
 
         # User wants an explicitly empty list
         result = parser.parse_args(["--foo", "[]"])
-        assert getattr(result, "foo") == []
+        assert isinstance(getattr(result, "foo"), ExplicitEmptyListValue)
 
         # User doesn't specify the list
         result = parser.parse_args([])