Incorrect calculation of canonical schema
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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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