Use bytecode transformation to reduce code duplication on jakarta mod…

…ules (#2269) Co-authored-by: Marvin Froeder <velobr@gmail.com>
OpenFeign · Dec 19, 2023 · 00b04bc · 00b04bc
1 parent 77759bb
commit 00b04bc
Show file tree

Hide file tree

Showing 13 changed files with 309 additions and 273 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -91,7 +91,7 @@ jobs:
       - run:
           name: 'Test'
           command: |
-            ./mvnw -ntp -B test
+            ./mvnw -ntp -B verify
       - verify-formatting
 
   deploy:

diff --git a/jakarta/pom.xml b/jakarta/pom.xml
@@ -30,38 +30,21 @@
   <properties>
     <main.java.version>11</main.java.version>
     <main.basedir>${project.basedir}/..</main.basedir>
-  </properties>
-
-  <dependencies>
-    <dependency>
-      <groupId>${project.groupId}</groupId>
-      <artifactId>feign-core</artifactId>
-    </dependency>
 
-    <dependency>
-      <groupId>jakarta.ws.rs</groupId>
-      <artifactId>jakarta.ws.rs-api</artifactId>
-      <version>3.1.0</version>
-    </dependency>
+    <moditect.skip>true</moditect.skip>
+  </properties>
 
-    <!-- for example -->
-    <dependency>
+  <distributionManagement>
+    <relocation>
       <groupId>${project.groupId}</groupId>
-      <artifactId>feign-gson</artifactId>
-      <scope>test</scope>
-    </dependency>
+      <artifactId>feign-jaxrs3</artifactId>
+    </relocation>
+  </distributionManagement>
 
+  <dependencies>
     <dependency>
       <groupId>${project.groupId}</groupId>
-      <artifactId>feign-core</artifactId>
-      <type>test-jar</type>
-      <scope>test</scope>
-    </dependency>
-    <dependency>
-      <groupId>${project.groupId}</groupId>
-      <artifactId>feign-jaxrs</artifactId>
-      <type>test-jar</type>
-      <scope>test</scope>
+      <artifactId>feign-jaxrs3</artifactId>
     </dependency>
   </dependencies>
 </project>
diff --git a/jakarta/src/main/java/feign/jaxrs/JakartaContract.java b/jakarta/src/main/java/feign/jaxrs/JakartaContract.java
@@ -13,228 +13,9 @@
  */
 package feign.jaxrs;
 
-import static feign.Util.checkState;
-import static feign.Util.emptyToNull;
-import static feign.Util.removeValues;
+import feign.jaxrs3.JAXRS3Contract;
 
-import feign.DeclarativeContract;
-import feign.MethodMetadata;
-import feign.Request;
-import jakarta.ws.rs.*;
-import jakarta.ws.rs.container.Suspended;
-import jakarta.ws.rs.core.Context;
-import java.lang.annotation.Annotation;
-import java.lang.reflect.Field;
-import java.lang.reflect.Method;
-import java.util.Collections;
-
-public class JakartaContract extends DeclarativeContract {
-
-  static final String ACCEPT = "Accept";
-  static final String CONTENT_TYPE = "Content-Type";
-
-  // Protected so unittest can call us
-  // XXX: Should parseAndValidateMetadata(Class, Method) be public instead? The old deprecated
-  // parseAndValidateMetadata(Method) was public..
-  @Override
-  protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method method) {
-    return super.parseAndValidateMetadata(targetType, method);
-  }
-
-  public JakartaContract() {
-    super.registerClassAnnotation(
-        Path.class,
-        (path, data) -> {
-          if (path != null && !path.value().isEmpty()) {
-            String pathValue = path.value();
-            if (!pathValue.startsWith("/")) {
-              pathValue = "/" + pathValue;
-            }
-            if (pathValue.endsWith("/")) {
-              // Strip off any trailing slashes, since the template has already had slashes
-              // appropriately
-              // added
-              pathValue = pathValue.substring(0, pathValue.length() - 1);
-            }
-            // jax-rs allows whitespace around the param name, as well as an optional regex. The
-            // contract
-            // should
-            // strip these out appropriately.
-            pathValue = pathValue.replaceAll("\\{\\s*(.+?)\\s*(:.+?)?\\}", "\\{$1\\}");
-            data.template().uri(pathValue);
-          }
-        });
-    super.registerClassAnnotation(Consumes.class, this::handleConsumesAnnotation);
-    super.registerClassAnnotation(Produces.class, this::handleProducesAnnotation);
-
-    registerMethodAnnotation(
-        methodAnnotation -> {
-          final Class<? extends Annotation> annotationType = methodAnnotation.annotationType();
-          final HttpMethod http = annotationType.getAnnotation(HttpMethod.class);
-          return http != null;
-        },
-        (methodAnnotation, data) -> {
-          final Class<? extends Annotation> annotationType = methodAnnotation.annotationType();
-          final HttpMethod http = annotationType.getAnnotation(HttpMethod.class);
-          checkState(
-              data.template().method() == null,
-              "Method %s contains multiple HTTP methods. Found: %s and %s",
-              data.configKey(),
-              data.template().method(),
-              http.value());
-          data.template().method(Request.HttpMethod.valueOf(http.value()));
-        });
-
-    super.registerMethodAnnotation(
-        Path.class,
-        (path, data) -> {
-          final String pathValue = emptyToNull(path.value());
-          if (pathValue == null) {
-            return;
-          }
-          String methodAnnotationValue = path.value();
-          if (!methodAnnotationValue.startsWith("/") && !data.template().url().endsWith("/")) {
-            methodAnnotationValue = "/" + methodAnnotationValue;
-          }
-          // jax-rs allows whitespace around the param name, as well as an optional regex. The
-          // contract
-          // should
-          // strip these out appropriately.
-          methodAnnotationValue =
-              methodAnnotationValue.replaceAll("\\{\\s*(.+?)\\s*(:.+?)?\\}", "\\{$1\\}");
-          data.template().uri(methodAnnotationValue, true);
-        });
-    super.registerMethodAnnotation(Consumes.class, this::handleConsumesAnnotation);
-    super.registerMethodAnnotation(Produces.class, this::handleProducesAnnotation);
-
-    // parameter with unsupported jax-rs annotations should not be passed as body params.
-    // this will prevent interfaces from becoming unusable entirely due to single (unsupported)
-    // endpoints.
-    // https://github.com/OpenFeign/feign/issues/669
-    super.registerParameterAnnotation(Suspended.class, (ann, data, i) -> data.ignoreParamater(i));
-    super.registerParameterAnnotation(Context.class, (ann, data, i) -> data.ignoreParamater(i));
-    // trying to minimize the diff
-    registerParamAnnotations();
-  }
-
-  private void handleProducesAnnotation(Produces produces, MethodMetadata data) {
-    final String[] serverProduces =
-        removeValues(produces.value(), mediaType -> emptyToNull(mediaType) == null, String.class);
-    checkState(serverProduces.length > 0, "Produces.value() was empty on %s", data.configKey());
-    data.template().header(ACCEPT, Collections.emptyList()); // remove any previous produces
-    data.template().header(ACCEPT, serverProduces);
-  }
-
-  private void handleConsumesAnnotation(Consumes consumes, MethodMetadata data) {
-    final String[] serverConsumes =
-        removeValues(consumes.value(), mediaType -> emptyToNull(mediaType) == null, String.class);
-    checkState(serverConsumes.length > 0, "Consumes.value() was empty on %s", data.configKey());
-    data.template().header(CONTENT_TYPE, serverConsumes);
-  }
-
-  protected void registerParamAnnotations() {
-
-    registerParameterAnnotation(
-        PathParam.class,
-        (param, data, paramIndex) -> {
-          final String name = param.value();
-          checkState(
-              emptyToNull(name) != null, "PathParam.value() was empty on parameter %s", paramIndex);
-          nameParam(data, name, paramIndex);
-        });
-    registerParameterAnnotation(
-        QueryParam.class,
-        (param, data, paramIndex) -> {
-          final String name = param.value();
-          checkState(
-              emptyToNull(name) != null,
-              "QueryParam.value() was empty on parameter %s",
-              paramIndex);
-          final String query = addTemplatedParam(name);
-          data.template().query(name, query);
-          nameParam(data, name, paramIndex);
-        });
-    registerParameterAnnotation(
-        HeaderParam.class,
-        (param, data, paramIndex) -> {
-          final String name = param.value();
-          checkState(
-              emptyToNull(name) != null,
-              "HeaderParam.value() was empty on parameter %s",
-              paramIndex);
-          final String header = addTemplatedParam(name);
-          data.template().header(name, header);
-          nameParam(data, name, paramIndex);
-        });
-    registerParameterAnnotation(
-        FormParam.class,
-        (param, data, paramIndex) -> {
-          final String name = param.value();
-          checkState(
-              emptyToNull(name) != null, "FormParam.value() was empty on parameter %s", paramIndex);
-          data.formParams().add(name);
-          nameParam(data, name, paramIndex);
-        });
-
-    // Reflect over the Bean Param looking for supported parameter annotations
-    registerParameterAnnotation(
-        BeanParam.class,
-        (param, data, paramIndex) -> {
-          final Field[] aggregatedParams =
-              data.method().getParameters()[paramIndex].getType().getDeclaredFields();
-
-          for (Field aggregatedParam : aggregatedParams) {
-
-            if (aggregatedParam.isAnnotationPresent(PathParam.class)) {
-              final String name = aggregatedParam.getAnnotation(PathParam.class).value();
-              checkState(
-                  emptyToNull(name) != null,
-                  "BeanParam parameter %s contains PathParam with empty .value() on field %s",
-                  paramIndex,
-                  aggregatedParam.getName());
-              nameParam(data, name, paramIndex);
-            }
-
-            if (aggregatedParam.isAnnotationPresent(QueryParam.class)) {
-              final String name = aggregatedParam.getAnnotation(QueryParam.class).value();
-              checkState(
-                  emptyToNull(name) != null,
-                  "BeanParam parameter %s contains QueryParam with empty .value() on field %s",
-                  paramIndex,
-                  aggregatedParam.getName());
-              final String query = addTemplatedParam(name);
-              data.template().query(name, query);
-              nameParam(data, name, paramIndex);
-            }
-
-            if (aggregatedParam.isAnnotationPresent(HeaderParam.class)) {
-              final String name = aggregatedParam.getAnnotation(HeaderParam.class).value();
-              checkState(
-                  emptyToNull(name) != null,
-                  "BeanParam parameter %s contains HeaderParam with empty .value() on field %s",
-                  paramIndex,
-                  aggregatedParam.getName());
-              final String header = addTemplatedParam(name);
-              data.template().header(name, header);
-              nameParam(data, name, paramIndex);
-            }
-
-            if (aggregatedParam.isAnnotationPresent(FormParam.class)) {
-              final String name = aggregatedParam.getAnnotation(FormParam.class).value();
-              checkState(
-                  emptyToNull(name) != null,
-                  "BeanParam parameter %s contains FormParam with empty .value() on field %s",
-                  paramIndex,
-                  aggregatedParam.getName());
-              data.formParams().add(name);
-              nameParam(data, name, paramIndex);
-            }
-          }
-        });
-  }
-
-  // Not using override as the super-type's method is deprecated and will be removed.
-  private String addTemplatedParam(String name) {
-    return String.format("{%s}", name);
-  }
-}
+/**
+ * @deprecated use {@link JAXRS3Contract} instead
+ */
+public class JakartaContract extends JAXRS3Contract {}
diff --git a/jaxrs/pom.xml b/jaxrs/pom.xml
@@ -70,6 +70,23 @@
           </execution>
         </executions>
       </plugin>
