Let a mergeable file name the setting keys it takes (hq issue 168, ADR 0326)

Every key of a module's layers merged into every mergeable file it owns:
the mesh's own words, a key meant for a ${setting:} in another file, a
key for a second configuration file. A module could not own two
configurable files. A file now names its keys in takes-settings and
takes only those; a key no file names is stray where it is set; a
module that declares a setting names the keys of every mergeable file,
and one that declares none keeps today's form (ADR 0164 §15).

internal/catalogue/manifest.go

@@ -1693,6 +1693,8 @@ func ParseManifest(raw []byte) (Manifest, error) {
 	problems = append(problems, LogProblems(m)...)
 	// And the defaults it gives its settings (setting_defaults.go, novox/hq ADR 0262).
 	problems = append(problems, SettingProblems(m)...)
+	// And which setting keys each mergeable file takes (setting_takes.go, novox/hq ADR 0326).
+	problems = append(problems, TakenSettingsProblems(m)...)
 	wellFormed := true
 	for _, c := range m.Claims {
 		if !name.MatchString(c.Name) {

internal/catalogue/setting_takes.go

@@ -0,0 +1,187 @@
+package catalogue
+
+import (
+	"fmt"
+	"strings"
+	"unicode"
+)
+
+// A mergeable file names the setting keys it takes (novox/hq ADR 0326, issue 168).
+//
+// Before this, every key of a module's layers merged into every one of its mergeable files: the
+// mesh's own words, a key meant for a `${setting:<key>}` in another file, a key for a second
+// configuration file. So a module could not own two configurable files, and every receiver had to
+// ignore what it did not know. ADR 0164 §11 decided that a setting names the files that read it; this
+// is that relation, stated from the file's side, so the operator's own values stay declared nowhere
+// (ADR 0262 §2).
+
+// TakesSettingsField is the key a mergeable file carries to name the top-level setting keys it takes.
+const TakesSettingsField = "takes-settings"
+
+// takenKeys is the keys a resource names in takes-settings, and whether it names any list at all. An
+// empty list is a list: that file takes no setting.
+func takenKeys(resource map[string]any) (map[string]bool, bool) {
+	raw, said := resource[TakesSettingsField]
+	if !said {
+		return nil, false
+	}
+	keys := map[string]bool{}
+	for _, k := range stringsOf(raw) {
+		keys[k] = true
+	}
+	return keys, true
+}
+
+// onlyTaken is the layers with every key the file does not name left out. The file's own defaults and
+// what the mesh reads itself are not touched here: settle already keeps both out of a file.
+func onlyTaken(layers []Layer, taken map[string]bool) []Layer {
+	kept := make([]Layer, 0, len(layers))
+	for _, layer := range layers {
+		values := map[string]any{}
+		for key, value := range layer.Values {
+			if taken[key] {
+				values[key] = value
+			}
+		}
+		kept = append(kept, Layer{From: layer.From, Values: values, Default: layer.Default})
+	}
+	return kept
+}
+
+// namesItsKeys says whether a module's mergeable files name the keys they take (ADR 0326 §2): true
+// only when it has one and every one carries takes-settings. A module with a silent mergeable file
+// keeps today's form (§3), where every key reaches every mergeable file.
+func namesItsKeys(m Manifest) bool {
+	merging := 0
+	for _, r := range m.Resources {
+		if how, _ := r["merge"].(string); how == "" {
+			continue
+		}
+		merging++
+		if _, said := r[TakesSettingsField]; !said {
+			return false
+		}
+	}
+	return merging > 0
+}
+
+// TakenSettingsProblems is every way a definition's takes-settings is wrong (ADR 0326 §2 and §4), in
+// its own words: on a file that does not merge, not a list of keys, a key named twice, one the mesh
+// reads itself, one the file keeps for itself, or one mergeable file naming its keys beside another
+// that names none.
+func TakenSettingsProblems(m Manifest) []string {
+	problems := takenSettingsProblems(m)
+	// **A module that declares a setting names the keys of every mergeable file** (ADR 0326 §3, ADR
+	// 0164 §11 and §15): only a module that declares none keeps today's form. Said where a definition
+	// is checked and registered, never where a stored one is composed: a definition stored before
+	// this record is brought into the form by its next change, not left out of a machine meanwhile.
+	if len(m.Settings) > 0 {
+		for _, r := range m.Resources {
+			how, _ := r["merge"].(string)
+			if _, said := r[TakesSettingsField]; how != "" && !said {
+				problems = append(problems, fmt.Sprintf("%s declares settings, and its mergeable %v names no %s: "+
+					"a module that declares a setting names the keys each mergeable file takes, an empty list "+
+					"for none (novox/hq ADR 0326, ADR 0164 §11)", m.Module, r["id"], TakesSettingsField))
+			}
+		}
+	}
+	return problems
+}
+
+// takenSettingsProblems is what TakenSettingsProblems says that a stored definition is judged by too.
+func takenSettingsProblems(m Manifest) []string {
+	var problems []string
+	var naming, silent []string
+	for _, r := range m.Resources {
+		id := fmt.Sprint(r["id"])
+		how, _ := r["merge"].(string)
+		raw, said := r[TakesSettingsField]
+		if how == "" {
+			if said {
+				problems = append(problems, fmt.Sprintf("%s's %s says %s and does not merge: only a mergeable "+
+					"file takes settings by key (novox/hq ADR 0326)", m.Module, id, TakesSettingsField))
+			}
+			continue
+		}
+		if !said {
+			silent = append(silent, id)
+			continue
+		}
+		naming = append(naming, id)
+		list, isList := raw.([]any)
+		if !isList {
+			problems = append(problems, fmt.Sprintf("%s's %s: %s is a list of setting keys, and this is %T",
+				m.Module, id, TakesSettingsField, raw))
+			continue
+		}
+		protected := map[string]bool{}
+		for _, k := range stringsOf(r["protected"]) {
+			protected[k] = true
+		}
+		seen := map[string]bool{}
+		for _, item := range list {
+			key, isString := item.(string)
+			switch {
+			case !isString || !usableKey(key):
+				problems = append(problems, fmt.Sprintf("%s's %s: %v in %s is not a usable setting key",
+					m.Module, id, item, TakesSettingsField))
+			case seen[key]:
+				problems = append(problems, fmt.Sprintf("%s's %s names %q twice in %s",
+					m.Module, id, key, TakesSettingsField))
+			case meshWords[key]:
+				problems = append(problems, fmt.Sprintf("%s's %s names %q in %s, and the mesh reads %q itself "+
+					"(novox/hq ADR 0164 §14)", m.Module, id, key, TakesSettingsField, key))
+			case protected[key]:
+				problems = append(problems, fmt.Sprintf("%s's %s names %q in %s and keeps it as protected: a "+
+					"key is the file's or the setting's, never both", m.Module, id, key, TakesSettingsField))
+			}
+			if isString {
+				seen[key] = true
+			}
+		}
+	}
+	if len(naming) > 0 {
+		for _, id := range silent {
+			problems = append(problems, fmt.Sprintf("%s's %s merges and names no %s while %s does: once one "+
+				"mergeable file of a module names the keys it takes, every one does, an empty list for none "+
+				"(novox/hq ADR 0326)", m.Module, id, TakesSettingsField, strings.Join(naming, ", ")))
+		}
+	}
+	return problems
+}
+
+// usableKey is a key a file can be said to take: not empty, and no space, line end or control
+// character. Not the lower-case rule of a `${setting:<key>}`: a file's own keys are its software's
+// spelling, such as `timeZone`.
+func usableKey(key string) bool {
+	if key == "" {
+		return false
+	}
+	for _, r := range key {
+		if unicode.IsSpace(r) || unicode.IsControl(r) || !unicode.IsPrint(r) {
+			return false
+		}
+	}
+	return true
+}
+
+// takenSaid is what a stray key's message adds for a module whose files name their keys: which keys
+// each file takes, so the person sees where the key would have had to be named.
+func takenSaid(m Manifest) string {
+	if !namesItsKeys(m) {
+		return ""
+	}
+	var files []string
+	for _, r := range m.Resources {
+		if how, _ := r["merge"].(string); how == "" {
+			continue
+		}
+		keys := stringsOf(r[TakesSettingsField])
+		if len(keys) == 0 {
+			files = append(files, fmt.Sprintf("%v takes none", r["id"]))
+			continue
+		}
+		files = append(files, fmt.Sprintf("%v takes %s", r["id"], strings.Join(keys, ", ")))
+	}
+	return " (" + strings.Join(files, "; ") + ")"
+}

internal/catalogue/setting_takes_test.go

@@ -0,0 +1,217 @@
+package catalogue
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+)
+
+// twoFiles is a module that owns two configurable files, which issue 168 said no module could:
+// every key of every layer reached both.
+func twoFiles(app, sidecar []any) Manifest {
+	appFile := map[string]any{"id": "app", "type": "file", "path": "/etc/app.json", "merge": MergeJSON,
+		"content": `{"server":{"port":8080},"search":{"safe":1}}`}
+	sideFile := map[string]any{"id": "sidecar", "type": "file", "path": "/etc/sidecar.json", "merge": MergeJSON,
+		"content": `{"url":"http://127.0.0.1:8080"}`}
+	if app != nil {
+		appFile[TakesSettingsField] = app
+	}
+	if sidecar != nil {
+		sideFile[TakesSettingsField] = sidecar
+	}
+	return Manifest{Module: "search", Resources: []map[string]any{
+		appFile, sideFile,
+		{"id": "env", "type": "file", "path": "/etc/search.env", "content": "SITE=${setting:site-name}\n"},
+	}}
+}
+
+func settledContent(t *testing.T, resource map[string]any, layers []Layer) map[string]any {
+	t.Helper()
+	out, err := ApplySettings(resource, layers)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, kept := out[TakesSettingsField]; kept {
+		t.Errorf("%v still carries %s; the host knows no such field", out["id"], TakesSettingsField)
+	}
+	var parsed map[string]any
+	if err := json.Unmarshal([]byte(out["content"].(string)), &parsed); err != nil {
+		t.Fatalf("%v is not JSON after settling: %v", out["id"], err)
+	}
+	return parsed
+}
+
+func TestEachMergeableFileTakesOnlyTheKeysItNames(t *testing.T) {
+	// novox/hq ADR 0326 §1, issue 168: two configurable files, each taking its own keys. A key for
+	// the other file, for a ${setting:…} elsewhere or for the mesh itself lands in neither.
+	m := twoFiles([]any{"server", "search"}, []any{"url"})
+	layers := []Layer{
+		{From: "the mesh", Values: map[string]any{
+			"server": map[string]any{"base_url": "https://search.example.tld/"},
+			"url":    "http://127.0.0.1:9090", "site-name": "Search",
+			"endpoints": map[string]any{"web": map[string]any{"reach": "public"}}}},
+		{From: "node-a", Values: map[string]any{"search": map[string]any{"safe": 0.0}}},
+	}
+
+	app := settledContent(t, m.Resources[0], layers)
+	for _, stray := range []string{"url", "site-name", "endpoints"} {
+		if _, in := app[stray]; in {
+			t.Errorf("app took %q, which it does not name: %v", stray, app)
+		}
+	}
+	server, _ := app["server"].(map[string]any)
+	if server["base_url"] != "https://search.example.tld/" || server["port"] != 8080.0 {
+		t.Errorf("app's server block is %v; the setting should merge into the file's own", server)
+	}
+	if search, _ := app["search"].(map[string]any); search["safe"] != 0.0 {
+		t.Errorf("app's search block is %v; the node's layer should win", search)
+	}
+
+	side := settledContent(t, m.Resources[1], layers)
+	if side["url"] != "http://127.0.0.1:9090" {
+		t.Errorf("sidecar's url is %v; it names url", side["url"])
+	}
+	if len(side) != 1 {
+		t.Errorf("sidecar took keys it does not name: %v", side)
+	}
+}
+
+func TestAnEmptyListTakesNoSetting(t *testing.T) {
+	// ADR 0164 §12: a file that names no setting takes none.
+	m := twoFiles([]any{"server"}, []any{})
+	side := settledContent(t, m.Resources[1], []Layer{{From: "the mesh", Values: map[string]any{
+		"server": map[string]any{"port": 1.0}, "url": "x"}}})
+	if len(side) != 1 || side["url"] != "http://127.0.0.1:8080" {
+		t.Errorf("a file taking none was changed: %v", side)
+	}
+}
+
+func TestAFileThatNamesNothingKeepsTodaysForm(t *testing.T) {
+	// ADR 0326 §3, ADR 0164 §15: a module none of whose files names keys is left as it was.
+	m := twoFiles(nil, nil)
+	app := settledContent(t, m.Resources[0], []Layer{{From: "the mesh", Values: map[string]any{"url": "x"}}})
+	if app["url"] != "x" {
+		t.Errorf("a silent file should still take every key until its module names them: %v", app)
+	}
+	if unused := UnusedSettings(m, []Layer{{From: "the mesh", Values: map[string]any{"anything": 1}}}); len(unused) != 0 {
+		t.Errorf("today's form has nothing stray, and said: %v", unused)
+	}
+}
+
+func TestAKeyNoFileNamesIsStray(t *testing.T) {
+	// ADR 0326 §5: once the files name their keys, a key none of them names — and nothing else
+	// takes — is stray, and is said with what each file takes.
+	m := twoFiles([]any{"server", "search"}, []any{"url"})
+	unused := UnusedSettings(m, []Layer{{From: "the mesh", Values: map[string]any{
+		"server": 1, "url": "x", "site-name": "Search", "ui": map[string]any{"theme": "dark"}}}})
+	if len(unused) != 1 || !strings.Contains(unused[0], `"ui"`) {
+		t.Fatalf("only ui reaches nothing; named: %v", unused)
+	}
+	if !strings.Contains(unused[0], "app takes server, search") || !strings.Contains(unused[0], "sidecar takes url") {
+		t.Errorf("the stray key's message does not say what each file takes: %s", unused[0])
+	}
+}
+
+func TestOneNamingFileMakesEveryMergeableFileName(t *testing.T) {
+	// ADR 0326 §2: a module with one naming and one silent mergeable file is refused, naming the
+	// silent one — the silent one would otherwise still take every key.
+	m := twoFiles([]any{"server"}, nil)
+	problems := TakenSettingsProblems(m)
+	if len(problems) != 1 || !strings.Contains(problems[0], "sidecar merges and names no takes-settings") {
+		t.Fatalf("problems: %v", problems)
+	}
+	if err := JudgeSettings(m, nil, false); err == nil || !strings.Contains(err.Error(), "sidecar") {
+		t.Errorf("composing it should refuse it too, and said: %v", err)
+	}
+	raw, err := json.Marshal(map[string]any{"module": m.Module, "version": "1", "resources": m.Resources})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := ParseManifest(raw); err == nil || !strings.Contains(err.Error(), "sidecar merges and names no") {
+		t.Errorf("the module check does not say it: %v", err)
+	}
+}
+
+func TestTakesSettingsIsRefusedWhereItCannotMeanAnything(t *testing.T) {
+	// ADR 0326 §4.
+	cases := map[string]struct {
+		resource map[string]any
+		says     string
+	}{
+		"on a file that does not merge": {
+			map[string]any{"id": "plain", "type": "file", "path": "/x", "content": "x",
+				TakesSettingsField: []any{"a"}},
+			"does not merge"},
+		"not a list": {
+			map[string]any{"id": "conf", "type": "file", "path": "/x", "merge": MergeJSON, "content": "{}",
+				TakesSettingsField: "server"},
+			"is a list of setting keys"},
+		"a mesh word": {
+			map[string]any{"id": "conf", "type": "file", "path": "/x", "merge": MergeJSON, "content": "{}",
+				TakesSettingsField: []any{"endpoints"}},
+			"the mesh reads"},
+		"a protected key": {
+			map[string]any{"id": "conf", "type": "file", "path": "/x", "merge": MergeJSON, "content": "{}",
+				"protected": []any{"secret"}, TakesSettingsField: []any{"secret"}},
+			"keeps it as protected"},
+		"named twice": {
+			map[string]any{"id": "conf", "type": "file", "path": "/x", "merge": MergeJSON, "content": "{}",
+				TakesSettingsField: []any{"a", "a"}},
+			"twice"},
+		"not a key": {
+			map[string]any{"id": "conf", "type": "file", "path": "/x", "merge": MergeJSON, "content": "{}",
+				TakesSettingsField: []any{"two words"}},
+			"not a usable setting key"},
+	}
+	for name, c := range cases {
+		problems := TakenSettingsProblems(Manifest{Module: "m", Resources: []map[string]any{c.resource}})
+		if !containsText(problems, c.says) {
+			t.Errorf("%s: want a problem saying %q, got %v", name, c.says, problems)
+		}
+	}
+	// And a file that does not merge is refused where it is settled, not handed to the host.
+	plain := map[string]any{"id": "plain", "type": "file", "path": "/x", "content": "x",
+		TakesSettingsField: []any{"a"}}
+	if _, err := ApplySettings(plain, nil); err == nil {
+		t.Error("a file that does not merge and names keys was settled")
+	}
+}
+
+func TestASoftwareSpelledKeyIsUsable(t *testing.T) {
+	// A file's keys are its software's: nodered's timeZone is not lower case and is still a key.
+	m := Manifest{Module: "flows", Resources: []map[string]any{{"id": "settings", "type": "file",
+		"path": "/x", "merge": MergeJSON, "content": "{}", TakesSettingsField: []any{"timeZone"}}}}
+	if problems := TakenSettingsProblems(m); len(problems) != 0 {
+		t.Errorf("timeZone was refused: %v", problems)
+	}
+}
+
+func containsText(list []string, text string) bool {
+	for _, s := range list {
+		if strings.Contains(s, text) {
+			return true
+		}
+	}
+	return false
+}
+
+func TestAModuleThatDeclaresASettingNamesItsFilesKeys(t *testing.T) {
+	// ADR 0326 §3, ADR 0164 §15: only a module that declares no settings keeps today's form. One that
+	// declares a preference and owns a silent mergeable file is refused where it is checked — and not
+	// where a stored definition is composed, so a machine does not lose it before its next change.
+	m := twoFiles(nil, nil)
+	m.Settings = map[string]SettingDeclaration{"site-name": {Kind: KindPreference, Default: "Search", Why: "a name"}}
+	problems := TakenSettingsProblems(m)
+	if len(problems) != 2 || !containsText(problems, "app names no takes-settings") ||
+		!containsText(problems, "sidecar names no takes-settings") {
+		t.Errorf("problems: %v", problems)
+	}
+	if err := JudgeSettings(m, nil, false); err != nil {
+		t.Errorf("a stored definition is composed as before, and was refused: %v", err)
+	}
+	m = twoFiles([]any{"server"}, []any{})
+	m.Settings = map[string]SettingDeclaration{"site-name": {Kind: KindPreference, Default: "Search", Why: "a name"}}
+	if problems := TakenSettingsProblems(m); len(problems) != 0 {
+		t.Errorf("every mergeable file names its keys, and was refused: %v", problems)
+	}
+}

internal/catalogue/settings.go

@@ -46,11 +46,24 @@ type Layer struct {
 // rather than by repeating everything the rest already say.
 func ApplySettings(resource map[string]any, layers []Layer) (map[string]any, error) {
 	how, _ := resource["merge"].(string)
+	taken, names := takenKeys(resource)
 	if how == "" {
+		if names {
+			// Refused by the module check too (TakenSettingsProblems); said here as well, for a
+			// definition stored by an older binary, rather than handed to the host, which knows no
+			// such field.
+			return nil, fmt.Errorf("%v says %s and does not merge: only a mergeable file takes "+
+				"settings by key (novox/hq ADR 0326)", resource["id"], TakesSettingsField)
+		}
 		// Not a mergeable file. Settings for it are ignored rather than silently doing nothing
 		// somewhere else — see UnusedSettings, which is what says so.
 		return resource, nil
 	}
+	if names {
+		// **The file takes the keys it names, and no other** (novox/hq ADR 0326, issue 168): a key
+		// for another file, for a `${setting:<key>}` or for the mesh itself does not land here.
+		layers = onlyTaken(layers, taken)
+	}
 	if how != MergeJSON {
 		return nil, fmt.Errorf(
 			"%v says it merges as %q, and this control plane knows how to merge %q",
@@ -91,6 +104,7 @@ func ApplySettings(resource map[string]any, layers []Layer) (map[string]any, err
 	delete(out, "merge")
 	delete(out, "protected")
 	delete(out, TrustedField)
+	delete(out, TakesSettingsField)
 	return out, nil
 }
 
@@ -324,8 +338,11 @@ func sortedKeysAny(m map[string]any) []string {
 // (novox/hq 04-ISSUES/173); and the mesh's own words — `expose`, `ports`, `reach`, `endpoints` —
 // are read by the mesh. A key none of those takes is stray, and is said so rather than dropped.
 func UnusedSettings(m Manifest, layers []Layer) []string {
+	named := namesItsKeys(m)
 	for _, r := range m.Resources {
-		if how, _ := r["merge"].(string); how != "" {
+		if how, _ := r["merge"].(string); how != "" && !named {
+			// Today's form (novox/hq ADR 0326 §3, ADR 0164 §15): a mergeable file that names no
+			// keys takes every key, so nothing is stray.
 			return nil
 		}
 	}
@@ -337,6 +354,13 @@ func UnusedSettings(m Manifest, layers []Layer) []string {
 	}
 
 	lands := settingKeysUsedBy(m)
+	// A mergeable file takes the keys it names (novox/hq ADR 0326 §5).
+	for _, r := range m.Resources {
+		taken, _ := takenKeys(r)
+		for key := range taken {
+			lands[key] = true
+		}
+	}
 	// A key the module gives a default reaches what asks for it (novox/hq ADR 0262): setting it
 	// overrides the default, which is how a running module gains a setting without a gap between.
 	for key := range m.Settings {
@@ -401,8 +425,8 @@ func UnusedSettings(m Manifest, layers []Layer) []string {
 			}
 			unused = append(unused, fmt.Sprintf(
 				"%s sets %q, and %s has no file that merges it, asks for no ${setting:%s}, and "+
-					"declares no %q in what it contributes or serves",
-				layer.From, key, m.Module, key, key))
+					"declares no %q in what it contributes or serves%s",
+				layer.From, key, m.Module, key, key, takenSaid(m)))
 		}
 	}
 	sort.Strings(unused)
@@ -512,6 +536,11 @@ func JudgeSettings(m Manifest, layers []Layer, adopted bool) error {
 	if err := settingsHoldOneLine(m.Module, layers); err != nil {
 		return err
 	}
+	// Which keys each mergeable file takes is the definition's to say right (novox/hq ADR 0326);
+	// judged here too for a definition stored by an older binary.
+	if problems := takenSettingsProblems(m); len(problems) > 0 {
+		return fmt.Errorf("%s", problems[0])
+	}
 	// With no layers too: a definition may ask for a setting nobody made — an access placed by
 	// nobody, a file's ${setting:…} nothing sets — and that is the same statement, missing.
 	if _, err := GivenPorts(m, layers); err != nil {