8000 remove_keys_from_values: new filter plugin, remove keys with certain values by tanganellilore · Pull Request #10139 · ansible-collections/community.general · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

remove_keys_from_values: new filter plugin, remove keys with certain values #10139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tanganellilore
Copy link
Contributor
SUMMARY

Hi team,
i will propose a special filter that i think is missing in this community, a filter that remove some keys on a dict (or list of dict) that contains certain values.

In origin I search a way to remove empty key in a dict and I can do it with select, match and so on, but i think that a filter that can remove key ina object absed on valuse can be very usefull.

This is why i'm poroposing a filter that can do it (as default behaviourremove keys with empty values), similar to remove_keysv filter.

ISSUE TYPE
  • New Module/Plugin Pull Request
COMPONENT NAME

remove_keys_from_values.py

ADDITIONAL INFORMATION

@ansibullbot ansibullbot added the plugins plugin (any type) label May 15, 2025
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label May 15, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels May 15, 2025
@felixfontein
Copy link
Collaborator

Thanks for your contribution!

If anyone is wondering how to do this with pure ansible-core + jinja:

- hosts: localhost
  gather_facts: false
  tasks:
    - ansible.builtin.debug:
        msg: >-
          {{
            input | dict2items | rejectattr("value", "none") | items2dict
          }}
      vars:
        input:
          foo: bar
          baz: bam
          bam: ~
    - ansible.builtin.debug:
        msg: >-
          {{
            input | map("dict2items") | map("rejectattr", "value", "none") | map("items2dict") | list
          }}
      vars:
        input:
          - foo: bar
            baz: bam
            bam: ~
          - foo:
            bar: baz
            baz:

