Constructors are now intercepted with appropriate interceptors only. (#…

…6648)

* Constructors are now intercepted with appropriate interceptors only.
* Added tests for additional use cases (commented out the failing ones + added issue)

pico/runtime/src/main/java/io/helidon/pico/runtime/Invocation.java

@@ -118,18 +118,18 @@ public static <T> List<Provider<T>> mergeAndCollapse(List<Provider<T>>... lists)
 
     @Override
     public V proceed(Object... args) {
-        if (!interceptorIterator.hasNext()) {
+        if (interceptorIterator.hasNext()) {
+            return interceptorIterator.next()
+                    .get()
+                    .proceed(ctx, this, args);
+        } else {
             if (this.call != null) {
                 Function<Object[], V> call = this.call;
                 this.call = null;
                 return call.apply(args);
             } else {
                 throw new IllegalStateException("Duplicate invocation, or unknown call type: " + this);
             }
-        } else {
-            return interceptorIterator.next()
-                    .get()
-                    .proceed(ctx, this, args);
         }
     }
 

pico/tests/interception/src/main/java/io/helidon/pico/tests/interception/Modify.java

@@ -30,6 +30,6 @@
 @Retention(RetentionPolicy.RUNTIME)
 @Documented
 @InterceptedTrigger
-@Target(ElementType.METHOD)
+@Target({ElementType.METHOD, ElementType.CONSTRUCTOR})
 @interface Modify {
 }

pico/tests/interception/src/main/java/io/helidon/pico/tests/interception/OtherContract.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2023 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     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 io.helidon.pico.tests.interception;
+
+import io.helidon.pico.api.Contract;
+
+@Contract
+interface OtherContract {
+    @Modify
+    @Repeat
+    @Return
+    String intercepted(String message, boolean modify, boolean repeat, boolean doReturn);
+
+    @Return
+    String interceptedSubset(String message, boolean modify, boolean repeat, boolean doReturn);
+
+    String notIntercepted(String message, boolean modify, boolean repeat, boolean doReturn);
+}

pico/tests/interception/src/main/java/io/helidon/pico/tests/interception/TheOtherService.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright (c) 2023 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     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 io.helidon.pico.tests.interception;
+
+import jakarta.inject.Singleton;
+
+@Singleton
+class TheOtherService implements OtherContract{
+    @Modify
+    TheOtherService() {
+    }
+
+    @Override
+    public String intercepted(String message, boolean modify, boolean repeat, boolean doReturn) {
+        return message;
+    }
+
+    // one interceptor on interface, one on implementation
+    @Repeat
+    @Override
+    public String interceptedSubset(String message, boolean modify, boolean repeat, boolean doReturn) {
+        return message;
+    }
+
+    @Override
+    public String notIntercepted(String message, boolean modify, boolean repeat, boolean doReturn) {
+        return message;
+    }
+}

pico/tests/interception/src/main/java/io/helidon/pico/tests/interception/TheService.java

@@ -20,6 +20,7 @@
 
 @Singleton
 class TheService {
+    @Modify
     TheService() {
     }
 
@@ -35,6 +36,12 @@ String intercepted(String message, boolean modify, boolean repeat, boolean doRet
         return message;
     }
 
+    @Repeat
+    @Return
+    String interceptedSubset(String message, boolean modify, boolean repeat, boolean doReturn) {
+        return message;
+    }
+
     String notIntercepted(String message, boolean modify, boolean repeat, boolean doReturn) {
         return message;
     }

pico/tests/interception/src/test/java/io/helidon/pico/tests/interception/InterceptionTest.java

@@ -20,6 +20,7 @@
 import io.helidon.pico.api.Services;
 
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
@@ -46,21 +47,25 @@ static void init() {
         services = picoServices.services();
         service = services.lookup(TheService.class).get();
 
-        /*
-        // ALL of these assertions fail, as constructor is intercepted even if not annotated
-        // existing issue: #6632
         assertAll(
-                () -> assertThat("Interceptors should not be called for constructors - returning",
+                () -> assertThat("Interceptors should not be called for constructor - returning",
                                  ReturningInterceptor.lastCall(),
                                  nullValue()),
-                () -> assertThat("Interceptors should not be called for constructors - modifying",
+                () -> assertThat("Interceptors should be called for constructor - modifying",
                                  ModifyingInterceptor.lastCall(),
-                                 nullValue()),
-                () -> assertThat("Interceptors should not be called for constructors - repeating",
+                                 notNullValue()),
+                () -> assertThat("Interceptors should not be called for constructor - repeating",
                                  RepeatingInterceptor.lastCall(),
                                  nullValue())
         );
-         */
+    }
+
+    @BeforeEach
+    void beforeEach() {
+        // cleanup possible last calls from failed tests
+        ReturningInterceptor.lastCall();
+        ModifyingInterceptor.lastCall();
+        RepeatingInterceptor.lastCall();
     }
 
     @Test
@@ -82,67 +87,132 @@ void testNotIntercepted() {
         assertThat(response, is("hello"));
     }
 
+    @Test
+    void testInterceptedSubset() {
+        // test that only the interceptors valid for annotations are invoked
+        String response = service.interceptedSubset("hello", true, false, false);
+
+        Invocation returning = ReturningInterceptor.lastCall();
+        Invocation modifying = ModifyingInterceptor.lastCall();
+        Invocation repeating = RepeatingInterceptor.lastCall();
+
+        // first make sure the interceptors were/were not called
+        assertAll(
+                () -> assertThat("Interceptors should not be called for method not annotated with @Modify",
+                                 modifying,
+                                 nullValue()),
+                () -> assertThat("Interceptor should be called for method annotated with @Return",
+                                 returning,
+                                 notNullValue()),
+                () -> assertThat("Interceptor should be called for method annotated with @Repeat",
+                                 repeating,
+                                 notNullValue())
+        );
+
+        // then assert the called values
+        assertAll(
+                () -> assertThat("Returning last call", returning.methodName(), is("interceptedSubset")),
+                () -> assertThat("Returning last call", returning.args(), is(new Object[] {"hello", true, false, false})),
+                () -> assertThat("Repeating last call", repeating.methodName(), is("interceptedSubset")),
+                () -> assertThat("Repeating last call", repeating.args(), is(new Object[] {"hello", true, false, false}))
+        );
+
+        // and finally the response string
+        assertThat(response, is("hello"));
+    }
+
     @Test
     void testReturn() {
         String response = service.intercepted("hello", false, false, true);
 
-        Invocation last = ReturningInterceptor.lastCall();
-
+        Invocation returning = ReturningInterceptor.lastCall();
+        Invocation modifying = ModifyingInterceptor.lastCall();
+        Invocation repeating = RepeatingInterceptor.lastCall();
+        // first make sure the interceptors were/were not called
         assertAll(
-                () -> assertThat("Returning last call", last, notNullValue()),
-                () -> assertThat("Returning last call", last.methodName(), is("intercepted")),
-                () -> assertThat("Returning last call", last.args(), is(new Object[] {"hello", false, false, true})),
-                () -> assertThat("Interceptors should not be called when first interceptor returns",
-                                 ModifyingInterceptor.lastCall(),
+                () -> assertThat("Interceptors should not be called as ReturningInterceptor should have returned",
+                                 modifying,
                                  nullValue()),
-                () -> assertThat("Interceptors should not be called when first interceptor returns",
-                                 RepeatingInterceptor.lastCall(),
+                () -> assertThat("Interceptor should be called for method annotated with @Return",
+                                 returning,
+                                 notNullValue()),
+                () -> assertThat("Interceptor should not be called as ReturningInterceptor should have returned",
+                                 repeating,
                                  nullValue())
         );
 
+        assertAll(
+                () -> assertThat("Returning last call", returning.methodName(), is("intercepted")),
+                () -> assertThat("Returning last call", returning.args(), is(new Object[] {"hello", false, false, true}))
+        );
+
         assertThat(response, is("fixed_answer"));
     }
 
     @Test
     void testModify() {
         String response = service.intercepted("hello", true, false, false);
-        assertThat(response, is("mod_hello"));
 
         Invocation returning = ReturningInterceptor.lastCall();
         Invocation modifying = ModifyingInterceptor.lastCall();
         Invocation repeating = RepeatingInterceptor.lastCall();
 
+        // first make sure the interceptors were/were not called
+        assertAll(
+                () -> assertThat("Interceptors should be called for method annotated with @Modify",
+                                 modifying,
+                                 notNullValue()),
+                () -> assertThat("Interceptor should be called for method annotated with @Return",
+                                 returning,
+                                 notNullValue()),
+                () -> assertThat("Interceptor should be called for method annotated with @Repeat",
+                                 repeating,
+                                 notNullValue())
+        );
+
+        // then assert the called values
         assertAll(
-                () -> assertThat("Returning last call", returning, notNullValue()),
                 () -> assertThat("Returning last call", returning.methodName(), is("intercepted")),
                 () -> assertThat("Returning last call", returning.args(), is(new Object[] {"hello", true, false, false})),
-                () -> assertThat("Modifying last call", modifying, notNullValue()),
                 () -> assertThat("Modifying last call", modifying.methodName(), is("intercepted")),
                 () -> assertThat("Modifying last call", modifying.args(), is(new Object[] {"hello", true, false, false})),
-                () -> assertThat("Repeating last call", repeating, notNullValue()),
                 () -> assertThat("Repeating last call", repeating.methodName(), is("intercepted")),
                 () -> assertThat("Repeating last call", repeating.args(), is(new Object[] {"mod_hello", true, false, false}))
         );
+
+        // and the message
+        assertThat(response, is("mod_hello"));
     }
 
     @Disabled("Known problem - issue #6629")
     @Test
     void testRepeat() {
         String response = service.intercepted("hello", false, true, false);
-        assertThat(response, is("mod_hello"));
+        assertThat(response, is("hello"));
 
         Invocation returning = ReturningInterceptor.lastCall();
         Invocation modifying = ModifyingInterceptor.lastCall();
         Invocation repeating = RepeatingInterceptor.lastCall();
 
+        // first make sure the interceptors were/were not called
+        assertAll(
+                () -> assertThat("Interceptors should be called for method annotated with @Modify",
+                                 modifying,
+                                 notNullValue()),
+                () -> assertThat("Interceptor should be called for method annotated with @Return",
+                                 returning,
+                                 notNullValue()),
+                () -> assertThat("Interceptor should be called for method annotated with @Repeat",
+                                 repeating,
+                                 notNullValue())
+        );
+
+        // then assert the called values
         assertAll(
-                () -> assertThat("Returning last call", returning, notNullValue()),
                 () -> assertThat("Returning last call", returning.methodName(), is("intercepted")),
                 () -> assertThat("Returning last call", returning.args(), is(new Object[] {"hello", false, true, false})),
-                () -> assertThat("Modifying last call", modifying, notNullValue()),
                 () -> assertThat("Modifying last call", modifying.methodName(), is("intercepted")),
                 () -> assertThat("Modifying last call", modifying.args(), is(new Object[] {"hello", false, true, false})),
-                () -> assertThat("Repeating last call", repeating, notNullValue()),
                 () -> assertThat("Repeating last call", repeating.methodName(), is("intercepted")),
                 () -> assertThat("Repeating last call", repeating.args(), is(new Object[] {"hello", true, false, false}))
         );