diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index 0d3f32d2..a8829328 100644 --- a/internal/catalogue/manifest.go +++ b/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{} diff --git a/internal/catalogue/optional_requirements.go b/internal/catalogue/optional_requirements.go index a6ccc2cd..f2db8adf 100644 --- a/internal/catalogue/optional_requirements.go +++ b/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 { diff --git a/internal/catalogue/optional_requirements_test.go b/internal/catalogue/optional_requirements_test.go index 64887ffe..0b6f8083 100644 --- a/internal/catalogue/optional_requirements_test.go +++ b/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{}) diff --git a/internal/catalogue/resolve.go b/internal/catalogue/resolve.go index 24d60f7a..2b5af88e 100644 --- a/internal/catalogue/resolve.go +++ b/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. //