I'm wondering whether it wouldn't be better to have a filter that's more flexible, by allowing the user to specify a test that's applied to values (similar to Jinja's reject/accept/rejectattr/acceptattr filters).

I'm not sure how simple that is to implement, though. You'd definitely need more Jinja2 internals.

@bcoca
Copy link
Contributor
bcoca commented May 15, 2025

A 'drop' filter could work with map, does not need to iterate over objects itself slimdicts = {{listofdicts | map( 'drop', ['key1', 'key2', 'key3']) }}

@tanganellilore
Copy link
Contributor Author

Hi @felixfontein,
Yes this is how i resolve actually, trasform dict2items and rejectattr, multiple time based on attributes to be rejected.

For the key we have the filter and the map, but i don'se something more complex for value.
I'm open to rework the solution.

Let me know if you want to track it with issue.

Thanks

@bcoca
Copy link
Contributor
bcoca commented May 15, 2025

will add to my collection, but here is a quick and dirty 'drop' filter

# -*- coding: utf-8 -*-
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

from __future__ import absolute_import, division, print_function
__metaclass__ = type


from collections.abc import Sequence

def do_drop(obj, keys):

    if isinstance(obj, dict):
        for k in keys:
            del obj[k]
    elif isinstance(obj, Sequence):
        for k in keys:
            obj.remove(k)
    else:
        raise TypeError("Drop only works on dictionaries or lists")

    return obj

class FilterModule(object):
    ''' Ansible core jinja2 filters '''

    def filters(self):
        return {
            'drop': do_drop,
        }            

@felixfontein
Copy link
Collaborator

@bcoca we already have a filter for removing keys from dictionaries: community.general.remove_keys. This PR is about removing specific values (in particular None).

@felixfontein
Copy link
Collaborator

Let me know if you want to track it with issue.

Using this PR for discussions is also fine IMO.

@bcoca
Copy link
Contributor
bcoca commented May 15, 2025

TIL

@tanganellilore
Copy link
Contributor Author

@bcoca we already have a filter for removing keys from dictionaries: community.general.remove_keys. This PR is about removing specific values (in particular None).

Yes, I also added the support for other value like "exact" or "regex", as the same as remove_keys filter.

In this implementation values are customizable, like list of match case or single match case, and with matching type exact we simple check if value of the key is in value, with regex we apply the match regex.
Moreorve is present a recursive param that allo us to avoid iterate inside nested dict or list of dict (so stop to first level of dict or list of dict key/value)

Let me now What do you think, Thanks.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label May 24, 2025
@felixfontein felixfontein removed the backport-10 Automatically create a backport for the stable-10 branch label May 24, 2025
@russoz russoz changed the title Add filter to remove keys with certain values remove_keys_from_values: new filter plugin, remove keys with certain values May 25, 2025
Copy link
Collaborator
@russoz russoz left a comment

Choose a reason for hiding this comment

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

hi @tanganellilore thanks for your contribution! Some adjustments are needed - regardless of the ongoing discussion.

@ansibullbot
Copy link
Collaborator

@tanganellilore this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_ci CI is older than 7 days, rerun before merging labels May 30, 2025
@tanganellilore
Copy link
Contributor Author

Hi @russoz ,
fix done as requested.

Thanks and sorry for delay.

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label May 30, 2025
@felixfontein
Copy link
Collaborator

Regarding my question about why not allow arbitrary Jinja2 tests, similar as for the reject filter (https://jinja.palletsprojects.com/en/stable/templates/#jinja-filters.reject).

For the key we have the filter and the map, but i don'se something more complex for value.
I'm open to rework the solution.

Was this related to that question? If yes I'm afraid I don't understand what you mean.

@tanganellilore
Copy link
Contributor Author

Regarding my question about why not allow arbitrary Jinja2 tests, similar as for the reject filter (https://jinja.palletsprojects.com/en/stable/templates/#jinja-filters.reject).

For the key we have the filter and the map, but i don'se something more complex for value.
I'm open to rework the solution.

Was this related to that question? If yes I'm afraid I don't understand what you mean.

Sorry, not clear to me the request :-).

@felixfontein
Copy link
Collaborator

The reject filter (look at its docs!) allows to use arbitrary Jinja2 tests, like some_list | reject("eq", "foo") will remove all entries in some_list that are equal to the string "foo" (docs for eq test: https://jinja.palletsprojects.com/en/stable/templates/#jinja-tests.eq); some_list | reject("community.general.a_module") will remove all entries in some_list that name Ansible modules (https://docs.ansible.com/ansible/devel/collections/community/general/a_module_test.html); and some_list | reject("community.general.ansible_type", "dict[str, str]") will remove all entries in some_list which are dictionaries with string keys and string values (https://docs.ansible.com/ansible/devel/collections/community/general/ansible_type_test.html). This makes the reject filter extremely flexible, since you don't have to stick to one of the few provided tests, but can use an arbitrary Jinja2 test.

@tanganellilore
Copy link
Contributor Author

@felixfontein thanks for clarification, now should be much clear.
If I understood, correct if I'm wrong, your use case is to reject element in a list that match a test case, so yes, for this use case reject with a custom testcase is the correct way to do it.

I tried also usage of rejectattr but require a dict2items (or map of dict2items for list of dict) to apply reject in the value key and a rollback with items2dict. Moreover this not cover the "recursive" use case covered by my proposal and as far as i know regex use case.

Some example tested as per ref:

- name: Remove empty keys from list of dictionaries
  set_fact:
    cleaned_list: "{{ test_list_dict | map('dict2items')
      | map('rejectattr', 'value', 'in', [None, '', [], {}]) | map('items2dict')
      | list }}"
  vars:
    test_list_dict: 
      - a: []
        b: test
        h: false
        subkey:
          c: ''
          d: []
          e: 0
      - f: []
        g: test
        h: test_h
        subkey:
          i: test
          l: []
          m: 0
- name: Remove empty keys from dictionaries
  set_fact:
    cleaned_list: "{{ test_dict | dict2items
      | rejectattr('value', 'in', [None, '', [], {}])
      | items2dict }}"
  vars:
    test_dict: 
      a: []
      b: test
      h: false
      subkey:
        c: ''
        d: []
        e: 0
      

My proposal is to remove a key of dict based on it's value, on first or all level of dict and with support of multiple test case, that seems to me different from remove whole element in a list (as rejecet) or remove element with test case starting frok him key.

Yes, my proposal support list of dict, but also in this case I will remove the key of dict element that match value/options passed.

Let me know if it's clear.

Thanks

@felixfontein
Copy link
Collaborator

@felixfontein thanks for clarification, now should be much clear.
If I understood, correct if I'm wrong, your use case is to reject element in a list that match a test case, so yes, for this use case reject with a custom testcase is the correct way to do it.

