diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 9899a11..0e437ee 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -2,6 +2,7 @@ package irverify import ( "reflect" + "strconv" "strings" "unicode" @@ -52,12 +53,16 @@ var nameOptional = map[reflect.Type]bool{ // vacuously true of the empty string: an entirely empty Naming satisfied all // three while leaving an emitter nothing to name the entity by (GitHub #251). // -// Only Canonical is checked for content. Naming.Hint — the generated-name +// Only Canonical is checked for neutrality. Naming.Hint — the generated-name // channel — is held to none of the content rules, so casing // and punctuation still reach the IR through it. That is GitHub #54, left open // deliberately: closing it means changing how the compilers derive hints and // regenerating every golden, which is a different change from tightening this // checker. +// +// Naming.Aliases is held instead to the two rules that need no neutrality — +// non-empty and non-repeating — because an alias is a verbatim channel like +// Source rather than a neutral one like Canonical. See appendAliasViolations. func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { var vs []Violation optional := map[string]bool{} @@ -74,23 +79,74 @@ func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { if v.Type() != namingType { return true } - source, canon, hint := namingChannels(v) + source, canon, hint, aliases := namingChannels(v) if !optional[path] { vs = appendAbsentViolation(vs, source, canon, hint, path) } vs = appendNamingViolations(vs, source, canon, path) + vs = appendAliasViolations(vs, aliases, path) return false // Naming holds no references or nested Naming to descend into }) return vs, truncated } -// namingChannels reads the three name channels off one Naming. It reads fields +// namingChannels reads the four name channels off one Naming. It reads fields // rather than converting the value back to an ir.Naming because a value the walk -// reached through an unexported field cannot be (see ir.WalkValues). -func namingChannels(naming reflect.Value) (source, canon, hint string) { +// reached through an unexported field cannot be (see ir.WalkValues) — which is +// also why the aliases are copied out element by element rather than through +// Interface(). +func namingChannels(naming reflect.Value) (source, canon, hint string, aliases []string) { + list := naming.FieldByName("Aliases") + aliases = make([]string, list.Len()) + for i := range list.Len() { + aliases[i] = list.Index(i).String() + } return naming.FieldByName("Source").String(), naming.FieldByName("Canonical").String(), - naming.FieldByName("Hint").String() + naming.FieldByName("Hint").String(), + aliases +} + +// appendAliasViolations reports the ways an alias list can be one no producer +// meant to write. +// +// An alias is matched against a name some other schema wrote — an Avro alias is +// a full name such as "com.example.User" — so it is a verbatim channel like +// Source, not a neutral one like Canonical, and none of the neutrality rules +// above apply to it. Holding it to Canonical's grammar would be the lossy +// direction: neutralizing "com.example.User" to words discards the separators +// and the casing the match is made of, and invariant #2 forbids a lowering that +// throws that away. The IR is not deciding this spelling, it is recording one. +// +// What is left is decidable without a grammar. An empty alias matches nothing +// and a repeated one matches twice, so neither can be what a producer intended: +// both say a list was built wrong rather than that a name was spelled wrong. +// +// Paths name the offending entry the way the walk would have reached it, so a +// violation on a list of several says which one. +func appendAliasViolations(vs []Violation, aliases []string, path string) []Violation { + seen := make(map[string]bool, len(aliases)) + for i, alias := range aliases { + at := path + ".Aliases[" + strconv.Itoa(i) + "]" + if alias == "" { + vs = append(vs, Violation{ + Code: "ir/naming-alias-empty", + Message: "alias is empty, so it matches no name", + Path: at, + }) + continue + } + if seen[alias] { + vs = append(vs, Violation{ + Code: "ir/naming-alias-duplicate", + Message: "alias " + alias + " is listed more than once", + Path: at, + }) + continue + } + seen[alias] = true + } + return vs } // appendAbsentViolation reports an entity that no channel names. diff --git a/ir/irverify/naming_alias_test.go b/ir/irverify/naming_alias_test.go new file mode 100644 index 0000000..67f5d4e --- /dev/null +++ b/ir/irverify/naming_alias_test.go @@ -0,0 +1,93 @@ +package irverify_test + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irverify" +) + +// aliasViolations returns the alias violations in a model named with aliases, +// filtered by code prefix so a test asserting none is not satisfied by some +// unrelated violation being absent. +func aliasViolations(t *testing.T, aliases ...string) []irverify.Violation { + t.Helper() + doc := modelNamed(ir.Naming{Source: "m", Canonical: "m", Aliases: aliases}) + var out []irverify.Violation + for _, v := range irverify.Verify(doc) { + if strings.HasPrefix(v.Code, "ir/naming-alias-") { + out = append(out, v) + } + } + return out +} + +// TestVerify_VerbatimAliasesAreClean pins the settlement this check rests on: an +// alias is matched against a name another schema wrote, so it is a verbatim +// channel like Source and none of Canonical's neutrality rules apply to it. +// "com.example.User" is what an Avro alias looks like, and every one of +// ir/naming-cased, ir/naming-not-words and ir/naming-unsegmented would fire on +// it if the alias were held to Canonical's grammar. +func TestVerify_VerbatimAliasesAreClean(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t, "com.example.User", "UserID", "user_id")) +} + +func TestVerify_EmptyAliasIsAViolation(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "ok", "") + require.Len(t, got, 1, "one violation for the one empty entry") + assert.Equal(t, "ir/naming-alias-empty", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[1]", got[0].Path, + "the violation names the offending entry, not just the naming") +} + +func TestVerify_DuplicateAliasIsAViolation(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "dup", "other", "dup") + require.Len(t, got, 1, "the repeat is reported, not the first occurrence") + assert.Equal(t, "ir/naming-alias-duplicate", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[2]", got[0].Path) + assert.Contains(t, got[0].Message, "dup") +} + +// TestVerify_RepeatedEmptyAliasReportsEachAsEmpty holds the interaction between +// the two rules: a second empty entry is a repeat as well as an empty one, and +// reporting it as a duplicate would name the wrong repair. +func TestVerify_RepeatedEmptyAliasReportsEachAsEmpty(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "", "") + require.Len(t, got, 2) + for _, v := range got { + assert.Equal(t, "ir/naming-alias-empty", v.Code) + } +} + +// TestVerify_IssueReproducerIsReported drives the exact value from the issue — +// cased, punctuated, empty and duplicated together — and states which of the +// four the IR objects to and which it accepts by design. +func TestVerify_IssueReproducerIsReported(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "UserID", "com.example.User", "", "dup", "dup") + require.Len(t, got, 2, "the cased and dotted entries are legitimate aliases") + + // Keyed rather than indexed: Verify sorts by (Code, Path), so asserting + // positionally would pin the sort order rather than what was reported. + byCode := map[string]string{} + for _, v := range got { + byCode[v.Code] = v.Path + } + assert.Equal(t, map[string]string{ + "ir/naming-alias-empty": "doc.Types[t/x/M].Name.Aliases[2]", + "ir/naming-alias-duplicate": "doc.Types[t/x/M].Name.Aliases[4]", + }, byCode) +} + +func TestVerify_NoAliasesIsClean(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t)) +} diff --git a/ir/naming.go b/ir/naming.go index 94a9fea..328eb06 100644 --- a/ir/naming.go +++ b/ir/naming.go @@ -28,6 +28,16 @@ type Naming struct { // Aliases are alternate names for schema-resolution matching (Avro // aliases). Versionless — rename history tied to version labels lives in // Availability.RenamedFrom. + // + // An alias is a verbatim channel like Source, not a neutral one like + // Canonical: it is matched against a name another schema wrote, so the + // casing and punctuation are the value. An Avro alias is a full name + // ("com.example.User"), and neutralizing it to words would lose the + // separators and the case the match depends on. So no neutrality rule + // applies to an entry, and irverify holds only what is decidable without + // one: every entry is non-empty and no entry repeats, since an empty alias + // matches nothing and a repeated one matches twice. Both mean a producer + // wrote a list it did not mean to write. Aliases []string `json:"aliases,omitempty"` }