Merge pull request #38265 from bpasson/issue/38247

Fixes incorrect rel=self web link
quarkusio · Jan 22, 2024 · 7aedf49 · 7aedf49
2 parents 375ad8d + be75b5d
commit 7aedf49
Show file tree

Hide file tree

Showing 19 changed files with 636 additions and 40 deletions.
diff --git a/docs/src/main/asciidoc/resteasy-reactive.adoc b/docs/src/main/asciidoc/resteasy-reactive.adoc
@@ -1643,7 +1643,8 @@ Assuming a `Record` looks like:
 ----
 public class Record {
 
-    // the class must contain/inherit either and `id` field or an `@Id` annotated field
+    // The class must contain/inherit either and `id` field, an `@Id` or `@RestLinkId` annotated field.
+    // When resolving the id the order of preference is: `@RestLinkId` > `@Id` > `id` field.
     private int id;
 
     public Record() {

diff --git a/...eployment/src/main/java/io/quarkus/resteasy/reactive/links/deployment/LinksProcessor.java b/...eployment/src/main/java/io/quarkus/resteasy/reactive/links/deployment/LinksProcessor.java
@@ -31,6 +31,7 @@
 import io.quarkus.deployment.pkg.builditem.ArtifactResultBuildItem;
 import io.quarkus.gizmo.ClassOutput;
 import io.quarkus.resteasy.reactive.common.deployment.JaxRsResourceIndexBuildItem;
+import io.quarkus.resteasy.reactive.links.RestLinkId;
 import io.quarkus.resteasy.reactive.links.runtime.GetterAccessorsContainer;
 import io.quarkus.resteasy.reactive.links.runtime.GetterAccessorsContainerRecorder;
 import io.quarkus.resteasy.reactive.links.runtime.LinkInfo;
@@ -150,35 +151,7 @@ private RuntimeValue<GetterAccessorsContainer> implementPathParameterValueGetter
                 validateClassHasFieldId(index, entityType);
 
                 for (String parameterName : linkInfo.getPathParameters()) {
-                    FieldInfoSupplier byParamName = new FieldInfoSupplier(c -> c.field(parameterName), className, index);
-
-                    // We implement a getter inside a class that has the required field.
-                    // We later map that getter's accessor with an entity type.
-                    // If a field is inside a parent class, the getter accessor will be mapped to each subclass which
-                    // has REST links that need access to that field.
-                    FieldInfo fieldInfo = byParamName.get();
-                    if ((fieldInfo == null) && parameterName.equals("id")) {
-                        // this is a special case where we want to go through the fields of the class
-                        // and see if any is annotated with any sort of @Id annotation
-                        // N.B. as this module does not depend on any other module that could supply this @Id annotation
-                        // (like Panache), we need this general lookup
-                        FieldInfoSupplier byIdAnnotation = new FieldInfoSupplier(
-                                c -> {
-                                    for (FieldInfo field : c.fields()) {
-                                        List<AnnotationInstance> annotationInstances = field.annotations();
-                                        for (AnnotationInstance annotationInstance : annotationInstances) {
-                                            if (annotationInstance.name().toString().endsWith("persistence.Id")) {
-                                                return field;
-                                            }
-                                        }
-                                    }
-                                    return null;
-                                },
-                                className,
-                                index);
-                        fieldInfo = byIdAnnotation.get();
-                    }
-
+                    FieldInfo fieldInfo = resolveField(index, parameterName, className);
                     if (fieldInfo != null) {
                         GetterMetadata getterMetadata = new GetterMetadata(fieldInfo);
                         if (!implementedGetters.contains(getterMetadata)) {
@@ -187,7 +160,8 @@ private RuntimeValue<GetterAccessorsContainer> implementPathParameterValueGetter
                         }
 
                         getterAccessorsContainerRecorder.addAccessor(getterAccessorsContainer,
-                                entityType, getterMetadata.getFieldName(), getterMetadata.getGetterAccessorName());
+                                entityType, parameterName,
+                                getterMetadata.getGetterAccessorName());
                     }
                 }
             }