+      <plugin>
+        <groupId>org.eclipse.transformer</groupId>
+        <artifactId>org.eclipse.transformer.maven</artifactId>
+        <version>0.2.0</version>
+        <executions>
+          <execution>
+            <id>jakarta-ee</id>
+            <phase>package</phase>
+            <goals>
+              <goal>run</goal>
+            </goals>
+            <configuration>
+              <classifier>jakarta</classifier>
+            </configuration>
+          </execution>
+        </executions>
+      </plugin>
     </plugins>
   </build>
 </project>
diff --git a/jaxrs2/pom.xml b/jaxrs2/pom.xml
@@ -101,4 +101,26 @@
       <scope>test</scope>
     </dependency>
   </dependencies>
+
+  <build>
+    <plugins>
+      <plugin>
+        <groupId>org.eclipse.transformer</groupId>
+        <artifactId>org.eclipse.transformer.maven</artifactId>
+        <version>0.2.0</version>
+        <executions>
+          <execution>
+            <id>jakarta-ee</id>
+            <phase>package</phase>
+            <goals>
+              <goal>run</goal>
+            </goals>
+            <configuration>
+              <classifier>jakarta</classifier>
+            </configuration>
+          </execution>
+        </executions>
+      </plugin>
+    </plugins>
+  </build>
 </project>
