AVRO-2122: Cannot validate schemas with recursive definitions

Track which symbols have been visited to avoid StackOverflowErrors when validating schemas with recursive definitions This closes apache#281 Signed-off-by: Nandor Kollar <[email protected]>
mkw · Jan 23, 2018 · 7f9cbca · 7f9cbca
1 parent 37bd84d
commit 7f9cbca
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 6 deletions.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -226,6 +226,9 @@ Trunk (not yet released)
 
     AVRO-2120: Fix NullPointerException thrown by Schema.Parser#parse("")
 
+    AVRO-2122: Cannot validate schemas with recursive definitions
+    (Bart via Nandor Kollar)
+
 Avro 1.8.1 (14 May 2016)
 
   INCOMPATIBLE CHANGES

diff --git a/lang/java/avro/src/main/java/org/apache/avro/io/parsing/Symbol.java b/lang/java/avro/src/main/java/org/apache/avro/io/parsing/Symbol.java
@@ -19,10 +19,12 @@
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
+import java.util.Set;
 
 import org.apache.avro.Schema;
 
@@ -369,9 +371,19 @@ public Repeater flatten(Map<Sequence, Sequence> map,
    * for some inputs.
    */
   public static boolean hasErrors(Symbol symbol) {
+    return hasErrors(symbol, new HashSet<Symbol>());
+  }
+
+  private static boolean hasErrors(Symbol symbol, Set<Symbol> visited) {
+    // avoid infinite recursion
+    if (visited.contains(symbol)) {
+      return false;
+    }
+    visited.add(symbol);
+
     switch(symbol.kind) {
     case ALTERNATIVE:
-      return hasErrors(symbol, ((Alternative) symbol).symbols);
+      return hasErrors(symbol, ((Alternative) symbol).symbols, visited);
     case EXPLICIT_ACTION:
       return false;
     case IMPLICIT_ACTION:
@@ -380,30 +392,30 @@ public static boolean hasErrors(Symbol symbol) {
       }
 
       if (symbol instanceof UnionAdjustAction) {
-        return hasErrors(((UnionAdjustAction) symbol).symToParse);
+        return hasErrors(((UnionAdjustAction) symbol).symToParse, visited);
       }
 
       return false;
     case REPEATER:
       Repeater r = (Repeater) symbol;
-      return hasErrors(r.end) || hasErrors(symbol, r.production);
+      return hasErrors(r.end, visited) || hasErrors(symbol, r.production, visited);
     case ROOT:
     case SEQUENCE:
-      return hasErrors(symbol, symbol.production);
+      return hasErrors(symbol, symbol.production, visited);
     case TERMINAL:
       return false;
     default:
       throw new RuntimeException("unknown symbol kind: " + symbol.kind);
     }
   }
 
-  private static boolean hasErrors(Symbol root, Symbol[] symbols) {
+  private static boolean hasErrors(Symbol root, Symbol[] symbols, Set<Symbol> visited) {
     if(null != symbols) {
       for(Symbol s: symbols) {
         if (s == root) {
           continue;
         }
-        if (hasErrors(s)) {
+        if (hasErrors(s, visited)) {
           return true;
         }
       }

diff --git a/lang/java/avro/src/test/java/org/apache/avro/TestSchemaValidation.java b/lang/java/avro/src/test/java/org/apache/avro/TestSchemaValidation.java
@@ -407,4 +407,16 @@ private void testValidatorFails(SchemaValidator validator,
     }
     Assert.assertTrue(threw);
   }
+  public static final org.apache.avro.Schema recursiveSchema = new org.apache.avro.Schema.Parser().parse("{\"type\":\"record\",\"name\":\"Node\",\"namespace\":\"avro\",\"fields\":[{\"name\":\"value\",\"type\":[\"null\",\"Node\"],\"default\":null}]}");
+
+  /**
+   * Unit test to verify that recursive schemas can be validated.
+   * See AVRO-2122.
+   */
+  @Test
+  public void testRecursiveSchemaValidation() throws SchemaValidationException {
+    // before AVRO-2122, this would cause a StackOverflowError
+    final SchemaValidator backwardValidator = builder.canReadStrategy().validateLatest();
+    backwardValidator.validate(recursiveSchema, Arrays.asList(recursiveSchema));
+  }
 }