This is an automated email from the ASF dual-hosted git repository. liyafan pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 316e5757818c9491d78eb55befa99a55d4f74d0d Author: Roman Puchkovskiy <[email protected]> AuthorDate: Thu Feb 10 16:17:14 2022 +0400 [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 | 1 + core/build.gradle.kts | 1 + .../apache/calcite/rel/metadata/MetadataDef.java | 3 +- .../calcite/rel/metadata/MetadataHandler.java | 19 ++++++ .../janino/RelMetadataHandlerGeneratorUtil.java | 3 +- .../calcite/rel/metadata/MetadataDefTest.java} | 23 +++++-- .../calcite/rel/metadata/MetadataHandlerTest.java | 72 ++++++++++++++++++++++ .../metadata/MetadataHandlerWithStaticMethod.java} | 11 ++-- .../apache/calcite/rel/metadata/TestMetadata.java} | 7 +-- .../calcite/rel/metadata/TestMetadataHandlers.java | 54 ++++++++++++++++ gradle.properties | 1 + 11 files changed, 176 insertions(+), 19 deletions(-) diff --git a/bom/build.gradle.kts b/bom/build.gradle.kts index f069a0c..b35d480 100644 --- a/bom/build.gradle.kts +++ b/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") diff --git a/core/build.gradle.kts b/core/build.gradle.kts index 8b6499e..6c31bfe 100644 --- a/core/build.gradle.kts +++ b/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") diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java index 1ee8af0..30ccea2 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java +++ b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java @@ -40,8 +40,7 @@ public class MetadataDef<M extends Metadata> { 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. diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java index d032fa2..d543580 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java +++ b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java @@ -16,6 +16,10 @@ */ 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. * @@ -23,4 +27,19 @@ package org.apache.calcite.rel.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); + } } diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java b/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java index d68d338..5dd1a5a 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java +++ b/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java @@ -62,8 +62,7 @@ public class RelMetadataHandlerGeneratorUtil { .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<>(); diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java b/core/src/test/java/org/apache/calcite/rel/metadata/MetadataDefTest.java similarity index 61% copy from core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java copy to core/src/test/java/org/apache/calcite/rel/metadata/MetadataDefTest.java index d032fa2..b1658ed 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java +++ b/core/src/test/java/org/apache/calcite/rel/metadata/MetadataDefTest.java @@ -16,11 +16,24 @@ */ package org.apache.calcite.rel.metadata; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; + /** - * Marker interface for a handler of metadata. - * - * @param <M> Kind of metadata + * Test cases for {@link MetadataDef}. */ -public interface MetadataHandler<M extends Metadata> { - MetadataDef<M> getDef(); +class MetadataDefTest { + @Test void staticMethodInHandlerIsIgnored() { + assertDoesNotThrow( + () -> MetadataDef.of(TestMetadata.class, MetadataHandlerWithStaticMethod.class) + ); + } + + @Test void synthenticMethodInHandlerIsIgnored() { + assertDoesNotThrow( + () -> MetadataDef.of(TestMetadata.class, + TestMetadataHandlers.handlerClassWithSyntheticMethod()) + ); + } } diff --git a/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java b/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java new file mode 100644 index 0000000..9c87a59 --- /dev/null +++ b/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(); + } +} diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java b/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerWithStaticMethod.java similarity index 79% copy from core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java copy to core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerWithStaticMethod.java index d032fa2..31750b9 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java +++ b/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerWithStaticMethod.java @@ -17,10 +17,11 @@ package org.apache.calcite.rel.metadata; /** - * Marker interface for a handler of metadata. - * - * @param <M> Kind of metadata + * A {@link MetadataHandler} having a static method. */ -public interface MetadataHandler<M extends Metadata> { - MetadataDef<M> getDef(); +interface MetadataHandlerWithStaticMethod extends MetadataHandler<TestMetadata> { + @SuppressWarnings("unused") + static void staticMethod() { + // do nothing + } } diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java b/core/src/test/java/org/apache/calcite/rel/metadata/TestMetadata.java similarity index 83% copy from core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java copy to core/src/test/java/org/apache/calcite/rel/metadata/TestMetadata.java index d032fa2..c2cc682 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java +++ b/core/src/test/java/org/apache/calcite/rel/metadata/TestMetadata.java @@ -17,10 +17,7 @@ package org.apache.calcite.rel.metadata; /** - * Marker interface for a handler of metadata. - * - * @param <M> Kind of metadata + * A test {@link Metadata} interface. */ -public interface MetadataHandler<M extends Metadata> { - MetadataDef<M> getDef(); +interface TestMetadata extends Metadata { } diff --git a/core/src/test/java/org/apache/calcite/rel/metadata/TestMetadataHandlers.java b/core/src/test/java/org/apache/calcite/rel/metadata/TestMetadataHandlers.java new file mode 100644 index 0000000..c2dc7dd --- /dev/null +++ b/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> { + } +} diff --git a/gradle.properties b/gradle.properties index 740d73d..b010dca 100644 --- a/gradle.properties +++ b/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
