fix: avoid mutable default arguments in custom filter plugins - #70
Merged
Merged
Conversation
Replace the shared `[]` / `{}` default arguments in filter_plugins/custom.py
with `None` and normalise inside each function, and rename filter_reserved's
parameter to `user_roles` to match remove_reserved. Adds unit tests for the
previously untested filters and for the omitted-argument defaults.
Co-authored-by: Cursor <cursoragent@cursor.com>
LukoJy3D
reviewed
Sep 10, 2026
LukoJy3D
left a comment
Member
There was a problem hiding this comment.
Review
The filter change is the right fix and the new tests are the valuable part of this PR. Existing Jinja call sites stay positional (filter_reserved, extract_role_users, array_to_str, filename), so the users_role → user_roles rename is safe inside this role.
CI is green (Molecule + CodeQL). Pytest is still local-only — same as #69 — so the new tests are not enforced on PRs.
I renamed this PR off the generic Daily code improvement (2026-09-08) title and added AGENTS.md so Cursor / Claude / DEX keep titles tied to the actual diff.
What looks good
- Replacing shared
[]/{}defaults is a real (latent) B006 footgun. - Tests now cover filters that previously had none (
modify_list,append_to_list,array_to_str,extract_role_users,filename) plus omitted-argument defaults. - No task/template changes; blast radius is the Python filters only.
Nits (non-blocking)
x = x or []is not the same as a missing argument. Empty string, empty list, andNoneare all falsy.append_to_list("")used to split to[""]and return[suffix]; it now becomes[]. Preferif values is None: values = []so only the default is rewritten. Same for the other filters.filter_reservedkeyword rename would break an external caller usingusers_role=. None in this repo; worth a one-liner in the description that Jinja keyword use is unsupported.- Pytest is not in
.github/workflows.python3 -m pytest tests -qshould be a CI job if we want these 13 tests to stay green.
Low risk to merge as-is for current call paths. The is None normalisation is the only behaviour I’d still tighten.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes the mutable default arguments from
filter_plugins/custom.py— a classic Python anti-pattern (flake8-bugbear / ruffB006). Six filter functions declaredvalues=[],users={},exclude_users=[]oruser_roles={}, meaning a single list/dict object is shared by every call for the lifetime of the process. Nothing mutates them today, so this is a latent bug rather than an active one, but it is exactly the kind of footgun that bites the first time someone adds an in-placeappend/update.Category: Simplification / consistency (low-risk, self-contained).
Changes
filter_plugins/custom.py:modify_list,append_to_list,array_to_str,extract_role_users,remove_reserved,filter_reservednow default their collection parameters toNoneand normalise withx = x or []/x or {}inside the function. Behaviour is unchanged for every existing call.filter_reserved(users_role=...)→filter_reserved(user_roles=...), so it matches its siblingremove_reserved(user_roles=...). Safe: all call sites pipe the value positionally ({{ user_list_response.json | filter_reserved }}intasks/xpack/security/elasticsearch-security-native.yml,{{ es_users.file | extract_role_users() }}inelasticsearch-security-file.yml,{{ es_data_dirs | array_to_str }}intemplates/elasticsearch.yml.j2) — no keyword usage anywhere in the role.list()wrapper around.items()inextract_role_users(a Python 2 leftover; the dict is only read, never mutated during iteration) and tidied themodify_listdocstring.Tests
tests/test_custom_filters.pygrows from 5 to 13 tests. The newNonenormalisation adds branches, so coverage is maintained and extended rather than diluted:modify_list(incl.ignorecase),append_to_list(list and comma-separated-string input),array_to_str(default and custom separator),extract_role_users(role/user pairing andexclude_users),filename.test_filters_tolerate_omitted_argumentscalls every filter with no arguments, pinning the new default-handling paths.Risk
Low. No behavioural change for any existing call path, no signature change visible to Jinja (all usages are positional pipes), no Ansible task or template touched.
Generated by DEX CODE
Triggered by
agent:weekly-code-improvements