Copilot commented on code in PR #2463: URL: https://github.com/apache/groovy/pull/2463#discussion_r3069350218
########## subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpClientHelper.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 org.apache.groovy.lang.annotation.Incubating + +import java.util.concurrent.CompletableFuture + +/** + * Runtime helper used by the generated declarative HTTP client implementations. + * Not intended for direct use. + * + * @since 6.0.0 + */ +@Incubating +final class HttpClientHelper { + private final HttpBuilder http + private final Map<String, String> defaultHeaders + + HttpClientHelper(String baseUrl, Map<String, String> defaultHeaders) { + this.http = HttpBuilder.http(baseUrl) + this.defaultHeaders = Collections.unmodifiableMap(new LinkedHashMap<>(defaultHeaders)) + } + + /** + * Execute an HTTP request synchronously. + * + * @param method HTTP method (GET, POST, PUT, DELETE, PATCH) + * @param urlTemplate URL template with {param} placeholders + * @param returnType the method's return type for response conversion + * @param pathParams map of path parameter names to values + * @param queryParams map of query parameter names to values + * @param headers map of additional headers for this request + * @param body request body (serialized as JSON if non-null) + * @return the converted response + */ + Object execute(String method, String urlTemplate, Class<?> returnType, + Map<String, Object> pathParams, Map<String, Object> queryParams, + Map<String, String> headers, Object body) { + String url = resolveUrl(urlTemplate, pathParams) + HttpResult result = http.request(method, url) { + defaultHeaders.each { k, v -> header(k, v) } + headers.each { k, v -> header(k, v) } + queryParams.each { k, v -> query(k, v) } + if (body != null) { json(body) } + } + return convertResult(result, returnType) + } + + /** + * Execute an HTTP request asynchronously. + * + * @return a CompletableFuture containing the converted response + */ + CompletableFuture<Object> executeAsync(String method, String urlTemplate, Class<?> returnType, + Map<String, Object> pathParams, Map<String, Object> queryParams, + Map<String, String> headers, Object body) { + CompletableFuture.supplyAsync { + execute(method, urlTemplate, returnType, pathParams, queryParams, headers, body) + } + } + + private static String resolveUrl(String template, Map<String, Object> params) { + String url = template + params.each { String k, Object v -> + url = url.replace("{${k}}", URLEncoder.encode(String.valueOf(v), 'UTF-8')) + } + url + } + Review Comment: `resolveUrl` uses `URLEncoder.encode` for path variables. `URLEncoder` is for form/query encoding (e.g. spaces become `+`) and is not correct for URL path segments. This can produce incorrect URLs for values containing spaces or `+`. Use proper path-segment encoding (e.g. percent-encode and replace `+` with `%20`) or build a `URI` and let it encode components appropriately. ```suggestion url = url.replace("{${k}}", encodePathSegment(String.valueOf(v))) } url } private static String encodePathSegment(String value) { URLEncoder.encode(value, 'UTF-8').replace("+", "%20") } ``` ########## subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpClientHelper.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 org.apache.groovy.lang.annotation.Incubating + +import java.util.concurrent.CompletableFuture + +/** + * Runtime helper used by the generated declarative HTTP client implementations. + * Not intended for direct use. + * + * @since 6.0.0 + */ +@Incubating +final class HttpClientHelper { + private final HttpBuilder http + private final Map<String, String> defaultHeaders + + HttpClientHelper(String baseUrl, Map<String, String> defaultHeaders) { + this.http = HttpBuilder.http(baseUrl) + this.defaultHeaders = Collections.unmodifiableMap(new LinkedHashMap<>(defaultHeaders)) + } + + /** + * Execute an HTTP request synchronously. + * + * @param method HTTP method (GET, POST, PUT, DELETE, PATCH) + * @param urlTemplate URL template with {param} placeholders + * @param returnType the method's return type for response conversion + * @param pathParams map of path parameter names to values + * @param queryParams map of query parameter names to values + * @param headers map of additional headers for this request + * @param body request body (serialized as JSON if non-null) + * @return the converted response + */ + Object execute(String method, String urlTemplate, Class<?> returnType, + Map<String, Object> pathParams, Map<String, Object> queryParams, + Map<String, String> headers, Object body) { + String url = resolveUrl(urlTemplate, pathParams) + HttpResult result = http.request(method, url) { + defaultHeaders.each { k, v -> header(k, v) } + headers.each { k, v -> header(k, v) } + queryParams.each { k, v -> query(k, v) } + if (body != null) { json(body) } + } + return convertResult(result, returnType) + } + + /** + * Execute an HTTP request asynchronously. + * + * @return a CompletableFuture containing the converted response + */ + CompletableFuture<Object> executeAsync(String method, String urlTemplate, Class<?> returnType, + Map<String, Object> pathParams, Map<String, Object> queryParams, + Map<String, String> headers, Object body) { + CompletableFuture.supplyAsync { + execute(method, urlTemplate, returnType, pathParams, queryParams, headers, body) + } + } + + private static String resolveUrl(String template, Map<String, Object> params) { + String url = template + params.each { String k, Object v -> + url = url.replace("{${k}}", URLEncoder.encode(String.valueOf(v), 'UTF-8')) + } + url + } + + private static Object convertResult(HttpResult result, Class<?> returnType) { + if (result.status() >= 400) { + throw new RuntimeException("HTTP ${result.status()}: ${result.body()}") + } + if (returnType == HttpResult) return result + if (returnType == String) return result.body() + if (returnType == void || returnType == Void) return null Review Comment: `returnType == void` is not a reliable/valid way to detect void at runtime. Use `returnType == Void.TYPE` / `returnType == void.class` (and optionally `returnType == Void`) to ensure this branch works consistently. ```suggestion if (returnType == Void.TYPE || returnType == Void) return null ``` ########## subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilderClientTransform.groovy: ########## @@ -0,0 +1,254 @@ +/* + * 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 org.codehaus.groovy.ast.* +import org.codehaus.groovy.ast.expr.* +import org.codehaus.groovy.ast.stmt.* +import org.codehaus.groovy.control.CompilePhase +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.transform.AbstractASTTransformation +import org.codehaus.groovy.transform.GroovyASTTransformation +import org.objectweb.asm.Opcodes + +import java.util.concurrent.CompletableFuture + +import static org.codehaus.groovy.ast.ClassHelper.* +import static org.codehaus.groovy.ast.tools.GeneralUtils.* + +/** + * AST transform that generates an implementation class for interfaces + * annotated with {@link HttpBuilderClient}. + * + * @since 6.0.0 + */ +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +class HttpBuilderClientTransform extends AbstractASTTransformation { + + private static final ClassNode HELPER_TYPE = make(HttpClientHelper) + private static final ClassNode FUTURE_TYPE = make(CompletableFuture) + private static final ClassNode HTTP_RESULT_TYPE = make(HttpResult) + + private static final Map<String, String> HTTP_METHOD_ANNOTATIONS = [ + 'groovy.http.Get' : 'GET', + 'groovy.http.Post' : 'POST', + 'groovy.http.Put' : 'PUT', + 'groovy.http.Delete': 'DELETE', + 'groovy.http.Patch' : 'PATCH', + ] + + @Override + void visit(ASTNode[] nodes, SourceUnit source) { + init(nodes, source) + AnnotationNode anno = (AnnotationNode) nodes[0] + AnnotatedNode target = (AnnotatedNode) nodes[1] + + if (!(target instanceof ClassNode) || !target.isInterface()) { + addError("@HttpBuilderClient can only be applied to interfaces", target) + return + } + + ClassNode interfaceNode = (ClassNode) target + String baseUrl = getMemberStringValue(anno, 'value') + if (!baseUrl) { + addError("@HttpBuilderClient requires a base URL", anno) + return + } + + // Collect interface-level @Header annotations + Map<String, String> interfaceHeaders = collectHeaders(interfaceNode) + + // Generate the implementation class + ClassNode implClass = generateImplClass(interfaceNode, baseUrl, interfaceHeaders) + source.AST.addClass(implClass) + + // Add static create() factory method to the interface + addCreateMethod(interfaceNode, implClass, baseUrl) + } + + private ClassNode generateImplClass(ClassNode interfaceNode, String baseUrl, Map<String, String> interfaceHeaders) { + String implName = interfaceNode.name + '$Client' + ClassNode implClass = new ClassNode(implName, Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC, + OBJECT_TYPE, [interfaceNode.getPlainNodeReference()] as ClassNode[], null) + implClass.sourcePosition = interfaceNode + + // Field: private final HttpClientHelper __helper + FieldNode helperField = implClass.addField('__helper', Opcodes.ACC_PRIVATE | Opcodes.ACC_FINAL, + HELPER_TYPE, null) + + // Constructor: takes baseUrl string with default + Parameter baseUrlParam = param(STRING_TYPE, 'baseUrl') + baseUrlParam.setInitialExpression(constX(baseUrl)) + BlockStatement ctorBody = block( + assignS(fieldX(helperField), + ctorX(HELPER_TYPE, args(varX(baseUrlParam), buildHeadersMapExpression(interfaceHeaders)))) + ) + implClass.addConstructor(Opcodes.ACC_PUBLIC, params(baseUrlParam), ClassNode.EMPTY_ARRAY, ctorBody) + + // Generate a method for each abstract interface method + for (MethodNode method : interfaceNode.abstractMethods) { + String httpMethod = null + String urlTemplate = null + + for (Map.Entry<String, String> entry : HTTP_METHOD_ANNOTATIONS) { + AnnotationNode methodAnno = method.getAnnotations(make(entry.key)).find() + if (methodAnno) { + httpMethod = entry.value + urlTemplate = getMemberStringValue(methodAnno, 'value') + break + } + } + + if (!httpMethod || !urlTemplate) continue + + Map<String, String> methodHeaders = collectHeaders(method) + MethodNode implMethod = generateMethod(method, httpMethod, urlTemplate, + methodHeaders, helperField) + implClass.addMethod(implMethod) + } + + return implClass + } + + private MethodNode generateMethod(MethodNode method, String httpMethod, String urlTemplate, + Map<String, String> methodHeaders, FieldNode helperField) { + Parameter[] params = method.parameters + boolean isAsync = isAsyncReturn(method.returnType) + Class<?> effectiveReturnType = resolveReturnType(method.returnType) + + // Build path params map: params whose names appear as {name} in the URL + MapExpression pathParams = new MapExpression() + MapExpression queryParams = new MapExpression() + Expression bodyExpr = constX(null) + + for (Parameter p : params) { + if (urlTemplate.contains("{${p.name}}")) { + pathParams.addMapEntryExpression( + new MapEntryExpression(constX(p.name), varX(p))) + } else if (hasAnnotation(p, Body)) { + bodyExpr = varX(p) + } else { + // Query parameter — use @Query name if specified, else param name + String queryName = getQueryParamName(p) + queryParams.addMapEntryExpression( + new MapEntryExpression(constX(queryName), varX(p))) + } + } + + String executeMethod = isAsync ? 'executeAsync' : 'execute' + Expression callExpr = callX(fieldX(helperField), executeMethod, args( + constX(httpMethod), + constX(urlTemplate), + classX(make(effectiveReturnType)), + pathParams, + queryParams, + buildHeadersMapExpression(methodHeaders), + bodyExpr + )) + + Statement body = returnS(callExpr) Review Comment: Generated methods always use `returnS(callExpr)`. For interface methods declared `void`, this will generate a `return <value>` in a void method, which fails compilation (and contradicts the docs' `void deleteBook(...)` example). Special-case void return types to emit an expression statement (invoke helper) and a plain `return` (or no explicit return). ```suggestion Statement body = method.returnType == VOID_TYPE ? stmt(callExpr) : returnS(callExpr) ``` ########## subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilderClientTransform.groovy: ########## @@ -0,0 +1,254 @@ +/* + * 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 org.codehaus.groovy.ast.* +import org.codehaus.groovy.ast.expr.* +import org.codehaus.groovy.ast.stmt.* +import org.codehaus.groovy.control.CompilePhase +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.transform.AbstractASTTransformation +import org.codehaus.groovy.transform.GroovyASTTransformation +import org.objectweb.asm.Opcodes + +import java.util.concurrent.CompletableFuture + +import static org.codehaus.groovy.ast.ClassHelper.* +import static org.codehaus.groovy.ast.tools.GeneralUtils.* + +/** + * AST transform that generates an implementation class for interfaces + * annotated with {@link HttpBuilderClient}. + * + * @since 6.0.0 + */ +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +class HttpBuilderClientTransform extends AbstractASTTransformation { + + private static final ClassNode HELPER_TYPE = make(HttpClientHelper) + private static final ClassNode FUTURE_TYPE = make(CompletableFuture) + private static final ClassNode HTTP_RESULT_TYPE = make(HttpResult) + + private static final Map<String, String> HTTP_METHOD_ANNOTATIONS = [ + 'groovy.http.Get' : 'GET', + 'groovy.http.Post' : 'POST', + 'groovy.http.Put' : 'PUT', + 'groovy.http.Delete': 'DELETE', + 'groovy.http.Patch' : 'PATCH', + ] + + @Override + void visit(ASTNode[] nodes, SourceUnit source) { + init(nodes, source) + AnnotationNode anno = (AnnotationNode) nodes[0] + AnnotatedNode target = (AnnotatedNode) nodes[1] + + if (!(target instanceof ClassNode) || !target.isInterface()) { + addError("@HttpBuilderClient can only be applied to interfaces", target) + return + } + + ClassNode interfaceNode = (ClassNode) target + String baseUrl = getMemberStringValue(anno, 'value') + if (!baseUrl) { + addError("@HttpBuilderClient requires a base URL", anno) + return + } + + // Collect interface-level @Header annotations + Map<String, String> interfaceHeaders = collectHeaders(interfaceNode) + + // Generate the implementation class + ClassNode implClass = generateImplClass(interfaceNode, baseUrl, interfaceHeaders) + source.AST.addClass(implClass) + + // Add static create() factory method to the interface + addCreateMethod(interfaceNode, implClass, baseUrl) + } + + private ClassNode generateImplClass(ClassNode interfaceNode, String baseUrl, Map<String, String> interfaceHeaders) { + String implName = interfaceNode.name + '$Client' + ClassNode implClass = new ClassNode(implName, Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC, + OBJECT_TYPE, [interfaceNode.getPlainNodeReference()] as ClassNode[], null) + implClass.sourcePosition = interfaceNode + + // Field: private final HttpClientHelper __helper + FieldNode helperField = implClass.addField('__helper', Opcodes.ACC_PRIVATE | Opcodes.ACC_FINAL, + HELPER_TYPE, null) + + // Constructor: takes baseUrl string with default + Parameter baseUrlParam = param(STRING_TYPE, 'baseUrl') + baseUrlParam.setInitialExpression(constX(baseUrl)) + BlockStatement ctorBody = block( + assignS(fieldX(helperField), + ctorX(HELPER_TYPE, args(varX(baseUrlParam), buildHeadersMapExpression(interfaceHeaders)))) + ) + implClass.addConstructor(Opcodes.ACC_PUBLIC, params(baseUrlParam), ClassNode.EMPTY_ARRAY, ctorBody) + + // Generate a method for each abstract interface method + for (MethodNode method : interfaceNode.abstractMethods) { + String httpMethod = null + String urlTemplate = null + + for (Map.Entry<String, String> entry : HTTP_METHOD_ANNOTATIONS) { + AnnotationNode methodAnno = method.getAnnotations(make(entry.key)).find() + if (methodAnno) { + httpMethod = entry.value + urlTemplate = getMemberStringValue(methodAnno, 'value') + break + } + } + + if (!httpMethod || !urlTemplate) continue + + Map<String, String> methodHeaders = collectHeaders(method) + MethodNode implMethod = generateMethod(method, httpMethod, urlTemplate, + methodHeaders, helperField) + implClass.addMethod(implMethod) + } + + return implClass + } + + private MethodNode generateMethod(MethodNode method, String httpMethod, String urlTemplate, + Map<String, String> methodHeaders, FieldNode helperField) { + Parameter[] params = method.parameters + boolean isAsync = isAsyncReturn(method.returnType) + Class<?> effectiveReturnType = resolveReturnType(method.returnType) + + // Build path params map: params whose names appear as {name} in the URL + MapExpression pathParams = new MapExpression() + MapExpression queryParams = new MapExpression() + Expression bodyExpr = constX(null) + + for (Parameter p : params) { + if (urlTemplate.contains("{${p.name}}")) { + pathParams.addMapEntryExpression( + new MapEntryExpression(constX(p.name), varX(p))) + } else if (hasAnnotation(p, Body)) { + bodyExpr = varX(p) + } else { + // Query parameter — use @Query name if specified, else param name + String queryName = getQueryParamName(p) + queryParams.addMapEntryExpression( + new MapEntryExpression(constX(queryName), varX(p))) + } + } + + String executeMethod = isAsync ? 'executeAsync' : 'execute' + Expression callExpr = callX(fieldX(helperField), executeMethod, args( + constX(httpMethod), + constX(urlTemplate), + classX(make(effectiveReturnType)), + pathParams, + queryParams, + buildHeadersMapExpression(methodHeaders), + bodyExpr + )) + + Statement body = returnS(callExpr) + + return new MethodNode(method.name, Opcodes.ACC_PUBLIC, method.returnType, + cloneParams(params), ClassNode.EMPTY_ARRAY, body) + } + + private void addCreateMethod(ClassNode interfaceNode, ClassNode implClass, String baseUrl) { + // create() — uses the annotation URL + Statement noArgBody = returnS(ctorX(implClass, args(constX(baseUrl)))) + MethodNode createNoArg = new MethodNode('create', + Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, interfaceNode.getPlainNodeReference(), + Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, noArgBody) + interfaceNode.addMethod(createNoArg) + + // create(String baseUrl) — override URL + Parameter baseUrlParam = param(STRING_TYPE, 'baseUrl') + Statement withArgBody = returnS(ctorX(implClass, args(varX(baseUrlParam)))) + MethodNode createWithArg = new MethodNode('create', + Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, interfaceNode.getPlainNodeReference(), + params(baseUrlParam), ClassNode.EMPTY_ARRAY, withArgBody) + interfaceNode.addMethod(createWithArg) + } + + private static Map<String, String> collectHeaders(AnnotatedNode node) { + Map<String, String> headers = new LinkedHashMap<>() + for (AnnotationNode anno : node.getAnnotations(make(Header))) { + headers[getMemberStringValue(anno, 'name')] = getMemberStringValue(anno, 'value') + } + for (AnnotationNode anno : node.getAnnotations(make(Headers))) { + Expression members = anno.getMember('value') + if (members instanceof ListExpression) { + for (Expression expr : ((ListExpression) members).expressions) { + if (expr instanceof AnnotationConstantExpression) { + AnnotationNode inner = ((AnnotationConstantExpression) expr).value + headers[getMemberStringValue(inner, 'name')] = getMemberStringValue(inner, 'value') + } + } + } + } + return headers + } + + private static MapExpression buildHeadersMapExpression(Map<String, String> headers) { + MapExpression map = new MapExpression() + headers.each { k, v -> + map.addMapEntryExpression(new MapEntryExpression(constX(k), constX(v))) + } + return map + } + + private static boolean isAsyncReturn(ClassNode returnType) { + return returnType.name == CompletableFuture.name || + returnType.redirect()?.name == CompletableFuture.name + } + + private static Class<?> resolveReturnType(ClassNode returnType) { + if (isAsyncReturn(returnType)) { + GenericsType[] generics = returnType.genericsTypes + if (generics?.length == 1) { + return generics[0].type.typeClass + } + return Object + } + if (returnType == VOID_TYPE || returnType.name == 'void') return void + try { + return returnType.typeClass + } catch (Exception ignored) { + return Object + } + } + Review Comment: `resolveReturnType` uses `ClassNode.typeClass` (and for async generics calls it without a try/catch). This can trigger classloading during compilation and may fail for types not yet loadable (e.g. `CompletableFuture<MyDto>` where `MyDto` is compiled in the same unit), degrading to `Object` or throwing. Consider keeping this as a `ClassNode` and generating a class literal expression from the AST type, instead of converting to `Class<?>` via `typeClass`. ```suggestion private static ClassNode resolveReturnType(ClassNode returnType) { if (isAsyncReturn(returnType)) { GenericsType[] generics = returnType.genericsTypes if (generics?.length == 1 && generics[0]?.type != null) { return generics[0].type.getPlainNodeReference() } return OBJECT_TYPE } if (returnType == VOID_TYPE || returnType.name == 'void') return VOID_TYPE return returnType?.getPlainNodeReference() ?: OBJECT_TYPE } private static ClassExpression buildReturnTypeClassExpression(ClassNode returnType) { return classX(resolveReturnType(returnType)) } ``` ########## subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilderClientTransform.groovy: ########## @@ -0,0 +1,254 @@ +/* + * 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 org.codehaus.groovy.ast.* +import org.codehaus.groovy.ast.expr.* +import org.codehaus.groovy.ast.stmt.* +import org.codehaus.groovy.control.CompilePhase +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.transform.AbstractASTTransformation +import org.codehaus.groovy.transform.GroovyASTTransformation +import org.objectweb.asm.Opcodes + +import java.util.concurrent.CompletableFuture + +import static org.codehaus.groovy.ast.ClassHelper.* +import static org.codehaus.groovy.ast.tools.GeneralUtils.* + +/** + * AST transform that generates an implementation class for interfaces + * annotated with {@link HttpBuilderClient}. + * + * @since 6.0.0 + */ +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +class HttpBuilderClientTransform extends AbstractASTTransformation { + + private static final ClassNode HELPER_TYPE = make(HttpClientHelper) + private static final ClassNode FUTURE_TYPE = make(CompletableFuture) + private static final ClassNode HTTP_RESULT_TYPE = make(HttpResult) + + private static final Map<String, String> HTTP_METHOD_ANNOTATIONS = [ + 'groovy.http.Get' : 'GET', + 'groovy.http.Post' : 'POST', + 'groovy.http.Put' : 'PUT', + 'groovy.http.Delete': 'DELETE', + 'groovy.http.Patch' : 'PATCH', + ] + + @Override + void visit(ASTNode[] nodes, SourceUnit source) { + init(nodes, source) + AnnotationNode anno = (AnnotationNode) nodes[0] + AnnotatedNode target = (AnnotatedNode) nodes[1] + + if (!(target instanceof ClassNode) || !target.isInterface()) { + addError("@HttpBuilderClient can only be applied to interfaces", target) + return + } + + ClassNode interfaceNode = (ClassNode) target + String baseUrl = getMemberStringValue(anno, 'value') + if (!baseUrl) { + addError("@HttpBuilderClient requires a base URL", anno) + return + } + + // Collect interface-level @Header annotations + Map<String, String> interfaceHeaders = collectHeaders(interfaceNode) + + // Generate the implementation class + ClassNode implClass = generateImplClass(interfaceNode, baseUrl, interfaceHeaders) + source.AST.addClass(implClass) + + // Add static create() factory method to the interface + addCreateMethod(interfaceNode, implClass, baseUrl) + } + + private ClassNode generateImplClass(ClassNode interfaceNode, String baseUrl, Map<String, String> interfaceHeaders) { + String implName = interfaceNode.name + '$Client' + ClassNode implClass = new ClassNode(implName, Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC, + OBJECT_TYPE, [interfaceNode.getPlainNodeReference()] as ClassNode[], null) + implClass.sourcePosition = interfaceNode + + // Field: private final HttpClientHelper __helper + FieldNode helperField = implClass.addField('__helper', Opcodes.ACC_PRIVATE | Opcodes.ACC_FINAL, + HELPER_TYPE, null) + + // Constructor: takes baseUrl string with default + Parameter baseUrlParam = param(STRING_TYPE, 'baseUrl') + baseUrlParam.setInitialExpression(constX(baseUrl)) + BlockStatement ctorBody = block( + assignS(fieldX(helperField), + ctorX(HELPER_TYPE, args(varX(baseUrlParam), buildHeadersMapExpression(interfaceHeaders)))) + ) + implClass.addConstructor(Opcodes.ACC_PUBLIC, params(baseUrlParam), ClassNode.EMPTY_ARRAY, ctorBody) + + // Generate a method for each abstract interface method + for (MethodNode method : interfaceNode.abstractMethods) { + String httpMethod = null + String urlTemplate = null + + for (Map.Entry<String, String> entry : HTTP_METHOD_ANNOTATIONS) { + AnnotationNode methodAnno = method.getAnnotations(make(entry.key)).find() + if (methodAnno) { + httpMethod = entry.value + urlTemplate = getMemberStringValue(methodAnno, 'value') + break + } + } + + if (!httpMethod || !urlTemplate) continue + + Map<String, String> methodHeaders = collectHeaders(method) + MethodNode implMethod = generateMethod(method, httpMethod, urlTemplate, + methodHeaders, helperField) + implClass.addMethod(implMethod) + } Review Comment: If an interface method lacks an HTTP method annotation (or has one but no URL), the transform currently `continue`s and silently omits the implementation. Since the impl class still implements the interface, this leads to a later/cryptic compilation failure. Emit a transform error when encountering an abstract method without exactly one supported HTTP annotation. ########## subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilderClientTransform.groovy: ########## @@ -0,0 +1,254 @@ +/* + * 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 org.codehaus.groovy.ast.* +import org.codehaus.groovy.ast.expr.* +import org.codehaus.groovy.ast.stmt.* +import org.codehaus.groovy.control.CompilePhase +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.transform.AbstractASTTransformation +import org.codehaus.groovy.transform.GroovyASTTransformation +import org.objectweb.asm.Opcodes + +import java.util.concurrent.CompletableFuture + +import static org.codehaus.groovy.ast.ClassHelper.* +import static org.codehaus.groovy.ast.tools.GeneralUtils.* + +/** + * AST transform that generates an implementation class for interfaces + * annotated with {@link HttpBuilderClient}. + * + * @since 6.0.0 + */ +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +class HttpBuilderClientTransform extends AbstractASTTransformation { + + private static final ClassNode HELPER_TYPE = make(HttpClientHelper) + private static final ClassNode FUTURE_TYPE = make(CompletableFuture) + private static final ClassNode HTTP_RESULT_TYPE = make(HttpResult) + + private static final Map<String, String> HTTP_METHOD_ANNOTATIONS = [ + 'groovy.http.Get' : 'GET', + 'groovy.http.Post' : 'POST', + 'groovy.http.Put' : 'PUT', + 'groovy.http.Delete': 'DELETE', + 'groovy.http.Patch' : 'PATCH', + ] + + @Override + void visit(ASTNode[] nodes, SourceUnit source) { + init(nodes, source) + AnnotationNode anno = (AnnotationNode) nodes[0] + AnnotatedNode target = (AnnotatedNode) nodes[1] + + if (!(target instanceof ClassNode) || !target.isInterface()) { + addError("@HttpBuilderClient can only be applied to interfaces", target) + return + } + + ClassNode interfaceNode = (ClassNode) target + String baseUrl = getMemberStringValue(anno, 'value') + if (!baseUrl) { + addError("@HttpBuilderClient requires a base URL", anno) + return + } + + // Collect interface-level @Header annotations + Map<String, String> interfaceHeaders = collectHeaders(interfaceNode) + + // Generate the implementation class + ClassNode implClass = generateImplClass(interfaceNode, baseUrl, interfaceHeaders) + source.AST.addClass(implClass) + + // Add static create() factory method to the interface + addCreateMethod(interfaceNode, implClass, baseUrl) + } + + private ClassNode generateImplClass(ClassNode interfaceNode, String baseUrl, Map<String, String> interfaceHeaders) { + String implName = interfaceNode.name + '$Client' + ClassNode implClass = new ClassNode(implName, Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC, + OBJECT_TYPE, [interfaceNode.getPlainNodeReference()] as ClassNode[], null) + implClass.sourcePosition = interfaceNode + + // Field: private final HttpClientHelper __helper + FieldNode helperField = implClass.addField('__helper', Opcodes.ACC_PRIVATE | Opcodes.ACC_FINAL, + HELPER_TYPE, null) + + // Constructor: takes baseUrl string with default + Parameter baseUrlParam = param(STRING_TYPE, 'baseUrl') + baseUrlParam.setInitialExpression(constX(baseUrl)) + BlockStatement ctorBody = block( + assignS(fieldX(helperField), + ctorX(HELPER_TYPE, args(varX(baseUrlParam), buildHeadersMapExpression(interfaceHeaders)))) + ) + implClass.addConstructor(Opcodes.ACC_PUBLIC, params(baseUrlParam), ClassNode.EMPTY_ARRAY, ctorBody) + + // Generate a method for each abstract interface method + for (MethodNode method : interfaceNode.abstractMethods) { + String httpMethod = null + String urlTemplate = null + + for (Map.Entry<String, String> entry : HTTP_METHOD_ANNOTATIONS) { + AnnotationNode methodAnno = method.getAnnotations(make(entry.key)).find() + if (methodAnno) { + httpMethod = entry.value + urlTemplate = getMemberStringValue(methodAnno, 'value') + break + } + } + + if (!httpMethod || !urlTemplate) continue + + Map<String, String> methodHeaders = collectHeaders(method) + MethodNode implMethod = generateMethod(method, httpMethod, urlTemplate, + methodHeaders, helperField) + implClass.addMethod(implMethod) + } + + return implClass + } + + private MethodNode generateMethod(MethodNode method, String httpMethod, String urlTemplate, + Map<String, String> methodHeaders, FieldNode helperField) { + Parameter[] params = method.parameters + boolean isAsync = isAsyncReturn(method.returnType) + Class<?> effectiveReturnType = resolveReturnType(method.returnType) + + // Build path params map: params whose names appear as {name} in the URL + MapExpression pathParams = new MapExpression() + MapExpression queryParams = new MapExpression() + Expression bodyExpr = constX(null) + + for (Parameter p : params) { + if (urlTemplate.contains("{${p.name}}")) { + pathParams.addMapEntryExpression( + new MapEntryExpression(constX(p.name), varX(p))) + } else if (hasAnnotation(p, Body)) { + bodyExpr = varX(p) + } else { + // Query parameter — use @Query name if specified, else param name + String queryName = getQueryParamName(p) + queryParams.addMapEntryExpression( + new MapEntryExpression(constX(queryName), varX(p))) + } + } + + String executeMethod = isAsync ? 'executeAsync' : 'execute' + Expression callExpr = callX(fieldX(helperField), executeMethod, args( + constX(httpMethod), + constX(urlTemplate), + classX(make(effectiveReturnType)), + pathParams, + queryParams, + buildHeadersMapExpression(methodHeaders), + bodyExpr + )) + + Statement body = returnS(callExpr) + + return new MethodNode(method.name, Opcodes.ACC_PUBLIC, method.returnType, + cloneParams(params), ClassNode.EMPTY_ARRAY, body) + } + + private void addCreateMethod(ClassNode interfaceNode, ClassNode implClass, String baseUrl) { + // create() — uses the annotation URL + Statement noArgBody = returnS(ctorX(implClass, args(constX(baseUrl)))) + MethodNode createNoArg = new MethodNode('create', + Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, interfaceNode.getPlainNodeReference(), + Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, noArgBody) + interfaceNode.addMethod(createNoArg) + + // create(String baseUrl) — override URL + Parameter baseUrlParam = param(STRING_TYPE, 'baseUrl') + Statement withArgBody = returnS(ctorX(implClass, args(varX(baseUrlParam)))) + MethodNode createWithArg = new MethodNode('create', + Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, interfaceNode.getPlainNodeReference(), + params(baseUrlParam), ClassNode.EMPTY_ARRAY, withArgBody) + interfaceNode.addMethod(createWithArg) + } + Review Comment: The transform unconditionally adds static `create()` overloads. If a user defines their own `create` method on the interface, this will cause a duplicate-method compilation error. Consider checking for existing `create` signatures before adding, and/or using a less collision-prone name. ```suggestion if (!hasMethodSignature(interfaceNode, 'create')) { Statement noArgBody = returnS(ctorX(implClass, args(constX(baseUrl)))) MethodNode createNoArg = new MethodNode('create', Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, interfaceNode.getPlainNodeReference(), Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, noArgBody) interfaceNode.addMethod(createNoArg) } // create(String baseUrl) — override URL Parameter baseUrlParam = param(STRING_TYPE, 'baseUrl') if (!hasMethodSignature(interfaceNode, 'create', STRING_TYPE)) { Statement withArgBody = returnS(ctorX(implClass, args(varX(baseUrlParam)))) MethodNode createWithArg = new MethodNode('create', Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC, interfaceNode.getPlainNodeReference(), params(baseUrlParam), ClassNode.EMPTY_ARRAY, withArgBody) interfaceNode.addMethod(createWithArg) } } private static boolean hasMethodSignature(ClassNode node, String name, ClassNode... parameterTypes) { node.methods.any { MethodNode method -> method.name == name && method.parameters.length == parameterTypes.length && method.parameters.indices.every { int i -> method.parameters[i].type == parameterTypes[i] || method.parameters[i].type?.name == parameterTypes[i]?.name } } } ``` ########## subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpClientHelper.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 org.apache.groovy.lang.annotation.Incubating + +import java.util.concurrent.CompletableFuture + +/** + * Runtime helper used by the generated declarative HTTP client implementations. + * Not intended for direct use. + * + * @since 6.0.0 + */ +@Incubating +final class HttpClientHelper { + private final HttpBuilder http + private final Map<String, String> defaultHeaders + + HttpClientHelper(String baseUrl, Map<String, String> defaultHeaders) { + this.http = HttpBuilder.http(baseUrl) + this.defaultHeaders = Collections.unmodifiableMap(new LinkedHashMap<>(defaultHeaders)) + } + + /** + * Execute an HTTP request synchronously. + * + * @param method HTTP method (GET, POST, PUT, DELETE, PATCH) + * @param urlTemplate URL template with {param} placeholders + * @param returnType the method's return type for response conversion + * @param pathParams map of path parameter names to values + * @param queryParams map of query parameter names to values + * @param headers map of additional headers for this request + * @param body request body (serialized as JSON if non-null) + * @return the converted response + */ + Object execute(String method, String urlTemplate, Class<?> returnType, + Map<String, Object> pathParams, Map<String, Object> queryParams, + Map<String, String> headers, Object body) { + String url = resolveUrl(urlTemplate, pathParams) + HttpResult result = http.request(method, url) { + defaultHeaders.each { k, v -> header(k, v) } + headers.each { k, v -> header(k, v) } + queryParams.each { k, v -> query(k, v) } + if (body != null) { json(body) } + } + return convertResult(result, returnType) + } + + /** + * Execute an HTTP request asynchronously. + * + * @return a CompletableFuture containing the converted response + */ + CompletableFuture<Object> executeAsync(String method, String urlTemplate, Class<?> returnType, + Map<String, Object> pathParams, Map<String, Object> queryParams, + Map<String, String> headers, Object body) { + CompletableFuture.supplyAsync { + execute(method, urlTemplate, returnType, pathParams, queryParams, headers, body) + } + } Review Comment: `executeAsync` wraps the synchronous, blocking `HttpClient.send(...)` call in `CompletableFuture.supplyAsync(...)` without specifying an executor, which uses the common pool. Blocking I/O on the common pool can lead to thread starvation under load. Prefer `HttpClient.sendAsync(...)` (or a dedicated executor) for async behavior. ########## subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderClientTest.groovy: ########## @@ -0,0 +1,223 @@ +/* + * 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 groovy.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.test.GroovyTestCase + +import java.util.concurrent.CompletableFuture + +class HttpBuilderClientTest extends GroovyTestCase { + + static HttpServer server + static int port + + static void setUpClass() { + server = HttpServer.create(new InetSocketAddress(0), 0) + port = server.address.port + + server.createContext('/users') { exchange -> + String path = exchange.requestURI.path + String method = exchange.requestMethod + + if (method == 'GET' && path =~ '/users/\\w+') { + String username = path.split('/')[-1] + respond(exchange, 200, [name: username, bio: "Bio of ${username}"]) + } else if (method == 'GET') { + def query = parseQuery(exchange.requestURI.query) + def users = [[name: 'Alice'], [name: 'Bob']] + if (query.name) { + users = users.findAll { it.name == query.name } + } + respond(exchange, 200, users) + } else if (method == 'POST') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + body.id = 42 + respond(exchange, 201, body) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.createContext('/echo-headers') { exchange -> + def headers = [:] + exchange.requestHeaders.each { k, v -> headers[k] = v.join(', ') } + respond(exchange, 200, headers) + } + + server.createContext('/items') { exchange -> + String method = exchange.requestMethod + String path = exchange.requestURI.path + if (method == 'PUT' && path =~ '/items/\\d+') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + respond(exchange, 200, body) + } else if (method == 'DELETE' && path =~ '/items/\\d+') { + respond(exchange, 204, null) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.start() + } + + static void tearDownClass() { + server?.stop(0) + } + + private static void respond(com.sun.net.httpserver.HttpExchange exchange, int status, Object body) { + String json = body != null ? JsonOutput.toJson(body) : '' + exchange.responseHeaders.set('Content-Type', 'application/json') + exchange.sendResponseHeaders(status, json ? json.bytes.length : -1) + if (json) { + exchange.responseBody.write(json.bytes) + } Review Comment: `respond(...)` calculates `Content-Length` using `json.bytes.length` and writes `json.bytes` without specifying a charset, which depends on the platform default encoding. Use a fixed charset (e.g. UTF-8) for consistent length calculation and response bytes. ########## subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderClientTest.groovy: ########## @@ -0,0 +1,223 @@ +/* + * 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 groovy.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.test.GroovyTestCase + +import java.util.concurrent.CompletableFuture + +class HttpBuilderClientTest extends GroovyTestCase { + + static HttpServer server + static int port + + static void setUpClass() { + server = HttpServer.create(new InetSocketAddress(0), 0) + port = server.address.port + + server.createContext('/users') { exchange -> + String path = exchange.requestURI.path + String method = exchange.requestMethod + + if (method == 'GET' && path =~ '/users/\\w+') { + String username = path.split('/')[-1] + respond(exchange, 200, [name: username, bio: "Bio of ${username}"]) + } else if (method == 'GET') { + def query = parseQuery(exchange.requestURI.query) + def users = [[name: 'Alice'], [name: 'Bob']] + if (query.name) { + users = users.findAll { it.name == query.name } + } + respond(exchange, 200, users) + } else if (method == 'POST') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + body.id = 42 + respond(exchange, 201, body) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.createContext('/echo-headers') { exchange -> + def headers = [:] + exchange.requestHeaders.each { k, v -> headers[k] = v.join(', ') } + respond(exchange, 200, headers) + } + + server.createContext('/items') { exchange -> + String method = exchange.requestMethod + String path = exchange.requestURI.path + if (method == 'PUT' && path =~ '/items/\\d+') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + respond(exchange, 200, body) + } else if (method == 'DELETE' && path =~ '/items/\\d+') { + respond(exchange, 204, null) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.start() + } + + static void tearDownClass() { + server?.stop(0) + } + + private static void respond(com.sun.net.httpserver.HttpExchange exchange, int status, Object body) { + String json = body != null ? JsonOutput.toJson(body) : '' + exchange.responseHeaders.set('Content-Type', 'application/json') + exchange.sendResponseHeaders(status, json ? json.bytes.length : -1) + if (json) { + exchange.responseBody.write(json.bytes) + } + exchange.close() + } + + private static Map<String, String> parseQuery(String query) { + if (!query) return [:] + query.split('&').collectEntries { String pair -> + def parts = pair.split('=', 2) + [(URLDecoder.decode(parts[0], 'UTF-8')): parts.length > 1 ? URLDecoder.decode(parts[1], 'UTF-8') : ''] + } + } + + // --- Test interface --- + + @Override + protected void setUp() { + super.setUp() + if (!server) setUpClass() + } + + @Override + protected void tearDown() { + super.tearDown() + } + + void testDeclarativeClient() { + assertScript """ + import groovy.http.* + import java.util.concurrent.CompletableFuture + + @HttpBuilderClient('http://localhost:${port}') + @Header(name = 'Accept', value = 'application/json') + interface TestApi { + @Get('/users/{username}') + Map getUser(String username) + + @Get('/users') + List listUsers() + + @Get('/users') + List searchUsers(@Query('name') String name) + + @Post('/users') + Map createUser(@Body Map user) + + @Get('/users/{username}') + CompletableFuture<Map> getUserAsync(String username) + } + + def api = TestApi.create() + + // GET with path parameter + def user = api.getUser('alice') + assert user.name == 'alice' + assert user.bio == 'Bio of alice' + + // GET list + def users = api.listUsers() + assert users.size() == 2 + + // GET with query parameter + def searched = api.searchUsers('Alice') + assert searched.size() == 1 + assert searched[0].name == 'Alice' + + // POST with body + def created = api.createUser([name: 'Charlie', email: '[email protected]']) + assert created.name == 'Charlie' + assert created.id == 42 + + // Async GET + def future = api.getUserAsync('bob') + assert future instanceof CompletableFuture + def asyncUser = future.get() + assert asyncUser.name == 'bob' Review Comment: `future.get()` has no timeout; if the async request hangs or the server isn't reachable, the test can block indefinitely. Use `get(timeout, unit)` (or `orTimeout(...)`) to keep CI failures bounded. ########## subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderClientTest.groovy: ########## @@ -0,0 +1,223 @@ +/* + * 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 groovy.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.test.GroovyTestCase + +import java.util.concurrent.CompletableFuture + +class HttpBuilderClientTest extends GroovyTestCase { + + static HttpServer server + static int port + + static void setUpClass() { Review Comment: This test extends `GroovyTestCase` (JUnit 3). The build uses `useJUnitPlatform()` (JUnit 5); without the Vintage engine these tests will not execute. Consider rewriting as JUnit Jupiter tests (like `HttpBuilderTest`) and using `GroovyShell`/`GroovyClassLoader` for the `assertScript` parts. ########## subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderClientTest.groovy: ########## @@ -0,0 +1,223 @@ +/* + * 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 groovy.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.test.GroovyTestCase + +import java.util.concurrent.CompletableFuture + +class HttpBuilderClientTest extends GroovyTestCase { + + static HttpServer server + static int port + + static void setUpClass() { + server = HttpServer.create(new InetSocketAddress(0), 0) + port = server.address.port + + server.createContext('/users') { exchange -> + String path = exchange.requestURI.path + String method = exchange.requestMethod + + if (method == 'GET' && path =~ '/users/\\w+') { + String username = path.split('/')[-1] + respond(exchange, 200, [name: username, bio: "Bio of ${username}"]) + } else if (method == 'GET') { + def query = parseQuery(exchange.requestURI.query) + def users = [[name: 'Alice'], [name: 'Bob']] + if (query.name) { + users = users.findAll { it.name == query.name } + } + respond(exchange, 200, users) + } else if (method == 'POST') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + body.id = 42 + respond(exchange, 201, body) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.createContext('/echo-headers') { exchange -> + def headers = [:] + exchange.requestHeaders.each { k, v -> headers[k] = v.join(', ') } + respond(exchange, 200, headers) + } + + server.createContext('/items') { exchange -> + String method = exchange.requestMethod + String path = exchange.requestURI.path + if (method == 'PUT' && path =~ '/items/\\d+') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + respond(exchange, 200, body) + } else if (method == 'DELETE' && path =~ '/items/\\d+') { + respond(exchange, 204, null) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.start() + } + + static void tearDownClass() { + server?.stop(0) + } + + private static void respond(com.sun.net.httpserver.HttpExchange exchange, int status, Object body) { + String json = body != null ? JsonOutput.toJson(body) : '' + exchange.responseHeaders.set('Content-Type', 'application/json') + exchange.sendResponseHeaders(status, json ? json.bytes.length : -1) + if (json) { + exchange.responseBody.write(json.bytes) + } + exchange.close() + } + + private static Map<String, String> parseQuery(String query) { + if (!query) return [:] + query.split('&').collectEntries { String pair -> + def parts = pair.split('=', 2) + [(URLDecoder.decode(parts[0], 'UTF-8')): parts.length > 1 ? URLDecoder.decode(parts[1], 'UTF-8') : ''] + } + } + + // --- Test interface --- + + @Override + protected void setUp() { + super.setUp() + if (!server) setUpClass() + } + + @Override + protected void tearDown() { + super.tearDown() + } + + void testDeclarativeClient() { + assertScript """ + import groovy.http.* + import java.util.concurrent.CompletableFuture + + @HttpBuilderClient('http://localhost:${port}') + @Header(name = 'Accept', value = 'application/json') + interface TestApi { + @Get('/users/{username}') + Map getUser(String username) + + @Get('/users') + List listUsers() + + @Get('/users') + List searchUsers(@Query('name') String name) + + @Post('/users') + Map createUser(@Body Map user) + + @Get('/users/{username}') + CompletableFuture<Map> getUserAsync(String username) + } + + def api = TestApi.create() + + // GET with path parameter + def user = api.getUser('alice') + assert user.name == 'alice' + assert user.bio == 'Bio of alice' + + // GET list + def users = api.listUsers() + assert users.size() == 2 + + // GET with query parameter + def searched = api.searchUsers('Alice') + assert searched.size() == 1 + assert searched[0].name == 'Alice' + + // POST with body + def created = api.createUser([name: 'Charlie', email: '[email protected]']) + assert created.name == 'Charlie' + assert created.id == 42 + + // Async GET + def future = api.getUserAsync('bob') + assert future instanceof CompletableFuture + def asyncUser = future.get() + assert asyncUser.name == 'bob' + """ + } + + void testHeadersPropagated() { + assertScript """ + import groovy.http.* + + @HttpBuilderClient('http://localhost:${port}') + @Header(name = 'X-Custom', value = 'test-value') + interface HeaderApi { + @Get('/echo-headers') + @Header(name = 'X-Method-Header', value = 'method-value') + Map echoHeaders() + } + + def api = HeaderApi.create() + def headers = api.echoHeaders() + assert headers['X-custom'] == 'test-value' + assert headers['X-method-header'] == 'method-value' + """ Review Comment: The header echo test asserts specific casing (`headers['X-custom']`). `com.sun.net.httpserver.Headers` is case-insensitive and may preserve/emit different key casing depending on JDK/version. Consider normalizing keys (e.g. lowercasing in the server response) or doing a case-insensitive lookup in the assertion. ########## subprojects/groovy-http-builder/src/spec/doc/http-builder.adoc: ########## @@ -180,4 +180,102 @@ include::../test/HttpBuilderSpecTest.groovy[tags=html_login,indent=0] | raw string body |=== +== Declarative HTTP Clients + +For APIs with multiple endpoints, you can define a typed interface and let +Groovy generate the implementation at compile time. Annotate an interface with +`@HttpBuilderClient` and each method with an HTTP method annotation: + +[source,groovy] +---- +@HttpBuilderClient('https://api.example.com') +@Header(name = 'Accept', value = 'application/json') +interface BookApi { + @Get('/books/{id}') + Map getBook(String id) + + @Get('/books') + List searchBooks(@Query('q') String query) + + @Post('/books') + Map createBook(@Body Map book) + + @Delete('/books/{id}') + void deleteBook(String id) +} Review Comment: The example uses `void deleteBook(String id)`, but the current AST transform generates client methods that always `return` the helper invocation. Unless void handling is added to the transform, this example won't compile as written; either update the transform to support `void`, or adjust the docs example to a supported return type. ########## subprojects/groovy-http-builder/src/main/java/groovy/http/HttpBuilderClient.java: ########## @@ -0,0 +1,55 @@ +/* + * 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 org.apache.groovy.lang.annotation.Incubating; +import org.codehaus.groovy.transform.GroovyASTTransformationClass; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks an interface as a declarative HTTP client. An implementation class + * is generated at compile time via AST transform, using {@link HttpBuilder} + * for request execution. + * <p> + * Example: + * <pre> + * {@code @HttpBuilderClient}('https://api.example.com') + * interface MyApi { + * {@code @Get}('/users/{id}') Review Comment: Javadoc example has malformed `{@code ...}` usage: `{@code @HttpBuilderClient}('...')` / `{@code @Get}('...')` will render oddly. Wrap the full annotation usage inside the `{@code ...}` block (or use `<code>`), e.g. `{@code @HttpBuilderClient("...")}`. ```suggestion * {@code @HttpBuilderClient('https://api.example.com')} * interface MyApi { * {@code @Get('/users/{id}')} ``` ########## subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderClientTest.groovy: ########## @@ -0,0 +1,223 @@ +/* + * 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 groovy.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.test.GroovyTestCase + +import java.util.concurrent.CompletableFuture + +class HttpBuilderClientTest extends GroovyTestCase { + + static HttpServer server + static int port + + static void setUpClass() { + server = HttpServer.create(new InetSocketAddress(0), 0) + port = server.address.port + + server.createContext('/users') { exchange -> + String path = exchange.requestURI.path + String method = exchange.requestMethod + + if (method == 'GET' && path =~ '/users/\\w+') { + String username = path.split('/')[-1] + respond(exchange, 200, [name: username, bio: "Bio of ${username}"]) + } else if (method == 'GET') { + def query = parseQuery(exchange.requestURI.query) + def users = [[name: 'Alice'], [name: 'Bob']] + if (query.name) { + users = users.findAll { it.name == query.name } + } + respond(exchange, 200, users) + } else if (method == 'POST') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + body.id = 42 + respond(exchange, 201, body) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.createContext('/echo-headers') { exchange -> + def headers = [:] + exchange.requestHeaders.each { k, v -> headers[k] = v.join(', ') } + respond(exchange, 200, headers) + } + + server.createContext('/items') { exchange -> + String method = exchange.requestMethod + String path = exchange.requestURI.path + if (method == 'PUT' && path =~ '/items/\\d+') { + def body = new JsonSlurper().parseText(exchange.requestBody.text) + respond(exchange, 200, body) + } else if (method == 'DELETE' && path =~ '/items/\\d+') { + respond(exchange, 204, null) + } else { + respond(exchange, 404, [error: 'not found']) + } + } + + server.start() + } + + static void tearDownClass() { + server?.stop(0) + } + Review Comment: `tearDownClass()` is never invoked by JUnit 3 `GroovyTestCase`, and the JUnit Platform runner won't call it either. As written, the embedded `HttpServer` will not be stopped, potentially leaking threads/ports across the build. If migrating to JUnit Jupiter, use `@BeforeAll`/`@AfterAll` static lifecycle methods; otherwise stop the server in `tearDown()` when appropriate. -- 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]
