Skip to content

Commit 14cd741

Browse files
authored
Fix Double User-Agent Headers (Azure#18871)
Fixes double header and header streaming issues with our Netty HttpHeader implementation
1 parent 105bd7e commit 14cd741

File tree

14 files changed

+299
-20
lines changed

14 files changed

+299
-20
lines changed

eng/versioning/version_client.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ com.azure.resourcemanager:azure-resourcemanager-confluent;1.0.0-beta.1;1.0.0-bet
208208
# unreleased_<groupId>:<artifactId>;dependency-version
209209
# note: The unreleased dependencies will not be manipulated with the automatic PR creation code.
210210
unreleased_com.azure:azure-core;1.13.0-beta.1
211+
unreleased_com.azure:azure-core-http-netty;1.8.0-beta.1
211212
unreleased_com.azure:azure-core-tracing-opentelemetry;1.0.0-beta.7
212213

213214
# Released Beta dependencies: Copy the entry from above, prepend "beta_", remove the current

sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClient.java

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import java.nio.ByteBuffer;
2626
import java.util.Objects;
27+
import java.util.concurrent.atomic.AtomicBoolean;
2728
import java.util.function.BiFunction;
2829

2930
/**
@@ -100,7 +101,22 @@ private static BiFunction<HttpClientRequest, NettyOutbound, Publisher<Void>> bod
100101
// is not possible in reactor-netty to do this without copying occurring within that library. This
101102
// issue has been reported to the reactor-netty team at
102103
// https://github.com/reactor/reactor-netty/issues/1479
103-
hdr.getValuesList().forEach(value -> reactorNettyRequest.addHeader(hdr.getName(), value));
104+
if (reactorNettyRequest.requestHeaders().contains(hdr.getName())) {
105+
// The Reactor-Netty request headers include headers by default, to prevent a scenario where we end
106+
// adding a header twice that isn't allowed, such as User-Agent, check against the initial request
107+
// header names. If our request header already exists in the Netty request we overwrite it initially
108+
// then append our additional values if it is a multi-value header.
109+
final AtomicBoolean first = new AtomicBoolean(true);
110+
hdr.getValuesList().forEach(value -> {
111+
if (first.compareAndSet(true, false)) {
112+
reactorNettyRequest.header(hdr.getName(), value);
113+
} else {
114+
reactorNettyRequest.addHeader(hdr.getName(), value);
115+
}
116+
});
117+
} else {
118+
hdr.getValuesList().forEach(value -> reactorNettyRequest.addHeader(hdr.getName(), value));
119+
}
104120
}
105121
if (restRequest.getBody() != null) {
106122
Flux<ByteBuf> nettyByteBufFlux = restRequest.getBody().map(Unpooled::wrappedBuffer);

sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/implementation/NettyToAzureCoreHttpHeadersWrapper.java

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import java.util.Map;
1616
import java.util.Set;
1717
import java.util.stream.Stream;
18-
import java.util.stream.StreamSupport;
1918

2019
// This class wraps a Netty HttpHeaders instance and provides an azure-core HttpHeaders view onto it.
2120
// This avoids the need to copy the Netty HttpHeaders into an azure-core HttpHeaders instance.
@@ -114,12 +113,14 @@ public HttpHeader remove(String name) {
114113

115114
@Override
116115
public String getValue(String name) {
117-
return nettyHeaders.get(name);
116+
final HttpHeader header = get(name);
117+
return (header == null) ? null : header.getValue();
118118
}
119119

120120
@Override
121121
public String[] getValues(String name) {
122-
return nettyHeaders.getAll(name).toArray(new String[] { });
122+
final HttpHeader header = get(name);
123+
return (header == null) ? null : header.getValues();
123124
}
124125

125126
@Override
@@ -160,9 +161,8 @@ public String get(final Object key) {
160161
// To alleviate some concerns with performance, we internally cache the joined string in the
161162
// innerJoinMap, so multiple requests to this get method will not incur the cost of string
162163
// concatenation.
163-
return innerJoinMap.computeIfAbsent((String) key, _key -> {
164-
return String.join(",", nettyHeaders.getAll(_key));
165-
});
164+
return innerJoinMap.computeIfAbsent((String) key, _key ->
165+
String.join(",", nettyHeaders.getAll(_key)));
166166
}
167167

168168
@Override
@@ -211,8 +211,8 @@ public Iterator<HttpHeader> iterator() {
211211

212212
@Override
213213
public Stream<HttpHeader> stream() {
214-
return StreamSupport.stream(nettyHeaders.spliterator(), false)
215-
.map(e -> new NettyHttpHeader(this, e.getKey(), e.getValue()));
214+
return nettyHeaders.names().stream()
215+
.map(name -> new NettyHttpHeader(this, name, nettyHeaders.getAll(name)));
216216
}
217217

218218
static class NettyHttpHeader extends HttpHeader {

sdk/core/azure-core-http-netty/src/test/java/com/azure/core/http/netty/ReactorNettyClientResponseTransformer.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@
1111
import com.github.tomakehurst.wiremock.http.Request;
1212
import com.github.tomakehurst.wiremock.http.Response;
1313

14+
import static com.azure.core.http.netty.ReactorNettyClientTests.EXPECTED_HEADER;
15+
import static com.azure.core.http.netty.ReactorNettyClientTests.NO_DOUBLE_UA_PATH;
16+
1417
/**
1518
* Mock response transformer used to test {@link NettyAsyncHttpClient}.
1619
*/
@@ -24,6 +27,14 @@ public Response transform(Request request, Response response, FileSource fileSou
2427

2528
if ("/httpHeaders".equalsIgnoreCase(url)) {
2629
return httpHeadersResponseHandler(request, response);
30+
} else if (NO_DOUBLE_UA_PATH.equalsIgnoreCase(url)) {
31+
if (EXPECTED_HEADER.equals(request.getHeader("User-Agent"))) {
32+
return response;
33+
} else {
34+
return Response.response()
35+
.status(400)
36+
.build();
37+
}
2738
}
2839

2940
return response;

sdk/core/azure-core-http-netty/src/test/java/com/azure/core/http/netty/ReactorNettyClientTests.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,9 @@ public class ReactorNettyClientTests {
5353
private static final String SHORT_BODY_PATH = "/short";
5454
private static final String LONG_BODY_PATH = "/long";
5555

56+
static final String NO_DOUBLE_UA_PATH = "/noDoubleUA";
57+
static final String EXPECTED_HEADER = "userAgent";
58+
5659
private static final String SHORT_BODY = "hi there";
5760
private static final String LONG_BODY = createLongBody();
5861

@@ -74,6 +77,8 @@ public static void beforeClass() {
7477
server.stubFor(post("/shortPost").willReturn(aResponse().withBody(SHORT_BODY)));
7578
server.stubFor(post("/httpHeaders").willReturn(aResponse()
7679
.withTransformers(ReactorNettyClientResponseTransformer.NAME)));
80+
server.stubFor(get(NO_DOUBLE_UA_PATH).willReturn(aResponse()
81+
.withTransformers(ReactorNettyClientResponseTransformer.NAME)));
7782
server.start();
7883
// ResourceLeakDetector.setLevel(Level.PARANOID);
7984
}
@@ -373,6 +378,16 @@ public void requestHeader(String headerValue, String expectedValue) {
373378
.verifyComplete();
374379
}
375380

381+
@Test
382+
public void validateRequestHasOneUserAgentHeader() {
383+
HttpClient httpClient = new ReactorNettyClientProvider().createInstance();
384+
385+
StepVerifier.create(httpClient.send(new HttpRequest(HttpMethod.GET, url(server, NO_DOUBLE_UA_PATH),
386+
new HttpHeaders().set("User-Agent", EXPECTED_HEADER), Flux.empty())))
387+
.assertNext(response -> assertEquals(200, response.getStatusCode()))
388+
.verifyComplete();
389+
}
390+
376391
private static Stream<Arguments> requestHeaderSupplier() {
377392
return Stream.of(
378393
Arguments.of(null, ReactorNettyClientResponseTransformer.NULL_REPLACEMENT),
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
package com.azure.core.http.netty.implementation;
5+
6+
import com.azure.core.http.HttpHeader;
7+
import io.netty.handler.codec.http.DefaultHttpHeaders;
8+
import io.netty.handler.codec.http.HttpHeaders;
9+
import org.junit.jupiter.params.ParameterizedTest;
10+
import org.junit.jupiter.params.provider.Arguments;
11+
import org.junit.jupiter.params.provider.MethodSource;
12+
13+
import java.util.ArrayList;
14+
import java.util.Arrays;
15+
import java.util.Collections;
16+
import java.util.List;
17+
import java.util.function.Function;
18+
import java.util.stream.Collectors;
19+
import java.util.stream.Stream;
20+
21+
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
22+
import static org.junit.jupiter.api.Assertions.assertEquals;
23+
import static org.junit.jupiter.api.Assertions.assertLinesMatch;
24+
import static org.junit.jupiter.api.Assertions.assertNull;
25+
26+
/**
27+
* Tests for {@link NettyToAzureCoreHttpHeadersWrapper}
28+
*/
29+
public class NettyToAzureCoreHttpHeadersWrapperTests {
30+
@ParameterizedTest
31+
@MethodSource("getValueSupplier")
32+
public void getValue(HttpHeaders nettyHeaders, String key, String expected) {
33+
assertEquals(expected, createHeaderWrapper(nettyHeaders).getValue(key));
34+
}
35+
36+
@ParameterizedTest
37+
@MethodSource("getValuesSupplier")
38+
public void getValues(HttpHeaders nettyHeaders, String key, String[] expected) {
39+
assertArrayEquals(expected, createHeaderWrapper(nettyHeaders).getValues(key));
40+
}
41+
42+
@ParameterizedTest
43+
@MethodSource("getHeaderValueSupplier")
44+
public void getHeaderValue(HttpHeaders nettyHeaders, String key, String expected) {
45+
NettyToAzureCoreHttpHeadersWrapper headerWrapper = createHeaderWrapper(nettyHeaders);
46+
if (expected == null) {
47+
assertNull(headerWrapper.get(key));
48+
} else {
49+
assertEquals(expected, headerWrapper.get(key).getValue());
50+
}
51+
}
52+
53+
@ParameterizedTest
54+
@MethodSource("getHeaderValuesSupplier")
55+
public void getHeaderValues(HttpHeaders nettyHeaders, String key, String[] expected) {
56+
NettyToAzureCoreHttpHeadersWrapper headerWrapper = createHeaderWrapper(nettyHeaders);
57+
if (expected == null) {
58+
assertNull(headerWrapper.get(key));
59+
} else {
60+
assertArrayEquals(expected, headerWrapper.get(key).getValues());
61+
}
62+
}
63+
64+
@ParameterizedTest
65+
@MethodSource("getHeaderValuesListSupplier")
66+
public void getHeaderValuesList(HttpHeaders nettyHeaders, String key, List<String> expected) {
67+
NettyToAzureCoreHttpHeadersWrapper headerWrapper = createHeaderWrapper(nettyHeaders);
68+
if (expected == null) {
69+
assertNull(headerWrapper.get(key));
70+
} else {
71+
assertLinesMatch(expected, headerWrapper.get(key).getValuesList());
72+
}
73+
}
74+
75+
private NettyToAzureCoreHttpHeadersWrapper createHeaderWrapper(HttpHeaders nettyHeaders) {
76+
return new NettyToAzureCoreHttpHeadersWrapper(nettyHeaders);
77+
}
78+
79+
80+
private static Stream<Arguments> getValueSupplier() {
81+
return getSupplierBase(value -> value);
82+
}
83+
84+
private static Stream<Arguments> getValuesSupplier() {
85+
return getSupplierBase(stringValue -> {
86+
if (stringValue == null) {
87+
return null;
88+
} else {
89+
return stringValue.split(",");
90+
}
91+
});
92+
}
93+
94+
private static Stream<Arguments> getHeaderValueSupplier() {
95+
return getSupplierBase(value -> value);
96+
}
97+
98+
private static Stream<Arguments> getHeaderValuesSupplier() {
99+
return getSupplierBase(stringValue -> {
100+
if (stringValue == null) {
101+
return null;
102+
} else {
103+
return stringValue.split(",");
104+
}
105+
});
106+
}
107+
108+
private static Stream<Arguments> getHeaderValuesListSupplier() {
109+
return getSupplierBase(stringValue -> {
110+
if (stringValue == null) {
111+
return null;
112+
} else {
113+
return Arrays.asList(stringValue.split(","));
114+
}
115+
});
116+
}
117+
118+
private static Stream<Arguments> getSupplierBase(Function<String, Object> expectedConverter) {
119+
// Null
120+
HttpHeaders nullValueHeader = new DefaultHttpHeaders()
121+
.remove("test");
122+
123+
// Single value
124+
HttpHeaders singleValueHeader = new DefaultHttpHeaders()
125+
.set("test", "value");
126+
127+
// Overwritten value
128+
HttpHeaders overwrittenValueHeader = new DefaultHttpHeaders()
129+
.set("test", "value")
130+
.set("test", "value2");
131+
132+
// Multi-value
133+
HttpHeaders multiValueHeader = new DefaultHttpHeaders()
134+
.set("test", "value")
135+
.add("test", "value2");
136+
137+
return Stream.of(
138+
Arguments.of(new DefaultHttpHeaders(), "notAKey", null),
139+
Arguments.of(nullValueHeader, "test", expectedConverter.apply(null)),
140+
Arguments.of(singleValueHeader, "test", expectedConverter.apply("value")),
141+
Arguments.of(overwrittenValueHeader, "test", expectedConverter.apply("value2")),
142+
Arguments.of(multiValueHeader, "test", expectedConverter.apply("value,value2"))
143+
);
144+
}
145+
146+
@ParameterizedTest
147+
@MethodSource("httpHeaderIteratorSupplier")
148+
public void httpHeaderIterator(HttpHeaders nettyHeaders, List<HttpHeader> expected) {
149+
List<HttpHeader> actual = new ArrayList<>();
150+
createHeaderWrapper(nettyHeaders)
151+
.iterator()
152+
.forEachRemaining(actual::add);
153+
154+
assertEquals(expected.size(), actual.size());
155+
for (int i = 0; i < expected.size(); i++) {
156+
HttpHeader expectedHeader = expected.get(i);
157+
HttpHeader actualHeader = actual.get(i);
158+
159+
assertEquals(expectedHeader.getName(), actualHeader.getName());
160+
assertEquals(expectedHeader.getValue(), actualHeader.getValue());
161+
assertArrayEquals(expectedHeader.getValues(), actualHeader.getValues());
162+
assertLinesMatch(expectedHeader.getValuesList(), actualHeader.getValuesList());
163+
}
164+
}
165+
166+
@ParameterizedTest
167+
@MethodSource("httpHeaderIteratorSupplier")
168+
public void httpHeaderStream(HttpHeaders nettyHeaders, List<HttpHeader> expected) {
169+
List<HttpHeader> actual = createHeaderWrapper(nettyHeaders)
170+
.stream()
171+
.collect(Collectors.toList());
172+
173+
assertEquals(expected.size(), actual.size());
174+
for (int i = 0; i < expected.size(); i++) {
175+
HttpHeader expectedHeader = expected.get(i);
176+
HttpHeader actualHeader = actual.get(i);
177+
178+
assertEquals(expectedHeader.getName(), actualHeader.getName());
179+
assertEquals(expectedHeader.getValue(), actualHeader.getValue());
180+
assertArrayEquals(expectedHeader.getValues(), actualHeader.getValues());
181+
assertLinesMatch(expectedHeader.getValuesList(), actualHeader.getValuesList());
182+
}
183+
}
184+
185+
private static Stream<Arguments> httpHeaderIteratorSupplier() {
186+
// No header
187+
HttpHeaders nullValueHeader = new DefaultHttpHeaders()
188+
.remove("test");
189+
190+
// Single value header
191+
HttpHeaders singleValueHeader = new DefaultHttpHeaders()
192+
.set("test", "value");
193+
194+
// Overwritten value header
195+
HttpHeaders overwrittenValueHeader = new DefaultHttpHeaders()
196+
.set("test", "value")
197+
.set("test", "value2");
198+
199+
// Multi-value header
200+
HttpHeaders multiValueHeader = new DefaultHttpHeaders()
201+
.set("test", "value")
202+
.add("test", "value2");
203+
204+
// Single value headers
205+
HttpHeaders singleValueHeaders = new DefaultHttpHeaders()
206+
.set("test", "value")
207+
.set("test2", "value");
208+
209+
// Overwritten value headers
210+
HttpHeaders overwrittenValueHeaders = new DefaultHttpHeaders()
211+
.set("test", "value")
212+
.set("test", "value2")
213+
.set("test2", "value")
214+
.set("test2", "value2");
215+
216+
// Multi-value headers
217+
HttpHeaders multiValueHeaders = new DefaultHttpHeaders()
218+
.set("test", "value")
219+
.add("test", "value2")
220+
.set("test2", "value")
221+
.add("test2", "value2");
222+
223+
return Stream.of(
224+
Arguments.of(nullValueHeader, Collections.emptyList()),
225+
Arguments.of(singleValueHeader, Collections.singletonList(new HttpHeader("test", "value"))),
226+
Arguments.of(overwrittenValueHeader, Collections.singletonList(new HttpHeader("test", "value2"))),
227+
Arguments.of(multiValueHeader, Collections.singletonList(new HttpHeader("test",
228+
Arrays.asList("value", "value2")))),
229+
Arguments.of(singleValueHeaders, Arrays.asList(new HttpHeader("test", "value"),
230+
new HttpHeader("test2", "value"))),
231+
Arguments.of(overwrittenValueHeaders, Arrays.asList(new HttpHeader("test", "value2"),
232+
new HttpHeader("test2", "value2"))),
233+
Arguments.of(multiValueHeaders, Arrays.asList(new HttpHeader("test", Arrays.asList("value", "value2")),
234+
new HttpHeader("test2", Arrays.asList("value", "value2"))))
235+
);
236+
}
237+
}

sdk/formrecognizer/azure-ai-formrecognizer/pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
<dependency>
4242
<groupId>com.azure</groupId>
4343
<artifactId>azure-core-http-netty</artifactId>
44-
<version>1.7.1</version> <!-- {x-version-update;com.azure:azure-core-http-netty;dependency} -->
44+
<version>1.8.0-beta.1</version> <!-- {x-version-update;unreleased_com.azure:azure-core-http-netty;dependency} -->
4545
</dependency>
4646

4747
<!-- Test Dependencies -->

0 commit comments

Comments
 (0)