[
https://issues.apache.org/jira/browse/GROOVY-11879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18067521#comment-18067521
]
ASF GitHub Bot commented on GROOVY-11879:
-----------------------------------------
Copilot commented on code in PR #2401:
URL: https://github.com/apache/groovy/pull/2401#discussion_r2972440611
##########
subprojects/groovy-http-builder/README.md:
##########
@@ -0,0 +1,236 @@
+<!--
+SPDX-License-Identifier: Apache-2.0
+
+Licensed 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
+
+ https://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.
+-->
+# groovy-http-builder (experimental)
+
+A tiny experiment showing that a small DSL over JDK `java.net.http.HttpClient`
can be pleasant to use while staying lightweight.
+
+## Goals
+
+- Keep implementation small and easy to maintain.
+- Use only JDK HTTP client primitives.
+- Make common request setup declarative with Groovy closures.
+- Handle only the simple cases that often pop up in scripting and not the full
use cases that Apache Geb covers.
+- Include JSON/XML/HTML response parsing hooks while intentionally keeping
request hooks minimal.
+
Review Comment:
In the Goals section, the README says "Use only JDK HTTP client primitives."
but the module also supports optional HTML parsing via jsoup (and has
jsoup-related examples below). Consider clarifying this goal to mention jsoup
as an optional dependency so the stated constraints match the actual feature
set.
##########
subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderTest.groovy:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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 com.sun.net.httpserver.HttpServer
+import org.junit.jupiter.api.AfterEach
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
+
+import java.nio.charset.StandardCharsets
+import java.time.Duration
+
+class HttpBuilderTest {
+
+ private HttpServer server
+ private URI rootUri
+
+ @BeforeEach
+ void setup() {
+ server = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0)
+ server.createContext("/hello") { exchange ->
+ String body =
"method=${exchange.requestMethod};query=${exchange.requestURI.query};ua=${exchange.requestHeaders.getFirst('User-Agent')}"
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.sendResponseHeaders(200, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext("/echo") { exchange ->
+ String requestBody =
exchange.requestBody.getText(StandardCharsets.UTF_8.name())
+ String body =
"method=${exchange.requestMethod};header=${exchange.requestHeaders.getFirst('X-Trace')};body=${requestBody}"
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.sendResponseHeaders(201, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext('/json') { exchange ->
+ String requestBody =
exchange.requestBody.getText(StandardCharsets.UTF_8.name())
+ String contentType =
exchange.requestHeaders.getFirst('Content-Type')
+ String body =
"{\"ok\":true,\"contentType\":\"${contentType}\",\"requestBody\":${requestBody}}"
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.responseHeaders.add('Content-Type', 'application/json')
+ exchange.sendResponseHeaders(200, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext('/xml') { exchange ->
+ String body = '<repo><name>groovy</name><license>Apache License
2.0</license></repo>'
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.responseHeaders.add('Content-Type', 'application/xml')
+ exchange.sendResponseHeaders(200, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext('/plain') { exchange ->
+ String body = 'just text'
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.responseHeaders.add('Content-Type', 'text/plain')
+ exchange.sendResponseHeaders(200, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext('/form') { exchange ->
+ String requestBody =
exchange.requestBody.getText(StandardCharsets.UTF_8.name())
+ String contentType =
exchange.requestHeaders.getFirst('Content-Type')
+ String body =
"method=${exchange.requestMethod};contentType=${contentType};body=${requestBody}"
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.sendResponseHeaders(200, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext('/html') { exchange ->
+ String body = '<!DOCTYPE html><html><head><link rel="preconnect"
crossorigin></head><body><span class="b lic">Apache License
2.0</span></body></html>'
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.responseHeaders.add('Content-Type', 'text/html;
charset=UTF-8')
+ exchange.sendResponseHeaders(200, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext('/redirect-target') { exchange ->
+ String body = 'redirect reached'
+ byte[] bytes = body.getBytes(StandardCharsets.UTF_8)
+ exchange.sendResponseHeaders(200, bytes.length)
+ exchange.responseBody.withCloseable { it.write(bytes) }
+ }
+ server.createContext('/redirect-me') { exchange ->
+ exchange.responseHeaders.add('Location', '/redirect-target')
+ exchange.sendResponseHeaders(302, -1)
+ exchange.close()
+ }
+ server.start()
+ rootUri = URI.create("http://127.0.0.1:${server.address.port}/";)
+ }
+
+ @AfterEach
+ void cleanup() {
+ server?.stop(0)
+ }
+
+ @Test
+ void getsWithBaseUriDefaultHeadersAndQueryDsl() {
+ HttpBuilder http = HttpBuilder.http {
+ baseUri rootUri
+ connectTimeout Duration.ofSeconds(2)
+ requestTimeout Duration.ofSeconds(2)
+ header 'User-Agent', 'groovy-http-builder-test'
+ }
+
+ HttpResult result = http.get('/hello') {
+ query lang: 'groovy', page: 1
+ }
+
+ assert result.status == 200
+ assert result.body.contains('method=GET')
+ assert result.body.contains('lang=groovy')
+ assert result.body.contains('page=1')
+ assert result.body.contains('ua=groovy-http-builder-test')
+ }
+
+ @Test
Review Comment:
There’s coverage for default headers and per-request headers independently,
but no test asserting that a per-request header overrides (rather than
duplicates) a default header value. Adding a test for this scenario would help
prevent regressions once header handling is adjusted (e.g., default
`User-Agent` overridden in the request closure).
```suggestion
@Test
void perRequestHeaderOverridesDefaultHeader() {
HttpBuilder http = HttpBuilder.http {
baseUri rootUri
connectTimeout Duration.ofSeconds(2)
requestTimeout Duration.ofSeconds(2)
header 'User-Agent', 'default-ua'
}
HttpResult result = http.get('/hello') {
header 'User-Agent', 'overridden-ua'
}
assert result.status == 200
assert result.body.contains('ua=overridden-ua')
assert !result.body.contains('ua=default-ua')
}
@Test
```
##########
subprojects/groovy-http-builder/build.gradle:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.
+ */
+plugins {
+ id 'org.apache.groovy-library'
+}
+
+dependencies {
+ api rootProject
+ implementation projects.groovyJson
+ implementation projects.groovyXml
+ testRuntimeOnly "org.jsoup:jsoup:1.19.1"
+ testImplementation projects.groovyTest
Review Comment:
`testRuntimeOnly "org.jsoup:jsoup:1.19.1"` doesn’t match the jsoup version
shown in the README (`@Grab('org.jsoup:jsoup:1.22.1')`). Aligning the
documented version with the build (or centralizing the version in a single
place) will help prevent docs/tests drifting and reduce confusion for users
trying the examples.
##########
subprojects/groovy-http-builder/README.md:
##########
@@ -0,0 +1,236 @@
+<!--
+SPDX-License-Identifier: Apache-2.0
+
+Licensed 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
+
+ https://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.
+-->
+# groovy-http-builder (experimental)
+
+A tiny experiment showing that a small DSL over JDK `java.net.http.HttpClient`
can be pleasant to use while staying lightweight.
+
+## Goals
+
+- Keep implementation small and easy to maintain.
+- Use only JDK HTTP client primitives.
+- Make common request setup declarative with Groovy closures.
+- Handle only the simple cases that often pop up in scripting and not the full
use cases that Apache Geb covers.
+- Include JSON/XML/HTML response parsing hooks while intentionally keeping
request hooks minimal.
+
+## Example
+
+```groovy
+import groovy.http.HttpBuilder
+
+def http = HttpBuilder.http {
+ baseUri 'https://example.com/'
+ header 'User-Agent', 'my-app/1.0'
+}
+
+def res = http.get('/api/items') {
+ query page: 1, size: 10
+}
+
+assert res.status == 200
+println res.body
+```
+
+## Non-DSL Equivalent (JDK HttpClient)
+
+```groovy
+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
+
+def baseUri = 'https://example.com/'
+def query = [page: 1, size: 10]
+ .collect { k, v ->
+ "${URLEncoder.encode(k.toString(), StandardCharsets.UTF_8)}=" +
+ URLEncoder.encode(v.toString(), StandardCharsets.UTF_8)
+ }
+ .join('&')
+
+def target = URI.create(baseUri).resolve("/api/items?${query}")
+
+def client = HttpClient.newHttpClient()
+def request = HttpRequest.newBuilder(target)
+ .header('User-Agent', 'my-app/1.0')
+ .GET()
+ .build()
+
+def response = client.send(request, HttpResponse.BodyHandlers.ofString())
+
+assert response.statusCode() == 200
+println response.body()
+```
+
+## JSON `get` Example
+
+```groovy
+import static groovy.http.HttpBuilder.http
+
+def github = http 'https://api.github.com'
+def res = github.get('/repos/apache/groovy')
+
+assert res.status == 200
+assert res.json.license.name == 'Apache License 2.0'
+assert res.parsed.license.name == 'Apache License 2.0' // auto-parsed from
Content-Type
+```
+
+### Non-DSL Equivalent (JDK HttpClient)
+
+```groovy
+import groovy.json.JsonSlurper
+
+import java.net.URI
+import java.net.http.HttpClient
+import java.net.http.HttpRequest
+import java.net.http.HttpResponse
+
+def client = HttpClient.newHttpClient()
+def request =
HttpRequest.newBuilder(URI.create('https://api.github.com/repos/apache/groovy'))
+ .GET()
+ .build()
+
+def response = client.send(request, HttpResponse.BodyHandlers.ofString())
+def payload = new JsonSlurper().parseText(response.body())
+
+assert response.statusCode() == 200
+assert payload.license.name == 'Apache License 2.0'
+```
+
+## JSON `post` Example
+
+```groovy
+def result = http.post('/api/items') {
+ json([name: 'book', qty: 2])
+}
+
+assert result.status == 200
+assert result.json.ok
+```
+
+## XML `get` Example
+
+```groovy
+def result = http.get('/api/repo.xml')
+
+assert result.status == 200
+assert result.xml.license.text() == 'Apache License 2.0'
+assert result.parsed.license.text() == 'Apache License 2.0' // auto-parsed
from Content-Type
+```
+
+## HTML `get` Example (jsoup)
+
+```groovy
+@Grab('org.jsoup:jsoup:1.22.1')
+import static groovy.http.HttpBuilder.http
+
+def client = http('https://mvnrepository.com')
+def res = client.get('/artifact/org.codehaus.groovy/groovy-all') {
+ header 'User-Agent', 'Mozilla/5.0 (Macintosh)'
+}
+
+assert res.status == 200
+
+def license = res.parsed.select('div.metadata-row
span.badge.badge-license')*.text().join(', ')
+assert license == 'Apache 2.0'
+```
+
+
+### Non-DSL Equivalent (JDK HttpClient + jsoup)
+
+```groovy
+@Grab('org.jsoup:jsoup:1.22.1')
+import org.jsoup.Jsoup
+
+import java.net.URI
+import java.net.http.HttpClient
+import java.net.http.HttpRequest
+import java.net.http.HttpResponse
+
+def client = HttpClient.newHttpClient()
+def request =
HttpRequest.newBuilder(URI.create('https://mvnrepository.com/artifact/org.codehaus.groovy/groovy-all'))
+ .header('User-Agent', 'Mozilla/5.0 (Macintosh)')
+ .GET()
+ .build()
+
+def response = client.send(request, HttpResponse.BodyHandlers.ofString())
+def document = Jsoup.parse(response.body())
+
+assert response.statusCode() == 200
+def license = document.select('div.metadata-row
span.badge.badge-license')*.text().join(', ')
+assert license == 'Apache 2.0'
+```
+
+## HTML login Example
+
+```groovy
+@Grab('org.jsoup:jsoup:1.22.1')
Review Comment:
The README examples use `@Grab('org.jsoup:jsoup:1.22.1')`, but the module's
test dependency is pinned to `org.jsoup:jsoup:1.19.1` (see build.gradle). It
would be better to keep these aligned to avoid confusing users about which
jsoup version is expected/known to work (or omit a fixed version in the README
if version isn’t important).
```suggestion
@Grab('org.jsoup:jsoup:1.19.1')
```
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -0,0 +1,292 @@
+/*
+ * 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.header(name, value)
Review Comment:
`HttpRequest.Builder.header()` appends header values; since defaults are
always applied and per-request headers are then added, a per-request
`header('X', ...)` won't override a default header and will instead send
duplicates (e.g., two `User-Agent`/`Content-Type` values). Consider using
`setHeader(...)` (or only applying defaults when not present in the request
spec) so request-level configuration can override defaults as expected.
```suggestion
requestBuilder.setHeader(name, value)
```
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpResult.groovy:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.json.JsonSlurper
+import groovy.xml.XmlSlurper
+import org.apache.groovy.lang.annotation.Incubating
+
+import java.net.http.HttpHeaders
+import java.net.http.HttpResponse
+import java.util.Locale
+
+/**
+ * Simple response wrapper for the {@link HttpBuilder} DSL.
+ */
+@Incubating
+record HttpResult(int status, String body, HttpHeaders headers,
HttpResponse<String> raw) {
+
+ HttpResult(final HttpResponse<String> response) {
+ this(response.statusCode(), response.body(), response.headers(),
response)
+ }
+
+ Object getJson() {
+ return new JsonSlurper().parseText(body)
+ }
+
+ Object getXml() {
+ return new XmlSlurper().parseText(body)
+ }
+
+ Object getHtml() {
+ try {
+ Class<?> jsoup = loadOptionalClass('org.jsoup.Jsoup')
+ if (jsoup == null) {
+ throw new ClassNotFoundException('org.jsoup.Jsoup')
+ }
+ return jsoup.getMethod('parse', String).invoke(null, body)
+ } catch (ClassNotFoundException e) {
+ throw new IllegalStateException("HTML parsing requires jsoup on
the classpath", e)
+ } catch (ReflectiveOperationException e) {
+ throw new IllegalStateException("Unable to parse HTML via jsoup",
e)
+ }
+ }
+
+ private static Class<?> loadOptionalClass(final String className) {
+ List<ClassLoader> classLoaders = [
+ Thread.currentThread().contextClassLoader,
+ HttpResult.class.classLoader,
+ ClassLoader.systemClassLoader
+ ].findAll { it != null }.unique()
+
+ for (ClassLoader classLoader : classLoaders) {
+ try {
+ return Class.forName(className, false, classLoader)
+ } catch (ClassNotFoundException ignore) {
+ // try next class loader
+ }
+ }
+ return null
+ }
+
+ Object getParsed() {
+ String contentType = headers.firstValue('Content-Type').orElse('')
+ String mediaType = contentType.split(';',
2)[0].trim().toLowerCase(Locale.ROOT)
+
+ if (mediaType == 'application/json' || mediaType.endsWith('+json')) {
+ return getJson()
+ }
+ if (mediaType == 'application/xml' || mediaType == 'text/xml' ||
mediaType.endsWith('+xml')) {
+ return getXml()
+ }
+ if (mediaType == 'text/html' || mediaType == 'application/xhtml+xml') {
+ try {
+ return getHtml()
+ } catch (IllegalStateException ignored) {
+ return body
Review Comment:
In `getParsed()`, the HTML branch catches any `IllegalStateException` from
`getHtml()` and falls back to the raw body. This will also silently swallow
real HTML parsing failures (e.g., `InvocationTargetException` from jsoup
parsing is wrapped into an `IllegalStateException`), making `parsed`
inconsistent and hiding errors. Consider only falling back when jsoup is
missing (e.g., detect `ClassNotFoundException` as the cause), and let genuine
parse failures surface.
```suggestion
} catch (IllegalStateException e) {
if (e.cause instanceof ClassNotFoundException) {
return body
}
throw e
```
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -0,0 +1,292 @@
+/*
+ * 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.header(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 = URLEncoder.encode(key, StandardCharsets.UTF_8)
+ String encodedValue = value == null ? "" :
URLEncoder.encode(value.toString(), StandardCharsets.UTF_8)
+ pairs.add(encodedKey + "=" + encodedValue)
+ }
+
+ String query = pairs.join("&")
+ return new URI(uri.scheme, uri.authority, uri.path, query,
uri.fragment)
+ }
Review Comment:
`appendQuery(...)` uses `uri.query` (decoded) and rebuilds the URI via `new
URI(scheme, authority, path, query, fragment)`. If the original URI contained
percent-encoded characters in its query/path, `getQuery()`/`getPath()` may
return decoded values, which can lead to invalid URIs or changed encoding when
reconstructing. Consider using `rawPath`/`rawQuery` (and possibly
`rawFragment`) to preserve the original encoding when appending additional
query parameters.
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -0,0 +1,292 @@
+/*
+ * 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.header(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 = URLEncoder.encode(key, StandardCharsets.UTF_8)
+ String encodedValue = value == null ? "" :
URLEncoder.encode(value.toString(), StandardCharsets.UTF_8)
+ pairs.add(encodedKey + "=" + encodedValue)
+ }
+
+ String query = pairs.join("&")
+ return new URI(uri.scheme, uri.authority, uri.path, query,
uri.fragment)
+ }
+
+ 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())
+ }
+
+ void connectTimeout(final Duration value) {
+ connectTimeout = value
+ }
+
+ void requestTimeout(final Duration value) {
+ requestTimeout = value
+ }
+
+ void followRedirects(final boolean value) {
+ followRedirects = value
+ }
+
+ void header(final String name, final Object value) {
+ headers.put(name, String.valueOf(value))
+ }
+
+ void headers(final Map<String, ?> values) {
+ values.each { String name, Object value -> header(name, value) }
+ }
+ }
+
+ static final class RequestSpec {
+ Duration timeout
+ Object body
+ HttpResponse.BodyHandler<String> bodyHandler =
HttpResponse.BodyHandlers.ofString()
+ final Map<String, String> headers = new LinkedHashMap<>()
+ final Map<String, Object> queryParameters = new LinkedHashMap<>()
+
+ void timeout(final Duration value) {
+ timeout = value
+ }
+
+ void header(final String name, final Object value) {
+ headers.put(name, String.valueOf(value))
+ }
+
+ void headers(final Map<String, ?> values) {
+ values.each { String name, Object value -> header(name, value) }
+ }
+
+ void query(final String name, final Object value) {
+ queryParameters.put(name, value)
+ }
+
+ void query(final Map<String, ?> values) {
+ values.each { String name, Object value -> query(name, value) }
+ }
+
+ void text(final Object value) {
+ body = value == null ? null : value.toString()
+ }
+
+ void bytes(final byte[] value) {
+ body = value
+ }
+
+ void body(final Object value) {
+ body = value
+ }
+
+ /**
+ * Encodes map entries as application/x-www-form-urlencoded and sets a
default content type.
+ */
+ void form(final Map<String, ?> values) {
+ if (!headers.containsKey('Content-Type')) {
+ header('Content-Type', 'application/x-www-form-urlencoded')
+ }
+ body = values.collect { String name, Object value ->
+ String encodedName = URLEncoder.encode(name,
StandardCharsets.UTF_8)
+ String encodedValue = value == null ? '' :
URLEncoder.encode(value.toString(), StandardCharsets.UTF_8)
+ encodedName + '=' + encodedValue
+ }.join('&')
+ }
+
+ /**
+ * Serializes the given value as JSON and sets a default content type.
+ */
+ void json(final Object value) {
+ if (!headers.containsKey('Content-Type')) {
+ header('Content-Type', 'application/json')
+ }
+ body = JsonOutput.toJson(value)
Review Comment:
`form(...)`/`json(...)` only treat `Content-Type` as present when the
exact-case key `'Content-Type'` exists. HTTP header names are case-insensitive,
so setting `content-type` (or any other casing) will still result in a second
`Content-Type` being added. Consider normalizing header names (e.g., store/look
up in a canonical case) or checking keys case-insensitively before setting the
default.
> 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)
