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

Remove empty event action params #464

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

Conversation

sxmichael
Copy link

In event_action_params() clean params from nil and empty values - when using dynamic fields (like document_id => "%{[@metadata][document_id]}"), empty or nil values could be produced for document_id and parent fields (in es_bulk metadata part), which is not valid es_bulk metadata format, thus those values should be cleaned.

I've added remove_empty_action_params configuration option to control this behavior (default is false).
Test is added as well.

In event_action_params() clean params from nil and empty values - when using dynamic fields (like document_id => "%{[@metadata][document_id]}"), empty or nil values could be produced for document_id and parent fields (in es_bulk metadata part), which is not valid es_bulk metadata format, thus those values should be cleaned.
@sxmichael sxmichael force-pushed the feature/clean_event_action_params branch from ebeafcd to ccb92b7 Compare August 16, 2016 06:14
@sxmichael
Copy link
Author

@untergeek @karmi any update?

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. I love the idea here. It needs some tweaks though!

expect(params).to include(:_id => "mytestid", :parent => "mytestparent")
end

it "should interpolate the requested id and parent values and leave them as is (sprintf behavior, not sure if correct one) when they missing" do
Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior isn't great, but I don't know what else we can do here. I think this should be documented in the option docs.

expect(params).to include(:_id => "%{myid}", :parent => "%{myparent}")
end

it "should interpolate the requested id and parent values and leave them as is (sprintf behavior, not sure if correct one) when they empty" do
Copy link
Contributor

Choose a reason for hiding this comment

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

This just repeats the previous test. I think you wanted to test that this should interpolate the requested id and parent values.

Copy link
Author

Choose a reason for hiding this comment

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

Yeas, those are two identical tests, just to see the actual difference in behavior what happens when proposed remove_empty_action_params param set to true and to false.

@andrewvc andrewvc self-assigned this Oct 26, 2016
@sxmichael
Copy link
Author

Hey @andrewvc could this be merged?

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

Successfully merging this pull request may close these issues.

5 participants