Skip to content

When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first #26068

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 9, 2017
Merged

When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first #26068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c6b12f7
bug : ThreadContext----->ThreadContextStruct:putHeaders()
hanbj Aug 5, 2017
2d0eaa7
test putHeaders
hanbj Aug 15, 2017
eaba577
delete println
hanbj Aug 15, 2017
b57ecbc
update wildcard imports and instead of try/catch.
hanbj Aug 17, 2017
f54fd7b
Merge branch 'master' into review/26068
rjernst Oct 9, 2017
09e46a6
Fix line length
rjernst Oct 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,11 +407,10 @@ private ThreadContextStruct putHeaders(Map<String, String> headers) {
if (headers.isEmpty()) {
return this;
} else {
final Map<String, String> newHeaders = new HashMap<>();
final Map<String, String> newHeaders = new HashMap<>(this.requestHeaders);
Copy link
Member

@rjernst rjernst Aug 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I just realized I'm unsure about this fix. The putSingleHeader below will fail if the key already exists, so anything in requestHeaders can then not be overriden. I think what we want instead is a loop over requestHeaders after the headers loop, but which uses putIfAbsent?

Copy link
Contributor Author

@hanbj hanbj Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't quite catch what you mean.
First of all, I didn't submit this PR before.
1, create a new HashMap instance (newHeaders).
2, traversal parameter headers.
The two step above means check whether the headers's key if exists in newHeaders.
The parameter headers is a Map. It doesn't have the same key. Just put the data in headers into newHeaders.

What we need to do is check if the parameter headers's key already exists in requestHeaders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this logic is not changed, the following:
newHeaders.putAll (this.requestHeaders);
requestHeaders also overwrites the same key in headers, but the user doesn't know it.
For instance：
headers: ("foo", "bar")
requestHeaders: ("foo", "boom")
After running newHeaders.putAll (this.requestHeaders);
Will become: newHeaders: ("foo", "boom")
However, the user thinks the value corresponding to the foo is bar

Copy link
Member

@rjernst rjernst Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think you misunderstood my comment. Look at the implementation of putSingleHeader, which is what the loop over headers calls to add each header. It will throw an exception if a key already exists. This means with your change if a key exists in requestHeaders and headers, an exception will be thrown, while before, the value from requestHeaders would have "won". The bug here is that the requestHeaders value should be the fallback, right? So then, instead of initializing the newHeaders to requestHeaders, you can iterate over requestHeaders in place of the current putAll call, and use newHeaders.putIfAbsent, so that the value would only be added if headers did not contain that key.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, you can still initialize the newHeaders to requestHeaders, but change the call to putSingleHeader to newHeaders.put. Since headers and requestHeaders are already maps, there is no possibility for duplicate keys within themselves.

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 know what you mean now. You don't want to thrown an exception.
I found it throw an exception in the putRequest method, So I did the same.
I've also found that in the stashAndMergeHeaders method, if a key exists in context.requestHeaders and headers, the value from requestHeaders would have "won".
I'm not sure now. I hope you can give me a conclusion. if a key exists in requestHeaders and headers, whether requestHeaders to overwrite headers or headers to overwrite requestHeaders.
Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

Since the method takes in headers, I assume those should "win" over what was in the context headers map before. However, I did notice that in stashAndMergeHeaders, the requestHeaders are merged into the headers being passed into putHeaders, and the order is again such that requestHeaders wins over headers. @s1monw can you clarify what the original intention was here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rjernst I think the fix is good. it's really just a shortcut for a single header. I wonder if we should de-optimize and just iterate over the map and use putHeader(string,string) instead? also the test looks great

for (Map.Entry<String, String> entry : headers.entrySet()) {
putSingleHeader(entry.getKey(), entry.getValue(), newHeaders);
}
newHeaders.putAll(this.requestHeaders);
return new ThreadContextStruct(newHeaders, responseHeaders, transientHeaders, isSystemContext);
}
}
Expand Down
20 changes: 15 additions & 5 deletions core/src/test/java/org/elasticsearch/common/util/concurrent/ThreadContextTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.hasSize;
Expand Down Expand Up @@ -215,8 +214,8 @@ public void testResponseHeaders() {
public void testCopyHeaders() {
Settings build = Settings.builder().put("request.headers.default", "1").build();
ThreadContext threadContext = new ThreadContext(build);
threadContext.copyHeaders(Collections.<String,String>emptyMap().entrySet());
threadContext.copyHeaders(Collections.<String,String>singletonMap("foo", "bar").entrySet());
threadContext.copyHeaders(Collections.<String, String>emptyMap().entrySet());
threadContext.copyHeaders(Collections.<String, String>singletonMap("foo", "bar").entrySet());
assertEquals("bar", threadContext.getHeader("foo"));
}

Expand Down Expand Up @@ -443,7 +442,7 @@ public void onAfter() {
assertEquals("bar", threadContext.getHeader("foo"));
assertEquals("bar_transient", threadContext.getTransient("foo"));
assertNotNull(threadContext.getTransient("failure"));
assertEquals("exception from doRun", ((RuntimeException)threadContext.getTransient("failure")).getMessage());
assertEquals("exception from doRun", ((RuntimeException) threadContext.getTransient("failure")).getMessage());
assertFalse(threadContext.isDefaultContext());
threadContext.putTransient("after", "after");
}
Expand Down Expand Up @@ -604,7 +603,7 @@ protected void doRun() throws Exception {
public void testMarkAsSystemContext() throws IOException {
try (ThreadContext threadContext = new ThreadContext(Settings.EMPTY)) {
assertFalse(threadContext.isSystemContext());
try(ThreadContext.StoredContext context = threadContext.stashContext()){
try (ThreadContext.StoredContext context = threadContext.stashContext()) {
assertFalse(threadContext.isSystemContext());
threadContext.markAsSystemContext();
assertTrue(threadContext.isSystemContext());
Expand All @@ -613,6 +612,17 @@ public void testMarkAsSystemContext() throws IOException {
}
}

public void testPutHeaders() {
Settings build = Settings.builder().put("request.headers.default", "1").build();
ThreadContext threadContext = new ThreadContext(build);
threadContext.putHeader(Collections.<String, String>emptyMap());
threadContext.putHeader(Collections.<String, String>singletonMap("foo", "bar"));
assertEquals("bar", threadContext.getHeader("foo"));
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
threadContext.putHeader(Collections.<String, String>singletonMap("foo", "boom")));
assertEquals("value for key [foo] already present", e.getMessage());
}

/**
* Sometimes wraps a Runnable in an AbstractRunnable.
*/
Expand Down