diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index bebe2675..97422dfd 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -121,7 +121,7 @@ - _Depends: 5.1_ - [ ] 6. API server: array submission -- [ ] 6.1 (P) Parse submit index expressions +- [x] 6.1 (P) Parse submit index expressions - Accept only `name[1-N]` and `name[1]`; reject interactive arrays, malformed expressions or invalid names (quoting the name), lists and steps, `%N`, start 0, reversed ranges, start above 1 and sizes above the limit, each with its specific message, reporting only the lowest-numbered broken rule - Saturate oversized numbers so the rule order still decides; accept leading zeros - Done when the index-expression unit tests pass for every rule, the precedence rows (`[0-0:2]`, `[99999999999999999999]`, `[99999999999999999999-5]`, `[0-99999999999999999999]`), leading zeros, `[1]` vs `[1-1]` vs `[5]` and the limit edge @@ -245,3 +245,4 @@ - 4.3: the controller acks `setpend` (`do_set_preason`, job.c ~3869-3880) and `jobrun` (`handle_jobdispatch`, job.c ~2246-2258) with status 0 before doing the work, so those two harness commands can never report not-found; 4.7/5.13 for them depend on a controller change outside this spec's boundary. `jobinfo`'s not-found is `rc == 0` with no row (`do_reply_joblist`), not -ENOENT. - 5.1: Go helpers: `ParseHumanJobIDStrict(s) (uint64, error)` returns sentinel `ErrJobIDSyntax` / `ErrJobIDWidth` (callers trim and split `[...]` first; no whitespace accepted); `MakeElementID(key uint64, k int) uint64`; `IsArrayCapableKey(key)`; `GenerateArrayJobID(c, a, n)` (non-cgo stub returns errNoCGO). - 5.4: handler fixture in `controllers/jobs_handler_test.go` (`newHandlerFixture`, `seedJob`/`seedOrdinaryJob`/`seedArrayElements`/`seedQueue`, `submit`, `controlByID`, `controlByFilter`, `dispatchedIDs`, `submissions`); submit bodies must be client-style JSON maps (marshalled `models.JobSubmitRequest` breaks on zero CustomTime). Handlers are registered without `RequirePermission`; in-handler RBAC/queue/status checks still run. +- 6.1: the design's "saturate oversized numbers" rule alone loses the order of two oversized numbers; `ParseSubmitIndexExpr` compares leading-zero-stripped digit strings for 2.6. Use the same approach for the selection parsers' reversed-range check (7.1, 8.1) if they saturate. Rule tags are strings "2.1".."2.8" on `*ArrayExprError`. diff --git a/src/frontend/apiserver/controllers/jobarray.go b/src/frontend/apiserver/controllers/jobarray.go new file mode 100644 index 00000000..01047ed5 --- /dev/null +++ b/src/frontend/apiserver/controllers/jobarray.go @@ -0,0 +1,204 @@ +package controllers + +import "fmt" + +// ArrayExprError is a rejection of a submitted job-array index expression. +// Rule names the requirement ("2.1" .. "2.8") that the expression breaks; +// Msg is the user-facing message. +type ArrayExprError struct { + Rule string + Msg string +} + +func (e *ArrayExprError) Error() string { return e.Msg } + +// maxArrayNameLen is the longest array name the array-name rule accepts. +const maxArrayNameLen = 4094 + +// exprNumSaturate is the value an index-expression number saturates at. It is +// far above any array size limit, so a saturated number is never mistaken for +// one in range, and the rule order (2.5-2.8) still decides which rule to report. +// It fits in a 32-bit int, as does every intermediate value of readExprNum. +// Saturated values lose their order, so order comparisons (2.6) use the digit +// strings instead (see digitsLess). +const exprNumSaturate = 1 << 30 + +type exprItem struct { + first, last int + firstDigits, lastDigits string // digits with leading zeros stripped + hasStep bool +} + +// digitsLess reports whether the decimal number a is less than b. Both are +// digit strings with leading zeros stripped, so a shorter string is smaller +// and equal-length strings compare lexicographically. +func digitsLess(a, b string) bool { + if len(a) != len(b) { + return len(a) < len(b) + } + return a < b +} + +func isArrayNameChar(c byte) bool { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-' +} + +func isValidSubmitArrayName(s string) bool { + if len(s) < 1 || len(s) > maxArrayNameLen { + return false + } + for i := 0; i < len(s); i++ { + if !isArrayNameChar(s[i]) { + return false + } + } + return true +} + +// readExprNum reads one or more decimal digits at s[pos:]. v saturates at +// exprNumSaturate; digits is the full digit run with leading zeros stripped +// (empty for zero), which keeps the exact value for order comparisons. +// ok is false when there is no digit at pos. +func readExprNum(s string, pos int) (v int, digits string, next int, ok bool) { + next = pos + for next < len(s) && s[next] >= '0' && s[next] <= '9' { + if v > exprNumSaturate/10 { + v = exprNumSaturate + } else { + v = v*10 + int(s[next]-'0') + if v > exprNumSaturate { + v = exprNumSaturate + } + } + next++ + } + start := pos + for start < next && s[start] == '0' { + start++ + } + return v, s[start:next], next, next > pos +} + +// parseExprBody parses the part of an index expression after the name, +// starting at the '['. It returns the items and whether a "%N" suffix was +// present; ok is false when the text does not follow the grammar of 2.2. +func parseExprBody(s string) (items []exprItem, hasLimit bool, ok bool) { + pos := 1 // past '[' + for { + var it exprItem + var good bool + it.first, it.firstDigits, pos, good = readExprNum(s, pos) + if !good { + return nil, false, false + } + it.last, it.lastDigits = it.first, it.firstDigits + if pos < len(s) && s[pos] == '-' { + it.last, it.lastDigits, pos, good = readExprNum(s, pos+1) + if !good { + return nil, false, false + } + if pos < len(s) && s[pos] == ':' { + if _, _, pos, good = readExprNum(s, pos+1); !good { + return nil, false, false + } + it.hasStep = true + } + } + items = append(items, it) + if pos < len(s) && s[pos] == ',' { + pos++ + continue + } + break + } + if pos >= len(s) || s[pos] != ']' { + return nil, false, false + } + pos++ + if pos < len(s) && s[pos] == '%' { + var good bool + if _, _, pos, good = readExprNum(s, pos+1); !good { + return nil, false, false + } + hasLimit = true + } + if pos != len(s) { + return nil, false, false + } + return items, hasLimit, true +} + +func exprErr(rule, format string, args ...any) (string, int, bool, *ArrayExprError) { + return "", 0, false, &ArrayExprError{Rule: rule, Msg: fmt.Sprintf(format, args...)} +} + +// ParseSubmitIndexExpr checks a submitted job name that contains '[' or ']' +// (names without them keep the existing createArrayName path, 1.4). Only +// name[1-N] (1 <= N <= limit) and name[1] are accepted; leading zeros are +// allowed (1.5). The rules are checked in requirement order 2.1 .. 2.8 and the +// first failure is returned (2.9). On success it returns the array name, the +// element count and isArray == true. +func ParseSubmitIndexExpr(name string, interactive bool, limit int) ( + arrayName string, n int, isArray bool, err *ArrayExprError) { + // 2.1 + if interactive { + return exprErr("2.1", "job name '%s': interactive jobs cannot be arrays", name) + } + + // 2.2: the name rule applies to the part before the first '[' + // (the whole name if it has no '['). + open := -1 + for i := 0; i < len(name); i++ { + if name[i] == '[' { + open = i + break + } + } + prefix := name + if open >= 0 { + prefix = name[:open] + } + if !isValidSubmitArrayName(prefix) { + return exprErr("2.2", + "job name '%s': the array name is invalid; it must be 1 to %d characters from [a-zA-Z0-9._-]", + name, maxArrayNameLen) + } + var items []exprItem + var hasLimit, ok bool + if open >= 0 { + items, hasLimit, ok = parseExprBody(name[open:]) + } + if !ok { + return exprErr("2.2", + "job name '%s': the array index expression is malformed; expected name[1-N] or name[1]", name) + } + + // 2.3 + if len(items) > 1 || items[0].hasStep { + return exprErr("2.3", + "job name '%s': only a single range name[1-N] is supported (lists and steps are not)", name) + } + // 2.4 + if hasLimit { + return exprErr("2.4", "job name '%s': running limits are not supported (%%N suffix)", name) + } + first, last := items[0].first, items[0].last + // 2.5 + if first == 0 { + return exprErr("2.5", "job name '%s': element numbers start at 1", name) + } + // 2.6 (compared on the digit strings so oversized numbers keep their order) + if digitsLess(items[0].lastDigits, items[0].firstDigits) { + return exprErr("2.6", "job name '%s': the range is reversed (last element number is less than the first)", name) + } + // 2.7 + if first > 1 { + return exprErr("2.7", "job name '%s': only name[1-N] and name[1] are currently supported", name) + } + // 2.8 (a saturated number is always above the limit) + if last > limit || last >= exprNumSaturate { + return exprErr("2.8", "job name '%s': the array size exceeds the array size limit of %d", name, limit) + } + return prefix, last, true, nil +} diff --git a/src/frontend/apiserver/controllers/jobarray_test.go b/src/frontend/apiserver/controllers/jobarray_test.go new file mode 100644 index 00000000..6670614a --- /dev/null +++ b/src/frontend/apiserver/controllers/jobarray_test.go @@ -0,0 +1,153 @@ +package controllers + +import ( + "strings" + "testing" +) + +// TestParseSubmitIndexExpr covers requirements 1.5 and 2.1-2.9 for the +// submit-time index expression parser (design: ApiSubmitExpr). +func TestParseSubmitIndexExpr(t *testing.T) { + longName := strings.Repeat("a", 4094) + tooLongName := strings.Repeat("a", 4095) + + type want struct { + arrayName string + n int + rule string // "" means success + msgHas []string // substrings the error message must contain + } + cases := []struct { + name string + input string + interactive bool + limit int + want want + }{ + // --- success (1.1, 1.2) --- + {"range 1-N", "arr[1-3]", false, 1000, want{arrayName: "arr", n: 3}}, + {"single 1", "arr[1]", false, 1000, want{arrayName: "arr", n: 1}}, + {"range 1-1", "arr[1-1]", false, 1000, want{arrayName: "arr", n: 1}}, + {"name charset", "a.B_c-9[1-2]", false, 1000, want{arrayName: "a.B_c-9", n: 2}}, + {"name 4094 chars", longName + "[1-2]", false, 1000, want{arrayName: longName, n: 2}}, + // --- leading zeros (1.5) --- + {"leading zeros range", "arr[001-0003]", false, 1000, want{arrayName: "arr", n: 3}}, + {"leading zeros single", "arr[01]", false, 1000, want{arrayName: "arr", n: 1}}, + {"leading zeros limit", "arr[1-0001000]", false, 1000, want{arrayName: "arr", n: 1000}}, + {"leading zeros zero", "arr[00-3]", false, 1000, want{rule: "2.5"}}, + // --- limit edge (2.8) --- + {"limit exact", "arr[1-1000]", false, 1000, want{arrayName: "arr", n: 1000}}, + {"limit plus one", "arr[1-1001]", false, 1000, want{rule: "2.8", msgHas: []string{"1000"}}}, + {"limit 32767 exact", "arr[1-32767]", false, 32767, want{arrayName: "arr", n: 32767}}, + {"limit 32767 plus one", "arr[1-32768]", false, 32767, want{rule: "2.8", msgHas: []string{"32767"}}}, + {"limit huge", "arr[1-99999999999999999999]", false, 1000, want{rule: "2.8", msgHas: []string{"1000"}}}, + {"limit custom", "arr[1-6]", false, 5, want{rule: "2.8", msgHas: []string{"5"}}}, + // --- 2.1 interactive --- + {"interactive array", "arr[1-3]", true, 1000, want{rule: "2.1", msgHas: []string{"interactive jobs cannot be arrays"}}}, + {"interactive malformed", "arr[", true, 1000, want{rule: "2.1", msgHas: []string{"interactive jobs cannot be arrays"}}}, + {"interactive bad name", "a b]", true, 1000, want{rule: "2.1"}}, + // --- 2.2 invalid name --- + {"empty name", "[1-3]", false, 1000, want{rule: "2.2", msgHas: []string{"'[1-3]'", "array name is invalid"}}}, + {"bad char in name", "a*b[1-3]", false, 1000, want{rule: "2.2", msgHas: []string{"'a*b[1-3]'", "array name is invalid"}}}, + {"space in name", "a b[1-3]", false, 1000, want{rule: "2.2", msgHas: []string{"array name is invalid"}}}, + {"leading space", " arr[1-3]", false, 1000, want{rule: "2.2", msgHas: []string{"array name is invalid"}}}, + {"name too long", tooLongName + "[1-2]", false, 1000, want{rule: "2.2", msgHas: []string{"array name is invalid"}}}, + {"only close bracket bad name", "a*b]", false, 1000, want{rule: "2.2", msgHas: []string{"array name is invalid"}}}, + {"bad name beats list", "a/b[1,2]", false, 1000, want{rule: "2.2", msgHas: []string{"array name is invalid"}}}, + // --- 2.2 malformed --- + {"only close bracket", "arr]", false, 1000, want{rule: "2.2", msgHas: []string{"'arr]'", "array name is invalid"}}}, + {"unterminated", "arr[1-3", false, 1000, want{rule: "2.2", msgHas: []string{"'arr[1-3'", "malformed"}}}, + {"empty brackets", "arr[]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"trailing text", "arr[1-3]x", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"trailing space", "arr[1-3] ", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"inner space", "arr[1 -3]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"sign", "arr[+1-3]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"negative", "arr[-1]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"empty item", "arr[1,,2]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"trailing comma", "arr[1,]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"missing end", "arr[1-]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"missing step", "arr[1-3:]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"step without range", "arr[1:2]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"pct without number", "arr[1-3]%", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"pct trailing", "arr[1-3]%2x", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"two brackets", "arr[1][2]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"nested bracket", "arr[[1]]", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"malformed beats list", "arr[1,2", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + {"malformed beats zero", "arr[0", false, 1000, want{rule: "2.2", msgHas: []string{"malformed"}}}, + // --- 2.3 lists / steps --- + {"list", "arr[1,2]", false, 1000, want{rule: "2.3", msgHas: []string{"only a single range name[1-N] is supported"}}}, + {"list of ranges", "arr[1-3,5-6]", false, 1000, want{rule: "2.3", msgHas: []string{"only a single range name[1-N] is supported"}}}, + {"step", "arr[1-10:2]", false, 1000, want{rule: "2.3", msgHas: []string{"only a single range name[1-N] is supported"}}}, + {"precedence 0-0:2", "arr[0-0:2]", false, 1000, want{rule: "2.3"}}, + {"list beats pct", "arr[1,2]%3", false, 1000, want{rule: "2.3"}}, + // --- 2.4 running limit --- + {"pct", "arr[1-3]%2", false, 1000, want{rule: "2.4", msgHas: []string{"running limits are not supported"}}}, + {"pct beats zero", "arr[0-3]%2", false, 1000, want{rule: "2.4"}}, + // --- 2.5 start 0 --- + {"start zero", "arr[0-3]", false, 1000, want{rule: "2.5", msgHas: []string{"element numbers start at 1"}}}, + {"single zero", "arr[0]", false, 1000, want{rule: "2.5"}}, + {"zero beats reversed", "arr[0-0]", false, 1000, want{rule: "2.5"}}, + {"precedence 0-huge", "arr[0-99999999999999999999]", false, 1000, want{rule: "2.5"}}, + // --- 2.6 reversed --- + {"reversed", "arr[3-1]", false, 1000, want{rule: "2.6", msgHas: []string{"reversed"}}}, + {"precedence huge-5", "arr[99999999999999999999-5]", false, 1000, want{rule: "2.6"}}, + {"reversed beats limit", "arr[5000-2000]", false, 1000, want{rule: "2.6"}}, + // --- 2.7 start > 1 --- + {"single 5", "arr[5]", false, 1000, want{rule: "2.7", msgHas: []string{"only name[1-N] and name[1] are currently supported"}}}, + {"start above 1", "arr[1001-2000]", false, 1000, want{rule: "2.7", msgHas: []string{"only name[1-N] and name[1] are currently supported"}}}, + {"start 2", "arr[2-3]", false, 1000, want{rule: "2.7"}}, + {"precedence huge single", "arr[99999999999999999999]", false, 1000, want{rule: "2.7"}}, + {"huge range equal", "arr[99999999999999999999-99999999999999999999]", false, 1000, want{rule: "2.7"}}, + {"huge reversed two oversized", "arr[99999999999999999999-99999999999999999998]", false, 1000, want{rule: "2.6"}}, + {"huge reversed just above 2^40", "arr[1099511627777-1099511627776]", false, 1000, want{rule: "2.6"}}, + {"huge ascending two oversized", "arr[99999999999999999998-99999999999999999999]", false, 1000, want{rule: "2.7"}}, + {"huge equal after leading zeros", "arr[0099999999999999999999-99999999999999999999]", false, 1000, want{rule: "2.7"}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + arrayName, n, isArray, err := ParseSubmitIndexExpr(tc.input, tc.interactive, tc.limit) + if tc.want.rule == "" { + if err != nil { + t.Fatalf("ParseSubmitIndexExpr(%q) unexpected error: rule %s: %s", tc.input, err.Rule, err.Msg) + } + if !isArray || arrayName != tc.want.arrayName || n != tc.want.n { + t.Fatalf("ParseSubmitIndexExpr(%q) = (%q, %d, %v), want (%q, %d, true)", + tc.input, arrayName, n, isArray, tc.want.arrayName, tc.want.n) + } + return + } + if err == nil { + t.Fatalf("ParseSubmitIndexExpr(%q) = (%q, %d, %v, nil), want rule %s", + tc.input, arrayName, n, isArray, tc.want.rule) + } + if err.Rule != tc.want.rule { + t.Fatalf("ParseSubmitIndexExpr(%q) rule = %s (%s), want %s", tc.input, err.Rule, err.Msg, tc.want.rule) + } + if isArray || n != 0 || arrayName != "" { + t.Fatalf("ParseSubmitIndexExpr(%q) returned (%q, %d, %v) alongside an error", tc.input, arrayName, n, isArray) + } + if err.Error() != err.Msg { + t.Fatalf("Error() = %q, want Msg %q", err.Error(), err.Msg) + } + for _, s := range tc.want.msgHas { + if !strings.Contains(err.Msg, s) { + t.Fatalf("ParseSubmitIndexExpr(%q) message %q does not contain %q", tc.input, err.Msg, s) + } + } + }) + } +} + +// TestParseSubmitIndexExprErrorAsError checks a non-nil *ArrayExprError +// satisfies the error interface (used by preCheckJobRequest / errors.As). +func TestParseSubmitIndexExprErrorAsError(t *testing.T) { + _, _, _, ae := ParseSubmitIndexExpr("arr[0]", false, 1000) + if ae == nil { + t.Fatal("expected an error for arr[0]") + } + var e error = ae + if !strings.Contains(e.Error(), "element numbers start at 1") { + t.Fatalf("Error() = %q", e.Error()) + } +}