@@ -196,6 +170,52 @@ private RuntimeValue<GetterAccessorsContainer> implementPathParameterValueGetter
         return getterAccessorsContainer;
     }
 
+    private FieldInfo resolveField(IndexView index, String parameterName, DotName className) {
+        FieldInfoSupplier byParamName = new FieldInfoSupplier(c -> c.field(parameterName), className, index);
+
+        // check if we have field matching the name
+        FieldInfo fieldInfo = byParamName.get();
+        if (parameterName.equals("id")) {
+            // this is a special case where we want to go through the fields of the class
+            // and see if any is annotated with any sort of @persistence.Id/@RestLinkId annotation
+            // N.B. as this module does not depend on any other module that could supply this @Id annotation
+            // (like Panache), we need this general lookup
+            // the order of preference for the annotations is @RestLinkId > @persistence.Id > id
+            FieldInfoSupplier byAnnotation = new FieldInfoSupplier(
+                    c -> {
+                        FieldInfo persistenceId = null;
+                        for (FieldInfo field : c.fields()) {
+                            // prefer RestLinId over Id
+                            if (field.hasAnnotation(RestLinkId.class)) {
+                                return field;
+                            }
+                            // keep the first found @persistence.Id annotation in case not @RestLinkId is found
+                            if (fieldAnnotatedWith(field, "persistence.Id") && persistenceId == null) {
+                                persistenceId = field;
+                            }
+                        }
+                        return persistenceId;
+                    },
+                    className,
+                    index);
+            FieldInfo annotatedField = byAnnotation.get();
+            if (annotatedField != null) {
+                fieldInfo = annotatedField;
+            }
+        }
+        return fieldInfo;
+    }
+
+    private boolean fieldAnnotatedWith(FieldInfo field, String annotation) {
+        List<AnnotationInstance> annotationInstances = field.annotations();
+        for (AnnotationInstance annotationInstance : annotationInstances) {
+            if (annotationInstance.name().toString().endsWith(annotation)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     /**
      * Validates if the given classname contains a field `id` or annotated with `@Id`
      *
@@ -227,15 +247,26 @@ private void validateRec(IndexView index, String entityType, ClassInfo classInfo
                 .filter(a -> a.name().toString().endsWith("persistence.Id"))
                 .toList();
 
+        List<AnnotationInstance> fieldsAnnotatedWithRestLinkId = classInfo.fields().stream()
+                .flatMap(f -> f.annotations(RestLinkId.class).stream())
+                .toList();
+
+        // @RestLinkId annotation count > 1 is not allowed
+        if (fieldsAnnotatedWithRestLinkId.size() > 1) {
+            throw new IllegalStateException("Cannot generate web links for the class " + entityType +
+                    " because it has multiple fields annotated with `@RestLinkId`, where a maximum of one is allowed");
+        }
+
         // Id field found, break the loop
-        if (!fieldsNamedId.isEmpty() || !fieldsAnnotatedWithId.isEmpty())
+        if (!fieldsNamedId.isEmpty() || !fieldsAnnotatedWithId.isEmpty() || !fieldsAnnotatedWithRestLinkId.isEmpty()) {
             return;
+        }
 
         // Id field not found and hope is gone
         DotName superClassName = classInfo.superName();
         if (superClassName == null) {
             throw new IllegalStateException("Cannot generate web links for the class " + entityType +
-                    " because is either missing an `id` field or a field with an `@Id` annotation");
+                    " because it is either missing an `id` field, a field with an `@Id` annotation or a field with a `@RestLinkId annotation");
         }
 
         // Id field not found but there's still hope

diff --git a/...ent/src/test/java/io/quarkus/resteasy/reactive/links/deployment/AbstractHalLinksTest.java b/...ent/src/test/java/io/quarkus/resteasy/reactive/links/deployment/AbstractHalLinksTest.java
@@ -28,4 +28,66 @@ void shouldGetHalLinksForInstance() {
 
         assertThat(response.body().jsonPath().getString("_links.list.href")).endsWith("/records");
     }
+
+    @Test
+    void shouldGetHalLinksForIdAndPersistenceIdAndRestLinkId() {
+        Response response = given().accept(RestMediaType.APPLICATION_HAL_JSON)
+                .get("/records/with-id-and-persistence-id-and-rest-link-id/100")
+                .thenReturn();
+
+        assertThat(response.body()
+                .jsonPath()
+                .getString("_links.self.href")).endsWith("/records/with-id-and-persistence-id-and-rest-link-id/100");
+    }
+
+    @Test
+    void shouldGetHalLinksForIdAndPersistenceId() {
+        Response response = given().accept(RestMediaType.APPLICATION_HAL_JSON)
+                .get("/records/with-id-and-persistence-id/10")
+                .thenReturn();
+
+        assertThat(response.body()
+                .jsonPath()
+                .getString("_links.self.href")).endsWith("/records/with-id-and-persistence-id/10");
+    }
+
+    @Test
+    void shouldGetHalLinksForIdAndRestLinkId() {
+        Response response = given().accept(RestMediaType.APPLICATION_HAL_JSON)
+                .get("/records/with-id-and-rest-link-id/100")
+                .thenReturn();
+
+        assertThat(response.body()
+                .jsonPath()
+                .getString("_links.self.href")).endsWith("/records/with-id-and-rest-link-id/100");
+    }
+
+    @Test
+    void shouldGetHalLinksForPersistenceIdAndRestLinkId() {
+        Response response = given().accept(RestMediaType.APPLICATION_HAL_JSON)
+                .get("/records/with-persistence-id-and-rest-link-id/100")
+                .thenReturn();
+
+        assertThat(response.body()
+                .jsonPath()
+                .getString("_links.self.href")).endsWith("/records/with-persistence-id-and-rest-link-id/100");
+    }
+
+    @Test
+    void shouldGetHalLinksForPersistenceId() {
+        Response response = given().accept(RestMediaType.APPLICATION_HAL_JSON)
+                .get("/records/with-persistence-id/10")
+                .thenReturn();
+
+        assertThat(response.body().jsonPath().getString("_links.self.href")).endsWith("/records/with-persistence-id/10");
+    }
+
+    @Test
+    void shouldGetHalLinksForRestLinkId() {
+        Response response = given().accept(RestMediaType.APPLICATION_HAL_JSON)
+                .get("/records/with-rest-link-id/100")
+                .thenReturn();
+
+        assertThat(response.body().jsonPath().getString("_links.self.href")).endsWith("/records/with-rest-link-id/100");
+    }
 }
diff --git a/.../src/test/java/io/quarkus/resteasy/reactive/links/deployment/HalLinksWithJacksonTest.java b/.../src/test/java/io/quarkus/resteasy/reactive/links/deployment/HalLinksWithJacksonTest.java
@@ -12,7 +12,10 @@ public class HalLinksWithJacksonTest extends AbstractHalLinksTest {
     @RegisterExtension
     static final QuarkusProdModeTest TEST = new QuarkusProdModeTest()
             .withApplicationRoot((jar) -> jar
-                    .addClasses(AbstractId.class, AbstractEntity.class, TestRecord.class, TestResource.class))
+                    .addClasses(AbstractId.class, AbstractEntity.class, TestRecord.class, TestResource.class,
+                            TestRecordWithIdAndPersistenceIdAndRestLinkId.class, TestRecordWithIdAndRestLinkId.class,
+                            TestRecordWithIdAndPersistenceId.class, TestRecordWithPersistenceId.class,
+                            TestRecordWithRestLinkId.class, TestRecordWithPersistenceIdAndRestLinkId.class))
             .setForcedDependencies(List.of(
                     Dependency.of("io.quarkus", "quarkus-resteasy-reactive-jackson", Version.getVersion()),
                     Dependency.of("io.quarkus", "quarkus-hal", Version.getVersion())))

diff --git a/...nt/src/test/java/io/quarkus/resteasy/reactive/links/deployment/HalLinksWithJsonbTest.java b/...nt/src/test/java/io/quarkus/resteasy/reactive/links/deployment/HalLinksWithJsonbTest.java
@@ -12,7 +12,10 @@ public class HalLinksWithJsonbTest extends AbstractHalLinksTest {
     @RegisterExtension
     static final QuarkusProdModeTest TEST = new QuarkusProdModeTest()
             .withApplicationRoot((jar) -> jar
-                    .addClasses(AbstractId.class, AbstractEntity.class, TestRecord.class, TestResource.class))
+                    .addClasses(AbstractId.class, AbstractEntity.class, TestRecord.class, TestResource.class,
+                            TestRecordWithIdAndPersistenceIdAndRestLinkId.class, TestRecordWithIdAndRestLinkId.class,
+                            TestRecordWithIdAndPersistenceId.class, TestRecordWithPersistenceId.class,
+                            TestRecordWithRestLinkId.class, TestRecordWithPersistenceIdAndRestLinkId.class))
             .setForcedDependencies(List.of(
                     Dependency.of("io.quarkus", "quarkus-resteasy-reactive-jsonb", Version.getVersion()),
                     Dependency.of("io.quarkus", "quarkus-hal", Version.getVersion())))

diff --git a/...t/src/test/java/io/quarkus/resteasy/reactive/links/deployment/RestLinksInjectionTest.java b/...t/src/test/java/io/quarkus/resteasy/reactive/links/deployment/RestLinksInjectionTest.java
@@ -19,7 +19,10 @@ public class RestLinksInjectionTest {
     @RegisterExtension
     static final QuarkusUnitTest TEST = new QuarkusUnitTest()
             .withApplicationRoot((jar) -> jar
-                    .addClasses(AbstractId.class, AbstractEntity.class, TestRecord.class, TestResource.class));
+                    .addClasses(AbstractId.class, AbstractEntity.class, TestRecord.class, TestResource.class,
+                            TestRecordWithIdAndPersistenceIdAndRestLinkId.class, TestRecordWithIdAndRestLinkId.class,
+                            TestRecordWithIdAndPersistenceId.class, TestRecordWithPersistenceId.class,
+                            TestRecordWithRestLinkId.class, TestRecordWithPersistenceIdAndRestLinkId.class));
 
     @TestHTTPResource("records")
     String recordsUrl;

diff --git a/...steasy/reactive/links/deployment/RestLinksWithFailureInjectionMultipleRestLinkIdTest.java b/...steasy/reactive/links/deployment/RestLinksWithFailureInjectionMultipleRestLinkIdTest.java
@@ -0,0 +1,31 @@
+package io.quarkus.resteasy.reactive.links.deployment;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.runtime.util.ExceptionUtil;
+import io.quarkus.test.QuarkusUnitTest;
+
+public class RestLinksWithFailureInjectionMultipleRestLinkIdTest {
+
+    @RegisterExtension
+    static final QuarkusUnitTest TEST = new QuarkusUnitTest()
+            .withApplicationRoot(
+                    jar -> jar.addClasses(TestRecordMultipleRestLinkIds.class, TestResourceMultipleRestLinkIds.class))
+            .assertException(t -> {
+                Throwable rootCause = ExceptionUtil.getRootCause(t);
+                assertThat(rootCause).isInstanceOf(IllegalStateException.class)
+                        .hasMessageContaining("Cannot generate web links for the class " +
+                                "io.quarkus.resteasy.reactive.links.deployment.TestRecordMultipleRestLinkIds" +
+                                " because it has multiple fields annotated with `@RestLinkId`, where a maximum of one is allowed");
+            });
+
+    @Test
+    void validationFailed() {
+        // Should not be reached: verify
+        fail();
+    }
+}
diff --git a/...java/io/quarkus/resteasy/reactive/links/deployment/RestLinksWithFailureInjectionTest.java b/...java/io/quarkus/resteasy/reactive/links/deployment/RestLinksWithFailureInjectionTest.java
@@ -1,7 +1,7 @@
 package io.quarkus.resteasy.reactive.links.deployment;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -17,13 +17,13 @@ public class RestLinksWithFailureInjectionTest {
                 Throwable rootCause = ExceptionUtil.getRootCause(t);
                 assertThat(rootCause).isInstanceOf(IllegalStateException.class)
                         .hasMessageContaining("Cannot generate web links for the class " +
-                                "io.quarkus.resteasy.reactive.links.deployment.TestRecordNoId because is either " +
-                                "missing an `id` field or a field with an `@Id` annotation");
+                                "io.quarkus.resteasy.reactive.links.deployment.TestRecordNoId because it is " +
+                                "either missing an `id` field, a field with an `@Id` annotation or a field with a `@RestLinkId annotation");
             });
 
     @Test
     void validationFailed() {
         // Should not be reached: verify
-        assertTrue(false);
+        fail();
     }
 }
diff --git a/...est/java/io/quarkus/resteasy/reactive/links/deployment/TestRecordMultipleRestLinkIds.java b/...est/java/io/quarkus/resteasy/reactive/links/deployment/TestRecordMultipleRestLinkIds.java
@@ -0,0 +1,46 @@
+package io.quarkus.resteasy.reactive.links.deployment;
+
+import io.quarkus.resteasy.reactive.links.RestLinkId;
+
+public class TestRecordMultipleRestLinkIds {
+
+    @RestLinkId
+    private long idOne;
+    @RestLinkId
+    private long idTwo;
+
+    private String name;
+
+    public TestRecordMultipleRestLinkIds() {
+    }
+
+    public TestRecordMultipleRestLinkIds(long idOne, long idTwo, String name) {
+        this.idOne = idOne;
+        this.idTwo = idTwo;
+        this.name = name;
+    }
+
+    public long getIdOne() {
+        return idOne;
+    }
+
+    public void setIdOne(long idOne) {
+        this.idOne = idOne;
+    }
+
+    public long getIdTwo() {
+        return idTwo;
+    }
+
+    public void setIdTwo(long idTwo) {
+        this.idTwo = idTwo;
+    }
+
+    public String getName() {
+        return name;
+    }
+
+    public void setName(String name) {
+        this.name = name;
+    }
+}
diff --git a/.../java/io/quarkus/resteasy/reactive/links/deployment/TestRecordWithIdAndPersistenceId.java b/.../java/io/quarkus/resteasy/reactive/links/deployment/TestRecordWithIdAndPersistenceId.java
@@ -0,0 +1,44 @@
+package io.quarkus.resteasy.reactive.links.deployment;
+
+import io.quarkus.resteasy.reactive.links.deployment.persistence.Id;
+
+public class TestRecordWithIdAndPersistenceId {
+
+    @Id
+    private int persistenceId;
+    private int id;
+    private String name;
+
+    public TestRecordWithIdAndPersistenceId() {
+    }
+
+    public TestRecordWithIdAndPersistenceId(int persistenceId, int id, String value) {
+        this.persistenceId = persistenceId;
+        this.id = id;
+        this.name = value;
+    }
+
+    public int getPersistenceId() {
+        return persistenceId;
+    }
+
+    public void setPersistenceId(int persistenceId) {
+        this.persistenceId = persistenceId;
+    }
+
+    public int getId() {
+        return id;
+    }
+
+    public void setId(int id) {
+        this.id = id;
+    }
+
+    public String getName() {
+        return name;
+    }
+
+    public void setName(String name) {
+        this.name = name;
+    }
+}