martin-g commented on a change in pull request #885:
URL: https://github.com/apache/avro/pull/885#discussion_r710947377
##########
File path: lang/java/avro/src/main/java/org/apache/avro/LogicalTypes.java
##########
@@ -51,9 +65,27 @@ default String getTypeName() {
public static void register(String logicalTypeName, LogicalTypeFactory
factory) {
Objects.requireNonNull(logicalTypeName, "Logical type name cannot be
null");
Objects.requireNonNull(factory, "Logical type factory cannot be null");
+
+ try {
+ String factoryTypeName = factory.getTypeName();
+ if (!logicalTypeName.equals(factoryTypeName)) {
+ throw new IllegalArgumentException(String.format(
Review comment:
This might break some old users of this API.
##########
File path: lang/java/avro/src/main/java/org/apache/avro/LogicalTypes.java
##########
@@ -51,9 +65,27 @@ default String getTypeName() {
public static void register(String logicalTypeName, LogicalTypeFactory
factory) {
Objects.requireNonNull(logicalTypeName, "Logical type name cannot be
null");
Objects.requireNonNull(factory, "Logical type factory cannot be null");
+
+ try {
+ String factoryTypeName = factory.getTypeName();
+ if (!logicalTypeName.equals(factoryTypeName)) {
+ throw new IllegalArgumentException(String.format(
+ "Provided logicalTypeName '%s' does not match factory typeName
'%s'", logicalTypeName, factoryTypeName));
+ }
+ } catch (UnsupportedOperationException ignore) {
+ // ignore exception, as by default this value has not been provided
Review comment:
Which value is not provided ? It is not very clear.
It seems only `factory.getTypeName();` can throw it. And it looks like a
hack to avoid the new protection.
##########
File path: lang/java/avro/src/test/java/org/apache/avro/TestLogicalType.java
##########
@@ -226,6 +228,73 @@ public void testLogicalTypeInSchemaEquals() {
assertEqualsFalse("Different logical type", schema1, schema3);
}
+ @Test
+ public void testRegisterLogicalTypeThrowsIfTypeNameInconsistent() {
+ assertThrows("Should error if type name is inconsistent",
IllegalArgumentException.class,
+ "Provided logicalTypeName 'name' does not match factory typeName
'different'", () -> {
+ LogicalTypes.register("name", new LogicalTypes.LogicalTypeFactory() {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return LogicalTypes.date();
+ }
+
+ @Override
+ public String getTypeName() {
+ return "different";
+ }
+ });
+ return null;
+ });
+ }
+
+ @Test
+ public void testRegisterLogicalTypeWithName() {
+ final LogicalTypes.LogicalTypeFactory factory = new
LogicalTypes.LogicalTypeFactory() {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return LogicalTypes.date();
+ }
+
+ @Override
+ public String getTypeName() {
+ return "same";
+ }
+ };
+
+ LogicalTypes.register("same", factory);
+
+ MatcherAssert.assertThat(LogicalTypes.getCustomRegisteredTypes(),
IsMapContaining.hasEntry("same", factory));
+ }
+
+ @Test
+ public void testRegisterLogicalTypeWithFactoryName() {
+ final LogicalTypes.LogicalTypeFactory factory = new
LogicalTypes.LogicalTypeFactory() {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return LogicalTypes.date();
+ }
+
+ @Override
+ public String getTypeName() {
+ return "factory";
+ }
+ };
+
+ LogicalTypes.register(factory);
+
+ MatcherAssert.assertThat(LogicalTypes.getCustomRegisteredTypes(),
IsMapContaining.hasEntry("factory", factory));
+ }
+
+ @Test
+ public void testRegisterLogicalTypeWithFactoryNameNotProvidedWithoutError() {
+ final LogicalTypes.LogicalTypeFactory factory = schema ->
LogicalTypes.date();
+
+ LogicalTypes.register("logicalTypeName", factory);
+
+ MatcherAssert.assertThat(LogicalTypes.getCustomRegisteredTypes(),
+ IsMapContaining.hasEntry("logicalTypeName", factory));
+ }
+
Review comment:
It is clear to me now!
I'll let others decide whether the improvement (the names check) is allowed
now. I see its value but as I said it might break someone's app.
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -300,8 +305,8 @@ public void testSettingOutputCharacterEncoding() throws
Exception {
is.close(); // close input stream otherwise delete might fail
if (!this.outputFile.delete()) {
throw new IllegalStateException("unable to delete " + this.outputFile);
// delete otherwise compiler might not
-
// overwrite because src timestamp hasn't
-
// changed.
+ // overwrite because src timestamp hasn't
Review comment:
Please restore the formatting or move the whole comment on the previous
line
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -516,6 +521,36 @@ public void testNullableTypesJavaUnbox() throws Exception {
"java.lang.Boolean");
}
+ @Test
+ public void testGetUsedCustomLogicalTypeFactories() throws Exception {
+ LogicalTypes.register("string-constant", new
StringCustomLogicalTypeFactory());
+
+ SpecificCompiler compiler = createCompiler();
+ compiler.setEnableDecimalLogicalType(true);
+
+ final Schema schema = new Schema.Parser().parse(
+
"{\"type\":\"record\",\"name\":\"NestedLogicalTypesRecord\",\"namespace\":\"org.apache.avro.codegentest.testdata\",\"doc\":\"Test
nested types with logical types in generated Java
classes\",\"fields\":[{\"name\":\"nestedRecord\",\"type\":{\"type\":\"record\",\"name\":\"NestedRecord\",\"fields\":[{\"name\":\"nullableDateField\",\"type\":[\"null\",{\"type\":\"int\",\"logicalType\":\"date\"}]}]}},{\"name\":\"myLogical\",\"type\":{\"type\":\"string\",\"logicalType\":\"string-constant\"}}]}");
Review comment:
let's split this line into several shorter concatenated ones
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -860,4 +895,11 @@ public void testAdditionalToolsAreInjectedIntoTemplate()
throws Exception {
assertEquals(1, itWorksFound);
}
+ public static class StringCustomLogicalTypeFactory implements
LogicalTypes.LogicalTypeFactory {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return new LogicalType("string-constant");
Review comment:
`"string-custom"` ?!
##########
File path:
lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
##########
@@ -282,37 +287,51 @@ public void addCustomConversion(Class<?> conversionClass)
{
public Collection<String> getUsedConversionClasses(Schema schema) {
Collection<String> result = new HashSet<>();
- for (Conversion<?> conversion : getUsedConversions(schema, new
HashSet<>(), new HashSet<>())) {
+ for (Conversion<?> conversion : getUsedConversions(schema)) {
result.add(conversion.getClass().getCanonicalName());
}
return result;
}
- private Set<Conversion<?>> getUsedConversions(Schema schema,
Set<Conversion<?>> result, Set<Schema> seenSchemas) {
+ public Set<String> getUsedCustomLogicalTypeFactories(Schema schema) {
+ final Set<String> logicalTypeNames =
getUsedLogicalTypes(schema).stream().map(LogicalType::getName)
+ .collect(Collectors.toSet());
+
+ return LogicalTypes.getCustomRegisteredTypes().entrySet().stream()
+ .filter(entry -> logicalTypeNames.contains(entry.getKey()))
+ .map(entry ->
entry.getValue().getClass().getCanonicalName()).collect(Collectors.toSet());
+ }
+
+ private void getUsedTypes(Schema schema, Set<Conversion<?>>
conversionResults, Set<LogicalType> logicalTypeResults,
Review comment:
Since the method's return type is `void` I would suggest to rename it to
`collectUsedTypes`.
##########
File path:
lang/java/integration-test/codegen-test/src/test/resources/avro/custom_conversion_logical_types_enum.avsc
##########
@@ -0,0 +1,19 @@
+{"namespace": "org.apache.avro.codegentest.testdata",
Review comment:
nit: move the namespace on a new line
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -516,6 +521,36 @@ public void testNullableTypesJavaUnbox() throws Exception {
"java.lang.Boolean");
}
+ @Test
+ public void testGetUsedCustomLogicalTypeFactories() throws Exception {
+ LogicalTypes.register("string-constant", new
StringCustomLogicalTypeFactory());
+
+ SpecificCompiler compiler = createCompiler();
+ compiler.setEnableDecimalLogicalType(true);
+
+ final Schema schema = new Schema.Parser().parse(
+
"{\"type\":\"record\",\"name\":\"NestedLogicalTypesRecord\",\"namespace\":\"org.apache.avro.codegentest.testdata\",\"doc\":\"Test
nested types with logical types in generated Java
classes\",\"fields\":[{\"name\":\"nestedRecord\",\"type\":{\"type\":\"record\",\"name\":\"NestedRecord\",\"fields\":[{\"name\":\"nullableDateField\",\"type\":[\"null\",{\"type\":\"int\",\"logicalType\":\"date\"}]}]}},{\"name\":\"myLogical\",\"type\":{\"type\":\"string\",\"logicalType\":\"string-constant\"}}]}");
+
+ final Set<String> usedCustomLogicalTypeFactories =
compiler.getUsedCustomLogicalTypeFactories(schema);
+ Assert.assertEquals(1, usedCustomLogicalTypeFactories.size());
+
Assert.assertEquals("org.apache.avro.compiler.specific.TestSpecificCompiler.StringCustomLogicalTypeFactory",
+ usedCustomLogicalTypeFactories.iterator().next());
+ }
+
+ @Test
+ public void testEmptyGetUsedCustomLogicalTypeFactories() throws Exception {
+ LogicalTypes.register("string-constant", new
StringCustomLogicalTypeFactory());
+
+ SpecificCompiler compiler = createCompiler();
+ compiler.setEnableDecimalLogicalType(true);
+
+ final Schema schema = new Schema.Parser().parse(
+
"{\"type\":\"record\",\"name\":\"NestedLogicalTypesRecord\",\"namespace\":\"org.apache.avro.codegentest.testdata\",\"doc\":\"Test
nested types with logical types in generated Java
classes\",\"fields\":[{\"name\":\"nestedRecord\",\"type\":{\"type\":\"record\",\"name\":\"NestedRecord\",\"fields\":[{\"name\":\"nullableDateField\",\"type\":[\"null\",{\"type\":\"int\",\"logicalType\":\"date\"}]}]}}]}");
Review comment:
line too long
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
