danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1926099736
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -214,92 +225,63 @@ private static void throwFailure(
throw new RESTException("Unhandled error: %s", errorResponse);
}
- private URI buildUri(String path, Map<String, String> params) {
- // if full path is provided, use the input path as path
- if (path.startsWith("/")) {
- throw new RESTException(
- "Received a malformed path for a REST request: %s. Paths should not
start with /", path);
- }
- String fullPath =
- (path.startsWith("https://";) || path.startsWith("http://";))
- ? path
- : String.format("%s/%s", uri, path);
- try {
- URIBuilder builder = new URIBuilder(fullPath);
- if (params != null) {
- params.forEach(builder::addParameter);
- }
- return builder.build();
- } catch (URISyntaxException e) {
- throw new RESTException(
- "Failed to create request URI from base %s, params %s", fullPath,
params);
- }
- }
-
- /**
- * Method to execute an HTTP request and process the corresponding response.
- *
- * @param method - HTTP method, such as GET, POST, HEAD, etc.
- * @param queryParams - A map of query parameters
- * @param path - URI to send the request to
- * @param requestBody - Content to place in the request body
- * @param responseType - Class of the Response type. Needs to have
serializer registered with
- * ObjectMapper
- * @param errorHandler - Error handler delegated for HTTP responses which
handles server error
- * responses
- * @param <T> - Class type of the response for deserialization. Must be
registered with the
- * ObjectMapper.
- * @return The response entity, parsed and converted to its type T
- */
- private <T> T execute(
- Method method,
+ @Override
+ protected HTTPRequest buildRequest(
+ HTTPMethod method,
String path,
Map<String, String> queryParams,
- Object requestBody,
- Class<T> responseType,
Map<String, String> headers,
- Consumer<ErrorResponse> errorHandler) {
- return execute(
- method, path, queryParams, requestBody, responseType, headers,
errorHandler, h -> {});
+ Object body) {
+
+ ImmutableHTTPRequest.Builder builder =
+ ImmutableHTTPRequest.builder()
+ .baseUri(baseUri)
+ .mapper(mapper)
+ .method(method)
+ .path(path)
+ .body(body)
+ .queryParameters(queryParams == null ? Map.of() : queryParams);
+
+ Map<String, String> allHeaders = Maps.newLinkedHashMap();
+ if (headers != null) {
+ allHeaders.putAll(headers);
+ }
+
+ allHeaders.putIfAbsent(HttpHeaders.ACCEPT,
ContentType.APPLICATION_JSON.getMimeType());
+
+ // Many systems require that content type is set regardless and will fail,
+ // even on an empty bodied request.
+ // Encode maps as form data (application/x-www-form-urlencoded),
+ // and other requests are assumed to contain JSON bodies
(application/json).
+ ContentType mimeType =
+ body instanceof Map
+ ? ContentType.APPLICATION_FORM_URLENCODED
+ : ContentType.APPLICATION_JSON;
+ allHeaders.putIfAbsent(HttpHeaders.CONTENT_TYPE, mimeType.getMimeType());
+
+ // Apply base headers now to mimic the behavior of
+ // org.apache.hc.client5.http.protocol.RequestDefaultHeaders
+ // We want these headers applied *before* the AuthSession authenticates
the request.
+ if (baseHeaders != null) {
Review Comment:
Doesn't this mean that `baseHeaders` will override the provided specific
headers, the mime type headers and accept headers? It feels like this should
go first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
