diff --git a/internal/carrier/decode.go b/internal/carrier/decode.go index 84a7c44..85aa043 100644 --- a/internal/carrier/decode.go +++ b/internal/carrier/decode.go @@ -8,6 +8,7 @@ import ( "io" "unicode/utf8" + "github.com/Judgment-Pack/judgment-pack-runtime/internal/display" "github.com/Judgment-Pack/judgment-pack-runtime/internal/result" ) @@ -45,6 +46,7 @@ type parser struct { decoder *json.Decoder limits Limits nodes int + data []byte } func Decode(data []byte, limits Limits) (any, *Failure) { @@ -53,7 +55,7 @@ func Decode(data []byte, limits Limits) (any, *Failure) { } decoder := json.NewDecoder(bytes.NewReader(data)) decoder.UseNumber() - p := parser{decoder: decoder, limits: limits} + p := parser{decoder: decoder, limits: limits, data: data} value, failure := p.value(nil, 0) if failure != nil { return nil, failure @@ -71,7 +73,7 @@ func (p *parser) value(location []string, depth int) (any, *Failure) { p.nodes++ token, err := p.decoder.Token() if err != nil { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input is not valid complete JSON.") + return nil, p.invalidJSON(Pointer(location), "", err) } switch typed := token.(type) { case json.Delim: @@ -84,7 +86,7 @@ func (p *parser) value(location []string, depth int) (any, *Failure) { case '[': return p.array(location, depth+1) default: - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an unexpected JSON delimiter.") + return nil, p.invalidJSON(Pointer(location), "unexpected JSON delimiter", nil) } case string: if len(typed) > p.limits.MaxStringBytes { @@ -99,7 +101,7 @@ func (p *parser) value(location []string, depth int) (any, *Failure) { case bool, nil: return typed, nil default: - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains a value outside the JSON data model.") + return nil, p.invalidJSON(Pointer(location), "value outside the JSON data model", nil) } } @@ -108,11 +110,11 @@ func (p *parser) object(location []string, depth int) (map[string]any, *Failure) for p.decoder.More() { token, err := p.decoder.Token() if err != nil { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an incomplete JSON object.") + return nil, p.invalidJSON(Pointer(location), "", err) } key, ok := token.(string) if !ok { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an invalid JSON object member.") + return nil, p.invalidJSON(Pointer(location), "object member name is not a string", nil) } memberLocation := appendLocation(location, key) if len(key) > p.limits.MaxStringBytes { @@ -128,7 +130,7 @@ func (p *parser) object(location []string, depth int) (map[string]any, *Failure) value[key] = item } if token, err := p.decoder.Token(); err != nil || token != json.Delim('}') { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an incomplete JSON object.") + return nil, p.invalidJSON(Pointer(location), "expected the end of the object", err) } return value, nil } @@ -144,7 +146,7 @@ func (p *parser) array(location []string, depth int) ([]any, *Failure) { value = append(value, item) } if token, err := p.decoder.Token(); err != nil || token != json.Delim(']') { - return nil, invalid("JPS-CARRIER-INVALID-JSON", Pointer(location), "Input contains an incomplete JSON array.") + return nil, p.invalidJSON(Pointer(location), "expected the end of the array", err) } return value, nil } @@ -182,3 +184,35 @@ func invalid(code, location, message string) *Failure { func resource(code, location, message string) *Failure { return &Failure{Resource: true, Diagnostic: result.ErrorDiagnostic(code, "operation", location, message)} } + +// invalidJSON reports a carrier parse failure with its line, column, and byte +// offset, preserving (and sanitizing) the decoder's own reason when one is +// available so an author can find and fix the exact spot. +func (p *parser) invalidJSON(location, detail string, err error) *Failure { + offset := p.decoder.InputOffset() + line, column := lineColumn(p.data, offset) + message := fmt.Sprintf("Input is not valid JSON at line %d, column %d (byte offset %d)", line, column, offset) + if err != nil { + detail = display.Sanitize(err.Error()) + } + if detail != "" { + message += ": " + detail + } + return invalid("JPS-CARRIER-INVALID-JSON", location, message+".") +} + +func lineColumn(data []byte, offset int64) (int, int) { + if offset > int64(len(data)) { + offset = int64(len(data)) + } + line, column := 1, 1 + for index := int64(0); index < offset; index++ { + if data[index] == '\n' { + line++ + column = 1 + } else { + column++ + } + } + return line, column +} diff --git a/internal/carrier/decode_test.go b/internal/carrier/decode_test.go index 5a21a65..3efd192 100644 --- a/internal/carrier/decode_test.go +++ b/internal/carrier/decode_test.go @@ -1,6 +1,9 @@ package carrier -import "testing" +import ( + "strings" + "testing" +) func TestDuplicateMemberReportsNestedPointer(t *testing.T) { _, failure := Decode([]byte(`{"outer":{"a":1,"\u0061":2}}`), DefaultLimits()) @@ -24,3 +27,19 @@ func TestDepthLimitIsOperational(t *testing.T) { t.Fatalf("unexpected failure: %#v", failure) } } + +func TestMissingCommaReportsLocationAndCause(t *testing.T) { + // A missing comma between members is a parse error; the diagnostic should + // carry a line, column, and byte offset rather than a vague "incomplete + // object" message. + _, failure := Decode([]byte("{\n \"a\": 1\n \"b\": 2\n}"), DefaultLimits()) + if failure == nil || failure.Resource || failure.Diagnostic.Code != "JPS-CARRIER-INVALID-JSON" { + t.Fatalf("expected a carrier invalid-JSON failure: %#v", failure) + } + message := failure.Diagnostic.Message + for _, want := range []string{"line", "column", "byte offset"} { + if !strings.Contains(message, want) { + t.Fatalf("carrier message should include %q: %q", want, message) + } + } +} diff --git a/internal/validation/validator.go b/internal/validation/validator.go index 7bb9898..7970921 100644 --- a/internal/validation/validator.go +++ b/internal/validation/validator.go @@ -332,7 +332,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics handled = true code := patternCode(typed.Want) if code != "JPS-STRUCTURE-EXTENSION-NAME" { - add(code, location, structuralMessage(code)) + add(code, location, patternMessage(code, typed)) } case *kind.MinItems: handled = true @@ -342,7 +342,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics } else if last(location) == "triggers" { code = "JPS-STRUCTURE-ESCALATION-TRIGGERS" } - add(code, location, structuralMessage(code)) + add(code, location, arityMessage(code, typed)) case *kind.Type: handled = true switch { @@ -360,7 +360,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics } case *kind.Enum: handled = true - add("JPS-STRUCTURE-ENUM", location, "Value is not one of the allowed values.") + add("JPS-STRUCTURE-ENUM", location, enumMessage(typed)) case *kind.MinLength: handled = true add("JPS-STRUCTURE-MIN-LENGTH", location, "String is shorter than the required minimum length.") @@ -377,7 +377,7 @@ func collectStructural(validationError *jsonschema.ValidationError, diagnostics relevantCauses = append(relevantCauses, cause) } if len(relevantCauses) == 0 { - add("JPS-STRUCTURE-CONDITION-SHAPE", location, "Condition does not match one declared condition shape.") + add("JPS-STRUCTURE-CONDITION-SHAPE", location, "Condition does not match one declared shape; its op must be one of: literal, all, any, not, fact, evidence-present.") return } for _, cause := range relevantCauses { @@ -444,6 +444,63 @@ func structuralMessage(code string) string { } } +// enumMessage lists the allowed values so an author does not have to open the +// schema to learn them. +func enumMessage(typed *kind.Enum) string { + allowed := make([]string, 0, len(typed.Want)) + for _, value := range typed.Want { + allowed = append(allowed, stringifyValue(value)) + } + if len(allowed) == 0 { + return "Value is not one of the allowed values." + } + return "Value is not one of the allowed values: " + joinCapped(allowed, 12) + "." +} + +func stringifyValue(value any) string { + if text, ok := value.(string); ok { + return text + } + return fmt.Sprint(value) +} + +// patternMessage names the offending value and gives a concrete example rather +// than dumping the grammar regex, for each pattern-constrained member. +func patternMessage(code string, typed *kind.Pattern) string { + switch code { + case "JPS-STRUCTURE-LOCAL-ID": + return fmt.Sprintf("Local identifier %q has an invalid shape; use lowercase words joined by single hyphens, for example %q.", typed.Got, "needs-review") + case "JPS-STRUCTURE-PACK-VERSION": + return fmt.Sprintf("Pack version %q has an invalid shape; use MAJOR.MINOR.PATCH, for example %q.", typed.Got, "1.0.0") + case "JPS-STRUCTURE-FACT-PATH": + return fmt.Sprintf("Fact path %q is not a valid JSON Pointer; it must be empty or begin with %q, for example %q.", typed.Got, "/", "/amount") + case "JPS-STRUCTURE-DECIMAL-OPERAND": + return fmt.Sprintf("Ordered comparison operand %q must be a decimal string, for example %q.", typed.Got, "5000") + default: + return fmt.Sprintf("Value %q does not satisfy the required pattern.", typed.Got) + } +} + +// arityMessage states the required minimum and the actual count for a +// too-short collection. +func arityMessage(code string, typed *kind.MinItems) string { + switch code { + case "JPS-STRUCTURE-CONDITION-ARITY": + return fmt.Sprintf("Condition needs at least %d child condition%s but has %d.", typed.Want, plural(typed.Want), typed.Got) + case "JPS-STRUCTURE-ESCALATION-TRIGGERS": + return fmt.Sprintf("Escalation needs at least %d trigger%s but has %d.", typed.Want, plural(typed.Want), typed.Got) + default: + return fmt.Sprintf("Collection needs at least %d item%s but has %d.", typed.Want, plural(typed.Want), typed.Got) + } +} + +func plural(count int) string { + if count == 1 { + return "" + } + return "s" +} + func last(values []string) string { if len(values) == 0 { return "" @@ -498,12 +555,19 @@ func typeMessage(typed *kind.Type) string { // more than one invalid child collapses to a single generic shape error and // loses the specific, well-located diagnostics for each child. func branchRejectedByDiscriminator(cause *jsonschema.ValidationError) bool { - if _, isGroup := cause.ErrorKind.(*kind.Group); !isGroup { - return false + // A losing branch that failed on its op discriminator directly, as a single + // bare failure rather than a group -- otherwise that lone op-const/enum + // failure leaks out as a diagnostic anchored at an op that is actually valid. + if discriminatorMismatch(cause) { + return true } - for _, sub := range cause.Causes { - if discriminatorMismatch(sub) { - return true + // A losing branch whose failures were grouped, one of which is the op + // discriminator. + if _, isGroup := cause.ErrorKind.(*kind.Group); isGroup { + for _, sub := range cause.Causes { + if discriminatorMismatch(sub) { + return true + } } } return false diff --git a/internal/validation/validator_test.go b/internal/validation/validator_test.go index 3032d53..a06b65a 100644 --- a/internal/validation/validator_test.go +++ b/internal/validation/validator_test.go @@ -430,3 +430,108 @@ func TestNestedOperandErrorsAreNotMaskedByCompositeShape(t *testing.T) { t.Fatalf("composite should not report a generic shape error when its children have specific errors: %#v", actual.Diagnostics) } } + +func TestEnumDiagnosticListsAllowedValues(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["rules"].([]any)[0].(map[string]any)["onUnknown"] = "bogus-mode" + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-ENUM", "/rules/0/onUnknown") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected an enum diagnostic at /rules/0/onUnknown: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "escalate") { + t.Fatalf("enum message should list the allowed values: %q", message) + } +} + +func TestLocalIDDiagnosticNamesValueAndExample(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["outcomes"].([]any)[0].(map[string]any)["id"] = "Bad Id" + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-LOCAL-ID", "/outcomes/0/id") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected a local-id diagnostic at /outcomes/0/id: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "Bad Id") || !strings.Contains(message, "needs-review") { + t.Fatalf("local-id message should name the offending value and give an example: %q", message) + } +} + +func TestArityDiagnosticStatesMinimumAndActual(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + document := validDocument(t) + document["outcomes"] = document["outcomes"].([]any)[:1] // one outcome; the minimum is two + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-COLLECTION-ARITY", "/outcomes") + if actual.Status != "invalid" || !ok { + t.Fatalf("expected a collection-arity diagnostic at /outcomes: %#v", actual.Diagnostics) + } + if !strings.Contains(message, "at least 2") || !strings.Contains(message, "has 1") { + t.Fatalf("arity message should state the required minimum and the actual count: %q", message) + } +} + +func TestBadConditionOpIsConsistentAndNamesValidOps(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + // The same mistake -- an op that selects no condition branch -- must report + // the same code and location regardless of sibling members, list the valid + // ops, and never spuriously flag the op member itself. + for _, when := range []map[string]any{ + {"op": "maybe"}, + {"op": "maybe", "value": true}, + {"op": "maybe", "conditions": []any{map[string]any{"op": "literal", "value": true}}}, + } { + document := validDocument(t) + document["rules"].([]any)[0].(map[string]any)["when"] = when + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + message, ok := diagnosticMessage(actual.Diagnostics, "JPS-STRUCTURE-CONDITION-SHAPE", "/rules/0/when") + if actual.Status != "invalid" || !ok { + t.Fatalf("bad op %v should report CONDITION-SHAPE at /rules/0/when: %#v", when, actual.Diagnostics) + } + if !strings.Contains(message, "fact") || !strings.Contains(message, "evidence-present") { + t.Fatalf("condition-shape message should list the valid op values: %q", message) + } + if hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-SCHEMA", "/rules/0/when/op") { + t.Fatalf("a bad op must not spuriously flag the op member: %#v", actual.Diagnostics) + } + } +} + +func TestValidConditionOpIsNotSpuriouslyFlagged(t *testing.T) { + engine, err := NewEngine() + if err != nil { + t.Fatal(err) + } + // op "all" is valid, but the object is shaped like a "not" (has "condition"). + // The losing branch's op-const failure must not leak onto the valid op; the + // object's real errors must still surface. + document := validDocument(t) + document["rules"].([]any)[0].(map[string]any)["when"] = map[string]any{ + "op": "all", + "condition": map[string]any{"op": "literal", "value": true}, + } + actual := validateDocument(t, engine, document, Options{Through: "structural", Limits: carrier.DefaultLimits()}) + if actual.Status != "invalid" { + t.Fatalf("expected invalid: %#v", actual) + } + if hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-SCHEMA", "/rules/0/when/op") { + t.Fatalf("a valid op must not be flagged: %#v", actual.Diagnostics) + } + if !hasDiagnostic(actual.Diagnostics, "JPS-STRUCTURE-REQUIRED-MEMBER", "/rules/0/when/conditions") { + t.Fatalf("the matched branch's real errors should surface: %#v", actual.Diagnostics) + } +}