diff --git a/internal/audit/run/adjust.go b/internal/audit/run/adjust.go index e299a32..402cb39 100644 --- a/internal/audit/run/adjust.go +++ b/internal/audit/run/adjust.go @@ -103,6 +103,23 @@ var ( reNotValid = regexp.MustCompile(`field (\S+) is not valid(?: when (\w+)=(\S+))?`) reReference = regexp.MustCompile(`field (\S+) must reference an existing (\w+)`) reRequired = regexp.MustCompile(`field (\S+) is required(?: when (\w+)=(\S+))?`) + + // reFieldNamed matches a refusal that names its field mid-sentence and + // states the complaint after a separator, which is how a framework that + // wraps its validation errors in prose reports them. + reFieldNamed = regexp.MustCompile(`(?i)\bfield\s+([\w.\[\]-]+)\s*[:\-]\s*(.+)`) + // reTheRequired matches the bare English an API writes when it names the + // field it wanted and nothing else about it. + reTheRequired = regexp.MustCompile(`(?i)\bthe\s+([\w.]+)\s+(?:is|are)\s+required\b`) + // reFieldSaid matches the field-prefixed refusal a validation framework + // emits: the property it rejected, a colon, then its complaint. The field + // is a dotted path when the API validates a nested request object, and the + // last segment is the name the request body spells. + reFieldSaid = regexp.MustCompile(`^\s*([\w.\[\]-]+)\s*:\s*(.+)$`) + // reAbsent matches the complaints that mean "you sent nothing for this", + // as distinct from "what you sent is wrong": only absence is healed by + // adding a value. + reAbsent = regexp.MustCompile(`(?i)\b(?:is required|is mandatory|must not be (?:null|empty|blank)|may not be (?:null|empty|blank)|cannot be (?:null|empty|blank)|must be (?:provided|specified|present)|missing)\b`) ) // classifyRefusal reads a 4xx response body and decides what to change. The @@ -126,9 +143,32 @@ func classifyRefusal(res *httpResult) refusalAction { if m := reRequired.FindStringSubmatch(msg); m != nil { return refusalAction{kind: actAdd, field: cleanField(m[1]), condGate: m[2], condVal: cleanField(m[3])} } + if m := reFieldNamed.FindStringSubmatch(msg); m != nil && reAbsent.MatchString(m[2]) { + return refusalAction{kind: actAdd, field: leafField(m[1])} + } + if m := reTheRequired.FindStringSubmatch(msg); m != nil { + return refusalAction{kind: actAdd, field: leafField(m[1])} + } + // Checked last, because it is the loosest: any sentence at all can be read + // as ": ", so it must not pre-empt a grammar that names + // two fields or a gate. + if m := reFieldSaid.FindStringSubmatch(msg); m != nil && reAbsent.MatchString(m[2]) { + return refusalAction{kind: actAdd, field: leafField(m[1])} + } return refusalAction{kind: actStop} } +// leafField is the last segment of a dotted refusal path, which is the name +// the request body spells: an API that validates its own nested request object +// reports "endpoint.url" for a property the document declares as "url". +func leafField(s string) string { + s = cleanField(s) + if i := strings.LastIndex(s, "."); i >= 0 { + s = s[i+1:] + } + return s +} + // refusalMessage pulls the human-legible sentence out of whichever error // envelope the API used — problem+json's detail, an oauth error_description, a // legacy errorMessage — falling back to the raw body when it is not JSON. It @@ -137,6 +177,12 @@ func classifyRefusal(res *httpResult) refusalAction { func refusalMessage(raw []byte) string { var m map[string]any if err := json.Unmarshal(raw, &m); err == nil { + // The listed complaints come first: an envelope that carries both + // names the field it rejected in the list and only summarises it in + // the sentence, and a summary heals nothing. + if listed := firstListed(m); listed != "" { + return listed + } for _, k := range []string{"detail", "message", "error_description", "errorMessage", "error", "title"} { if s, ok := m[k].(string); ok && s != "" { return s @@ -147,6 +193,54 @@ func refusalMessage(raw []byte) string { return string(raw) } +// firstListed pulls the first complaint out of an envelope that carries them +// as a list rather than a sentence, which is how an API that validates every +// property before answering reports what it rejected. +// +// Only the first is read: one refusal heals one field, and the next attempt +// re-reads whatever the API then complains about, so taking them one at a time +// converges without assuming the list is ordered or complete. +func firstListed(m map[string]any) string { + for _, k := range []string{"errors", "messages", "details", "errorMessages", "validationErrors"} { + listed, ok := m[k].([]any) + if !ok { + continue + } + for _, entry := range listed { + switch e := entry.(type) { + case string: + if e != "" { + return e + } + case map[string]any: + // An entry that names the field separately is spelled back + // into the ": " shape the grammar reads. + field, _ := firstString(e, "field", "name", "property", "path", "pointer", "code") + complaint, found := firstString(e, "message", "defaultMessage", "detail", "description", "error", "reason") + if !found { + continue + } + if field != "" { + return field + ": " + complaint + } + return complaint + } + } + } + return "" +} + +// firstString returns the first of the named keys the map carries as a +// non-empty string, and whether it found one. +func firstString(m map[string]any, keys ...string) (string, bool) { + for _, k := range keys { + if s, ok := m[k].(string); ok && s != "" { + return s, true + } + } + return "", false +} + // cleanField strips the trailing punctuation a refusal sentence might carry // after a field name. func cleanField(s string) string { @@ -162,6 +256,17 @@ func cleanField(s string) string { // for a per-enum-value create — so cycling searches the other enum fields for a // body the API accepts without abandoning what the step is exercising. func (r *runner) adjustCreate(ctx context.Context, ent *entityState, rec *entityRecipe, body map[string]any, held string) (adjustResult, error) { + return r.adjustCreateRecording(ctx, ent, rec, body, held, true) +} + +// adjustCreateRecording is adjustCreate, and also says whether the healing it +// does is a fact about the entity. +// +// Re-creating a parent so a child has something to address is a means to an +// end: the fields that create needs are facts about the parent, and the +// parent's own steps record them against the parent. Recorded here they would +// be attributed to whichever child happened to need the parent first. +func (r *runner) adjustCreateRecording(ctx context.Context, ent *entityState, rec *entityRecipe, body map[string]any, held string, record bool) (adjustResult, error) { applied := map[string]bool{} var last *httpResult adjusted := false @@ -177,7 +282,7 @@ func (r *runner) adjustCreate(ctx context.Context, ent *entityState, rec *entity if res == nil || !res.refused() { return adjustResult{res: res, body: body, adjusted: adjusted, gaveUp: true}, nil } - if r.applyAdjustment(ctx, ent, body, res, applied) { + if r.applyAdjustment(ctx, ent, body, res, applied, record) { adjusted = true continue } @@ -204,7 +309,7 @@ func (r *runner) adjustCreate(ctx context.Context, ent *entityState, rec *entity // already present, a remove of a field already absent, a nested "a.b" target // it cannot synthesise, or a borrow that returns the same value all stop the // loop rather than spin it. -func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map[string]any, res *httpResult, applied map[string]bool) bool { +func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map[string]any, res *httpResult, applied map[string]bool, record bool) bool { act := classifyRefusal(res) switch act.kind { case actAdd: @@ -213,7 +318,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } body[act.field] = r.synthField(ent, act.field) applied["a:"+act.field] = true - r.recordAdjustAdd(ent, act.field, act.condGate, act.condVal, res.excerpt) + if record { + r.recordAdjustAdd(ent, act.field, act.condGate, act.condVal, res.excerpt) + } return true case actRequires: if strings.Contains(act.field, ".") || applied["a:"+act.field] || present(body, act.field) { @@ -221,7 +328,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } body[act.field] = r.synthField(ent, act.field) applied["a:"+act.field] = true - r.recordAdjustment(ent, infer.AdjustRequires, act.field, act.trigger, "") + if record { + r.recordAdjustment(ent, infer.AdjustRequires, act.field, act.trigger, "") + } return true case actRemove: if !present(body, act.field) || applied["r:"+act.field] { @@ -229,7 +338,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } delete(body, act.field) applied["r:"+act.field] = true - r.recordAdjustment(ent, infer.AdjustRemove, act.field, act.condGate, act.condVal) + if record { + r.recordAdjustment(ent, infer.AdjustRemove, act.field, act.condGate, act.condVal) + } return true case actBorrow: id, ok := r.borrow(ctx, ent, act.collection) @@ -238,7 +349,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } body[act.field] = id applied["b:"+act.field] = true - r.recordAdjustment(ent, infer.AdjustBorrow, act.field, act.collection, "") + if record { + r.recordAdjustment(ent, infer.AdjustBorrow, act.field, act.collection, "") + } return true default: return false diff --git a/internal/audit/run/adjust_unit_test.go b/internal/audit/run/adjust_unit_test.go index 074a347..0622de9 100644 --- a/internal/audit/run/adjust_unit_test.go +++ b/internal/audit/run/adjust_unit_test.go @@ -63,6 +63,56 @@ func TestUnit_Adjust_ClassifyRefusalGrammar(t *testing.T) { {"plain-text", `field interval is required`, actAdd, "interval", ""}, {"empty", ``, actStop, "", ""}, {"unparseable", `{"weird":true}`, actStop, "", ""}, + + // An envelope that lists its complaints rather than stating one, and + // spells the rejected property as a path into its own request object. + {"listed-strings", + `{"errors":["endpoint.streamEndpointUrl: Endpoint URL cannot be blank"],"httpStatus":"400 BAD_REQUEST"}`, + actAdd, "streamEndpointUrl", ""}, + {"listed-objects", + `{"errors":[{"field":"interval","message":"must not be null"}]}`, + actAdd, "interval", ""}, + {"listed-under-messages", + `{"messages":["interval: is required"]}`, + actAdd, "interval", ""}, + {"listed-empty", `{"errors":[]}`, actStop, "", ""}, + // A field-prefixed complaint about the value that was sent, rather + // than about its absence: adding a value cannot heal it. + {"field-said-not-absence", + `{"errors":["interval: must be one of 60, 120, 300"]}`, + actStop, "", ""}, + // A sentence that merely contains a colon is not a field complaint. + {"prose-with-colon", + `{"detail":"Validation failed: the request was rejected"}`, + actStop, "", ""}, + // Both shapes at once: the sentence only summarises, the list names + // the field, so the list is what the loop must act on. + {"summary-beside-listed", + `{"detail":"There are invalid or missing fields","errors":[{"field":"testName","message":"must not be null"}],"title":"Request validation failed"}`, + actAdd, "testName", ""}, + // A validation framework's own error object, which spells the field + // as a code and the complaint as a default message. + {"listed-code-and-default-message", + `{"errors":[{"code":"name","defaultMessage":"must not be blank"}]}`, + actAdd, "name", ""}, + // A refusal that names its field mid-sentence, wrapped in prose. + {"field-named-in-prose", + `{"title":"There were some errors in your request, please correct them before trying again. Error in field roleName : must not be null."}`, + actAdd, "roleName", ""}, + // The same shape, but complaining about the value rather than its + // absence: adding one cannot heal it. + {"field-named-in-prose-not-absence", + `{"title":"Error in field roleName : must be one of a, b"}`, + actStop, "", ""}, + // Bare English naming only the field it wanted. + {"the-field-is-required", + `{"title":"The loginAccountGroup is required"}`, + actAdd, "loginAccountGroup", ""}, + // "field X is required" still wins over the bare-English reading, so + // the field is X and not the word "field". + {"field-keyword-beats-bare-english", + `{"detail":"the field interval is required"}`, + actAdd, "interval", ""}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -213,3 +263,29 @@ func TestUnit_Borrow_CollectionPaths(t *testing.T) { t.Errorf("collectionPaths(agents) = %v", got) } } + +// TestUnit_Adjust_ParentRecreationHealsWithoutRecording pins the split +// between healing and recording. A parent re-created so a child has something +// to address must still be healed — otherwise every child of an understated +// parent blocks — but the fields it needed are facts about the parent, and +// recording them here would attribute them to the child that asked. +func TestUnit_Adjust_ParentRecreationHealsWithoutRecording(t *testing.T) { + t.Parallel() + + refusal := &httpResult{body: []byte( + `{"detail":"There are invalid or missing fields","errors":[{"field":"testName","message":"must not be null"}]}`)} + + r := &runner{opts: Options{NamePrefix: "tfpfgen"}} + ent := &entityState{plan: &plan.EntityPlan{Entity: "scheduled_test"}} + body := map[string]any{} + + if !r.applyAdjustment(context.Background(), ent, body, refusal, map[string]bool{}, false) { + t.Fatal("a listed field complaint did not heal the body") + } + if _, added := body["testName"]; !added { + t.Errorf("the named field was not added: %#v", body) + } + if len(r.adjustments) != 0 { + t.Errorf("a silent heal recorded %d adjustment(s)", len(r.adjustments)) + } +} diff --git a/internal/audit/run/entity.go b/internal/audit/run/entity.go index d1c4b86..1c291f8 100644 --- a/internal/audit/run/entity.go +++ b/internal/audit/run/entity.go @@ -247,10 +247,14 @@ func (r *runner) resolveCreated(ctx context.Context, ent *entityState, entity st if !ok || rec.minimalBody == nil { return "", blockedError{reason: fmt.Sprintf("no created %s object exists and its entity has no create recipe", entity)} } - obj, _, err := r.createObject(ctx, ent, rec, rec.minimalBody) + // Healed like any other create: a parent the document understates is + // refused the same way its own create was, and without the loop every + // child of it blocks on a refusal the loop can read. + rr, err := r.adjustCreateRecording(ctx, ent, rec, cloneAnyMap(rec.minimalBody), "", false) if err != nil { return "", err } + obj := rr.obj if obj == nil { return "", blockedError{reason: fmt.Sprintf("re-creating the %s parent object was refused", entity)} } diff --git a/internal/audit/run/steps_update.go b/internal/audit/run/steps_update.go index 7459b71..1213293 100644 --- a/internal/audit/run/steps_update.go +++ b/internal/audit/run/steps_update.go @@ -33,7 +33,7 @@ func (r *runner) adjustUpdate(ctx context.Context, ent *entityState, step *plan. if res.ok() || !res.refused() { return res, sent, nil } - if !r.applyAdjustment(ctx, ent, body, res, applied) { + if !r.applyAdjustment(ctx, ent, body, res, applied, true) { return res, sent, nil } }