[ 
https://issues.apache.org/jira/browse/AVRO-2143?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16372274#comment-16372274
 ] 

ASF GitHub Bot commented on AVRO-2143:
--------------------------------------

cutting closed pull request #283: AVRO-2143. Fix Java reflect to stop using 
dollar sign in namespaces of nested classes.
URL: https://github.com/apache/avro/pull/283
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java 
b/lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java
index 2eea133dd..1c179007a 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java
@@ -575,7 +575,7 @@ protected Schema createSchema(Type type, Map<String,Schema> 
names) {
         String name = c.getSimpleName();
         String space = c.getPackage() == null ? "" : c.getPackage().getName();
         if (c.getEnclosingClass() != null)                   // nested class
-          space = c.getEnclosingClass().getName() + "$";
+          space = c.getEnclosingClass().getName();
         Union union = c.getAnnotation(Union.class);
         if (union != null) {                                 // union annotated
           return getAnnotatedUnion(union, names);
diff --git 
a/lang/java/avro/src/main/java/org/apache/avro/specific/SpecificData.java 
b/lang/java/avro/src/main/java/org/apache/avro/specific/SpecificData.java
index f74640d80..44de5c434 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/specific/SpecificData.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/specific/SpecificData.java
@@ -161,7 +161,11 @@ public Class getClass(Schema schema) {
         try {
           c = ClassUtils.forName(getClassLoader(), getClassName(schema));
         } catch (ClassNotFoundException e) {
-          c = NO_CLASS;
+          try {                                   // nested class?
+            c = ClassUtils.forName(getClassLoader(), 
getNestedClassName(schema));
+          } catch (ClassNotFoundException ex) {
+            c = NO_CLASS;
+          }
         }
         classCache.put(name, c);
       }
@@ -205,10 +209,18 @@ public static String getClassName(Schema schema) {
     String name = schema.getName();
     if (namespace == null || "".equals(namespace))
       return name;
-    String dot = namespace.endsWith("$") ? "" : ".";
+    String dot = namespace.endsWith("$") ? "" : "."; // back-compatibly handle 
$
     return namespace + dot + name;
   }
 
+  private String getNestedClassName(Schema schema) {
+    String namespace = schema.getNamespace();
+    String name = schema.getName();
+    if (namespace == null || "".equals(namespace))
+      return name;
+    return namespace + "$" + name;
+  }
+
   private final LoadingCache<java.lang.reflect.Type,Schema> schemaCache =
       CacheBuilder.newBuilder()
           .weakKeys()
diff --git 
a/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflect.java 
b/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflect.java
index 7165c0768..21244fbd2 100644
--- a/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflect.java
+++ b/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflect.java
@@ -134,7 +134,7 @@
   @Test public void testUnionWithEnum() {
     Schema s = new Schema.Parser().parse
         ("[\"null\", {\"type\":\"enum\",\"name\":\"E\",\"namespace\":" +
-            
"\"org.apache.avro.reflect.TestReflect$\",\"symbols\":[\"A\",\"B\"]}]");
+            
"\"org.apache.avro.reflect.TestReflect\",\"symbols\":[\"A\",\"B\"]}]");
     GenericData data = ReflectData.get();
     assertEquals(1, data.resolveUnion(s, E.A));
   }
@@ -525,13 +525,13 @@ void checkReadWrite(Object object, Schema s) throws 
Exception {
   public static enum E { A, B };
   @Test public void testEnum() throws Exception {
     check(E.class, "{\"type\":\"enum\",\"name\":\"E\",\"namespace\":"
-          
+"\"org.apache.avro.reflect.TestReflect$\",\"symbols\":[\"A\",\"B\"]}");
+          
+"\"org.apache.avro.reflect.TestReflect\",\"symbols\":[\"A\",\"B\"]}");
   }
 
   public static class R { int a; long b; }
   @Test public void testRecord() throws Exception {
     check(R.class, "{\"type\":\"record\",\"name\":\"R\",\"namespace\":"
-          +"\"org.apache.avro.reflect.TestReflect$\",\"fields\":["
+          +"\"org.apache.avro.reflect.TestReflect\",\"fields\":["
           +"{\"name\":\"a\",\"type\":\"int\"},"
           +"{\"name\":\"b\",\"type\":\"long\"}]}");
   }
@@ -539,20 +539,20 @@ void checkReadWrite(Object object, Schema s) throws 
Exception {
   public static class RAvroIgnore { @AvroIgnore int a; }
   @Test public void testAnnotationAvroIgnore() throws Exception {
     check(RAvroIgnore.class, 
"{\"type\":\"record\",\"name\":\"RAvroIgnore\",\"namespace\":"
-          +"\"org.apache.avro.reflect.TestReflect$\",\"fields\":[]}");
+          +"\"org.apache.avro.reflect.TestReflect\",\"fields\":[]}");
   }
 
   public static class RAvroMeta { @AvroMeta(key="K", value="V") int a; }
   @Test public void testAnnotationAvroMeta() throws Exception {
     check(RAvroMeta.class, 
"{\"type\":\"record\",\"name\":\"RAvroMeta\",\"namespace\":"
-          +"\"org.apache.avro.reflect.TestReflect$\",\"fields\":["
+          +"\"org.apache.avro.reflect.TestReflect\",\"fields\":["
           +"{\"name\":\"a\",\"type\":\"int\",\"K\":\"V\"}]}");
   }
 
   public static class RAvroName { @AvroName("b") int a; }
   @Test public void testAnnotationAvroName() throws Exception {
     check(RAvroName.class, 
"{\"type\":\"record\",\"name\":\"RAvroName\",\"namespace\":"
-          +"\"org.apache.avro.reflect.TestReflect$\",\"fields\":["
+          +"\"org.apache.avro.reflect.TestReflect\",\"fields\":["
           +"{\"name\":\"b\",\"type\":\"int\"}]}");
   }
 
@@ -560,7 +560,7 @@ void checkReadWrite(Object object, Schema s) throws 
Exception {
   @Test(expected=Exception.class)
   public void testAnnotationAvroNameCollide() throws Exception {
     check(RAvroNameCollide.class, 
"{\"type\":\"record\",\"name\":\"RAvroNameCollide\",\"namespace\":"
-          +"\"org.apache.avro.reflect.TestReflect$\",\"fields\":["
+          +"\"org.apache.avro.reflect.TestReflect\",\"fields\":["
           +"{\"name\":\"b\",\"type\":\"int\"},"
           +"{\"name\":\"b\",\"type\":\"int\"}]}");
   }
@@ -568,7 +568,7 @@ public void testAnnotationAvroNameCollide() throws 
Exception {
   public static class RAvroStringableField { @Stringable int a; }
   public void testAnnotationAvroStringableFields() throws Exception {
     check(RAvroStringableField.class, 
"{\"type\":\"record\",\"name\":\"RAvroNameCollide\",\"namespace\":"
-          +"\"org.apache.avro.reflect.TestReflect$\",\"fields\":["
+          +"\"org.apache.avro.reflect.TestReflect\",\"fields\":["
           +"{\"name\":\"a\",\"type\":\"String\"}]}");
   }
 
@@ -703,7 +703,7 @@ public void testMultipleAnnotations() throws IOException {
   public void testAvroEncodeInducing() throws IOException {
     Schema schm = ReflectData.get().getSchema(AvroEncRecord.class);
     assertEquals(schm.toString(), 
"{\"type\":\"record\",\"name\":\"AvroEncRecord\",\"namespace" +
-      
"\":\"org.apache.avro.reflect.TestReflect$\",\"fields\":[{\"name\":\"date\"," +
+      
"\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[{\"name\":\"date\"," +
       
"\"type\":{\"type\":\"long\",\"CustomEncoding\":\"DateAsLongEncoding\"}}]}");
   }
 
@@ -1059,9 +1059,19 @@ public void testReflectFieldError() throws Exception {
 
   @Test
   public void testAvroAliasOnClass() {
-    check(AliasA.class, 
"{\"type\":\"record\",\"name\":\"AliasA\",\"namespace\":\"org.apache.avro.reflect.TestReflect$\",\"fields\":[],\"aliases\":[\"b.a\"]}");
-    check(AliasB.class, 
"{\"type\":\"record\",\"name\":\"AliasB\",\"namespace\":\"org.apache.avro.reflect.TestReflect$\",\"fields\":[],\"aliases\":[\"a\"]}");
-    check(AliasC.class, 
"{\"type\":\"record\",\"name\":\"AliasC\",\"namespace\":\"org.apache.avro.reflect.TestReflect$\",\"fields\":[],\"aliases\":[\"a\"]}");
+    check(AliasA.class, 
"{\"type\":\"record\",\"name\":\"AliasA\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"aliases\":[\"b.a\"]}");
+    check(AliasB.class, 
"{\"type\":\"record\",\"name\":\"AliasB\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"aliases\":[\"a\"]}");
+    check(AliasC.class, 
"{\"type\":\"record\",\"name\":\"AliasC\",\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":[],\"aliases\":[\"a\"]}");
+  }
+
+  private static class Z {}
+
+  @Test public void testDollarTerminatedNamespaceCompatibility() {
+    ReflectData data = ReflectData.get();
+    Schema s = new Schema.Parser().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\":[]}");
   }
 
   private static class ClassWithAliasOnField {
@@ -1078,7 +1088,7 @@ public void testAvroAliasOnClass() {
   public void testAvroAliasOnField() {
 
     Schema expectedSchema = 
SchemaBuilder.record(ClassWithAliasOnField.class.getSimpleName())
-        
.namespace("org.apache.avro.reflect.TestReflect$").fields().name("primitiveField").aliases("aliasName")
+        
.namespace("org.apache.avro.reflect.TestReflect").fields().name("primitiveField").aliases("aliasName")
         
.type(Schema.create(org.apache.avro.Schema.Type.INT)).noDefault().endRecord();
 
     check(ClassWithAliasOnField.class, expectedSchema.toString());
@@ -1098,7 +1108,7 @@ public void 
namespaceDefinitionOnFieldAliasMustThrowException() {
   public void testAvroDefault() {
     check(DefaultTest.class,
           "{\"type\":\"record\",\"name\":\"DefaultTest\","
-          
+"\"namespace\":\"org.apache.avro.reflect.TestReflect$\",\"fields\":["
+          +"\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":["
           +"{\"name\":\"foo\",\"type\":\"int\",\"default\":1}]}");
   }
 
diff --git 
a/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflectLogicalTypes.java
 
b/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflectLogicalTypes.java
index 4943c531b..23348be2d 100644
--- 
a/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflectLogicalTypes.java
+++ 
b/lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflectLogicalTypes.java
@@ -120,7 +120,7 @@ public int hashCode() {
   public void testDecimalBytes() throws IOException {
     Schema schema = REFLECT.getSchema(DecimalRecordBytes.class);
     Assert.assertEquals("Should have the correct record name",
-        "org.apache.avro.reflect.TestReflectLogicalTypes$",
+        "org.apache.avro.reflect.TestReflectLogicalTypes",
         schema.getNamespace());
     Assert.assertEquals("Should have the correct record name",
         "DecimalRecordBytes",
@@ -179,7 +179,7 @@ public int hashCode() {
   public void testDecimalFixed() throws IOException {
     Schema schema = REFLECT.getSchema(DecimalRecordFixed.class);
     Assert.assertEquals("Should have the correct record name",
-        "org.apache.avro.reflect.TestReflectLogicalTypes$",
+        "org.apache.avro.reflect.TestReflectLogicalTypes",
         schema.getNamespace());
     Assert.assertEquals("Should have the correct record name",
         "DecimalRecordFixed",
@@ -298,7 +298,7 @@ public LogicalType fromSchema(Schema schema) {
 
     Schema schema = model.getSchema(PairRecord.class);
     Assert.assertEquals("Should have the correct record name",
-        "org.apache.avro.reflect.TestReflectLogicalTypes$",
+        "org.apache.avro.reflect.TestReflectLogicalTypes",
         schema.getNamespace());
     Assert.assertEquals("Should have the correct record name",
         "PairRecord",


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[email protected]


> ReflectData should determine inner class names in a different way.
> ------------------------------------------------------------------
>
>                 Key: AVRO-2143
>                 URL: https://issues.apache.org/jira/browse/AVRO-2143
>             Project: Avro
>          Issue Type: Improvement
>    Affects Versions: 1.7.7
>            Reporter: Miguel
>            Assignee: Doug Cutting
>            Priority: Major
>             Fix For: 1.9.0
>
>
> I used ReflectData to generate schemas containing inner classes. These 
> classes were named automatically by ReflectData with dollar ('$'), with, for 
> example com.company.avro.House$Type. Nevertheless @AvroAlias does not support 
> aliases containing '$' in names. In particulr, it fails in validateName in 
> Schema core class. The failure is correct given that AVRO name specs does 
> notinclude the '$' symbol. 
> I suppose that ReflectData should, for example, generate inner class names in 
> a different way, such as replacing '$' by dots ('.'), or it should fail.
> And a detail: Although I cannot add aliases with '$', I can still encode and 
> decode such objects and inner classes using that schema. 
> I think that it newer versions it is also happening.
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to