Skip to content
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

Assigning a dictionary to the "extra" field does not handle missing keys or empty values correctly #1335

Closed
bernhard-herzog opened this issue Oct 15, 2018 · 3 comments
Labels
bug Indicates an unexpected problem or unintended behavior component: core
Milestone

Comments

@bernhard-herzog
Copy link
Contributor

IntelMQ 1.1 changed how the "extra" field works but tries to provide a
backwards compatible interface. In particular, it's still possible to
assign a new dictionary to "extra" with add method with
overwrite=True. There are at least two cases that are not handled
correctly when doing this:

  • Keys not present in the new dictionary that were present before are
    not removed

  • Keys with empty values, e.g. empty dictionaries, are ignored and
    therefore are not changed.

Example session:

>>> import intelmq.lib.message
>>> m = intelmq.lib.message.Event()
>>> m["extra"] = {"a": {"x": 1}, "b": "foo"}
>>> m["extra"]
'{"b": "foo", "a": {"x": 1}}'

so far, so good.

>>> m.add("extra", {"a": {}}, overwrite=True)
True
>>> m["extra"]
'{"b": "foo", "a": {"x": 1}}'

I would have expected this to return {"a": {}}

@ghost ghost added bug Indicates an unexpected problem or unintended behavior component: core labels Oct 16, 2018
@ghost ghost added this to the 1.1.1 milestone Oct 16, 2018
ghost pushed a commit that referenced this issue Oct 16, 2018
@ghost
Copy link

ghost commented Oct 16, 2018

So you need to keep empty values (empty dictionaries)? The current behavior is to ignore them (same as for all non-extra fields).

@bernhard-herzog
Copy link
Contributor Author

So you need to keep empty values (empty dictionaries)?

Not necessarily. Removing the item altogether would also be OK for our purposes, so that assigning {"a": {}}' would be equivalent to assigning {}`

The current behavior is to ignore them (same as for all non-extra fields).

This is inconsistent with direct assignments to "extra.foo":

>>> import intelmq.lib.message
>>> m = intelmq.lib.message.Event()
>>> m["extra"] = {"a": {}, "b": 123}
>>> m["extra"]
'{"b": 123}'
>>> m["extra.a"] = {}
>>> m["extra"]
'{"b": 123, "a": {}}'

If this inconsistency is due to backwards compatibility it's best to keep that behavior, of course.

@ghost
Copy link

ghost commented Oct 16, 2018

Pre 1.1 there were no restriction on anything inside extra, so I agree that we shouldn't ignore empty dicts there. Will create a patch for it.

@ghost ghost closed this as completed in bcde098 Oct 16, 2018
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior component: core
Projects
None yet
Development

No branches or pull requests

1 participant