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. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
