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

Fix incorrect calculation of canonical schema #268

Open
wants to merge 1 commit into
base: master
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions canonical.go
Expand Up @@ -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
Expand Down
41 changes: 41 additions & 0 deletions canonical_test.go
Expand Up @@ -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
}
}