#340 fix match["query_key"] malformation

[AntoineBlaud]

No description provided.

[jertel]

Thanks for the PR. Some unit tests to cover that new logic would be helpful, to ensure no one accidentally breaks your logic on future commits.

[mrfroggg]

We have many field names with . in them in our Elasticsearch and I use many of them in our query_key.
I have not tested this PR, but seeing _expand_string_into_dict and expand_string_into_dict, I have a doubt that this could break my setup.
What do you think?

[JeffAshton]

We have many field names with . in them in our Elasticsearch and I use many of them in our query_key.
I have not tested this PR, but seeing _expand_string_into_dict and expand_string_into_dict, I have a doubt that this could break my setup.
What do you think?

I agree with @jertel here. Yelp didn't do a great job of ensuring quality tests, and the rest of us need to pay down that debt. This should be an easy deterministic thing to test.

[AntoineBlaud]

@mrfroggg it can break your setup only if you use enhancement by doing some works with the query key.
@jertel how can I create a match from specific documents inside a test method ? Do you have a example ? Thank you.

[jertel]

@AntoineBlaud The two new functions you added won't need mocked documents, those unit tests should be relatively simple since you're dealing with simplistic inputs, such as generic dicts, strings, etc. As far as unit testing the check_matches function, look at tests/rules_test.py to see what's already being unit tested.

@mrfroggg I agree, if you look at #340 I raised this concern as well. If it's not possible to have a "foo" key and a "foo.bar" key in the same elastic data structure then another option would be to add a configurable toggle to enable or disable the string-to-dict conversion. Then we'd have to decide which would be the default toggle: If we default it to enabled, it could break existing users, but then the fix is a simple toggle back to false, assuming they read the release notes. Doing this would allow us to deprecate the old method of "foo.bar" keys and eventually remove it after some time. If instead we default it to disabled then people like @AntoineBlaud will have to enable it manually to accomplish their desired behavior. Yet long term is seems that converting these to dicts for consistency with the other rule types is the better way forward. Thoughts?

[jertel]

This PR has been out here for 9 days, giving everyone sufficient time to comment or request changes. As there are no further comments I am merging this today. However, if there ends up being a unexpected difficulties by the community from this we can react quickly and release a new version with a configuration toggle.