Exclude parameter values from API request bodies (#681)

Co-authored-by: ykim-1 <[email protected]>

Author: lgarber-akamai

linodecli/api_request.py

@@ -259,11 +259,13 @@ def _build_request_body(ctx, operation, parsed_args) -> Optional[str]:
     if ctx.defaults:
         parsed_args = ctx.config.update(parsed_args, operation.allowed_defaults)
 
+    param_names = {param.name for param in operation.params}
+
     expanded_json = {}
 
     # expand paths
     for k, v in vars(parsed_args).items():
-        if v is None:
+        if v is None or k in param_names:
             continue
 
         cur = expanded_json

linodecli/baked/operation.py

@@ -371,6 +371,20 @@ def __init__(
             if param.name not in {"apiVersion"}
         ]
 
+        # Validation to ensure no conflicting arguments & param names are found.
+        # This is necessary because arguments and parameters are both parsed into the
+        # same result namespace by argparse.
+        if self.request is not None and hasattr(self.request, "attrs"):
+            param_names = {param.name for param in self.params}
+
+            for attr in self.request.attrs:
+                if attr not in param_names:
+                    continue
+
+                raise ValueError(
+                    f"Attribute {attr.name} conflicts with parameter of the same name"
+                )
+
         (
             self.url_base,
             self.url_path,

tests/fixtures/api_request_test_foobar_put.yaml

@@ -0,0 +1,126 @@
+openapi: 3.0.1
+info:
+  title: API Specification
+  version: 1.0.0
+servers:
+  - url: http://localhost/v4
+
+paths:
+  /foo/bar/{barId}:
+    parameters:
+      - name: barId
+        description: The ID of the bar.
+        in: path
+        required: true
+        schema:
+          type: string
+    x-linode-cli-command: foo
+    put:
+      x-linode-cli-action: bar-update
+      summary: update foobar
+      operationId: fooBarPut
+      description: This is description
+      requestBody:
+        description: >
+          The parameters to set when updating the Foobar.
+        required: True
+        content:
+          application/json:
+            schema:
+              allOf:
+              - $ref: '#/components/schemas/FooBarUpdate'
+      responses:
+        '200':
+          description: Successful response
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/OpenAPIResponseAttr'
+
+components:
+  schemas:
+    OpenAPIResponseAttr:
+      type: object
+      properties:
+        filterable_result:
+          x-linode-filterable: true
+          type: string
+          description: Filterable result value
+    PaginationEnvelope:
+      type: object
+      properties:
+        pages:
+          type: integer
+          readOnly: true
+          description: The total number of pages.
+          example: 1
+        page:
+          type: integer
+          readOnly: true
+          description: The current page.
+          example: 1
+        results:
+          type: integer
+          readOnly: true
+          description: The total number of results.
+          example: 1
+    FooBarUpdate:
+      type: object
+      description: Foobar object request
+      properties:
+        test_param:
+          x-linode-filterable: true
+          type: integer
+          description: The test parameter
+        generic_arg:
+          x-linode-filterable: true
+          type: string
+          description: The generic argument
+        region:
+          x-linode-filterable: true
+          type: string
+          description: The region
+        nullable_int:
+          type: integer
+          nullable: true
+          description: An arbitrary nullable int
+        nullable_string:
+          type: string
+          nullable: true
+          description: An arbitrary nullable string
+        nullable_float:
+          type: number
+          nullable: true
+          description: An arbitrary nullable float
+        object_list:
+          type: array
+          description: An arbitrary list of objects.
+          items:
+            type: object
+            description: An arbitrary object.
+            properties:
+              field_dict:
+                type: object
+                description: An arbitrary nested dict.
+                properties:
+                  nested_string:
+                    type: string
+                    description: A deeply nested string.
+                  nested_int:
+                    type: number
+                    description: A deeply nested integer.
+              field_array:
+                type: array
+                description: An arbitrary deeply nested array.
+                items:
+                  type: string
+              field_string:
+                type: string
+                description: An arbitrary field.
+              field_int:
+                type: number
+                description: An arbitrary field.
+              nullable_string:
+                type: string
+                description: An arbitrary nullable string.
+                nullable: true

tests/integration/cli/test_help.py

@@ -4,6 +4,7 @@
 
 from tests.integration.helpers import (
     contains_at_least_one_of,
+    exec_failing_test_command,
     exec_test_command,
 )
 
@@ -50,3 +51,30 @@ def test_help_page_for_aliased_actions():
 
     assert "You may filter results with:" in wrapped_output
     assert "--tags" in wrapped_output
+
+
+def test_debug_output_contains_request_url(monkeypatch: pytest.MonkeyPatch):
+    env_vars = {
+        "LINODE_CLI_API_HOST": "api.linode.com",
+        "LINODE_CLI_API_VERSION": "v4",
+        "LINODE_CLI_API_SCHEME": "https",
+    }
+    for key, value in env_vars.items():
+        monkeypatch.setenv(key, value)
+
+    output = exec_failing_test_command(
+        [
+            "linode-cli",
+            "linodes",
+            "update",
+            "--label",
+            "foobar",
+            "12345",
+            "--debug",
+        ]
+    ).stderr.decode()
+    wrapped_output = textwrap.fill(output, width=180).replace("\n", "")
+
+    assert (
+        "PUT https://api.linode.com/v4/linode/instances/12345" in wrapped_output
+    )

tests/unit/conftest.py

@@ -182,6 +182,37 @@ def create_operation():
     return create_operation
 
 
+@pytest.fixture
+def update_operation():
+    """
+    Creates the following CLI operation:
+
+    linode-cli foo bar-update --generic_arg [generic_arg] test_param
+
+    PUT http://localhost/v4/foo/bar/{fooId}
+    {
+        "generic_arg": "[generic_arg]",
+        "test_param": test_param
+    }
+    """
+
+    spec = _get_parsed_spec("api_request_test_foobar_put.yaml")
+
+    dict_values = list(spec.paths.values())
+
+    # Get parameters for OpenAPIOperation() from yaml fixture
+    path = dict_values[0]
+    command = path.extensions.get("linode-cli-command", "default")
+    operation = getattr(path, "put")
+    method = "put"
+
+    create_operation = make_test_operation(
+        command, operation, method, path.parameters
+    )
+
+    return create_operation
+
+
 @pytest.fixture
 def list_operation_for_output_tests():
     """

tests/unit/test_api_request.py

@@ -389,6 +389,33 @@ def validate_http_request(url, headers=None, data=None, **kwargs):
 
         assert result == mock_response
 
+    def test_do_request_put(self, mock_cli, update_operation):
+        mock_response = Mock(status_code=200, reason="OK")
+
+        def validate_http_request(url, headers=None, data=None, **kwargs):
+            assert url == "http://localhost/v4/foo/bar/567"
+            assert data == json.dumps(
+                {
+                    "test_param": 12345,
+                    "generic_arg": "foobar",
+                    "region": "us-southeast",  # default
+                }
+            )
+            assert "Authorization" in headers
+
+            return mock_response
+
+        update_operation.allowed_defaults = ["region"]
+
+        with patch("linodecli.api_request.requests.put", validate_http_request):
+            result = api_request.do_request(
+                mock_cli,
+                update_operation,
+                ["--generic_arg", "foobar", "--test_param", "12345", "567"],
+            )
+
+        assert result == mock_response
+
     def test_outdated_cli(self, mock_cli):
         # "outdated" version
         mock_cli.suppress_warnings = False