nastra commented on code in PR #11769: URL: https://github.com/apache/iceberg/pull/11769#discussion_r1883552348
########## core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java: ########## @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest; + +import com.fasterxml.jackson.databind.ObjectMapper; +import java.net.URI; +import java.util.Map; +import javax.annotation.Nullable; +import org.immutables.value.Value; + +/** Represents an HTTP request. */ +@Value.Style(redactedMask = "****", depluralize = true) +@Value.Immutable +@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"}) +public interface HTTPRequest { + + enum HTTPMethod { + GET, + HEAD, + POST, + DELETE + } + + /** + * Returns the base URI configured at the REST client level. The base URI is used to construct the + * full {@link #requestUri()}. + */ + URI baseUri(); + + /** + * Returns the full URI of this request. The URI is constructed from the base URI, path, and query + * parameters. It cannot be modified directly. + */ + @Value.Lazy + default URI requestUri() { + return RESTUtil.buildRequestUri(this); Review Comment: to me it seems like we should rather embed the functionality of building an URI from a `HTTPRequest` here (and also do the same for encoding the body). Additionally, building an URI from the path does some validation to make sure it doesn't start with a `/`. I'd say this is something that we can move into a check method annotated with `@Value.Check`. That way we don't need the util methods in `RESTUtil` and can move testing into `TestHTTPRequest`. I've added an example diff below: ``` --- a/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java @@ -18,10 +18,14 @@ */ package org.apache.iceberg.rest; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import java.net.URI; +import java.net.URISyntaxException; import java.util.Map; import javax.annotation.Nullable; +import org.apache.hc.core5.net.URIBuilder; +import org.apache.iceberg.exceptions.RESTException; import org.immutables.value.Value; /** Represents an HTTP request. */ @@ -49,7 +53,19 @@ public interface HTTPRequest { */ @Value.Lazy default URI requestUri() { - return RESTUtil.buildRequestUri(this); + // if full path is provided, use the input path as path + String fullPath = + (path().startsWith("https://";) || path().startsWith("http://";)) + ? path() + : String.format("%s/%s", baseUri(), path()); + try { + URIBuilder builder = new URIBuilder(RESTUtil.stripTrailingSlash(fullPath)); + queryParameters().forEach(builder::addParameter); + return builder.build(); + } catch (URISyntaxException e) { + throw new RESTException( + "Failed to create request URI from base %s, params %s", fullPath, queryParameters()); + } } /** Returns the HTTP method of this request. */ @@ -77,7 +93,17 @@ public interface HTTPRequest { @Nullable @Value.Redacted default String encodedBody() { - return RESTUtil.encodeRequestBody(this); + Object body = body(); + if (body instanceof Map) { + return RESTUtil.encodeFormData((Map<?, ?>) body); + } else if (body != null) { + try { + return mapper().writeValueAsString(body); + } catch (JsonProcessingException e) { + throw new RESTException(e, "Failed to encode request body: %s", body); + } + } + return null; } /** @@ -88,4 +114,13 @@ public interface HTTPRequest { default ObjectMapper mapper() { return RESTObjectMapper.mapper(); } + + @Value.Check + default void check() { + if (path().startsWith("/")) { + throw new RESTException( + "Received a malformed path for a REST request: %s. Paths should not start with /", + path()); + } + } } ``` -- 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
