diff --git a/sdk/core/azure-core-http-jdk-httpclient/pom.xml b/sdk/core/azure-core-http-jdk-httpclient/pom.xml index 371108942cb66..4c2f2723a5e07 100644 --- a/sdk/core/azure-core-http-jdk-httpclient/pom.xml +++ b/sdk/core/azure-core-http-jdk-httpclient/pom.xml @@ -126,27 +126,6 @@ 5.9.3 test - - - org.mockito - mockito-core - 4.11.0 - test - - - - - net.bytebuddy - byte-buddy - 1.14.8 - test - - - net.bytebuddy - byte-buddy-agent - 1.14.8 - test - diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClient.java b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClient.java index 3d25ff329d722..99d6c23173458 100644 --- a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClient.java +++ b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClient.java @@ -4,28 +4,22 @@ package com.azure.core.http.jdk.httpclient; import com.azure.core.http.HttpClient; -import com.azure.core.http.HttpHeader; import com.azure.core.http.HttpHeaders; import com.azure.core.http.HttpRequest; import com.azure.core.http.HttpResponse; -import com.azure.core.http.jdk.httpclient.implementation.BodyPublisherUtils; +import com.azure.core.http.jdk.httpclient.implementation.AzureJdkHttpRequest; import com.azure.core.http.jdk.httpclient.implementation.JdkHttpResponseAsync; import com.azure.core.http.jdk.httpclient.implementation.JdkHttpResponseSync; import com.azure.core.util.Context; -import com.azure.core.util.Contexts; import com.azure.core.util.CoreUtils; -import com.azure.core.util.ProgressReporter; import com.azure.core.util.logging.ClientLogger; -import reactor.core.Exceptions; import reactor.core.publisher.Mono; import java.io.IOException; import java.io.UncheckedIOException; -import java.net.URISyntaxException; import java.util.Set; import static com.azure.core.http.jdk.httpclient.implementation.JdkHttpUtils.fromJdkHttpHeaders; -import static java.net.http.HttpRequest.BodyPublishers.noBody; import static java.net.http.HttpResponse.BodyHandlers.ofByteArray; import static java.net.http.HttpResponse.BodyHandlers.ofInputStream; import static java.net.http.HttpResponse.BodyHandlers.ofPublisher; @@ -119,38 +113,7 @@ public HttpResponse sendSync(HttpRequest request, Context context) { * @return the HttpRequest */ private java.net.http.HttpRequest toJdkHttpRequest(HttpRequest request, Context context) { - ProgressReporter progressReporter = Contexts.with(context).getHttpRequestProgressReporter(); - - final java.net.http.HttpRequest.Builder builder = java.net.http.HttpRequest.newBuilder(); - try { - builder.uri(request.getUrl().toURI()); - } catch (URISyntaxException e) { - throw LOGGER.logExceptionAsError(Exceptions.propagate(e)); - } - final HttpHeaders headers = request.getHeaders(); - if (headers != null) { - for (HttpHeader header : headers) { - final String headerName = header.getName(); - if (!restrictedHeaders.contains(headerName)) { - header.getValuesList().forEach(headerValue -> builder.header(headerName, headerValue)); - } else { - LOGGER.warning("The header '" + headerName + "' is restricted by default in JDK HttpClient 12 " - + "and above. This header can be added to allow list in JAVA_HOME/conf/net.properties " - + "or in System.setProperty() or in Configuration. Use the key 'jdk.httpclient" - + ".allowRestrictedHeaders' and a comma separated list of header names."); - } - } - } - switch (request.getHttpMethod()) { - case GET: - return builder.GET().build(); - case HEAD: - return builder.method("HEAD", noBody()).build(); - default: - java.net.http.HttpRequest.BodyPublisher bodyPublisher = BodyPublisherUtils.toBodyPublisher(request, - progressReporter); - return builder.method(request.getHttpMethod().toString(), bodyPublisher).build(); - } + return new AzureJdkHttpRequest(request, context, restrictedHeaders, LOGGER); } /** diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilder.java b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilder.java index f0dacdee13fa3..4c2065562ffdd 100644 --- a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilder.java +++ b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilder.java @@ -18,10 +18,11 @@ import java.nio.file.Paths; import java.time.Duration; import java.util.Collections; +import java.util.HashSet; +import java.util.Locale; import java.util.Objects; import java.util.Properties; import java.util.Set; -import java.util.TreeSet; import java.util.concurrent.Executor; /** @@ -44,15 +45,7 @@ public class JdkHttpClientBuilder { static final Set DEFAULT_RESTRICTED_HEADERS; static { - TreeSet treeSet = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); - treeSet.addAll(Set.of( - "connection", - "content-length", - "expect", - "host", - "upgrade" - )); - DEFAULT_RESTRICTED_HEADERS = Collections.unmodifiableSet(treeSet); + DEFAULT_RESTRICTED_HEADERS = Set.of("connection", "content-length", "expect", "host", "upgrade"); } private static final ClientLogger LOGGER = new ClientLogger(JdkHttpClientBuilder.class); @@ -204,37 +197,29 @@ public HttpClient build() { Set getRestrictedHeaders() { // Compute the effective restricted headers by removing the allowed headers from default restricted headers - Set allowRestrictedHeaders = getAllowRestrictedHeaders(); - Set restrictedHeaders = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); - restrictedHeaders.addAll(DEFAULT_RESTRICTED_HEADERS); - restrictedHeaders.removeAll(allowRestrictedHeaders); + Set restrictedHeaders = new HashSet<>(DEFAULT_RESTRICTED_HEADERS); + removeAllowedHeaders(restrictedHeaders); return restrictedHeaders; } - private Set getAllowRestrictedHeaders() { + private void removeAllowedHeaders(Set restrictedHeaders) { Properties properties = getNetworkProperties(); String[] allowRestrictedHeadersNetProperties = properties.getProperty(JDK_HTTPCLIENT_ALLOW_RESTRICTED_HEADERS, "").split(","); // Read all allowed restricted headers from configuration - Configuration config = (this.configuration == null) - ? Configuration.getGlobalConfiguration() - : configuration; + Configuration config = (this.configuration == null) ? Configuration.getGlobalConfiguration() : configuration; String[] allowRestrictedHeadersSystemProperties = config.get(JDK_HTTPCLIENT_ALLOW_RESTRICTED_HEADERS, "") .split(","); - Set allowRestrictedHeaders = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); - // Combine the set of all allowed restricted headers from both sources for (String header : allowRestrictedHeadersSystemProperties) { - allowRestrictedHeaders.add(header.trim()); + restrictedHeaders.remove(header.trim().toLowerCase(Locale.ROOT)); } for (String header : allowRestrictedHeadersNetProperties) { - allowRestrictedHeaders.add(header.trim()); + restrictedHeaders.remove(header.trim().toLowerCase(Locale.ROOT)); } - - return allowRestrictedHeaders; } Properties getNetworkProperties() { diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/implementation/AzureJdkHttpRequest.java b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/implementation/AzureJdkHttpRequest.java new file mode 100644 index 0000000000000..dc18b2689fd08 --- /dev/null +++ b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/implementation/AzureJdkHttpRequest.java @@ -0,0 +1,101 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +package com.azure.core.http.jdk.httpclient.implementation; + +import com.azure.core.http.HttpMethod; +import com.azure.core.implementation.util.HttpHeadersAccessHelper; +import com.azure.core.util.Context; +import com.azure.core.util.Contexts; +import com.azure.core.util.ProgressReporter; +import com.azure.core.util.logging.ClientLogger; +import reactor.core.Exceptions; + +import java.net.URI; +import java.net.URISyntaxException; +import java.net.http.HttpClient; +import java.net.http.HttpHeaders; +import java.net.http.HttpRequest; +import java.time.Duration; +import java.util.Optional; +import java.util.Set; +import java.util.TreeMap; + +import static java.net.http.HttpRequest.BodyPublishers.noBody; + +/** + * Implementation of the JDK {@link HttpRequest}. + *

+ * Using this instead of {@link HttpRequest#newBuilder()} allows us to optimize some cases now allowed by the builder. + * For example, setting headers requires each key-value for the same header to be set individually. This class allows + * us to set all headers at once. And given that the headers are backed by a {@link TreeMap} it reduces the number of + * String comparisons performed. + */ +public final class AzureJdkHttpRequest extends HttpRequest { + private final BodyPublisher bodyPublisher; + private final String method; + private final URI uri; + private final HttpHeaders headers; + + /** + * Creates a new instance of the JDK HttpRequest. + * + * @param azureCoreRequest The Azure Core request to create the JDK HttpRequest from. + * @param context The context of the request. + * @param restrictedHeaders The set of restricted headers. + * @param logger The logger to log warnings to. + */ + public AzureJdkHttpRequest(com.azure.core.http.HttpRequest azureCoreRequest, Context context, + Set restrictedHeaders, ClientLogger logger) { + HttpMethod method = azureCoreRequest.getHttpMethod(); + ProgressReporter progressReporter = Contexts.with(context).getHttpRequestProgressReporter(); + + this.method = method.toString(); + this.bodyPublisher = (method == HttpMethod.GET || method == HttpMethod.HEAD) + ? noBody() : BodyPublisherUtils.toBodyPublisher(azureCoreRequest, progressReporter); + + try { + uri = azureCoreRequest.getUrl().toURI(); + } catch (URISyntaxException e) { + throw logger.logExceptionAsError(Exceptions.propagate(e)); + } + + this.headers = HttpHeaders.of(new HeaderFilteringMap( + HttpHeadersAccessHelper.getRawHeaderMap(azureCoreRequest.getHeaders()), restrictedHeaders, logger), + (ignored1, ignored2) -> true); + } + + @Override + public Optional bodyPublisher() { + return Optional.ofNullable(bodyPublisher); + } + + @Override + public String method() { + return method; + } + + @Override + public Optional timeout() { + return Optional.empty(); + } + + @Override + public boolean expectContinue() { + return false; + } + + @Override + public URI uri() { + return uri; + } + + @Override + public Optional version() { + return Optional.empty(); + } + + @Override + public HttpHeaders headers() { + return headers; + } +} diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/implementation/HeaderFilteringMap.java b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/implementation/HeaderFilteringMap.java new file mode 100644 index 0000000000000..944ff536a8f11 --- /dev/null +++ b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/implementation/HeaderFilteringMap.java @@ -0,0 +1,62 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +package com.azure.core.http.jdk.httpclient.implementation; + +import com.azure.core.http.HttpHeader; +import com.azure.core.util.logging.ClientLogger; + +import java.util.AbstractMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.BiConsumer; +import java.util.function.BiPredicate; + +/** + * This class is a {@link Map} implementation that filters out headers that are not allowed to be set on an HTTP + * request. + *

+ * Based on logic used in {@link java.net.http.HttpHeaders#of(Map, BiPredicate)} it's known that the headers will be + * accessed using {@link Map#forEach(BiConsumer)}. Give that, this class can be optimized to use the forEach method + * on the raw HttpHeaders map used in azure-core which can filter out the headers that are not allowed to be set using + * the known lowercase header names, then return the cased header name and values. + */ +final class HeaderFilteringMap extends AbstractMap> { + private final Map rawHeaders; + private final Set restrictedHeaders; + private final ClientLogger logger; + + /** + * Creates a new HeaderFilteringMap. + * + * @param rawHeaders The raw headers map. + * @param restrictedHeaders The header filter. + * @param logger The logger to log any errors. + */ + HeaderFilteringMap(Map rawHeaders, Set restrictedHeaders, ClientLogger logger) { + this.rawHeaders = rawHeaders; + this.restrictedHeaders = restrictedHeaders; + this.logger = logger; + } + + @Override + public Set>> entrySet() { + throw logger.logExceptionAsError( + new UnsupportedOperationException("The only operation permitted by this Map is forEach.")); + } + + @Override + public void forEach(BiConsumer> action) { + rawHeaders.forEach((headerName, header) -> { + if (restrictedHeaders.contains(headerName)) { + logger.atWarning() + .addKeyValue("headerName", headerName) + .log("The header is restricted by 'java.net.http.HttpClient' and will be ignored. To allow this " + + "header to be set on the request, configure 'jdk.httpclient.allowRestrictedHeaders' with the " + + "header added in the comma-separated list."); + } else { + action.accept(header.getName(), header.getValuesList()); + } + }); + } +} diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilderTests.java b/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilderTests.java index 0cd6ab14ec8af..02d294c223f41 100644 --- a/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilderTests.java +++ b/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkHttpClientBuilderTests.java @@ -40,8 +40,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.when; /** * Tests {@link JdkHttpClientBuilder}. @@ -220,10 +218,10 @@ private static Stream buildWithExplicitConfigurationProxySupplier() { @Test void testAllowedHeadersFromNetworkProperties() { - JdkHttpClientBuilder jdkHttpClientBuilder = spy(new JdkHttpClientBuilder()); Properties properties = new Properties(); properties.put("jdk.httpclient.allowRestrictedHeaders", "content-length, upgrade"); - when(jdkHttpClientBuilder.getNetworkProperties()).thenReturn(properties); + + JdkHttpClientBuilder jdkHttpClientBuilder = new JdkHttpClientBuilderOverriddenProperties(properties); Set expectedRestrictedHeaders = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); expectedRestrictedHeaders.addAll(com.azure.core.http.jdk.httpclient.JdkHttpClientBuilder.DEFAULT_RESTRICTED_HEADERS); @@ -239,11 +237,10 @@ void testAllowedHeadersFromConfiguration() { EMPTY_SOURCE) .build(); - JdkHttpClientBuilder jdkHttpClientBuilder = spy( - new JdkHttpClientBuilder().configuration(configuration)); - Properties properties = new Properties(); - when(jdkHttpClientBuilder.getNetworkProperties()).thenReturn(properties); + + JdkHttpClientBuilder jdkHttpClientBuilder = new JdkHttpClientBuilderOverriddenProperties(properties) + .configuration(configuration); Set expectedRestrictedHeaders = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); expectedRestrictedHeaders.addAll(com.azure.core.http.jdk.httpclient.JdkHttpClientBuilder.DEFAULT_RESTRICTED_HEADERS); @@ -259,12 +256,11 @@ void testAllowedHeadersFromBoth() { new TestConfigurationSource()) .build(); - JdkHttpClientBuilder jdkHttpClientBuilder = spy( - new JdkHttpClientBuilder().configuration(configuration)); - Properties properties = new Properties(); properties.put("jdk.httpclient.allowRestrictedHeaders", "host, connection, upgrade"); - when(jdkHttpClientBuilder.getNetworkProperties()).thenReturn(properties); + + JdkHttpClientBuilder jdkHttpClientBuilder = new JdkHttpClientBuilderOverriddenProperties(properties) + .configuration(configuration); Set expectedRestrictedHeaders = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); expectedRestrictedHeaders.addAll(com.azure.core.http.jdk.httpclient.JdkHttpClientBuilder.DEFAULT_RESTRICTED_HEADERS); @@ -278,8 +274,7 @@ void testAllowedHeadersFromSystemProperties() { Properties properties = new Properties(); properties.setProperty("jdk.httpclient.allowRestrictedHeaders", "content-length, upgrade"); - JdkHttpClientBuilder jdkHttpClientBuilder = spy(new JdkHttpClientBuilder()); - when(jdkHttpClientBuilder.getNetworkProperties()).thenReturn(properties); + JdkHttpClientBuilder jdkHttpClientBuilder = new JdkHttpClientBuilderOverriddenProperties(properties); Set expectedRestrictedHeaders = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); expectedRestrictedHeaders.addAll(com.azure.core.http.jdk.httpclient.JdkHttpClientBuilder.DEFAULT_RESTRICTED_HEADERS); @@ -293,17 +288,12 @@ void testCaseInsensitivity() { Properties properties = new Properties(); properties.setProperty("jdk.httpclient.allowRestrictedHeaders", "content-LENGTH"); - JdkHttpClientBuilder jdkHttpClientBuilder = spy(new JdkHttpClientBuilder()); - when(jdkHttpClientBuilder.getNetworkProperties()).thenReturn(properties); + JdkHttpClientBuilder jdkHttpClientBuilder = new JdkHttpClientBuilderOverriddenProperties(properties); Set restrictedHeaders = jdkHttpClientBuilder.getRestrictedHeaders(); - assertTrue(restrictedHeaders.contains("Connection"), "connection header is missing"); assertTrue(restrictedHeaders.contains("connection"), "connection header is missing"); - assertTrue(restrictedHeaders.contains("CONNECTION"), "connection header is missing"); - assertFalse(restrictedHeaders.contains("Content-Length"), "content-length not removed"); assertFalse(restrictedHeaders.contains("content-length"), "content-length not removed"); - assertFalse(restrictedHeaders.contains("CONTENT-length"), "content-length not removed"); } @@ -325,4 +315,17 @@ private void validateRestrictedHeaders(JdkHttpClientBuilder jdkHttpClientBuilder assertEquals(expectedRestrictedHeaders, restrictedHeaders); } + private static final class JdkHttpClientBuilderOverriddenProperties extends JdkHttpClientBuilder { + private final Properties properties; + + JdkHttpClientBuilderOverriddenProperties(Properties properties) { + this.properties = properties; + } + + @Override + Properties getNetworkProperties() { + return properties; + } + }; + } diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/http/HttpHeaders.java b/sdk/core/azure-core/src/main/java/com/azure/core/http/HttpHeaders.java index 499e457ca046c..2241ccafee961 100644 --- a/sdk/core/azure-core/src/main/java/com/azure/core/http/HttpHeaders.java +++ b/sdk/core/azure-core/src/main/java/com/azure/core/http/HttpHeaders.java @@ -3,6 +3,7 @@ package com.azure.core.http; +import com.azure.core.implementation.util.HttpHeadersAccessHelper; import com.azure.core.util.CoreUtils; import java.util.Collections; @@ -21,6 +22,10 @@ public class HttpHeaders implements Iterable { // This map is a case-insensitive key (i.e. lower-cased), but the returned HttpHeader key will be as-provided to us private final Map headers; + static { + HttpHeadersAccessHelper.setAccessor(headers -> headers.headers); + } + /** * Create an empty HttpHeaders instance. */ diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/implementation/util/HttpHeadersAccessHelper.java b/sdk/core/azure-core/src/main/java/com/azure/core/implementation/util/HttpHeadersAccessHelper.java new file mode 100644 index 0000000000000..304f082277fa2 --- /dev/null +++ b/sdk/core/azure-core/src/main/java/com/azure/core/implementation/util/HttpHeadersAccessHelper.java @@ -0,0 +1,50 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +package com.azure.core.implementation.util; + +import com.azure.core.http.HttpHeader; +import com.azure.core.http.HttpHeaders; + +import java.util.Map; + +/** + * This class is used to access internal methods on {@link HttpHeaders}. + */ +public final class HttpHeadersAccessHelper { + private static HttpHeadersAccessor accessor; + + /** + * Type defining the methods to access the internal methods on {@link HttpHeaders}. + */ + public interface HttpHeadersAccessor { + /** + * Gets the raw header map from {@link HttpHeaders}. + * + * @param headers The {@link HttpHeaders} to get the raw header map from. + * @return The raw header map. + */ + Map getRawHeaderMap(HttpHeaders headers); + } + + /** + * Gets the raw header map from {@link HttpHeaders}. + * + * @param headers The {@link HttpHeaders} to get the raw header map from. + * @return The raw header map. + */ + public static Map getRawHeaderMap(HttpHeaders headers) { + return accessor.getRawHeaderMap(headers); + } + + /** + * Sets the {@link HttpHeadersAccessor}. + * + * @param accessor The {@link HttpHeadersAccessor}. + */ + public static void setAccessor(HttpHeadersAccessor accessor) { + HttpHeadersAccessHelper.accessor = accessor; + } + + private HttpHeadersAccessHelper() { + } +}