Skip to content

config: T0000: fix mutable default argument in config API methods#5075

Draft
andamasov wants to merge 1 commit intocurrentfrom
fix/mutable-default-args
Draft

config: T0000: fix mutable default argument in config API methods#5075
andamasov wants to merge 1 commit intocurrentfrom
fix/mutable-default-args

Conversation

@andamasov
Copy link
Copy Markdown
Member

Summary

  • Fix mutable default argument (default=[]) in four Config class methods: return_values, list_nodes, return_effective_values, and list_effective_nodes
  • Changed each default=[] parameter to default=None with an explicit if default is None: default = [] guard inside the function body
  • This prevents the classic Python bug where a mutable default argument is shared across all calls, which can lead to silent state corruption

Test plan

  • Verify that callers passing no default argument still receive an empty list as before
  • Verify that callers passing an explicit default value still receive that value when the node is missing
  • Run existing unit/integration tests to confirm no regressions

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@github-actions
Copy link
Copy Markdown

👍
No issues in PR Title / Commit Title

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the vyos.config.Config API to avoid using mutable list literals as default argument values in four list-returning helper methods.

Changes:

  • Changed default=[] to default=None for return_values(), list_nodes(), return_effective_values(), and list_effective_nodes().
  • Added an if default is None: default = [] guard in each method to preserve previous behavior while preventing mutable-default pitfalls.
Comments suppressed due to low confidence (1)

python/vyos/config.py:493

  • The default parameter is part of the public signature here, but it isn't documented in the docstring Args section, and the Returns section implies [] is always returned when missing. Please document default (and that it's returned when the node is missing/empty) so callers understand the behavior.
    def return_values(self, path, default=None):
        """
        Retrieve all values of a multi-value leaf node in the running or proposed config

        Args:
            path (str): Configuration tree path

        Returns:
            str list: Node values, if it has any
            []: if node does not exist


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 628 to 637
@@ -629,6 +635,8 @@ def list_effective_nodes(self, path, default=[]):
Returns:
str list: child node names
"""
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is part of the signature but isn't documented. Please add it to the docstring and clarify that the method returns default.copy() when no nodes are found.

Copilot uses AI. Check for mistakes.
Comment on lines +483 to 484
def return_values(self, path, default=None):
"""
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says mutable defaults were fixed for the Config API methods listed, but this file still contains mutable default arguments (e.g., get_config_dict(self, path=...) and get_config_defaults(self, path=...) use path=[]). Either include those in this change set or adjust the PR description/scope so it’s not misleading.

Copilot uses AI. Check for mistakes.
Comment on lines 513 to 523
@@ -519,6 +521,8 @@ def list_nodes(self, path, default=[]):
string list: child node names

"""
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is accepted by this method but not described in the docstring. Please add it to Args and clarify what is returned when the path doesn't exist (currently default.copy() when no nodes are found).

Copilot uses AI. Check for mistakes.
Comment on lines 603 to 612
@@ -606,6 +610,8 @@ def return_effective_values(self, path, default=[]):
Returns:
str list: A list of values
"""
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring doesn't mention the default parameter even though it's part of the signature, and it doesn't explain the fallback behavior when the node is missing/empty. Please document default and the exact return semantics.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests ❌ failed
  • RAID1 tests 👍 passed
  • CLI Smoketests VPP 👍 passed
  • Config tests VPP 👍 passed
  • TPM tests 👍 passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants