Copilot commented on code in PR #2401: URL: https://github.com/apache/groovy/pull/2401#discussion_r2974277190
########## 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.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 = 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(...)` and `json(...)` only check `headers.containsKey('Content-Type')` before setting a default. Since header names are case-insensitive, a caller-provided `content-type` (different case) won't be detected and the default `Content-Type` will be added later, potentially overriding the caller’s value due to insertion order. Please make the check case-insensitive (or normalize header keys on insertion) so user-specified content type is never accidentally overridden. ########## 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') { Review Comment: `getParsed()` currently only treats `text/html` as HTML. The README for this module says `application/xhtml+xml` should also dispatch to jsoup when available; with the current logic those responses will fall back to the raw string body. Please update the media type handling (and ideally add/adjust a test) so XHTML is parsed consistently with the documentation. ```suggestion if (mediaType == 'text/html' || mediaType == 'application/xhtml+xml') { ``` ########## subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderTest.groovy: ########## @@ -0,0 +1,257 @@ +/* + * 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 + void getsUsingStringBaseUriFactoryWithoutClosureConfig() { + HttpBuilder http = HttpBuilder.http(rootUri.toString()) + + HttpResult result = http.get('/hello') { + query page: 1 + } + + assert result.status == 200 + assert result.body.contains('method=GET') + assert result.body.contains('page=1') + } + + @Test + void postsWithBodyAndPerRequestHeader() { + HttpBuilder http = HttpBuilder.http { + baseUri rootUri + } + + HttpResult result = http.post('/echo') { + header 'X-Trace', 'trace-42' + text 'hello from DSL' + } + + assert result.status == 201 + assert result.body == 'method=POST;header=trace-42;body=hello from DSL' + } + + @Test + void formHookEncodesBodyAndSetsDefaultContentType() { + HttpBuilder http = HttpBuilder.http(rootUri.toString()) + + HttpResult result = http.post('/form') { + form([username: 'admin', password: 'p@ss word']) + } + + assert result.status == 200 + assert result.body == 'method=POST;contentType=application/x-www-form-urlencoded;body=username=admin&password=p%40ss+word' + } + + @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 + void jsonHookSerializesRequestAndParsesResponse() { + HttpBuilder http = HttpBuilder.http { + baseUri rootUri + } + + HttpResult result = http.post('/json') { + json([name: 'Groovy', version: 6]) + } + + assert result.status == 200 + Map payload = (Map) result.getJson() + assert payload.ok == true + assert payload.contentType == 'application/json' + assert payload.requestBody.name == 'Groovy' + assert payload.requestBody.version == 6 + + Map parsed = (Map) result.parsed + assert parsed.ok == true + } + + @Test + void xmlHookParsesResponseBody() { + HttpBuilder http = HttpBuilder.http(rootUri.toString()) + + HttpResult result = http.get('/xml') + + assert result.status == 200 + def xml = result.xml + assert xml.name.text() == 'groovy' + assert xml.license.text() == 'Apache License 2.0' + + def parsed = result.parsed + assert parsed.name.text() == 'groovy' + } + + @Test + void parsedFallsBackToRawBodyForUnsupportedContentType() { + HttpBuilder http = HttpBuilder.http(rootUri.toString()) + + HttpResult result = http.get('/plain') + + assert result.status == 200 + assert result.parsed == 'just text' + } + + @Test + void htmlHookParsesMalformedHtmlViaJsoup() { + HttpBuilder http = HttpBuilder.http(rootUri.toString()) + + HttpResult result = http.get('/html') + + assert result.status == 200 + assert result.html.select('span.b.lic').text() == 'Apache License 2.0' + assert result.parsed.select('span.b.lic').text() == 'Apache License 2.0' + } + + @Test Review Comment: Tests cover `text/html` parsing, but there’s no coverage for the documented `application/xhtml+xml` auto-parsing path (and currently `HttpResult.getParsed()` doesn’t handle it). Once XHTML is supported, add a small test case and a server context returning `Content-Type: application/xhtml+xml` to prevent regressions. ```suggestion @Test void xhtmlContentTypeIsParsedAsHtml() { def xhtmlServer = HttpServer.create(new java.net.InetSocketAddress(0), 0) xhtmlServer.createContext('/xhtml') { exchange -> def body = '''<?xml version="1.0" encoding="UTF-8"?> <html xmlns="http://www.w3.org/1999/xhtml";> <body> <span class="b lic">Apache License 2.0</span> </body> </html>''' byte[] bytes = body.getBytes(StandardCharsets.UTF_8) exchange.responseHeaders.set('Content-Type', 'application/xhtml+xml; charset=utf-8') exchange.sendResponseHeaders(200, bytes.length) exchange.responseBody.withCloseable { it.write(bytes) } } xhtmlServer.start() try { String xhtmlRoot = "http://localhost:${xhtmlServer.address.port}"; HttpBuilder http = HttpBuilder.http(xhtmlRoot) HttpResult result = http.get('/xhtml') assert result.status == 200 assert result.parsed.select('span.b.lic').text() == 'Apache License 2.0' } finally { xhtmlServer.stop(0) } } @Test ``` -- 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]
