From 88f15ff3c02c86eceec20b943f728d2a117591a1 Mon Sep 17 00:00:00 2001
From: Tim Ebringer <tim.ebringer@gmail.com>
Date: Fri, 14 Apr 2023 11:15:42 -0600
Subject: [PATCH] Fix incorrect calculation of canonical schema

For some valid schemas, goavro will incorrectly compute the
canonical form. This results in an incorrect Rabin fingerprint.
Due to the random nature of map iteration in golang, the
fingerprint for the exact same schema can change between
consecutive runs, which should not happen.

The error occurs with namespaces, with two nested names that
are the same. The outer name can get incorrectly namespaced
if it is processed last, due to the name collision in the
typeLookup map.

The fix is to remove the colliding name if the type is not
one of the types that is supposed to be namespaced.
---
 canonical.go      | 16 ++++++++++++++++
 canonical_test.go | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/canonical.go b/canonical.go
index ec608c0..05cb9a4 100644
--- a/canonical.go
+++ b/canonical.go
@@ -120,6 +120,22 @@ func pcfObject(jsonMap map[string]interface{}, parentNamespace string, typeLooku
 			}
 		}
 
+		// This fixes a fairly subtle bug that can occur during schema canonicalization,
+		// which can randomly namespace a name that should not be, depending on the
+		// order of map traversal.
+		if theType, ok := jsonMap["type"]; ok {
+			// Check it's not a type that should be namespaced in canonical form, i.e.
+			// this should be map[string]interface{} and not "record".
+			if _, ook := theType.(string); !ook {
+				if valStr, oook := v.(string); oook {
+					// if we're an interface{} type which has an entry in typeLookup,
+					// remove it before we recurse into parsingCanonicalForm, as
+					// the wrong namespace will be applied.
+					delete(typeLookup, valStr)
+				}
+			}
+		}
+
 		pk, err := parsingCanonicalForm(k, parentNamespace, typeLookup)
 		if err != nil {
 			return "", err
diff --git a/canonical_test.go b/canonical_test.go
index 811e0eb..9fa90d7 100644
--- a/canonical_test.go
+++ b/canonical_test.go
@@ -285,3 +285,44 @@ func TestCanonicalSchema(t *testing.T) {
 		}
 	}
 }
+
+func TestCanonicalSchemaFingerprintFlake(t *testing.T) {
+	const schema = `{
+  "name" : "thename",
+  "type" : "record",
+  "namespace" : "com.fooname",
+  "fields" : [
+    {
+      "name" : "bar" ,
+      "type" : {
+        "name" : "bar",
+        "type" : "record",
+        "fields" : [
+          {
+            "name" : "car",
+            "type" : "int"
+          }
+        ]
+      }
+    }
+  ]
+}
+`
+	codec, err := NewCodec(schema)
+	if err != nil {
+		t.Fatalf("unexpected schema parse failure, err=%v", err)
+	}
+	prevRun := codec.Rabin
+
+	// This test is flaky. It exposes a bug that manifests due to
+	// randomized map iteration. However, 32 iterations should give
+	// high probability to see the failure.
+	for i := 0; i < 32; i++ {
+		codec, err = NewCodec(schema)
+		currentRun := codec.Rabin
+		if prevRun != currentRun {
+			t.Fatalf("same schema should always have same fingerprint, rabin1: %d, rabin2: %d", prevRun, currentRun)
+		}
+		prevRun = currentRun
+	}
+}