diff --git a/docs/ir-design.md b/docs/ir-design.md index 011e565..e7354e5 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -232,10 +232,44 @@ category alone would lose the second. `irverify` holds every `Naming` to this: the shape rules run over `Canonical` and `Hint` alike, and it recomputes the canonical from the `Source` beside it, so a boundary in the wrong place is a -compiler bug rather than a variant reading. What no check can say -is that the grammar itself is right — a check that recomputes moves with what it recomputes through -— so the answers are pinned by a conformance table and the properties every answer must satisfy by -a fuzz target beside it (GitHub #186). +compiler bug rather than a variant reading. What no check can say is that the grammar itself is +right — a check that recomputes moves with what it recomputes through — so the answers are pinned +by a conformance table and the properties every answer must satisfy by a fuzz target beside it +(GitHub #186). + +One rule sits under all of those and under `Aliases` too, because it is about the encoding rather +than the spelling: **every channel's bytes must decode** (`ir/naming-invalid-utf8`). Ill-formed +UTF-8 survives a marshal as the replacement rune, so a document carrying it decodes to one that +re-marshals to different bytes and the "Serializable" invariant above stops holding — broken by a +name nothing else here objects to. + +**`Aliases` is held to none of the shape rules, and to rules of its own instead.** An alias is +matched against a name *another* schema wrote — an Avro alias is a full name, `com.example.User`, +and resolution compares it verbatim against the writer schema's full name — so the separators and +the casing are what the match is made of rather than a spelling the IR gets to decide. +Neutralizing one would throw away precisely that, which is the lossy direction +lossless-by-default rules out. `Source` is the internal precedent: it carries `UserID` today and +no content rule touches it, because it records what the spec said rather than deciding a spelling. +What is left is decidable without any grammar, and `irverify` holds an alias to exactly that much: + +- **Every entry names something** (`ir/naming-alias-blank`). An entry whose every rune is one + Unicode classifies as invisible — a space, a control, a format character, or a default-ignorable + one — matches nothing under any grammar. `""`, `" "`, `"\u200b"` and `"\u3164"` are alike here. + An invisible rune sitting *beside* a visible one is a different question and is not asked: + whether `com.example.User` is a legal name is decidable only under the grammar of the + format it will be matched against, which the IR does not know. +- **Every entry decodes** — the shared byte rule above, which reaches an alias the same way it + reaches the other three channels. +- **No entry repeats another, or the entity's own `Source`** (`ir/naming-alias-duplicate`, + `ir/naming-alias-redundant`), reported at the later entry and naming the earlier, so the message + says which to delete and which to keep. Either admits no name that was not already admitted, so a + producer that wrote one built the list wrong. Only `Source` is compared against: `Canonical` and + `Hint` are names the IR derived for an emitter to render, never names a writer schema could have + spelled. + +That such an entry is inert is also why a *source* declaring one is recorded once with a diagnostic +rather than carried through: dropping it is not the lossy direction, since the same set of names +resolves to the entity either way. ### 3.3 Type references diff --git a/ir/irverify/duplicates.go b/ir/irverify/duplicates.go index 0e40f6d..8ed228f 100644 --- a/ir/irverify/duplicates.go +++ b/ir/irverify/duplicates.go @@ -112,7 +112,7 @@ func propertyFingerprints(doc *ir.Document) (map[string]string, bool) { // checkDuplicateIDs). It reads fields off the walked value rather than // converting it back to an ir.Property, because a value the walk reached through // an unexported field cannot be converted (see ir.WalkValues); checkNaming's -// namingChannels reads its three channels the same way. +// namingChannels reads its channels the same way. // // The parts are joined on NUL, which no source name, wire name or ID contains, // so no two properties can agree on the rendering while disagreeing on the diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 264652f..5966d6d 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -2,8 +2,10 @@ package irverify import ( "reflect" + "strconv" "strings" "unicode" + "unicode/utf8" "github.com/dexpace/morphic/ir" ) @@ -41,12 +43,14 @@ var nameOptional = map[reflect.Type]bool{ reflect.TypeFor[ir.Primitive](): true, } -// checkNaming asserts every named entity has a name at all, and that the names -// it carries are what invariant #4 promises: neutral lower_snake word -// sequences, carrying no casing an emitter should own and no character that is -// not part of a word. It reuses the shared bounded walk to reach every ir.Naming -// value in the document, and reports whether that walk was cut short so a name -// past the cap cannot go unchecked in silence. +// checkNaming asserts every named entity has a name at all; that the names it +// carries are what invariant #4 promises — neutral lower_snake word sequences, +// carrying no casing an emitter should own and no character that is not part of +// a word; and that every channel's bytes decode, which is a claim about the +// encoding rather than the spelling and so is the one rule they all share. It +// reuses the shared bounded walk to reach every ir.Naming value in the document, +// and reports whether that walk was cut short so a name past the cap cannot go +// unchecked in silence. // // Presence is separate from those content rules because each of them is // vacuously true of the empty string: an entirely empty Naming satisfied all @@ -62,7 +66,10 @@ var nameOptional = map[reflect.Type]bool{ // Only the grammar rule stays canonical-only, because a hint has no source // spelling beside it to be recomputed from. // -// Naming.Aliases is still read by no rule here (GitHub #317). +// Naming.Aliases is held to none of those and to rules of its own instead, +// because it is a verbatim channel rather than a name the IR decides — see +// appendAliasViolations, and ir.Naming.Aliases for why. The one rule every +// channel shares, aliases included, is appendUTF8Violation's. func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { var vs []Violation optional := map[string]bool{} @@ -79,23 +86,103 @@ 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, hint, path) + vs = appendAliasViolations(vs, source, 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 every name channel 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(). +// +// Nothing guards the field lookups. A rename of any of these fields is a +// compile-clean change that reddens the naming tests on the next run either way: +// the three String() reads degrade to "", and Len() on the +// invalid Value panics. Neither is reachable from a document — checkNaming only +// calls this for a value whose type is ir.Naming, so every field is present — +// and a guard for the unreachable one would be a statement no test can cover. +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 holds one alias list to the rules ir.Naming.Aliases +// states. That comment is where the argument for them lives, and for why none of +// the neutrality rules above apply. +// +// Each is decidable from the list and the Naming carrying it, with no grammar +// and no second node: whether an entry has anything visible in it +// (isBlankName), whether its bytes decode at all, and whether it admits a name +// some earlier entry — or the entity's own Source — already did. A repeat is +// reported at its later occurrence, naming the earlier one, so the message says +// which to delete and which to keep. A blank repeat is reported blank: the +// repair is to fill it in or drop it, not to distinguish it from the other +// blank. +// +// Only Source is compared against. Canonical and Hint are names the IR derived +// for an emitter to render, never names a writer schema could have spelled, so +// an alias equal to one of those is not the redundancy this rule is about. +// +// Two neighbouring defects are deliberately left out of scope here. Repeats +// across two Namings — the ambiguity that actually changes what a reader +// resolves — need the whole document rather than one list, and land in +// checkDuplicateIDs' shape (GitHub #398); TestVerify_AliasSharedByTwoNamings +// pins that they go unreported today, so implementing that rule cannot move the +// boundary in silence. And every other []string in the IR (Namespace, Tags, +// Scopes, ContentTypes …) admits the same blank and repeated entries this rule +// rejects, held by nothing (GitHub #399). +func appendAliasViolations(vs []Violation, source string, aliases []string, path string) []Violation { + seen := make(map[string]int, len(aliases)) + for i, alias := range aliases { + switch first, repeated := seen[alias]; { + case isBlankName(alias): + vs = append(vs, Violation{ + Code: "ir/naming-alias-blank", + Message: "alias is blank, so it matches no name", + Path: aliasPath(path, i), + }) + case !utf8.ValidString(alias): + vs = append(vs, utf8Violation("alias", aliasPath(path, i))) + case repeated: + vs = append(vs, Violation{ + Code: "ir/naming-alias-duplicate", + Message: "alias " + alias + " is listed here and at index " + strconv.Itoa(first), + Path: aliasPath(path, i), + }) + case alias == source: + vs = append(vs, Violation{ + Code: "ir/naming-alias-redundant", + Message: "alias " + alias + " is the entity's own source name, so it matches nothing more", + Path: aliasPath(path, i), + }) + default: + seen[alias] = i + } + } + return vs +} + +// aliasPath spells one alias entry the way ir.WalkValues would have reached it. +// checkNaming prunes at ir.Naming, so the walk never renders these itself — +// TestVerify_AliasPathIsSpelledAsTheWalkWould is what holds the two spellings +// together. +func aliasPath(path string, i int) string { + return path + ".Aliases[" + strconv.Itoa(i) + "]" } // appendAbsentViolation reports an entity that no channel names. @@ -117,13 +204,53 @@ func appendAbsentViolation(vs []Violation, source, canon, hint, path string) []V // appendNamingViolations reports the ways one Naming can break neutrality: the // grammar rule over the canonical, and the content rules over each channel that -// carries a name for an emitter to render. +// carries a name for an emitter to render — plus the byte rule below, which +// every channel is held to because none of them can be read back otherwise. func appendNamingViolations(vs []Violation, source, canon, hint, path string) []Violation { + vs = appendUTF8Violation(vs, "source name", source, path) + vs = appendUTF8Violation(vs, "canonical name", canon, path) + vs = appendUTF8Violation(vs, "name hint", hint, path) vs = appendGrammarViolation(vs, source, canon, path) vs = appendContentViolations(vs, "canonical name", canon, path) return appendContentViolations(vs, "name hint", hint, path) } +// utf8Violation is the report for a name channel carrying bytes no decoder +// reads back as what was written. It is the one rule every channel shares, +// aliases included, because it is about the encoding rather than the spelling: +// an ill-formed sequence survives a marshal as the replacement rune, so the +// document decodes to something that re-marshals to different bytes and +// invariant #7 is broken by a name nothing else here objects to. +// +// checkDiagnostics makes the same claim over the only other free-form spec text +// the IR carries (ir/diagnostic-invalid-utf8), and like it this message quotes +// nothing: repeating the bytes would put them in the report too. That is also +// why the value is not a parameter — there is nothing here to say about it +// beyond which channel it arrived in. +// +// Canonical and Hint are only incidentally covered without this rule — the +// replacement rune is not a word character, so isWordSequence rejects it — and +// incidentally is not covered: the violation would name the wrong repair, since +// splitting on non-word characters is not what fixes undecodable bytes. +func utf8Violation(channel, path string) Violation { + return Violation{ + Code: "ir/naming-invalid-utf8", + Message: channel + " is not valid UTF-8", + Path: path, + } +} + +// appendUTF8Violation reports channel's name when its bytes are ill-formed. The +// alias rule decides the same thing in its own switch and appends +// utf8Violation directly, since a switch branch needs the test separate from +// the report. +func appendUTF8Violation(vs []Violation, channel, name, path string) []Violation { + if utf8.ValidString(name) { + return vs + } + return append(vs, utf8Violation(channel, path)) +} + // appendContentViolations reports the ways the name in one channel can break // neutrality. channel is how the message spells which channel was wrong: the // two share a Path and a defect class, so the message is what tells them @@ -249,6 +376,38 @@ func isWordSequence(s string) bool { return true } +// isBlankName reports whether s holds no rune a name could be made of. Every +// rune it accepts as invisible is one Unicode itself classifies that way — a +// space, a control, a format character, or a default-ignorable one — so the +// judgement needs no format's grammar and this function decides nothing on its +// own account. +// +// strings.TrimSpace is not that test, and neither is IsSpace-plus-Cf. IsSpace +// reports false for the zero-width joiners, the soft hyphen and the BOM (all +// Cf), and all three predicates report false for U+3164 HANGUL FILLER and its +// two jamo siblings, which are default-ignorable and are the characters +// conventionally used to pass off a name as empty. An alias of nothing but any +// of these names exactly as little as " " does. +// +// It does not reach every rune that renders as whitespace: U+2800 BRAILLE +// PATTERN BLANK is a graphic character Unicode does not call invisible, so it is +// left alone rather than judged here — that is the boundary this test declines +// to cross without knowing the grammar the name is read under. +func isBlankName(s string) bool { + for _, r := range s { + if !isInvisible(r) { + return false + } + } + return true +} + +// isInvisible reports whether Unicode classifies r as carrying no visible mark. +func isInvisible(r rune) bool { + return unicode.IsSpace(r) || unicode.IsControl(r) || unicode.Is(unicode.Cf, r) || + unicode.Is(unicode.Other_Default_Ignorable_Code_Point, r) +} + // isCased reports whether s still carries casing an emitter should own. The test // is lowercase-idempotence, not unicode.IsUpper: a compiler neutralizes names // with strings.ToLower, so a rune that has no lowercase form (double-struck ℤ, diff --git a/ir/irverify/naming_alias_test.go b/ir/irverify/naming_alias_test.go new file mode 100644 index 0000000..d6b7cd5 --- /dev/null +++ b/ir/irverify/naming_alias_test.go @@ -0,0 +1,292 @@ +package irverify_test + +import ( + "fmt" + "reflect" + "testing" + "unicode/utf8" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irverify" +) + +// aliasViolations returns everything Verify reports on a model named with +// aliases. Its Source is "m", which no fixture below lists, so the +// redundant-with-Source rule is out of the way of the ones being measured. +// +// Unfiltered on purpose. TestVerify_VerbatimAliasesAreClean is the one pinning +// that no neutrality rule reaches an alias, so it has to be able to see +// ir/naming-cased and ir/naming-not-words if some later change starts holding +// aliases to Canonical's grammar; filtering to the alias codes would leave that +// test unable to fail for the reason it exists. Nothing unrelated is in the way +// either — TestVerify_NoAliasesIsClean asserts this exact document, with the +// alias list empty, verifies empty. +func aliasViolations(t *testing.T, aliases ...string) []irverify.Violation { + t.Helper() + return irverify.Verify(modelNamed(ir.Naming{Source: "m", Canonical: "m", Aliases: aliases})) +} + +// 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 between them these +// four aliases would draw all three of ir/naming-cased, ir/naming-not-words and +// ir/naming-unsegmented if an alias were held to Canonical's grammar — the +// dotted name the first two, "API2Key" the first and last. +func TestVerify_VerbatimAliasesAreClean(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t, "com.example.User", "UserID", "user_id", "API2Key")) +} + +func TestVerify_NoAliasesIsClean(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t)) +} + +// TestVerify_BlankAliasIsAViolation covers the whole of what "names nothing" +// means. An entry made only of runes Unicode calls invisible is as blank as "", +// and testing emptiness alone — or trimming only what unicode.IsSpace reports — +// would let most of these through the one rule that exists to catch an entry +// matching nothing. +func TestVerify_BlankAliasIsAViolation(t *testing.T) { + t.Parallel() + blanks := map[string]string{ + "empty": "", + "space": " ", + "tab": "\t", + "newline": "\n", + "mixed spaces": " \t ", + "no-break": "\u00a0", + "ideographic": "\u3000", + "zero width": "\u200b", + "byte order": "\ufeff", + "joiner": "\u200d", + "soft hyphen": "\u00ad", + "word joiner": "\u2060", + "nul": "\x00", + "hangul filler": "\u3164", + "jamo filler": "\u115f", + "invisible mix": "\u200b\t\ufeff\u3164", + } + for name, alias := range blanks { + t.Run(name, func(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "ok", alias) + require.Len(t, got, 1, "one violation for the one blank entry") + assert.Equal(t, "ir/naming-alias-blank", 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") + }) + } +} + +// TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank holds one side of the line +// isBlankName draws — see its comment for why the IR declines to judge an +// invisible rune that is not the whole entry. +func TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t, "com.example.\u200bUser", " padded ")) +} + +// TestVerify_BlankLookingGraphicAliasIsNotBlank holds the other side. U+2800 +// BRAILLE PATTERN BLANK renders as whitespace but is a graphic character +// Unicode does not classify as invisible, so the rule leaves it alone rather +// than deciding on its own account what looks empty. +func TestVerify_BlankLookingGraphicAliasIsNotBlank(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t, "\u2800")) +} + +// TestVerify_IllFormedAliasIsAViolation covers the rule every channel shares. +// The bytes here are a lone continuation byte: json.Marshal writes it as the +// replacement rune, so a document carrying one decodes to a different document +// and stops round-tripping. No other rule here would notice — the replacement +// rune is visible, so an ill-formed entry is not blank. +func TestVerify_IllFormedAliasIsAViolation(t *testing.T) { + t.Parallel() + ill := string([]byte{'c', 'a', 'f', 0xe9}) + require.False(t, utf8.ValidString(ill), "the fixture has to be ill-formed to test anything") + + got := aliasViolations(t, "ok", ill) + require.Len(t, got, 1) + assert.Equal(t, "ir/naming-invalid-utf8", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[1]", got[0].Path) + assert.NotContains(t, got[0].Message, ill, "the report does not repeat the bad bytes") +} + +// TestVerify_DuplicateAliasIsAViolation asserts both ends of the pair. The path +// carries the entry to delete; the message carries the one it repeats, so a +// reader of a long list is not left scanning for the twin — which is how +// checkDuplicateIDs words the same defect ("declared here and at …"). +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.Equal(t, "alias dup is listed here and at index 0", got[0].Message) +} + +// TestVerify_AliasRepeatingItsOwnSourceIsAViolation covers the other way an +// entry can admit no name that was not already admitted. A reader matches the +// entity's own Source before any alias, so listing it again adds nothing — the +// argument the duplicate rule rests on, one channel over. +func TestVerify_AliasRepeatingItsOwnSourceIsAViolation(t *testing.T) { + t.Parallel() + got := irverify.Verify(modelNamed( + ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"User"}})) + require.Len(t, got, 1) + assert.Equal(t, "ir/naming-alias-redundant", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[0]", got[0].Path) + assert.Equal(t, "alias User is the entity's own source name, so it matches nothing more", + got[0].Message) +} + +// TestVerify_AliasMatchingDerivedChannelsIsClean holds the boundary that rule +// stops at. Canonical and Hint are names the IR derived for an emitter to +// render, not names a writer schema could have spelled, so an alias equal to +// one of them is not redundant with anything a reader would match. +func TestVerify_AliasMatchingDerivedChannelsIsClean(t *testing.T) { + t.Parallel() + for channel, n := range map[string]ir.Naming{ + "canonical": {Source: "User", Canonical: "user", Aliases: []string{"user"}}, + "hint": {Hint: "user", Aliases: []string{"user"}}, + } { + t.Run(channel, func(t *testing.T) { + t.Parallel() + assert.Empty(t, irverify.Verify(modelNamed(n))) + }) + } +} + +// TestVerify_RepeatedBlankAliasReportsEachAsBlank holds the interaction between +// the blank rule and the duplicate rule: a second blank entry is a repeat as +// well as a blank one, and reporting it as a duplicate would name the wrong +// repair. +// +// Each case repeats *the same* string, which is what makes the claim testable. +// With two different blanks the duplicate rule cannot fire whatever the +// implementation does, so the natural rewrite — report the repeat, then the +// blank, setting seen unconditionally — passes a two-different-blanks fixture +// while emitting exactly the wrong repair this test forbids. +func TestVerify_RepeatedBlankAliasReportsEachAsBlank(t *testing.T) { + t.Parallel() + for name, alias := range map[string]string{"empty": "", "space": " ", "zero width": "\u200b"} { + t.Run(name, func(t *testing.T) { + t.Parallel() + got := aliasViolations(t, alias, alias) + require.Len(t, got, 2) + for _, v := range got { + assert.Equal(t, "ir/naming-alias-blank", v.Code) + } + }) + } +} + +// TestVerify_RepeatedSourceAliasReportsEachAsRedundant holds the same +// interaction one rule over. Both entries name what Source already names, so +// both go; recording a redundant entry in seen would report the second as a +// duplicate of the first and leave a reader deleting one of two entries that +// are each wrong on their own. +func TestVerify_RepeatedSourceAliasReportsEachAsRedundant(t *testing.T) { + t.Parallel() + got := irverify.Verify(modelNamed( + ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"User", "User"}})) + require.Len(t, got, 2) + for i, v := range got { + assert.Equal(t, "ir/naming-alias-redundant", v.Code) + assert.Equal(t, fmt.Sprintf("doc.Types[t/x/M].Name.Aliases[%d]", i), v.Path) + } +} + +// 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-blank": "doc.Types[t/x/M].Name.Aliases[2]", + "ir/naming-alias-duplicate": "doc.Types[t/x/M].Name.Aliases[4]", + }, byCode) +} + +// TestVerify_AliasSharedByTwoNamings pins the scope boundary +// appendAliasViolations declares: a repeat across two Namings goes unreported +// today, and closing GitHub #398 is what should change it. +// +// Without this, nothing holds seen to being per-Naming. Hoisting it into +// checkNaming's closure — the one-line change anyone implementing #398 reaches +// for first — makes this document report ir/naming-alias-duplicate, and every +// other test in this file stays green, because each drives a document with one +// Naming in it. +func TestVerify_AliasSharedByTwoNamings(t *testing.T) { + t.Parallel() + const shared = "com.example.User" + a := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/A", + Name: ir.Naming{Source: "a", Canonical: "a", Aliases: []string{shared}}}} + b := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/B", + Name: ir.Naming{Source: "b", Canonical: "b", Aliases: []string{shared}}}} + + got := irverify.Verify(&ir.Document{IRVersion: ir.IRVersion, + Types: ir.TypeRegistry{a.ID: a, b.ID: b}}) + assert.Empty(t, got, "out of scope until GitHub #398; this is the fixture that says so") +} + +// TestVerify_AliasPathIsSpelledAsTheWalkWould ties the hand-assembled violation +// path to ir.WalkValues' own grammar. +// +// checkNaming prunes at ir.Naming — it holds no reference and no nested Naming +// to descend into — so the walk never renders these paths itself and aliasPath +// spells them by hand. That leaves two statements of one grammar with nothing +// between them: were ir's slice-index rendering to change, every walk-produced +// path in every other check would move while these codes alone kept the old +// spelling, and no test would say so. This is that seam, so it reddens here. +// +// Past the single digits too, which is where a hand-built path and a formatted +// one last agree. +func TestVerify_AliasPathIsSpelledAsTheWalkWould(t *testing.T) { + t.Parallel() + const size = 12 + aliases := make([]string, size) + for i := range aliases { + aliases[i] = fmt.Sprintf("alias.%d", i) // distinct, so no entry is a repeat + } + doc := modelNamed(ir.Naming{Source: "m", Canonical: "m", Aliases: aliases}) + + walked := map[string]string{} // alias → the path the walk renders for it + ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { + if v.Kind() == reflect.String && v.String() != "" { + walked[v.String()] = path + } + return true + }) + for _, alias := range aliases { + require.Contains(t, walked, alias, "the walk reaches every entry when nothing prunes it") + } + + // Blank every entry so the check reports one violation per index, then hold + // each reported path to the one the walk rendered at that same index. + got := aliasViolations(t, make([]string, size)...) + require.Len(t, got, size) + paths := make([]string, len(got)) + for i, v := range got { + paths[i] = v.Path + } + want := make([]string, 0, size) + for _, alias := range aliases { + want = append(want, walked[alias]) + } + assert.ElementsMatch(t, want, paths) +} diff --git a/ir/irverify/naming_test.go b/ir/irverify/naming_test.go index fb8af19..0fdfa9e 100644 --- a/ir/irverify/naming_test.go +++ b/ir/irverify/naming_test.go @@ -2,6 +2,7 @@ package irverify_test import ( "testing" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -368,3 +369,44 @@ func TestVerify_PresenceReachesANamingNoNameFieldOwns(t *testing.T) { assert.Equal(t, "ir/naming-absent", got[0].Code) assert.Equal(t, "doc.Services[0].Renames[t/x/Model]", got[0].Path) } + +// TestVerify_IllFormedNameIsAViolation covers the one rule every channel of a +// Naming shares. It is about the encoding rather than the spelling: ill-formed +// bytes survive a marshal as the replacement rune, so a document carrying them +// decodes to one that re-marshals differently and stops round-tripping. +// +// Canonical and Hint would each draw a violation without this rule — the +// replacement rune is not a word character, so isWordSequence rejects it — but +// one naming the wrong repair, since splitting on non-word characters is not +// what fixes undecodable bytes. Source draws nothing at all without it. So the +// assertion is that ir/naming-invalid-utf8 is among what is reported, not that +// it is all of it. +// +// Only this rule's own message is held to quoting nothing. The content rules +// beside it deliberately carry the spelling they object to, which puts the +// ill-formed bytes in their message (GitHub #400) — a separate question from +// whether the encoding rule fires. +func TestVerify_IllFormedNameIsAViolation(t *testing.T) { + t.Parallel() + ill := string([]byte{'c', 'a', 'f', 0xe9}) + require.False(t, utf8.ValidString(ill), "the fixture has to be ill-formed to test anything") + + for channel, n := range map[string]ir.Naming{ + "source": {Source: ill, Canonical: ir.CanonicalWords(ill)}, + "canonical": {Canonical: ill}, + "hint": {Hint: ill}, + } { + t.Run(channel, func(t *testing.T) { + t.Parallel() + var reported *irverify.Violation + for _, v := range irverify.Verify(modelNamed(n)) { + if v.Code == "ir/naming-invalid-utf8" { + reported = &v + } + } + require.NotNil(t, reported, "the encoding rule fires on %s", channel) + assert.Equal(t, "doc.Types[t/x/M].Name", reported.Path) + assert.NotContains(t, reported.Message, ill, "the report does not repeat the bad bytes") + }) + } +} diff --git a/ir/naming.go b/ir/naming.go index 36c868c..1fd93e7 100644 --- a/ir/naming.go +++ b/ir/naming.go @@ -31,6 +31,27 @@ 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 names something, since one with nothing visible in it + // matches nothing; + // - every entry decodes, since one holding ill-formed UTF-8 does not + // survive the round-trip invariant #7 promises; + // - no entry repeats another, or the entity's own Source, since either + // admits no name that was not already admitted — so a producer that + // wrote one built the list wrong. + // + // That such an entry is inert is also why a source declaring one is + // recorded once, with a Diagnostic naming it, rather than carried through: + // dropping it is not the lossy flattening invariant #2 forbids, because the + // same set of names resolves to this entity either way. Aliases []string `json:"aliases,omitempty"` }