Store optional requirements beside a plain requires list (hq issue 166, review of #281)

A controller older than optional requirements reads `requires` as names, and Inventory.Catalogue
stops at the first manifest it cannot read: the long form stored inside `requires` would have left
a rollback below this controller composing no machine. Stored, `requires` stays plain and the
optional names sit in `optional-requires`, a key an older controller sets aside, leaving out only
the module carrying it. A refusal of a hard requirement now names a module that wants it hard.

internal/catalogue/manifest.go

@@ -386,8 +386,8 @@ type Manifest struct {
 	// Requires are names that must be provided by something assigned to the same node.
 	Requires []string `json:"requires,omitempty"`
 	// OptionalRequires is which of Requires the module can run without (novox/hq ADR 0330): filled
-	// from the long form of a `requires` entry, {name, optional}, by UnmarshalJSON; never written by
-	// hand.
+	// by UnmarshalJSON from the long form of a `requires` entry, {name, optional}, or from the stored
+	// form's `optional-requires` list; never written by hand.
 	OptionalRequires map[string]bool `json:"-"`
 
 	// Emits are the events this module publishes, named **locally**: `order.placed`, not a subject
@@ -1371,6 +1371,22 @@ func (m *Manifest) UnmarshalJSON(raw []byte) error {
 		keys["requires"] = plainNames
 		optionalRequires = optional
 	}
+	// The stored form: `requires` plain, and which are optional beside it (OptionalRequiresKey).
+	if stored, ok := keys[OptionalRequiresKey]; ok {
+		if string(stored) != "null" {
+			optional, err := readOptionalRequires(stored)
+			if err != nil {
+				return err
+			}
+			for name := range optional {
+				if optionalRequires == nil {
+					optionalRequires = map[string]bool{}
+				}
+				optionalRequires[name] = true
+			}
+		}
+		delete(keys, OptionalRequiresKey)
+	}
 	rest, err := json.Marshal(keys)
 	if err != nil {
 		return err
@@ -1432,12 +1448,14 @@ func (m Manifest) MarshalJSON() ([]byte, error) {
 	if err := json.Unmarshal(raw, &keys); err != nil {
 		return nil, err
 	}
-	if len(m.OptionalRequires) > 0 {
-		requires, err := writeRequires(m)
+	if optional := optionalRequiresList(m); len(optional) > 0 {
+		// Beside `requires`, never inside it: an older controller reads `requires` as plain names, and one
+		// manifest it cannot read stops the whole catalogue (OptionalRequiresKey).
+		listed, err := json.Marshal(optional)
 		if err != nil {
 			return nil, err
 		}
-		keys["requires"] = requires
+		keys[OptionalRequiresKey] = listed
 	}
 	if len(m.ContributesMany) > 0 {
 		mergedContributes := map[string]any{}

internal/catalogue/optional_requirements.go

@@ -69,18 +69,37 @@ func readRequires(raw json.RawMessage) ([]string, map[string]bool, error) {
 	return names, optional, nil
 }
 
-// writeRequires is `requires` in the form it was read: a bare name for a hard requirement, the long
-// form for an optional one, so a manifest that went through the mesh comes out as it went in.
-func writeRequires(m Manifest) (json.RawMessage, error) {
-	out := make([]any, 0, len(m.Requires))
-	for _, r := range m.Requires {
-		if m.OptionalRequires[r] {
-			out = append(out, requirementForm{Name: r, Optional: true})
-			continue
-		}
-		out = append(out, r)
+// OptionalRequiresKey is where a stored manifest keeps which of its requirements are optional: beside
+// `requires`, which stays a plain list of names (mesh-controller #281's review). A controller older than
+// optional requirements reads `requires` as names; the long form inside it failed that read, and with it
+// the whole catalogue, so a rollback below this controller would have composed no machine. An older
+// controller sets this unknown key aside instead and leaves only the module that carries it out. A
+// manifest may be written either way; it is stored this way.
+const OptionalRequiresKey = "optional-requires"
+
+// readOptionalRequires reads the stored form's list of optional requirements.
+func readOptionalRequires(raw json.RawMessage) (map[string]bool, error) {
+	var names []string
+	if err := json.Unmarshal(raw, &names); err != nil {
+		return nil, fmt.Errorf("%s: a list of the names in `requires` that are optional: %w", OptionalRequiresKey, err)
 	}
-	return json.Marshal(out)
+	out := map[string]bool{}
+	for _, n := range names {
+		out[n] = true
+	}
+	return out, nil
+}
+
+// optionalRequiresList is the stored form's list: the optional requirements, sorted.
+func optionalRequiresList(m Manifest) []string {
+	var out []string
+	for name, optional := range m.OptionalRequires {
+		if optional {
+			out = append(out, name)
+		}
+	}
+	sort.Strings(out)
+	return out
 }
 
 // IsOptional is whether this module requires the name optionally (ADR 0330 §1). A name it contributes
@@ -185,6 +204,12 @@ func OptionalProblems(m Manifest) []string {
 		}
 		required[r] = true
 	}
+	for _, r := range optionalRequiresList(m) {
+		if !required[r] {
+			problems = append(problems, fmt.Sprintf(
+				"%s says %q is optional in %s and does not require it", m.Module, r, OptionalRequiresKey))
+		}
+	}
 	// A contribution is to whoever answers the requirement, so it goes with it: contributing to one
 	// that is optional is allowed, and composed only while it is bound.
 	for _, r := range m.Resources {

internal/catalogue/optional_requirements_test.go

@@ -49,8 +49,56 @@ func TestARequirementIsHardUnlessMarkedOptional(t *testing.T) {
 	if !again.IsOptional("influxdb-api") || again.IsOptional("route") || len(again.Requires) != 3 {
 		t.Fatalf("written and read back, it says %v optional %v (%s)", again.Requires, again.OptionalRequires, raw)
 	}
-	if strings.Contains(string(raw), `"name":"route"`) {
-		t.Errorf("a hard requirement was written in the long form: %s", raw)
+	if strings.Contains(string(raw), `"name":`) {
+		t.Errorf("a requirement was stored in the long form: %s", raw)
+	}
+}
+
+// The stored form reads on a controller older than optional requirements (review of mesh-controller
+// #281). Inventory.Catalogue stops at the first manifest it cannot read, so a long-form entry inside
+// `requires` would have stopped every plan after a rollback. Stored, `requires` is plain names, as main's
+// manifest type reads it, and which are optional sits beside it in a key such a controller sets aside.
+func TestTheStoredFormReadsWhereRequiresIsAListOfNames(t *testing.T) {
+	m := mustParse(t, `{"module":"grafana","version":"1",
+		"requires":["route",{"name":"influxdb-api","optional":true}]}`)
+	raw, err := json.Marshal(m)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// main's field, exactly: `Requires []string`.
+	var older struct {
+		Requires []string `json:"requires"`
+	}
+	if err := json.Unmarshal(raw, &older); err != nil {
+		t.Fatalf("a controller reading requires as names cannot read the stored form: %v\n%s", err, raw)
+	}
+	if strings.Join(older.Requires, " ") != "route influxdb-api" {
+		t.Fatalf("it reads the requirements as %v", older.Requires)
+	}
+	var keys map[string]json.RawMessage
+	if err := json.Unmarshal(raw, &keys); err != nil {
+		t.Fatal(err)
+	}
+	if string(keys[OptionalRequiresKey]) != `["influxdb-api"]` {
+		t.Fatalf("which are optional is stored as %s", keys[OptionalRequiresKey])
+	}
+	// And this controller reads the stored form back the same, leniently and strictly.
+	var back Manifest
+	if err := json.Unmarshal(raw, &back); err != nil {
+		t.Fatal(err)
+	}
+	strict, err := ParseManifest(raw)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, got := range []Manifest{back, strict} {
+		if !got.IsOptional("influxdb-api") || got.IsOptional("route") || len(got.Requires) != 2 {
+			t.Fatalf("read back as %v optional %v", got.Requires, got.OptionalRequires)
+		}
+	}
+	// A stored list naming something the module does not require is refused.
+	if _, err := ParseManifest([]byte(`{"module":"x","version":"1","requires":["route"],"optional-requires":["mqtt-topic"]}`)); err == nil {
+		t.Error("an optional requirement the module does not require passed the check")
 	}
 }
 
@@ -120,6 +168,10 @@ func TestOneConsumersOptionalRequirementIsAnothersHardOne(t *testing.T) {
 	if err == nil || !strings.Contains(err.Error(), "assign influxdb") {
 		t.Fatalf("a hard requirement with no provider was not refused naming the candidate: %v", err)
 	}
+	// And the refusal names telegraf, whose remedy it is, not grafana, which met the name first.
+	if !strings.Contains(err.Error(), "wanted by telegraf") {
+		t.Errorf("the refusal does not name the module that wants it hard: %v", err)
+	}
 	// And a module pulled in later (suite → editor) that wants the shell hard installs it, even
 	// though grafana, met first, wanted it only optionally — and grafana is then bound to it.
 	got, err := Resolve(optionalShelf(), []string{"grafana", "suite"}, workstation(), World{})

internal/catalogue/resolve.go

@@ -442,6 +442,16 @@ func Resolve(catalogue map[string]Manifest, assigned []string, node Node, world
 			deferred = append(deferred, want)
 			continue
 		}
+		// Walked as a hard requirement: a refusal names a module that wants it hard, never one that
+		// wanted it only optionally and met it first — whose remedy it is not.
+		if first, known := catalogue[because[want]]; !settling[want] && known && !first.wantsHard(want) {
+			for _, name := range order {
+				if catalogue[name].wantsHard(want) {
+					because[want] = name
+					break
+				}
+			}
+		}
 		// Already answered by something in the set. This is the case that makes assigning zsh do
 		// what a person meant by it.
 		//