That's not my use case, that was an example of what "uses an arbitrary test plugin" means. My suggestion is to create a filter plugin remove_keys_from_values that uses the same technique to test values, instead of having hard-coded methods as in the current implementation.

With that you'd do something like my_list | community.general.remove_empty_keys("in", ['foo', 'bar']) insead of my_list | community.general.remove_empty_keys(values=['foo', 'bar']).

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jun 7, 2025
@felixfontein felixfontein added the backport-11 Automatically create a backport for the stable-10 branch label Jun 16, 2025
Copy link
Collaborator
@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi @tanganellilore

Still a couple of adjustments needed. Please keep in mind that new 9E88 plugins are required to come with tests. I would suggest you write integration tests and use the code from EXAMPLES as the base for it.

Comment on lines +2 to +3
# Copyright (c) 2024 Vladimir Botka <vbotka@gmail.com>
# Copyright (c) 2024 Felix Fontein <felix@fontein.de>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing something here ;-)

DOCUMENTATION = r"""
name: remove_keys_from_values
short_description: Remove keys from a list of dictionaries or a dictionary in base of value content
version_added: "11.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

That boat has sailed, please use:

Suggested change
version_added: "11.0.0"
version_added: "11.1.0"

- A single value or value pattern to remove, or a list of values or values patterns to remove.
- If O(matching_parameter=regex) there must be equally one pattern provided.
type: raw
required: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Err... the code below disagrees with that - if nothing is passed, it will assume the default value.

Comment on lines +43 to +44
equal: Matches values of equally one of the O(values[]) items.
regex: Matches values that match one of the regular expressions provided in O(values[]).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
equal: Matches values of equally one of the O(values[]) items.
regex: Matches values that match one of the regular expressions provided in O(values[]).
equal: Matches values of equally one of the O(values) items.
regex: Matches values that match one of the regular expressions provided in O(values).

values:
description:
- A single value or value pattern to remove, or a list of values or values patterns to remove.
- If O(matching_parameter=regex) there must be equally one pattern provided.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not clear to me what that means. Should it be only the pattern? Should it be at least on pattern? Will it accept more than one pattern?

description:
- This filter removes keys from a list of dictionaries or a dictionary,
recursively or not depending on the parameters.
- The type of values to be removed is defined by the O(values) parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line would be better place in the description of values, but there is already a similar description there, so I would argue this line is redundant.

- This filter removes keys from a list of dictionaries or a dictionary,
recursively or not depending on the parameters.
- The type of values to be removed is defined by the O(values) parameter.
- The default is to remove keys with values like C(''), C([]), C({}), C(None), usefull for removing empty keys.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would argur that this line should go to the description of the values parameter, with the adjustments:

Suggested change
- The default is to remove keys with values like C(''), C([]), C({}), C(None), usefull for removing empty keys.
- The default is to remove keys with values like V(''), V([]), V({}), V(None), useful for removing empty keys.

b: {}
c: None
- debug:
msg: "{{ my_list | remove_empty_keys }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The examples need updating:

Suggested change
msg: "{{ my_list | remove_empty_keys }}"
msg: "{{ my_list | remove_keys_from_values }}"

from ansible.errors import AnsibleFilterError


def remove_keys_from_value(data, values=None, recursive=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

err... where is this function ever used?

if matching_parameter not in ("equal", "regex"):
raise AnsibleFilterError("matching_parameter must be 'equal' or 'regex'")

values = values if isinstance(values, list) else [values or '', [], {}, None]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to be trickier than that:

>>> values = "a"
>>> values if isinstance(values, list) else [values or '', [], {}, None]
['a', [], {}, None]
>>> values = [1,2,3]
>>> values if isinstance(values, list) else [values or '', [], {}, None]
[1, 2, 3]
>>> values = None
>>> values if isinstance(values, list) else [values or '', [], {}, None]
['', [], {}, None]
>>> 
>>> values = "a"
>>> values if isinstance(values, list) else [values or ['', [], {}, None]]
['a']
>>> values = None
>>> values if isinstance(values, list) else [values or ['', [], {}, None]]
[['', [], {}, None]]

I think lines 139-143 solve the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-11 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0