Add Compile-time annotation-processor tool to clientcore #43457
Conversation
sdk/clientcore/tools/pom.xml
Outdated
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <modelVersion>4.0.0</modelVersion> | ||
| <groupId>io.clientcore</groupId> | ||
| <artifactId>clientcore-tools-service</artifactId> |
There was a problem hiding this comment.
clientcore-tools-parent or something might be more appropriate - but more generally, do we need to have a parent for tools, or can we just put the codegen a level higher?
There was a problem hiding this comment.
I was just thinking if we add any more tooling specifically for clientcore this would be a good place to club it. I will rename it to clientcore-tools-parent for now (easy to take it away if we dont need it later).
...re/tools/sdk-codegen-tool/src/main/java/io/generation/tools/codegen/AnnotationProcessor.java
Outdated
Show resolved
Hide resolved
samvaity
force-pushed
the
compile-time-codegen
branch
from
cadcf9b to
149a750
Compare
samvaity
changed the title
| @@ -0,0 +1,135 @@ | |||
| # Client Core Compile-Time Annotation Processor | |||
|
|
|||
| The client-core annotation processor for introducing compile-time code generation for libraries based on client core | |||
There was a problem hiding this comment.
Should there be a note here, at least temporarily, that "this project is for experimentation and exploring new ideas that may or may not make it into a supported GA release."
| templateInput.addImport("io.clientcore.core.util.Context"); | ||
| templateInput.addImport("io.clientcore.core.util.binarydata.BinaryData"); | ||
| templateInput.addImport("io.clientcore.core.http.models.HttpHeaders"); | ||
| templateInput.addImport("io.clientcore.core.http.pipeline.HttpPipeline"); | ||
| templateInput.addImport("io.clientcore.core.http.models.HttpHeaderName"); | ||
| templateInput.addImport("io.clientcore.core.http.models.HttpMethod"); | ||
| templateInput.addImport("io.clientcore.core.http.models.HttpResponse"); | ||
| templateInput.addImport("io.clientcore.core.http.models.HttpRequest"); | ||
| templateInput.addImport("io.clientcore.core.http.models.Response"); | ||
| templateInput.addImport("java.util.Map"); | ||
| templateInput.addImport("java.util.HashMap"); | ||
| templateInput.addImport("java.util.Arrays"); | ||
| templateInput.addImport("java.lang.Void"); | ||
| templateInput.addImport("java.util.List"); |
There was a problem hiding this comment.
Given we have a dependency on io.clientcore:core could we change all these to being something like Content.class.getName() as core is still in development and if these move it'll be obvious as compilation here will break.
| templateInput.setHttpRequestContexts(serviceInterface.getEnclosedElements().stream() | ||
| .filter(element -> element.getKind() == ElementKind.METHOD) | ||
| .filter(element -> element.getAnnotation(HttpRequestInformation.class) != null) | ||
| .map(ExecutableElement.class::cast) | ||
| .map(e -> createHttpRequestContext(e, templateInput)) | ||
| .collect(Collectors.toList())); | ||
|
|
||
| // template input set UnexpectedResponseExceptionDetails | ||
| templateInput.setUnexpectedResponseExceptionDetails(serviceInterface.getEnclosedElements().stream() | ||
| .filter(element -> element.getKind() == ElementKind.METHOD) | ||
| .filter(element -> element.getAnnotation(HttpRequestInformation.class) != null) | ||
| .map(ExecutableElement.class::cast) | ||
| .map(e -> e.getAnnotation(UnexpectedResponseExceptionDetail.class)) | ||
| .collect(Collectors.toList())); |
There was a problem hiding this comment.
Given the streams here have the same initial filters and mapping to ExecutableElement could we create an initial list and work from there?
| final String serviceInterfaceImplFQN = serviceInterfaceFQN + "Impl"; | ||
| final String serviceInterfaceImplShortName = serviceInterfaceImplFQN.substring(lastDot + 1); | ||
|
|
||
| templateInput.setPackageName(packageName); |
There was a problem hiding this comment.
What happens if packageName is null at this point?
| final String serviceInterfaceShortName = serviceInterfaceFQN.substring(lastDot + 1); | ||
| final String serviceInterfaceImplFQN = serviceInterfaceFQN + "Impl"; | ||
| final String serviceInterfaceImplShortName = serviceInterfaceImplFQN.substring(lastDot + 1); |
There was a problem hiding this comment.
| final String serviceInterfaceShortName = serviceInterfaceFQN.substring(lastDot + 1); | |
| final String serviceInterfaceImplFQN = serviceInterfaceFQN + "Impl"; | |
| final String serviceInterfaceImplShortName = serviceInterfaceImplFQN.substring(lastDot + 1); | |
| final String serviceInterfaceShortName = serviceInterfaceFQN.substring(lastDot + 1); | |
| final String serviceInterfaceImplShortName = serviceInterfaceShortName + "Impl"; |
No need for serviceInterfaceImplFQN as it just gets thrown away and we're repeating the logic to substring.
| method.addHeader(headerParam.value(), param.getSimpleName().toString()); | ||
| } else if (queryParam != null) { | ||
| // we do not support query param substitutions, so we just add the query param name and value | ||
| method.addQueryParam(queryParam.value(), param.getSimpleName().toString()); |
There was a problem hiding this comment.
Are we missing support for encoded and multipleQueryParams?
|
|
||
| final boolean hasQueryParams = !method.getQueryParams().isEmpty(); | ||
|
|
||
| Pattern pattern = Pattern.compile("\\{(.+?)\\}"); |
There was a problem hiding this comment.
nit: Make this a constant instead of creating a new Pattern each time as it never changes.
| String paramName = matcher.group(1); | ||
| Substitution substitution = method.getSubstitution(paramName); |
There was a problem hiding this comment.
Real niche corner case if I'm understanding this code correctly. If this is handling host and path substitutions at the same time it's possible that the host and path have substitutions with the same name:
@ServiceInterface(name = "HostSubstitutionMethods", host = "https://{sub1}.host.com")
interface WeirdDesign {
@HttpRequestInformation(method = HttpMethod.GET, path = "{sub1}")
void substitution(@HostParam("sub1") String hostSub1, @PathParam("sub1") String pathSub1);
}
There was a problem hiding this comment.
Let me add this to the test case and validate.
|
|
||
| if (hasQueryParams) { | ||
| buffer.append("?"); | ||
| for (Map.Entry<String, String> entry : method.getQueryParams().entrySet()) { |
There was a problem hiding this comment.
Will need to handle encoded and multi for path params. May want to create utility methods in core which this can use to handle that. That way any time there's an update to any handling in core we don't need to update here and core and core can just self-encompass the update.
| buffer.append(entry.getKey()).append("=\" + ").append(entry.getValue()).append(" + \"&"); | ||
| } | ||
| // Remove the last "&\"" | ||
| buffer.delete(buffer.length() - 5, buffer.length()); |
There was a problem hiding this comment.
If we're just removing &\" why are we deleting 5 characters?
|
|
||
| // strip out unnecessary `+ ""` in the buffer | ||
| String result = buffer.toString(); | ||
| result = result.replaceAll(" \\+ \"\"", ""); |
There was a problem hiding this comment.
Could we just handle this while generating the buffer?
Description
Creates a new
/sdk/clientcore/toolspackage named annotation-processor, which serves as a shippable replacement for the in-house RestProxy annotation processing utilities used to generate HTTP client service implementations.This package addresses common challenges with service client implementations relying on the intermediate RestProxy layer, which introduces reflection-based inefficiencies, negatively impacts performance, and produces less readable stack traces. Instead, annotation-processor uses JavaPoet to process annotations and generate service client implementations. Currently, it generates the respective service client implementation in
${project.build.directory}/generated-sources/.Remaining Work:
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines