[
https://issues.apache.org/jira/browse/GROOVY-11879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18067633#comment-18067633
]
ASF GitHub Bot commented on GROOVY-11879:
-----------------------------------------
Copilot commented on code in PR #2401:
URL: https://github.com/apache/groovy/pull/2401#discussion_r2974641276
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -0,0 +1,299 @@
+/*
+ * 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 groovy.http
+
+import groovy.lang.DelegatesTo
+import groovy.lang.Closure
+import groovy.json.JsonOutput
+import org.apache.groovy.lang.annotation.Incubating
+
+import java.net.URI
+import java.net.URLEncoder
+import java.net.http.HttpClient
+import java.net.http.HttpRequest
+import java.net.http.HttpResponse
+import java.nio.charset.StandardCharsets
+import java.time.Duration
+
+/**
+ * Tiny DSL over JDK {@link HttpClient}.
+ */
+@Incubating
+final class HttpBuilder {
+ private final HttpClient client
+ private final URI baseUri
+ private final Map<String, String> defaultHeaders
+ private final Duration defaultRequestTimeout
+
+ private HttpBuilder(final Config config) {
+ HttpClient.Builder clientBuilder = HttpClient.newBuilder()
+ if (config.connectTimeout != null) {
+ clientBuilder.connectTimeout(config.connectTimeout)
+ }
+ if (config.followRedirects) {
+ clientBuilder.followRedirects(HttpClient.Redirect.NORMAL)
+ }
+ client = clientBuilder.build()
+ baseUri = config.baseUri
+ defaultHeaders = Collections.unmodifiableMap(new
LinkedHashMap<>(config.headers))
+ defaultRequestTimeout = config.requestTimeout
+ }
+
+ static HttpBuilder http(
+ @DelegatesTo(value = Config, strategy = Closure.DELEGATE_FIRST)
+ final Closure<?> spec
+ ) {
+ Config config = new Config()
+ Closure<?> code = (Closure<?>) spec.clone()
+ code.resolveStrategy = Closure.DELEGATE_FIRST
+ code.delegate = config
+ code.call()
+ return new HttpBuilder(config)
+ }
+
+ static HttpBuilder http(final String baseUri) {
+ Config config = new Config()
+ config.baseUri(baseUri)
+ return new HttpBuilder(config)
+ }
+
+ HttpResult get(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('GET', uri, spec)
+ }
+
+ HttpResult post(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('POST', uri, spec)
+ }
+
+ HttpResult put(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('PUT', uri, spec)
+ }
+
+ HttpResult delete(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('DELETE', uri, spec)
+ }
+
+ HttpResult request(final String method,
+ final Object uri,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ RequestSpec requestSpec = new RequestSpec()
+ if (spec != null) {
+ Closure<?> code = (Closure<?>) spec.clone()
+ code.resolveStrategy = Closure.DELEGATE_FIRST
+ code.delegate = requestSpec
+ code.call()
+ }
+
+ URI resolvedUri = resolveUri(uri, requestSpec.queryParameters)
+ HttpRequest.Builder requestBuilder =
HttpRequest.newBuilder(resolvedUri)
+
+ Duration timeout = requestSpec.timeout ?: defaultRequestTimeout
+ if (timeout != null) {
+ requestBuilder.timeout(timeout)
+ }
+
+ defaultHeaders.each { String name, String value ->
+ requestBuilder.header(name, value)
+ }
+ requestSpec.headers.each { String name, String value ->
+ requestBuilder.setHeader(name, value)
+ }
+
+ requestBuilder.method(method, bodyPublisher(method, requestSpec.body))
+
+ HttpResponse<String> response = client.send(
+ requestBuilder.build(),
+ requestSpec.bodyHandler
+ )
Review Comment:
`HttpClient#send` can throw `IOException` and `InterruptedException`. Right
now those propagate without preserving the interrupt flag and without any
context about which request failed. Consider catching `InterruptedException` to
re-interrupt the thread and wrapping both exceptions in a runtime exception
that includes the HTTP method and resolved URI (or exposing a checked/typed
exception in the API).
```suggestion
HttpResponse<String> response
try {
response = client.send(
requestBuilder.build(),
requestSpec.bodyHandler
)
} catch (InterruptedException e) {
Thread.currentThread().interrupt()
throw new RuntimeException("HTTP request " + method + " " +
resolvedUri + " was interrupted", e)
} catch (java.io.IOException e) {
throw new RuntimeException("I/O error during HTTP request " +
method + " " + resolvedUri, e)
}
```
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -0,0 +1,299 @@
+/*
+ * 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 groovy.http
+
+import groovy.lang.DelegatesTo
+import groovy.lang.Closure
+import groovy.json.JsonOutput
+import org.apache.groovy.lang.annotation.Incubating
+
+import java.net.URI
+import java.net.URLEncoder
+import java.net.http.HttpClient
+import java.net.http.HttpRequest
+import java.net.http.HttpResponse
+import java.nio.charset.StandardCharsets
+import java.time.Duration
+
+/**
+ * Tiny DSL over JDK {@link HttpClient}.
+ */
+@Incubating
+final class HttpBuilder {
+ private final HttpClient client
+ private final URI baseUri
+ private final Map<String, String> defaultHeaders
+ private final Duration defaultRequestTimeout
+
+ private HttpBuilder(final Config config) {
+ HttpClient.Builder clientBuilder = HttpClient.newBuilder()
+ if (config.connectTimeout != null) {
+ clientBuilder.connectTimeout(config.connectTimeout)
+ }
+ if (config.followRedirects) {
+ clientBuilder.followRedirects(HttpClient.Redirect.NORMAL)
+ }
+ client = clientBuilder.build()
+ baseUri = config.baseUri
+ defaultHeaders = Collections.unmodifiableMap(new
LinkedHashMap<>(config.headers))
+ defaultRequestTimeout = config.requestTimeout
+ }
+
+ static HttpBuilder http(
+ @DelegatesTo(value = Config, strategy = Closure.DELEGATE_FIRST)
+ final Closure<?> spec
+ ) {
+ Config config = new Config()
+ Closure<?> code = (Closure<?>) spec.clone()
+ code.resolveStrategy = Closure.DELEGATE_FIRST
+ code.delegate = config
+ code.call()
+ return new HttpBuilder(config)
+ }
+
+ static HttpBuilder http(final String baseUri) {
+ Config config = new Config()
+ config.baseUri(baseUri)
+ return new HttpBuilder(config)
+ }
+
+ HttpResult get(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('GET', uri, spec)
+ }
+
+ HttpResult post(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('POST', uri, spec)
+ }
+
+ HttpResult put(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('PUT', uri, spec)
+ }
+
+ HttpResult delete(final Object uri = null,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ return request('DELETE', uri, spec)
+ }
+
+ HttpResult request(final String method,
+ final Object uri,
+ @DelegatesTo(value = RequestSpec, strategy =
Closure.DELEGATE_FIRST)
+ final Closure<?> spec = null) {
+ RequestSpec requestSpec = new RequestSpec()
+ if (spec != null) {
+ Closure<?> code = (Closure<?>) spec.clone()
+ code.resolveStrategy = Closure.DELEGATE_FIRST
+ code.delegate = requestSpec
+ code.call()
+ }
+
+ URI resolvedUri = resolveUri(uri, requestSpec.queryParameters)
+ HttpRequest.Builder requestBuilder =
HttpRequest.newBuilder(resolvedUri)
+
+ Duration timeout = requestSpec.timeout ?: defaultRequestTimeout
+ if (timeout != null) {
+ requestBuilder.timeout(timeout)
+ }
+
+ defaultHeaders.each { String name, String value ->
+ requestBuilder.header(name, value)
+ }
+ requestSpec.headers.each { String name, String value ->
+ requestBuilder.setHeader(name, value)
+ }
+
+ requestBuilder.method(method, bodyPublisher(method, requestSpec.body))
+
+ HttpResponse<String> response = client.send(
+ requestBuilder.build(),
+ requestSpec.bodyHandler
+ )
+ return new HttpResult(response)
+ }
+
+ private URI resolveUri(final Object uri, final Map<String, Object> query) {
+ URI target = toUri(uri)
+ if (baseUri != null && !target.isAbsolute()) {
+ target = baseUri.resolve(target.toString())
+ }
+ if (baseUri == null && !target.isAbsolute()) {
+ throw new IllegalArgumentException('Request URI must be absolute
when no baseUri is configured')
+ }
+ return appendQuery(target, query)
+ }
+
+ private URI toUri(final Object value) {
+ if (value == null) {
+ if (baseUri == null) {
+ throw new IllegalArgumentException('URI must be provided when
no baseUri is configured')
+ }
+ return baseUri
+ }
+ if (value instanceof URI) {
+ return (URI) value
+ }
+ return URI.create(value.toString())
+ }
+
+ private static URI appendQuery(final URI uri, final Map<String, Object>
queryValues) {
+ if (queryValues.isEmpty()) {
+ return uri
+ }
+
+ List<String> pairs = new ArrayList<>()
+ if (uri.query != null && !uri.query.isEmpty()) {
+ pairs.add(uri.query)
+ }
+
+ queryValues.each { String key, Object value ->
+ String encodedKey = encodeQueryComponent(key)
+ String encodedValue = value == null ? '' :
encodeQueryComponent(value.toString())
+ pairs.add(encodedKey + '=' + encodedValue)
+ }
+
+ String query = pairs.join('&')
+ return new URI(uri.scheme, uri.authority, uri.path, query,
uri.fragment)
+ }
+
+ private static String encodeQueryComponent(final String value) {
+ return URLEncoder.encode(value, StandardCharsets.UTF_8)
+ .replace('+', '%20')
+ .replace('*', '%2A')
+ .replace('%7E', '~')
+ }
+
+ private static HttpRequest.BodyPublisher bodyPublisher(final String
method, final Object body) {
+ if (body == null) {
+ return HttpRequest.BodyPublishers.noBody()
+ }
+ if ('GET'.equalsIgnoreCase(method)) {
+ throw new IllegalArgumentException('GET requests do not support a
body in this DSL')
+ }
+ if (body instanceof byte[]) {
+ return HttpRequest.BodyPublishers.ofByteArray((byte[]) body)
+ }
+ return HttpRequest.BodyPublishers.ofString(body.toString())
+ }
+
+ static final class Config {
+ URI baseUri
+ Duration connectTimeout
+ Duration requestTimeout
+ boolean followRedirects
+ final Map<String, String> headers = new LinkedHashMap<>()
+
+ void baseUri(final Object value) {
+ baseUri = value instanceof URI ? (URI) value :
URI.create(value.toString())
Review Comment:
`Config.baseUri` currently accepts any `URI.create(...)` result, including
relative URIs. Since the DSL relies on `baseUri` to make relative request URIs
absolute, it would be better to validate that the configured base URI is
absolute (has scheme/host) and fail fast with a clear
`IllegalArgumentException` if it isn't.
```suggestion
URI uri = value instanceof URI ? (URI) value :
URI.create(value.toString())
if (!uri.isAbsolute() || uri.getScheme() == null ||
uri.getHost() == null) {
throw new IllegalArgumentException("Config.baseUri must be
an absolute URI with scheme and host: " + uri)
}
baseUri = uri
```
> A very simple DSL over the JDK's HTTP client
> --------------------------------------------
>
> Key: GROOVY-11879
> URL: https://issues.apache.org/jira/browse/GROOVY-11879
> Project: Groovy
> Issue Type: New Feature
> Reporter: Paul King
> Priority: Major
>
> In the past there have been very handy libraries like HttpBuilder and
> HttpBuilderNG. They are no longer supported. Instead folks use the native JDK
> client in JDK11+, libraries like okhttp, or our own Apache Geb. For simple
> scripting scenarios, none of the options are as clean as what the old http
> builder libraries provided.
> We rejected a previous proposal to have a built-in client: GROOVY-8209. At
> the time it was going to add additional dependencies and the JDK builtin
> client wasn't out. Times have moved on and there are times when doing simple
> scripting when a tiny bit of syntactic sugar over the JDKs client would
> provide useful without adding too much maintenance to our project.
> It isn't meant to handle many tricky cases - we have Geb for that and we can
> make use of the existing APIs of the JDK client for some less common things
> not covered by the little DSL.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
