Copilot commented on code in PR #2401: URL: https://github.com/apache/groovy/pull/2401#discussion_r2974471303
########## 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 Review Comment: Key error semantics (e.g., throwing when a relative URI is used without a configured `baseUri`, or when `uri` is omitted) aren't currently covered by tests. Adding negative tests for these exceptions would help lock in the intended contract and prevent accidental behavior changes. ########## subprojects/groovy-http-builder/README.md: ########## @@ -0,0 +1,228 @@ +<!-- +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 (we optionally allow Jsoup for HTML parsing as a minor breakage of this rule). +- 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') +import static groovy.http.HttpBuilder.http + +def app = http { + baseUri 'http://myapp.com' + followRedirects true + header 'User-Agent', 'Mozilla/5.0 (Macintosh)' +} + +def loginPage = app.get('/login') +assert loginPage.status == 200 +assert loginPage.html.select('h1').text() == 'Please Login' + +def afterLogin = app.post('/login') { + form(username: 'admin', password: 'p@ssw0rd') +} + +assert afterLogin.status == 200 +assert afterLogin.html.select('h1').text() == 'Admin Section' +``` + +### Form URL-Encoding Helper + +This example shows the `form` helper. + +```groovy +def result = http.post('/login') { + form(username: 'admin', password: 'p@ssw0rd') +} + +assert result.status == 200 +``` + +`form(...)` encodes values as `application/x-www-form-urlencoded` and sets +`Content-Type` automatically (unless you override it with `header`). +`result.parsed` dispatches by response `Content-Type`: +- `application/json` and `application/*+json` -> JSON object +- `application/xml`, `text/xml`, and `application/*+xml` -> XML object +- `text/html` -> jsoup `Document` if jsoup is found on the classpath, otherwise raw string body Review Comment: The README says HTML auto-parsing is triggered only for `text/html`, but the PR description also claims `application/xhtml+xml` is supported. Please make the README and implementation/PR description consistent (either document only what is implemented, or extend the parser dispatch to include XHTML). ```suggestion - `text/html` and `application/xhtml+xml` -> jsoup `Document` if jsoup is found on the classpath, otherwise raw string body ``` ########## 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) + } Review Comment: `URLEncoder.encode(...)` applies `application/x-www-form-urlencoded` rules (e.g., spaces become `+`), which isn't strictly correct for encoding URI query components. Consider using an RFC 3986–style percent-encoding for query params (at minimum converting `+` to `%20`) so URLs behave correctly with servers/frameworks that treat `+` as a literal plus in the query string. ########## 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') { + try { + return getHtml() Review Comment: `getParsed()` only treats `text/html` as HTML, but the PR description mentions also supporting `application/xhtml+xml`. If XHTML is intended to be auto-parsed, include it (and potentially other HTML-ish media types) in the dispatch logic; otherwise please align the docs/description with the actual behavior. -- 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]
