linkedin / linkedin/goavro

Incorrect calculation of canonical schema

Open
#267 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
1.1k
Forks
232
PR merge metrics
No merged PRs in 30d

Description

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.

A unit test is included below that can reproduce the issue. I also have a fix in a PR. I am not sure the fix is sound, however all tests pass.

The problem occurs in schemas with nested types with the same name, combined with the use of namespaces. Take the following schema:

```json
{
"name" : "thename",
"type" : "record",
"namespace" : "com.fooname",
"fields" : [
{
"name" : "bar" ,
"type" : {
"name" : "bar",
"type" : "record",
"fields" : [
{
"name" : "car",
"type" : "int"
}
]
}
}
]
}
```

The bug appears to occur when the inner `bar` is processed before the outer `bar` (but not vice-versa). In that case, the inner `bar` is properly namespaced to `com.fooname.bar`. However, an entry is made in `typeLookup` for the name `foo` to this namespaced value. The issue manifests when the outer `foo` is subsequently processed, as due to the name collision, it incorrectly also gets namespaced to `com.fooname.bar`. It should be `bar` in canonical form. If the outer bar is processed first, it works correctly, but this is random, due to random map iteration in golang.

The following test checks for the bug, by checking multiple runs of canonicalizations of the above schema generating different Rabin fingerprints. I have a PR that I will submit which adds a fix, but I am not sure the fix is correct in all cases.

```go
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
}
}
```

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with NewCodec and the canonical-schema processing that builds typeLookup, then run the TestCanonicalSchemaFingerprintFlake reproduction. Check how nested names and namespaces affect canonicalization across repeated constructions. Done means the same schema consistently produces the same Rabin fingerprint and the regression test passes without relying on random behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
data-engineering
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.