From e457d1eb9242c1c4f9a3a43a1e5e0e778b8c9bce Mon Sep 17 00:00:00 2001 From: Al B Date: Wed, 6 Apr 2016 12:41:10 +0100 Subject: [PATCH 1/2] Move TestSpecificCompiler into org.apache.avro.compiler.specific --- .../avro/compiler/{ => specific}/TestSpecificCompiler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename lang/java/compiler/src/test/java/org/apache/avro/compiler/{ => specific}/TestSpecificCompiler.java (99%) diff --git a/lang/java/compiler/src/test/java/org/apache/avro/compiler/TestSpecificCompiler.java b/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java similarity index 99% rename from lang/java/compiler/src/test/java/org/apache/avro/compiler/TestSpecificCompiler.java rename to lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java index 7c4fcc2a89f..f4eb71497fc 100644 --- a/lang/java/compiler/src/test/java/org/apache/avro/compiler/TestSpecificCompiler.java +++ b/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java @@ -15,7 +15,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.avro.compiler; +package org.apache.avro.compiler.specific; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; From 98954064ee8bcafbd7bddf969585a13441bdde77 Mon Sep 17 00:00:00 2001 From: Al B Date: Wed, 6 Apr 2016 15:52:14 +0100 Subject: [PATCH 2/2] Implement all requested code review changes for AVRO-1642 --- .../compiler/specific/SpecificCompiler.java | 57 +++++++++++ .../specific/templates/java/classic/record.vm | 9 ++ .../specific/TestSpecificCompiler.java | 98 ++++++++++++++++++- 3 files changed, 163 insertions(+), 1 deletion(-) diff --git a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java index c1fa08a8d8d..6faf3687287 100644 --- a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java +++ b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java @@ -61,6 +61,27 @@ * Java reserved keywords are mangled to preserve compilation. */ public class SpecificCompiler { + + /* + * From Section 4.10 of the Java VM Specification: + * A method descriptor is valid only if it represents method parameters with a total length of 255 or less, + * where that length includes the contribution for this in the case of instance or interface method invocations. + * The total length is calculated by summing the contributions of the individual parameters, where a parameter + * of type long or double contributes two units to the length and a parameter of any other type contributes one unit. + * + * Arguments of type Double/Float contribute 2 "parameter units" to this limit, all other types contribute 1 + * "parameter unit". All instance methods for a class are passed a reference to the instance (`this), and hence, + * they are permitted at most `JVM_METHOD_ARG_LIMIT-1` "parameter units" for their arguments. + * + * @see JVM Spec: Section 4.10 + */ + private static final int JVM_METHOD_ARG_LIMIT = 255; + + /* + * Note: This is protected instead of private only so it's visible for testing. + */ + protected static final int MAX_FIELD_PARAMETER_UNIT_COUNT = JVM_METHOD_ARG_LIMIT - 1; + public static enum FieldVisibility { PUBLIC, PUBLIC_DEPRECATED, PRIVATE } @@ -71,8 +92,16 @@ public static enum FieldVisibility { private String templateDir; private FieldVisibility fieldVisibility = FieldVisibility.PUBLIC_DEPRECATED; private boolean createSetters = true; + private boolean createAllArgsConstructor = true; private String outputCharacterEncoding; + /* + * Used in the record.vm template. + */ + public boolean isCreateAllArgsConstructor() { + return createAllArgsConstructor; + } + /* Reserved words for accessor/mutator methods */ private static final Set ACCESSOR_MUTATOR_RESERVED_WORDS = new HashSet(Arrays.asList(new String[] { @@ -357,6 +386,33 @@ static String makePath(String name, String space) { } } + /** + * Returns the number of parameter units required by fields for the + * AllArgsConstructor. + * + * @param record a Record schema + */ + protected int calcAllArgConstructorParameterUnits(Schema record) { + + if (record.getType() != Schema.Type.RECORD) + throw new RuntimeException("This method must only be called for record schemas."); + + return record.getFields().size(); + } + + protected void validateRecordForCompilation(Schema record) { + this.createAllArgsConstructor = + calcAllArgConstructorParameterUnits(record) <= MAX_FIELD_PARAMETER_UNIT_COUNT; + + if (!this.createAllArgsConstructor) + new Slf4jLogChute().log(LogChute.WARN_ID, "Record '" + record.getFullName() + + "' contains more than " + MAX_FIELD_PARAMETER_UNIT_COUNT + + " parameters which exceeds the JVM " + + "spec for the number of permitted constructor arguments. Clients must " + + "rely on the builder pattern to create objects instead. For more info " + + "see JIRA ticket AVRO-1642."); + } + OutputFile compile(Schema schema) { schema = addStringType(schema); // annotate schema as needed String output = ""; @@ -366,6 +422,7 @@ OutputFile compile(Schema schema) { switch (schema.getType()) { case RECORD: + validateRecordForCompilation(schema); output = renderTemplate(templateDir+"record.vm", context); break; case ENUM: diff --git a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm index 1cd17e61501..5673fa4b6d8 100644 --- a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm +++ b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm @@ -66,6 +66,7 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or * one should use newBuilder(). */ public ${this.mangle($schema.getName())}() {} +#if ($this.isCreateAllArgsConstructor()) /** * All-args constructor. @@ -79,6 +80,14 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or this.${this.mangle($field.name())} = ${this.mangle($field.name())}; #end } +#else + /** + * This schema contains more than 254 fields which exceeds the maximum number + * of permitted constructor parameters in the JVM. An all-args constructor + * will not be generated. Please use newBuilder() to instantiate + * objects instead. + */ +#end #end #end diff --git a/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java b/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java index f4eb71497fc..7605724ffef 100644 --- a/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java +++ b/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java @@ -31,10 +31,13 @@ import java.io.IOException; import java.net.URISyntaxException; import java.nio.charset.Charset; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; import org.apache.avro.AvroTestUtil; import org.apache.avro.Schema; -import org.apache.avro.compiler.specific.SpecificCompiler; +import org.apache.avro.SchemaBuilder; import org.apache.avro.generic.GenericData.StringType; import org.junit.After; import org.junit.Before; @@ -42,6 +45,10 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import javax.tools.JavaCompiler; +import javax.tools.StandardJavaFileManager; +import javax.tools.ToolProvider; + @RunWith(JUnit4.class) public class TestSpecificCompiler { private final String schemaSrcPath = "src/test/resources/simple_record.avsc"; @@ -65,6 +72,40 @@ public void tearDow() { } } + /** Uses the system's java compiler to actually compile the generated code. */ + static void assertCompilesWithJavaCompiler(Collection outputs) + throws IOException { + if (outputs.isEmpty()) + return; // Nothing to compile! + + JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); + StandardJavaFileManager fileManager = + compiler.getStandardFileManager(null, null, null); + + File dstDir = AvroTestUtil.tempFile(TestSpecificCompiler.class, "realCompiler"); + List javaFiles = new ArrayList(); + for (SpecificCompiler.OutputFile o : outputs) { + javaFiles.add(o.writeToDestination(null, dstDir)); + } + + JavaCompiler.CompilationTask cTask = compiler.getTask(null, fileManager, + null, null, null, fileManager.getJavaFileObjects( + javaFiles.toArray(new File[javaFiles.size()]))); + boolean compilesWithoutError = cTask.call(); + assertTrue(compilesWithoutError); + } + + private static Schema createSampleRecordSchema(int numStringFields, int numDoubleFields) { + SchemaBuilder.FieldAssembler sb = SchemaBuilder.record("sample.record").fields(); + for (int i = 0; i < numStringFields; i++) { + sb.name("sf_" + i).type().stringType().noDefault(); + } + for (int i = 0; i < numDoubleFields; i++) { + sb.name("df_" + i).type().doubleType().noDefault(); + } + return sb.endRecord(); + } + private SpecificCompiler createCompiler() throws IOException { Schema.Parser parser = new Schema.Parser(); Schema schema = parser.parse(this.src); @@ -102,6 +143,61 @@ public void testPublicFieldVisibility() throws IOException { } } + @Test + public void testCreateAllArgsConstructor() throws Exception { + SpecificCompiler compiler = createCompiler(); + compiler.compileToDestination(this.src, this.outputDir); + assertTrue(this.outputFile.exists()); + BufferedReader reader = new BufferedReader(new FileReader(this.outputFile)); + String line = null; + boolean foundAllArgsConstructor = false; + while (!foundAllArgsConstructor && (line = reader.readLine()) != null) { + foundAllArgsConstructor = line.contains("All-args constructor"); + } + assertTrue(foundAllArgsConstructor); + } + + @Test + public void testMaxValidParameterCounts() throws Exception { + Schema validSchema1 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT, 0); + assertCompilesWithJavaCompiler(new SpecificCompiler(validSchema1).compile()); + + Schema validSchema2 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT - 2, 1); + assertCompilesWithJavaCompiler(new SpecificCompiler(validSchema1).compile()); + } + + @Test + public void testInvalidParameterCounts() throws Exception { + Schema invalidSchema1 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT + 1, 0); + SpecificCompiler compiler = new SpecificCompiler(invalidSchema1); + assertCompilesWithJavaCompiler(compiler.compile()); + + Schema invalidSchema2 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT, 10); + compiler = new SpecificCompiler(invalidSchema2); + assertCompilesWithJavaCompiler(compiler.compile()); + } + + @Test + public void testMaxParameterCounts() throws Exception { + Schema validSchema1 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT, 0); + assertTrue(new SpecificCompiler(validSchema1).compile().size() > 0); + + Schema validSchema2 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT - 2, 1); + assertTrue(new SpecificCompiler(validSchema2).compile().size() > 0); + + Schema validSchema3 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT - 1, 1); + assertTrue(new SpecificCompiler(validSchema3).compile().size() > 0); + + Schema validSchema4 = createSampleRecordSchema(SpecificCompiler.MAX_FIELD_PARAMETER_UNIT_COUNT + 1, 0); + assertTrue(new SpecificCompiler(validSchema4).compile().size() > 0); + } + + @Test(expected=RuntimeException.class) + public void testCalcAllArgConstructorParameterUnitsFailure() { + Schema nonRecordSchema = SchemaBuilder.array().items().booleanType(); + new SpecificCompiler().calcAllArgConstructorParameterUnits(nonRecordSchema); + } + @Test public void testPublicDeprecatedFieldVisibility() throws IOException { SpecificCompiler compiler = createCompiler();