Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVRO-3968 - [Java] Support for custom @AvroNamespace annotation #2887

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* 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
*
* 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.apache.avro.reflect;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

/**
* Sets the avrotypename for this java type. When reading into this class, a
* reflectdatumreader looks for a schema field with the avrotypename.
*/
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE })
public @interface AvroNamespace {
String value() default "";
}
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,11 @@ public Class getClass(Schema schema) {
return Short.TYPE;
if (Character.class.getName().equals(intClass))
return Character.TYPE;
case RECORD:
case ENUM:
Class className = getClassProp(schema, CLASS_PROP);
if (className != null)
return className;
default:
return super.getClass(schema);
}
Expand Down Expand Up @@ -706,9 +711,7 @@ protected Schema createSchema(Type type, Map<String, Schema> names) {
AvroDoc annotatedDoc = c.getAnnotation(AvroDoc.class); // Docstring
String doc = (annotatedDoc != null) ? annotatedDoc.value() : null;
String name = c.getSimpleName();
String space = c.getPackage() == null ? "" : c.getPackage().getName();
if (c.getEnclosingClass() != null) // nested class
space = c.getEnclosingClass().getName().replace('$', '.');
String space = getNamespace(c);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the annotation @AvroTypeName not specify the type name?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not clear on this. Could you please explain this?

Do you mean to rename the function getNamespace to getTypeName ?

Union union = c.getAnnotation(Union.class);
if (union != null) { // union annotated
return getAnnotatedUnion(union, names);
Expand All @@ -722,17 +725,20 @@ protected Schema createSchema(Type type, Map<String, Schema> names) {
for (Enum constant : constants)
symbols.add(constant.name());
schema = Schema.createEnum(name, doc, space, symbols);
schema.addProp(CLASS_PROP, c.getName());
consumeAvroAliasAnnotation(c, schema);
} else if (GenericFixed.class.isAssignableFrom(c)) { // fixed
int size = c.getAnnotation(FixedSize.class).value();
schema = Schema.createFixed(name, doc, space, size);
schema.addProp(CLASS_PROP, c.getName());
consumeAvroAliasAnnotation(c, schema);
} else if (IndexedRecord.class.isAssignableFrom(c)) { // specific
return super.createSchema(type, names);
} else { // record
List<Schema.Field> fields = new ArrayList<>();
boolean error = Throwable.class.isAssignableFrom(c);
schema = Schema.createRecord(name, doc, space, error);
schema.addProp(CLASS_PROP, c.getName());
consumeAvroAliasAnnotation(c, schema);
names.put(c.getName(), schema);
for (Field field : getCachedFields(c))
Expand Down Expand Up @@ -802,6 +808,27 @@ private String simpleName(Class<?> c) {
return simpleName;
}

/*
* Function checks if there is @AvroTypeName annotation on the class. If present
* then returns the value of the annotation else returns the package of the
* class
*/
private String getNamespace(Class<?> c) {
AvroNamespace avroNamespace = c.getAnnotation(AvroNamespace.class);
if (avroNamespace != null) {
return avroNamespace.value();
}
if (c.getEnclosingClass() != null) { // nested class
AvroNamespace enclosingClassAvroNamespace = c.getEnclosingClass().getAnnotation(AvroNamespace.class);
if (enclosingClassAvroNamespace != null) {
return enclosingClassAvroNamespace.value();
}
return c.getEnclosingClass().getName().replace('$', '.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the enclosing class has an @AvroTypeName?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@opwvhk Thats a great point! I was assuming that we should only be overriding the namespace when the class has the AvroTypeName annotation.

Just to clarify if the class structure is as below:

@AvroTypeName("org.apache.avro.reflect.OverrideNamespace")
  private static class NamespaceTest {
    private static class EnclosingNamespaceTest {
    }
  }

When the schema definition is generated for EnclosingNamespaceTest.class, the namespace should be org.apache.avro.reflect.OverrideNamespace ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now addressed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a reviewer, but have a few questions.

  1. Classes can be nested further, e.g. a class within a class within a top-level class. Would this method need recursion? Or should only one level of nesting be supported?
  2. Without the namespace annotation, the namespace of a top-level class is its package, and the namespace of a nested class is package plus name of its enclosing class. In case the enclosing class has the namespace annotation, should its name also be added to the nested namespace in the same way?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find my comments below

  1. Classes can be nested further, e.g. a class within a class within a top-level class. Would this method need recursion? Or should only one level of nesting be supported? - To avoid any deviation I have kept the implementation same as the current implementation (without the namespace annotation) which only looks at the immediate top enclosing class for forming the namepsace.

  2. Without the namespace annotation, the namespace of a top-level class is its package, and the namespace of a nested class is package plus name of its enclosing class. In case the enclosing class has the namespace annotation, should its name also be added to the nested namespace in the same way? - Thats a valid point. Happy to change if the reviewer suggests so

}

return c.getPackage() == null ? "" : c.getPackage().getName();
}

private static final Schema THROWABLE_MESSAGE = makeNullable(Schema.create(Schema.Type.STRING));

// if array element type is a class with a union annotation, note it
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,10 @@
<p> The {@link org.apache.avro.reflect.AvroName AvroName} annotation renames
the field in the schema to the given name. The reflect datum reader will look
for a schema field with the given name, when trying to read into such an
annotated java field.
annotated java field.

<p> The {@link org.apache.avro.reflect.AvroNamespace AvroTypeName} annotation renames
the namespace in the schema to the given namespace.

<p>The {@link org.apache.avro.reflect.AvroMeta AvroMeta} annotation adds an
arbitrary key:value pair in the schema at the node of the java field.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,8 @@ public void testAvroNullableDefault() {
check(NullableDefaultTest.class,
"{\"type\":\"record\",\"name\":\"NullableDefaultTest\","
+ "\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"foo\",\"type\":[\"null\",\"int\"],\"default\":1}]}");
+ "{\"name\":\"foo\",\"type\":[\"null\",\"int\"],\"default\":1}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$NullableDefaultTest\"}");
}

private static class UnionDefaultTest {
Expand All @@ -579,7 +580,8 @@ public void testAvroUnionDefault() {
check(UnionDefaultTest.class,
"{\"type\":\"record\",\"name\":\"UnionDefaultTest\","
+ "\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"foo\",\"type\":[\"int\",\"string\"],\"default\":1}]}");
+ "{\"name\":\"foo\",\"type\":[\"int\",\"string\"],\"default\":1}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$UnionDefaultTest\"}");
}

@Test
Expand Down Expand Up @@ -639,8 +641,10 @@ public static enum E {

@Test
void testEnum() throws Exception {
check(E.class, "{\"type\":\"enum\",\"name\":\"E\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"symbols\":[\"A\",\"B\"]}");
check(E.class,
"{\"type\":\"enum\",\"name\":\"E\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"symbols\":[\"A\",\"B\"]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$E\"}");
}

public static class R {
Expand All @@ -652,7 +656,8 @@ public static class R {
void record() throws Exception {
check(R.class,
"{\"type\":\"record\",\"name\":\"R\",\"namespace\":" + "\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"a\",\"type\":\"int\"}," + "{\"name\":\"b\",\"type\":\"long\"}]}");
+ "{\"name\":\"a\",\"type\":\"int\"}," + "{\"name\":\"b\",\"type\":\"long\"}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$R\"}");
}

public static class RAvroIgnore {
Expand All @@ -663,7 +668,7 @@ public static class RAvroIgnore {
@Test
void annotationAvroIgnore() throws Exception {
check(RAvroIgnore.class, "{\"type\":\"record\",\"name\":\"RAvroIgnore\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":[]}");
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"java-class\":\"org.apache.avro.reflect.TestReflect$RAvroIgnore\"}");
}

@AvroMeta(key = "X", value = "Y")
Expand All @@ -677,7 +682,7 @@ void annotationAvroMeta() throws Exception {
check(RAvroMeta.class,
"{\"type\":\"record\",\"name\":\"RAvroMeta\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":[" + "{\"name\":\"a\",\"type\":\"int\",\"K\":\"V\"}]"
+ ",\"X\":\"Y\"}");
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$RAvroMeta\"" + ",\"X\":\"Y\"}");
}

@AvroMeta(key = "X", value = "Y")
Expand All @@ -693,7 +698,8 @@ void annotationMultiAvroMeta() {
check(RAvroMultiMeta.class,
"{\"type\":\"record\",\"name\":\"RAvroMultiMeta\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"a\",\"type\":\"int\",\"K\":\"V\",\"L\":\"W\"}]" + ",\"X\":\"Y\",\"A\":\"B\"}");
+ "{\"name\":\"a\",\"type\":\"int\",\"K\":\"V\",\"L\":\"W\"}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$RAvroMultiMeta\"" + ",\"X\":\"Y\",\"A\":\"B\"}");
}

public static class RAvroDuplicateFieldMeta {
Expand Down Expand Up @@ -729,8 +735,10 @@ public static class RAvroName {

@Test
void annotationAvroName() throws Exception {
check(RAvroName.class, "{\"type\":\"record\",\"name\":\"RAvroName\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":[" + "{\"name\":\"b\",\"type\":\"int\"}]}");
check(RAvroName.class,
"{\"type\":\"record\",\"name\":\"RAvroName\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":[" + "{\"name\":\"b\",\"type\":\"int\"}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$RAvroName\"}");
}

public static class RAvroNameCollide {
Expand All @@ -756,8 +764,10 @@ public static class RAvroStringableField {

@Test
void annotationAvroStringableFields() throws Exception {
check(RAvroStringableField.class, "{\"type\":\"record\",\"name\":\"RAvroStringableField\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":[" + "{\"name\":\"a\",\"type\":\"string\"}]}");
check(RAvroStringableField.class,
"{\"type\":\"record\",\"name\":\"RAvroStringableField\",\"namespace\":"
+ "\"org.apache.avro.reflect.TestReflect\",\"fields\":[" + "{\"name\":\"a\",\"type\":\"string\"}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$RAvroStringableField\"}");
}

private void check(Object o, String schemaJson) {
Expand Down Expand Up @@ -883,7 +893,8 @@ void avroEncodeInducing() throws IOException {
assertEquals(schm.toString(),
"{\"type\":\"record\",\"name\":\"AvroEncRecord\",\"namespace"
+ "\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[{\"name\":\"date\","
+ "\"type\":{\"type\":\"long\",\"CustomEncoding\":\"DateAsLongEncoding\"}}]}");
+ "\"type\":{\"type\":\"long\",\"CustomEncoding\":\"DateAsLongEncoding\"}}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$AvroEncRecord\"}");
}

@Test
Expand Down Expand Up @@ -1248,11 +1259,11 @@ private static class AliasC {
@Test
void avroAliasOnClass() {
check(AliasA.class,
"{\"type\":\"record\",\"name\":\"AliasA\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"aliases\":[\"b.a\"]}");
"{\"type\":\"record\",\"name\":\"AliasA\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"java-class\":\"org.apache.avro.reflect.TestReflect$AliasA\",\"aliases\":[\"b.a\"]}");
check(AliasB.class,
"{\"type\":\"record\",\"name\":\"AliasB\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"aliases\":[\"a\"]}");
"{\"type\":\"record\",\"name\":\"AliasB\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"java-class\":\"org.apache.avro.reflect.TestReflect$AliasB\",\"aliases\":[\"a\"]}");
check(AliasC.class,
"{\"type\":\"record\",\"name\":\"AliasC\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"aliases\":[\"a\"]}");
"{\"type\":\"record\",\"name\":\"AliasC\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"java-class\":\"org.apache.avro.reflect.TestReflect$AliasC\",\"aliases\":[\"a\"]}");
}

@AvroAlias(alias = "alias1", space = "space1")
Expand All @@ -1264,7 +1275,9 @@ private static class MultipleAliasRecord {
@Test
void multipleAliasAnnotationsOnClass() {
check(MultipleAliasRecord.class,
"{\"type\":\"record\",\"name\":\"MultipleAliasRecord\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"aliases\":[\"space1.alias1\",\"space2.alias2\"]}");
"{\"type\":\"record\",\"name\":\"MultipleAliasRecord\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$MultipleAliasRecord\""
+ ",\"aliases\":[\"space1.alias1\",\"space2.alias2\"]}");

}

Expand All @@ -1277,7 +1290,7 @@ void dollarTerminatedNamespaceCompatibility() {
Schema s = new Schema.Parser(NameValidator.NO_VALIDATION).parse(
"{\"type\":\"record\",\"name\":\"Z\",\"namespace\":\"org.apache.avro.reflect.TestReflect$\",\"fields\":[]}");
assertEquals(data.getSchema(data.getClass(s)).toString(),
"{\"type\":\"record\",\"name\":\"Z\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[]}");
"{\"type\":\"record\",\"name\":\"Z\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"java-class\":\"org.apache.avro.reflect.TestReflect$Z\"}");
}

@Test
Expand Down Expand Up @@ -1312,7 +1325,7 @@ void avroAliasOnField() {
Schema expectedSchema = SchemaBuilder.record(ClassWithAliasOnField.class.getSimpleName())
.namespace("org.apache.avro.reflect.TestReflect").fields().name("primitiveField").aliases("aliasName")
.type(Schema.create(org.apache.avro.Schema.Type.INT)).noDefault().endRecord();

expectedSchema.addProp("java-class", "org.apache.avro.reflect.TestReflect$ClassWithAliasOnField");
check(ClassWithAliasOnField.class, expectedSchema.toString());
}

Expand All @@ -1330,7 +1343,7 @@ public void testMultipleFieldAliases() {
field.addAlias("alias2");
Schema avroMultiMeta = Schema.createRecord("ClassWithMultipleAliasesOnField", null,
"org.apache.avro.reflect.TestReflect", false, Arrays.asList(field));

avroMultiMeta.addProp("java-class", "org.apache.avro.reflect.TestReflect$ClassWithMultipleAliasesOnField");
Schema schema = ReflectData.get().getSchema(ClassWithMultipleAliasesOnField.class);
assertEquals(avroMultiMeta, schema);
}
Expand All @@ -1344,7 +1357,8 @@ public void testOptional() {
check(OptionalTest.class,
"{\"type\":\"record\",\"name\":\"OptionalTest\","
+ "\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"foo\",\"type\":[\"null\",\"int\"],\"default\":null}]}");
+ "{\"name\":\"foo\",\"type\":[\"null\",\"int\"],\"default\":null}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$OptionalTest\"}");
}

private static class DefaultTest {
Expand All @@ -1357,7 +1371,8 @@ void avroDefault() {
check(DefaultTest.class,
"{\"type\":\"record\",\"name\":\"DefaultTest\","
+ "\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"foo\",\"type\":\"int\",\"default\":1}]}");
+ "{\"name\":\"foo\",\"type\":\"int\",\"default\":1}]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$DefaultTest\"}");
}

public static class NullableBytesTest {
Expand Down Expand Up @@ -1407,12 +1422,33 @@ private static class DocTest {
void avroDoc() {
check(DocTest.class,
"{\"type\":\"record\",\"name\":\"DocTest\",\"namespace\":\"org.apache.avro.reflect.TestReflect\","
+ "\"doc\":\"DocTest class docs\"," + "\"fields\":["
+ "{\"name\":\"defaultTest\",\"type\":{\"type\":\"record\",\"name\":\"DefaultTest\","
+ "\"fields\":[{\"name\":\"foo\",\"type\":\"int\",\"default\":1}]},\"doc\":\"And again\"},"
+ "{\"name\":\"enums\",\"type\":{\"type\":\"enum\",\"name\":\"DocTestEnum\","
+ "\"symbols\":[\"ENUM_1\",\"ENUM_2\"]},\"doc\":\"Some other Documentation\"},"
+ "{\"name\":\"foo\",\"type\":\"int\",\"doc\":\"Some Documentation\"}" + "]}");
+ "\"doc\":\"DocTest class docs\",\"fields\":[{\"name\":\"defaultTest\",\"type\":{\"type\":\"record\","
+ "\"name\":\"DefaultTest\",\"fields\":[{\"name\":\"foo\",\"type\":\"int\",\"default\":1}],"
+ "\"java-class\":\"org.apache.avro.reflect.TestReflect$DefaultTest\"},\"doc\":\"And again\"},"
+ "{\"name\":\"enums\",\"type\":{\"type\":\"enum\",\"name\":\"DocTestEnum\",\"symbols\":[\"ENUM_1\",\"ENUM_2\"],"
+ "\"java-class\":\"org.apache.avro.reflect.TestReflect$DocTestEnum\"},\"doc\":\"Some other Documentation\"},"
+ "{\"name\":\"foo\",\"type\":\"int\",\"doc\":\"Some Documentation\"}],\"java-class\":\"org.apache.avro.reflect.TestReflect$DocTest\"}");
}

@AvroNamespace("org.apache.avro.reflect.OverrideNamespace")
private static class NamespaceTest {

private static class InnerNamespaceTest {
}
}

@Test
void avroOverrideNamespaceTest() {
check(NamespaceTest.class,
"{\"type\":\"record\",\"name\":\"NamespaceTest\",\"namespace\":\"org.apache.avro.reflect.OverrideNamespace\",\"fields\":[]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$NamespaceTest\"}");
}

@Test
void avroOverrideEnclosingNamespaceTest() {
check(NamespaceTest.InnerNamespaceTest.class,
"{\"type\":\"record\",\"name\":\"InnerNamespaceTest\",\"namespace\":\"org.apache.avro.reflect.OverrideNamespace\",\"fields\":[]"
+ ",\"java-class\":\"org.apache.avro.reflect.TestReflect$NamespaceTest$InnerNamespaceTest\"}");
}

}
Loading
Loading