diff --git a/jaxrs2/src/main/java/feign/jaxrs2/JAXRS2Contract.java b/jaxrs2/src/main/java/feign/jaxrs2/JAXRS2Contract.java
@@ -26,7 +26,7 @@
  * Please refer to the <a href="https://github.com/Netflix/feign/tree/master/feign-jaxrs2">Feign
  * JAX-RS 2 README</a>.
  */
-public final class JAXRS2Contract extends JAXRSContract {
+public class JAXRS2Contract extends JAXRSContract {
 
   public JAXRS2Contract() {
     // parameter with unsupported jax-rs annotations should not be passed as body params.

diff --git a/jaxrs3/README.md b/jaxrs3/README.md
@@ -0,0 +1,36 @@
+# Feign Jakarta
+This module overrides annotation processing to instead use standard ones supplied by the Jakarta specification.  This is currently targeted at the 3.1 spec.
+
+## Limitations
+While it may appear possible to reuse the same interface across client and server, bear in mind that Jakarta resource
+ annotations were not designed to be processed by clients. Finally, Jakarta is a large spec and attempts to implement
+it completely would be a project larger than feign itself.  In other words, this implementation is *best efforts* and 
+concedes far from 100% compatibility with server interface behavior.
+
+## Currently Supported Annotation Processing
+Feign only supports processing java interfaces (not abstract or concrete classes).
+
+Here are a list of behaviors currently supported.
+### Type Annotations
+#### `@Path`
+Appends the value to `Target.url()`.  Can have tokens corresponding to `@PathParam` annotations.
+### Method Annotations
+#### `@HttpMethod` meta-annotation (present on `@GET`, `@POST`, etc.)
+Sets the request method.
+#### `@Path`
+Appends the value to `Target.url()`.  Can have tokens corresponding to `@PathParam` annotations.
+#### `@Produces`
+Adds all values into the `Accept` header.
+#### `@Consumes`
+Adds the first value as the `Content-Type` header.
+### Parameter Annotations
+#### `@PathParam`
+Links the value of the corresponding parameter to a template variable declared in the path.
+#### `@QueryParam`
+Links the value of the corresponding parameter to a query parameter.  When invoked, null will skip the query param.
+#### `@HeaderParam`
+Links the value of the corresponding parameter to a header.
+#### `@FormParam`
+Links the value of the corresponding parameter to a key passed to `Encoder.Text<Map<String, Object>>.encode()`.
+#### `@BeanParm`
+Aggregates the above supported parameter annotations under a single value object.