feat(irverify): hold Naming.Aliases to a rule

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.

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))
+}

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