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

Event#toMap should return copy #5296

Closed
wants to merge 1 commit into from
Closed

Conversation

talevy
Copy link
Contributor

@talevy talevy commented May 13, 2016

No description provided.

@colinsurprenant
Copy link
Contributor

I think we need to verify current usages of toMap() to make sure there's no double-cloning involved like https://github.com/talevy/logstash/blob/1339b096b18de958eafb6cef08627f6a5a5c64c7/logstash-core-event-java/src/main/java/com/logstash/ext/JrubyEventExtLibrary.java#L248

@talevy
Copy link
Contributor Author

talevy commented May 13, 2016

ah, I forgot about this one that I changed in the previous PR. will update

@talevy
Copy link
Contributor Author

talevy commented May 13, 2016

@colinsurprenant thanks, updated. I see no other usages

@@ -4,6 +4,9 @@

import java.io.IOException;
import java.util.*;

import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.core.IsSame.sameInstance;
Copy link
Contributor

Choose a reason for hiding this comment

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

are we introducing a new org.hamcrest dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like it has already been included. I just autocompleted. unless I am mistaken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked the sameInstance assertion from there, so I started typing it. and it worked. so I stuck with it

@talevy
Copy link
Contributor Author

talevy commented May 16, 2016

Looks like it is somehow shadowed inside the json-unit testCompile dependency?

@colinsurprenant
Copy link
Contributor

I don't really mind if we introduce a new testCompile dep but I am not sure how this can work if it is not included in build.gradle?

@talevy
Copy link
Contributor Author

talevy commented May 16, 2016

It is being brought in by json-unit

@talevy
Copy link
Contributor Author

talevy commented May 16, 2016

well. it doesn't matter anymore. I removed the code dependency

@colinsurprenant
Copy link
Contributor

LGTM

@elasticsearch-bot
Copy link

Tal Levy merged this into the following branches!

Branch Commits
master f2d8be1
5.0 7a1851d

elasticsearch-bot pushed a commit that referenced this pull request May 16, 2016
@suyograo suyograo changed the title have Event#toMap return copy Event#toMap should return copy May 23, 2016
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.

3 participants