ByteBuddyMockFactory adds @Internal annotation to Groovy MOP methods (

#1729)

The Groovy 3 & 4 runtime expects to have the `@Internal` annotation on the MOP methods from `GroovyObject`. That `AbstractCallSite.createGroovyObjectGetPropertySite()` processes it as `GroovyObject`. So the `ByteBuddyMockFactory` now adds the `@Internal` annotation to the intercepted MOP methods.

After that the "Unable to access protected constant when spying instances" problems are gone, because we take the normal Groovy route.

Also some strange inconsistencies for Groovy  2 <=> 3,4 are now gone, but a new inconsistency appeared Groovy 4 prefers is over get for boolean. But this is not a Spock issue, because in Groovy only code the same thing happens. See tests in JavaMocksForGroovyClasses.

This fixes #1501, #1452, #1608 and #1145.

Co-authored-by: Björn Kautler <Bjoern@Kautler.net>
Co-authored-by: Leonard Brünings <leonard.bruenings@gradle.com>

spock-core/src/main/java/org/spockframework/mock/runtime/BaseMockInterceptor.java

@@ -22,14 +22,13 @@ protected String handleGetProperty(GroovyObject target, Object[] args) {
       //go.getProperty("foo") is treated as is (to allow for its stubbing)
       String propertyName = (String)args[0];
       MetaClass metaClass = target.getMetaClass();
-      methodName = GroovyRuntimeUtil.propertyToMethodName("get", propertyName);
-      MetaMethod metaMethod = metaClass.getMetaMethod(methodName, GroovyRuntimeUtil.EMPTY_ARGUMENTS);
-      if (metaMethod == null) {
-        MetaMethod booleanVariant = metaClass
-          .getMetaMethod(GroovyRuntimeUtil.propertyToMethodName("is", propertyName), GroovyRuntimeUtil.EMPTY_ARGUMENTS);
-        if (booleanVariant != null && booleanVariant.getReturnType() == boolean.class) {
-          methodName = booleanVariant.getName();
-        }
+      //First try the isXXX before getXXX, because this is the expected behavior, if it is boolean property.
+      MetaMethod booleanVariant = metaClass
+        .getMetaMethod(GroovyRuntimeUtil.propertyToMethodName("is", propertyName), GroovyRuntimeUtil.EMPTY_ARGUMENTS);
+      if (booleanVariant != null && booleanVariant.getReturnType() == boolean.class) {
+        methodName = booleanVariant.getName();
+      } else {
+        methodName = GroovyRuntimeUtil.propertyToMethodName("get", propertyName);
       }
     }
     return methodName;

spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java

@@ -16,8 +16,12 @@
 
 package org.spockframework.mock.runtime;
 
+import groovy.lang.GroovyObject;
+import groovy.transform.Internal;
 import net.bytebuddy.ByteBuddy;
 import net.bytebuddy.TypeCache;
+import net.bytebuddy.description.annotation.AnnotationDescription;
+import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.modifier.SynchronizationState;
 import net.bytebuddy.description.modifier.SyntheticState;
 import net.bytebuddy.description.modifier.Visibility;
@@ -28,6 +32,7 @@
 import net.bytebuddy.implementation.FieldAccessor;
 import net.bytebuddy.implementation.MethodDelegation;
 import net.bytebuddy.implementation.bind.annotation.Morph;
+import org.codehaus.groovy.runtime.callsite.AbstractCallSite;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.VisibleForTesting;
 import org.spockframework.mock.ISpockMockObject;
@@ -39,7 +44,6 @@
 import java.util.concurrent.Callable;
 import java.util.concurrent.ThreadLocalRandom;
 
-import static net.bytebuddy.matcher.ElementMatchers.any;
 import static net.bytebuddy.matcher.ElementMatchers.none;
 
 class ByteBuddyMockFactory {
@@ -55,6 +59,7 @@ class ByteBuddyMockFactory {
 
   private static final Class<?> CODEGEN_TARGET_CLASS = Target.class;
   private static final String CODEGEN_PACKAGE = CODEGEN_TARGET_CLASS.getPackage().getName();
+  private static final AnnotationDescription INTERNAL_ANNOTATION = AnnotationDescription.Builder.ofType(Internal.class).build();
 
   /**
    * This array contains {@link TypeCachingLock} instances, which are used as java monitor locks for
@@ -106,11 +111,13 @@ Object createMock(final Class<?> type,
           .name(name)
           .implement(additionalInterfaces)
           .implement(ISpockMockObject.class)
-          .method(any())
-          .intercept(MethodDelegation.withDefaultConfiguration()
-            .withBinders(Morph.Binder.install(ByteBuddyInvoker.class))
-            .to(ByteBuddyInterceptorAdapter.class))
-          .transform(Transformer.ForMethod.withModifiers(SynchronizationState.PLAIN, Visibility.PUBLIC)) // Overridden methods should be public and non-synchronized.
+          .method(m -> isGroovyMOPMethod(type, m))
+          .intercept(mockInterceptor())
+          .transform(mockTransformer())
+          .annotateMethod(INTERNAL_ANNOTATION) //Annotate the Groovy MOP methods with @Internal
+          .method(m -> !isGroovyMOPMethod(type, m))
+          .intercept(mockInterceptor())
+          .transform(mockTransformer())
           .implement(ByteBuddyInterceptorAdapter.InterceptorAccess.class)
           .intercept(FieldAccessor.ofField("$spock_interceptor"))
           .defineField("$spock_interceptor", IProxyBasedMockInterceptor.class, Visibility.PRIVATE, SyntheticState.SYNTHETIC)
@@ -124,6 +131,34 @@ Object createMock(final Class<?> type,
     return proxy;
   }
 
+  private static Transformer<MethodDescription> mockTransformer() {
+    return Transformer.ForMethod.withModifiers(SynchronizationState.PLAIN, Visibility.PUBLIC); //Overridden methods should be public and non-synchronized.
+  }
+
+  private static MethodDelegation mockInterceptor() {
+    return MethodDelegation.withDefaultConfiguration()
+      .withBinders(Morph.Binder.install(ByteBuddyInvoker.class))
+      .to(ByteBuddyInterceptorAdapter.class);
+  }
+
+  /**
+   * Checks if the passed method, is a Groovy MOP method from {@link GroovyObject}.
+   *
+   * <p>{@code GroovyObject} defined MOP methods {@code getProperty()}, {@code setProperty()} and {@code invokeMethod()},
+   * because these methods are handled in a special way in the {@link AbstractCallSite} when marked with {@link Internal @Internal}.
+   * See also {@link GroovyObject} comments.
+   * So we need to mark the method with {@link Internal @Internal} annotation, if we intercept it.
+   *
+   * @param type   the type the mock
+   * @param method the method to intercept
+   * @return {@code true}, if the method is a Groovy MOP method
+   */
+  private static boolean isGroovyMOPMethod(Class<?> type, MethodDescription method) {
+    return GroovyObject.class.isAssignableFrom(type) &&
+      method.getDeclaredAnnotations().isAnnotationPresent(Internal.class) &&
+      method.isDefaultMethod();
+  }
+
   /**
    * Returns a {@link TypeCachingLock}, which locks the {@link TypeCache#findOrInsert(ClassLoader, Object, Callable, Object)}.
    *

spock-core/src/main/java/org/spockframework/mock/runtime/GroovyMockMetaClass.java

@@ -53,7 +53,11 @@ public Object invokeConstructor(Object[] arguments) {
 
   @Override
   public Object getProperty(Object target, String property) {
-    String methodName = GroovyRuntimeUtil.propertyToMethodName("get", property);
+    String methodName = GroovyRuntimeUtil.propertyToMethodName("is", property);
+    MetaMethod metaMethod = delegate.getMetaMethod(methodName, GroovyRuntimeUtil.EMPTY_ARGUMENTS);
+    if (metaMethod == null || metaMethod.getReturnType() != boolean.class) {
+      methodName = GroovyRuntimeUtil.propertyToMethodName("get", property);
+    }
     return invokeMethod(target, methodName, GroovyRuntimeUtil.EMPTY_ARGUMENTS);
   }
 

spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy

@@ -0,0 +1,102 @@
+/*
+ * Copyright 2023 the original author or authors.
+ *
+ * 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
+ *
+ *     https://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 org.spockframework.mock
+
+import spock.lang.Issue
+import spock.lang.Specification
+import spock.lang.Stepwise
+
+@Stepwise
+class AccessProtectedPropsSpec extends Specification {
+
+  /**
+   * The problem with https://github.com/spockframework/spock/issues/1501 is that the ByteBuddyMockFactory overrides
+   * the {@code getProperty(String)} method from GroovyObject without having the {@code groovy.transform.Internal}
+   * annotation. Therefore {@code org.codehaus.groovy.runtime.callsite.AbstractCallSite.createGroovyObjectGetPropertySite(java.lang.Object)}
+   * takes another code path.
+   */
+  @Issue("https://github.com/spockframework/spock/issues/1501")
+  @Issue("https://github.com/spockframework/spock/issues/1608")
+  def "Access protected const should be accessible in Groovy 3&4 Issue #1501"() {
+    when:
+    AccessProtectedSubClass mySpy = Spy()
+    then:
+    mySpy.accessStaticFlag()
+  }
+
+  @Issue("https://github.com/spockframework/spock/issues/1501")
+  @Issue("https://github.com/spockframework/spock/issues/1608")
+  def "Access protected should be accessible in Groovy 3&4 Issue #1501"() {
+    when:
+    AccessProtectedSubClass mySpy = Spy()
+    then:
+    mySpy.accessNonStaticFlag()
+  }
+
+  /**
+   * This feature must run after the {@code Access protected const should be accessible in Groovy 3&4 Issue #1501},
+   * otherwise the MetaClass gets cached in the GroovyRuntime, therefore we need the {@code @Stepwise}.
+   */
+  def "Access protected fields via access methods without spy"() {
+    when:
+    AccessProtectedSubClass myNonSpy = new AccessProtectedSubClass()
+    then:
+    myNonSpy.accessNonStaticFlag()
+    myNonSpy.accessStaticFlag()
+  }
+
+  @Issue("https://github.com/spockframework/spock/issues/1501")
+  def "Access protected fields should be accessible in Groovy 3&4 with Java class Issue #1501"() {
+    when:
+    AccessProtectedJavaSubClass mySpy = Spy()
+    then:
+    mySpy.accessNonStaticFlag()
+  }
+
+  @Issue("https://github.com/spockframework/spock/issues/1452")
+  def "Access protected fields Issue #1452"() {
+    when:
+    AccessProtectedSubClass mySpy = Spy()
+    then:
+    mySpy.nonStaticFlag
+    mySpy.staticFlag
+  }
+
+  def "Access protected fields without spy"() {
+    when:
+    AccessProtectedSubClass myNonSpy = new AccessProtectedSubClass()
+    then:
+    myNonSpy.nonStaticFlag
+    myNonSpy.staticFlag
+  }
+}
+
+class AccessProtectedBaseClass {
+  protected static boolean staticFlag = true
+  protected boolean nonStaticFlag = true
+}
+
+class AccessProtectedSubClass extends AccessProtectedBaseClass {
+  boolean accessNonStaticFlag() {
+    return nonStaticFlag
+  }
+
+  @SuppressWarnings('GrMethodMayBeStatic')
+  boolean accessStaticFlag() {
+    return staticFlag
+  }
+}

spock-specs/src/test/groovy/org/spockframework/mock/MockMapDelegateSpec.groovy

@@ -0,0 +1,42 @@
+/*
+ * Copyright 2023 the original author or authors.
+ *
+ * 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
+ *
+ *     https://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 org.spockframework.mock
+
+import spock.lang.Issue
+import spock.lang.Specification
+
+class MockMapDelegateSpec extends Specification {
+
+  @Issue("https://github.com/spockframework/spock/issues/1145")
+  def "Mock a Map delegate Issue #1145"() {
+    given:
+    MapDelegate ctx = Mock()
+
+    expect:
+    ctx.size() == 0
+    ctx.foo == null
+  }
+}
+
+class MapDelegate implements Map<String, Object> {
+  @Delegate
+  private Map<String, Object> target
+
+  MapDelegate(Map entries) {
+    this.target = entries
+  }
+}

spock-specs/src/test/groovy/org/spockframework/runtime/GroovyRuntimeUtilSpec.groovy

@@ -86,5 +86,17 @@ class GroovyRuntimeUtilSpec extends Specification {
     expect:
     GroovyRuntimeUtil.instantiateClosure(cl.getClass(), owner, thisObject).call(3) == 6
   }
+
+  def "asArgumentList"() {
+    when:
+    def l = GroovyRuntimeUtil.asArgumentList(null)
+    then:
+    l.isEmpty()
+    when:
+    l = GroovyRuntimeUtil.asArgumentList("A")
+    then:
+    l.size() == 1
+    l == ["A"]
+  }
 }
 

spock-specs/src/test/groovy/org/spockframework/smoke/mock/GroovyMocksForGroovyClasses.groovy

@@ -279,9 +279,9 @@ class GroovyMocksForGroovyClasses extends Specification {
     when: "query via property syntax"
     def result = mockData.active ? "Data is current" : "Data is not current"
 
-    then: "calls mock, preferring 'get' to 'is' for boolean getters (surprise!)"
-    1 * mockData.getActive() >> true
-    0 * mockData.isActive()
+    then: "calls mock, preferring 'is' to 'get' for boolean getters"
+    1 * mockData.isActive() >> true
+    0 * mockData.getActive()
 
     and:
     result == "Data is current"

spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaMocksForGroovyClasses.groovy

@@ -183,19 +183,7 @@ class JavaMocksForGroovyClasses extends Specification {
   }
 
   // TODO: swallowed when mocking static inner class because the latter implements methodMissing/propertyMissing
-  @Requires({ GroovyRuntimeUtil.isGroovy2() }) //different exception in Groovy 2 and 3
   @FailsWith(MissingPropertyException)
-  def "dynamic properties are considered to not exist (Groovy 2)"() {
-    when:
-    mockMe.someProperty
-
-    then:
-    1 * mockMe.someProperty
-  }
-
-  // TODO: swallowed when mocking static inner class because the latter implements methodMissing/propertyMissing
-  @Requires({ GroovyRuntimeUtil.isGroovy3orNewer() })
-  @FailsWith(MissingMethodException)
   def "dynamic properties are considered to not exist"() {
     when:
     mockMe.someProperty
@@ -299,21 +287,59 @@ class JavaMocksForGroovyClasses extends Specification {
   }
 
   @Issue("https://github.com/spockframework/spock/issues/1256")
-  def "Mock object boolean (get + is) accessor via dot-notation" () {
+  @Requires({ GroovyRuntimeUtil.isGroovy3orOlder() })
+  def "Mock object boolean (get + is) accessor via dot-notation (Groovy 2&3)"() {
     given:
     ExampleData mockData = Mock(ExampleData)
 
     when: "query via property syntax"
     def result = mockData.active ? "Data is current" : "Data is not current"
 
-    then: "calls mock, preferring 'get' to 'is' for boolean getters (surprise!)"
+    then: "calls mock, preferring 'get' to 'is' for boolean getters (surprise!) in Groovy <=3"
     1 * mockData.getActive() >> true
     0 * mockData.isActive()
 
     and:
     result == "Data is current"
   }
 
+  /**
+   * The resolution of properties changed in Groovy 4 for the ExampleData.active Groovy 4 resolves the 'is'
+   * whereas Groovy 2 & 3 resolved the 'get'.
+   */
+  @Issue("https://github.com/spockframework/spock/issues/1256")
+  @Requires({ GroovyRuntimeUtil.isGroovy4orNewer() })
+  def "Mock object boolean (get + is) accessor via dot-notation (Groovy 4)"() {
+    given:
+    ExampleData mockData = Mock(ExampleData)
+
+    when: "query via property syntax"
+    def result = mockData.active ? "Data is current" : "Data is not current"
+
+    then: "calls mock, preferring 'is' for boolean getters in Groovy >=4"
+    1 * mockData.isActive() >> true
+    0 * mockData.getActive()
+
+    and:
+    result == "Data is current"
+  }
+
+  @Requires({ GroovyRuntimeUtil.isGroovy3orOlder() })
+  def "Real object boolean (get + is) accessor via dot-notation (Groovy 2&3)"() {
+    given:
+    def data = new ExampleData()
+    expect: 'The getActive() method returns true'
+    data.active
+  }
+
+  @Requires({ GroovyRuntimeUtil.isGroovy4orNewer() })
+  def "Real object boolean (get + is) accessor via dot-notation (Groovy 4)"() {
+    given:
+    def data = new ExampleData()
+    expect: 'The isActive() method returns false'
+    !data.active
+  }
+
   @Issue("https://github.com/spockframework/spock/issues/1256")
   def "Mock object non-boolean (get + is) accessor via dot-notation" () {
     given:
@@ -399,7 +425,7 @@ class ExampleData {
   }
 
   boolean getActive() {
-    false
+    true
   }
 
   String isName() {