[CALCITE-5008] Ignore synthetic and static methods in MetadataDef

According to https://www.jacoco.org/jacoco/trunk/doc/faq.html , Jacoco
adds a synthetic static method called $jacocoInit() during
instrumentation.

MetadataDef constructor breaks if it encounters such method on
the class given to it because it does not expect it. But it looks like
it simply should not consider synthetic methods (as well as static
methods) at all.

- extract code for finding handler methods
- reuse it in both places
- add tests concerning synthetic methods

bom/build.gradle.kts

@@ -114,6 +114,7 @@ dependencies {
         apiv("org.apiguardian:apiguardian-api")
         apiv("org.bouncycastle:bcpkix-jdk15on", "bouncycastle")
         apiv("org.bouncycastle:bcprov-jdk15on", "bouncycastle")
+        apiv("net.bytebuddy:byte-buddy")
         apiv("org.cassandraunit:cassandra-unit")
         apiv("org.codehaus.janino:commons-compiler", "janino")
         apiv("org.codehaus.janino:janino")

core/build.gradle.kts

@@ -85,6 +85,7 @@ dependencies {
 
     testImplementation(project(":testkit"))
     testImplementation("commons-lang:commons-lang")
+    testImplementation("net.bytebuddy:byte-buddy")
     testImplementation("net.hydromatic:foodmart-queries")
     testImplementation("net.hydromatic:quidem")
     testImplementation("org.apache.calcite.avatica:avatica-server")

core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java

@@ -40,8 +40,7 @@ private MetadataDef(Class<M> metadataClass,
     this.metadataClass = metadataClass;
     this.handlerClass = handlerClass;
     this.methods = ImmutableList.copyOf(methods);
-    final Method[] handlerMethods = Arrays.stream(handlerClass.getDeclaredMethods())
-        .filter(m -> !m.getName().equals("getDef")).toArray(i -> new Method[i]);
+    final Method[] handlerMethods = MetadataHandler.handlerMethods(handlerClass);
 
     // Handler must have the same methods as Metadata, each method having
     // additional "subclass-of-RelNode, RelMetadataQuery" parameters.

core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java

@@ -16,11 +16,30 @@
  */
 package org.apache.calcite.rel.metadata;
 
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+
 /**
  * Marker interface for a handler of metadata.
  *
  * @param <M> Kind of metadata
  */
 public interface MetadataHandler<M extends Metadata> {
   MetadataDef<M> getDef();
+
+  /**
+   * Finds handler methods defined by a {@link MetadataHandler}. Static and synthetic methods
+   * are ignored.
+   *
+   * @param handlerClass the handler class to inspect
+   * @return handler methods
+   */
+  static Method[] handlerMethods(Class<? extends MetadataHandler<?>> handlerClass) {
+    return Arrays.stream(handlerClass.getDeclaredMethods())
+        .filter(m -> !m.getName().equals("getDef"))
+        .filter(m -> !m.isSynthetic())
+        .filter(m -> !Modifier.isStatic(m.getModifiers()))
+        .toArray(Method[]::new);
+  }
 }

core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java

@@ -62,8 +62,7 @@ public static HandlerNameAndGeneratedCode generateHandler(
         .getName();
     final String name =
         "GeneratedMetadata_" + simpleNameForHandler(handlerClass);
-    final Method[] declaredMethods = Arrays.stream(handlerClass.getDeclaredMethods())
-        .filter(m -> !m.getName().equals("getDef")).toArray(i -> new Method[i]);
+    final Method[] declaredMethods = MetadataHandler.handlerMethods(handlerClass);
     Arrays.sort(declaredMethods, Comparator.comparing(Method::getName));
 
     final Map<MetadataHandler<?>, String> handlerToName = new LinkedHashMap<>();

core/src/test/java/org/apache/calcite/rel/metadata/MetadataDefTest.java

@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you 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 org.apache.calcite.rel.metadata;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+/**
+ * Test cases for {@link MetadataDef}.
+ */
+class MetadataDefTest {
+  @Test void staticMethodInHandlerIsIgnored() {
+    assertDoesNotThrow(
+        () -> MetadataDef.of(TestMetadata.class, MetadataHandlerWithStaticMethod.class)
+    );
+  }
+
+  @Test void synthenticMethodInHandlerIsIgnored() {
+    assertDoesNotThrow(
+        () -> MetadataDef.of(TestMetadata.class,
+            TestMetadataHandlers.handlerClassWithSyntheticMethod())
+    );
+  }
+}

core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java

@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you 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 org.apache.calcite.rel.metadata;
+
+import org.junit.jupiter.api.Test;
+
+import java.lang.reflect.Method;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.emptyArray;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Tests for {@link MetadataHandler}.
+ */
+class MetadataHandlerTest {
+  @Test void findsHandlerMethods() {
+    Method[] methods = MetadataHandler.handlerMethods(TestMetadataHandler.class);
+
+    assertThat(methods.length, is(1));
+    assertThat(methods[0].getName(), is("getTestMetadata"));
+  }
+
+  @Test void getDefMethodInHandlerIsIgnored() {
+    Method[] methods = MetadataHandler.handlerMethods(
+        MetadataHandlerWithGetDefMethodOnly.class);
+
+    assertThat(methods, is(emptyArray()));
+  }
+
+  @Test void staticMethodInHandlerIsIgnored() {
+    Method[] methods = MetadataHandler.handlerMethods(MetadataHandlerWithStaticMethod.class);
+
+    assertThat(methods, is(emptyArray()));
+  }
+
+  @Test void synthenticMethodInHandlerIsIgnored() {
+    Method[] methods = MetadataHandler.handlerMethods(
+        TestMetadataHandlers.handlerClassWithSyntheticMethod());
+
+    assertThat(methods, is(emptyArray()));
+  }
+
+  /**
+   * {@link MetadataHandler} which has a handler method.
+   */
+  interface TestMetadataHandler extends MetadataHandler<TestMetadata> {
+    @SuppressWarnings("unused")
+    TestMetadata getTestMetadata();
+  }
+
+  /**
+   * {@link MetadataHandler} which only has getDef() method.
+   */
+  interface MetadataHandlerWithGetDefMethodOnly extends MetadataHandler<TestMetadata> {
+    MetadataDef<TestMetadata> getDef();
+  }
+}

core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerWithStaticMethod.java

@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you 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 org.apache.calcite.rel.metadata;
+
+/**
+ * A {@link MetadataHandler} having a static method.
+ */
+interface MetadataHandlerWithStaticMethod extends MetadataHandler<TestMetadata> {
+  @SuppressWarnings("unused")
+  static void staticMethod() {
+    // do nothing
+  }
+}

core/src/test/java/org/apache/calcite/rel/metadata/TestMetadata.java

@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you 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 org.apache.calcite.rel.metadata;
+
+/**
+ * A test {@link Metadata} interface.
+ */
+interface TestMetadata extends Metadata {
+}

core/src/test/java/org/apache/calcite/rel/metadata/TestMetadataHandlers.java

@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you 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 org.apache.calcite.rel.metadata;
+
+import net.bytebuddy.ByteBuddy;
+import net.bytebuddy.description.modifier.SyntheticState;
+import net.bytebuddy.description.modifier.Visibility;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+import net.bytebuddy.implementation.FixedValue;
+
+/**
+ * Constructs {@link MetadataHandler} classes useful for tests.
+ */
+class TestMetadataHandlers {
+  /**
+   * Returns a class representing an interface extending {@link MetadataHandler} and having
+   * a synthetic method.
+   *
+   * @return MetadataHandler class with a synthetic method
+   */
+  static Class<? extends MetadataHandler<TestMetadata>> handlerClassWithSyntheticMethod() {
+    return new ByteBuddy()
+        .redefine(BlankMetadataHandler.class)
+        .defineMethod("syntheticMethod", Void.class, SyntheticState.SYNTHETIC, Visibility.PUBLIC)
+        .intercept(FixedValue.nullValue())
+        .make()
+        .load(TestMetadataHandlers.class.getClassLoader(), ClassLoadingStrategy.Default.CHILD_FIRST)
+        .getLoaded();
+  }
+
+  private TestMetadataHandlers() {
+    // prevent instantiation
+  }
+
+  /**
+   * A blank {@link MetadataHandler} that is used as a base for adding a synthetic method.
+   */
+  private interface BlankMetadataHandler extends MetadataHandler<TestMetadata> {
+  }
+}

gradle.properties

@@ -80,6 +80,7 @@ aggdesigner-algorithm.version=6.0
 apiguardian-api.version=1.1.0
 asm.version=7.2
 bouncycastle.version=1.60
+byte-buddy.version=1.9.3
 cassandra-all.version=4.0.1
 cassandra-java-driver-core.version=4.13.0
 cassandra-unit.version=4.3.1.0