From 3457d892b68c4f012819537df6258582d1507df4 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 1 Oct 2026 12:21:11 +0000 Subject: [PATCH] feat(job-array-one-based-index): query handlers evaluate element selections (task 7.3) GetJobsInfo validates every filter entry in request order before any store access (including the exec_hosts lookup) and GetJobByIDs every key before querying; the first rejection returns 400 with the parser's message. Bracketed job_id/job_name entries are evaluated as row predicates before sorting and pagination, so job_id:eq:X[2] lists only element 2, GET /jobs/X[1-3] lists elements 1-3 and [0] lists nothing. The unevaluated compound conditions and the old index parsing are gone from both handlers. Fixes bug-1898 on the query paths. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../specs/job-array-one-based-index/tasks.md | 3 +- src/frontend/apiserver/controllers/jobs.go | 489 +++++------------- .../controllers/jobs_handler_test.go | 242 +++++++++ 3 files changed, 364 insertions(+), 370 deletions(-) diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index cb98cc04..f9bb1fcc 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -155,7 +155,7 @@ - _Requirements: 5.2, 5.6, 5.11, 7.8, 8.2_ - _Depends: 5.3, 7.1_ -- [ ] 7.3 Route query handlers through selections (fixes bug-1898) +- [x] 7.3 Route query handlers through selections (fixes bug-1898) - Validate every filter entry and key before any store access (a pre-pass ahead of the existing filter loop, including before host resolution), returning the first rejection as 400 with its specific message - Apply width and marked-decimal checks to identity-operator keys and converted entries, and the alignment check only to keys with an element part - Evaluate selections through the predicates on the job listing and job-detail paths; remove the unevaluated compound conditions and the old index-section parsing on these paths (the filter-based control path follows in 7.5) @@ -248,3 +248,4 @@ - 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`. - 7.1: `ParseNameSelection` on a value without `[`/`]` returns `SelBare` with `Name` = the raw untrimmed value (`Raw` is trimmed); 7.2 must match bare names on `Name` and must not treat a bare result as validated. Rule tags on `*SelError` are "5.16", "5.14", "5.17", "5.8", "5.7", "5.5"; decimal keys above uint64 are 5.14. - 7.2: `BuildSelectionPredicates(filters []utils.FilterCondition) (plain []utils.FilterCondition, preds []func(utils.OrderMap) bool, err error)` in `controllers/jobmatch.go`; parser rejections come back as `*SelError`, other rejections with today's texts. Accepted consequence of the one-rule decision: bare `job_id` values under identity operators now go through `ParseIdentitySelection`, so `job_id:eq:+5` / `-1` (previously accepted) are rejected under 5.16. For 7.3: keep the `array_stats` bracket rejection before the builder, and run the builder in the validate-all-first pass before the `exec_hosts` store lookup. +- 7.3: query pre-pass is `validateJobQueryFilters`; `returnSelectionError` maps `*SelError` → 400 with its message. `RestartJobByID` (jobs.go ~:863) still builds unevaluated `CompFilterCondition`s — restart is out of scope, so bug-1898 stays partial for that path. Pre-existing: GetJobsInfo `page_token` does not advance (logged separately). diff --git a/src/frontend/apiserver/controllers/jobs.go b/src/frontend/apiserver/controllers/jobs.go index dd303623..8785bfaf 100644 --- a/src/frontend/apiserver/controllers/jobs.go +++ b/src/frontend/apiserver/controllers/jobs.go @@ -133,6 +133,17 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { }) } + // ---- validate every filter entry before any store access ---- + // (validate-all-first, job-array-one-based-index): the filter loop below + // can query the store (exec_hosts) and return early with count 0, so + // every entry -- including every job selection -- is checked here first + // and the first rejection, in request order, rejects the whole query. + // Entries with a bracketed job_id/job_name value become row predicates. + filters, selPreds, ok := validateJobQueryFilters(ctx, filters, typeValue) + if !ok { + return + } + permission, err := GetRequireResourcePermission(submitUser, "queue:view") if err != nil { ReturnError(ctx, common.ERROR_CODE, common.ERROR_ACCESS_FORBIDDEN_MSG, nil) @@ -182,31 +193,7 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { }) } - // job_id:eq:101, [array_idx:btw:(1,3), array_idx:btw:(6,8) ... ] - compfilters := make(utils.CompFilterCondition) - - //filtercds := make([]utils.Condition, len(filters)) - //subFilters is used to store internal filter extract from job_id:in:(101[1-2, 6-10], 102[1-2, 6-10]) - //will be placed in a OR tree. - for i, filter := range filters { - // check the filter is supported. - if typeValue == "array_stats" { - if !utils.ContainsField(models.ARRAY_JOB_SUPPORT_FILTERS, filter.Field) { - err := errors.New("unsupported filter: " + filter.Field) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } else { - if !utils.ContainsField(models.JOB_SUPPORT_FILTERS, filter.Field) { - err := errors.New("unsupported filter: " + filter.Field) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } - // check whether exist queue_name for rbac if filter.Field == "queue_name" { if accessQueues != nil { @@ -223,220 +210,6 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { } } - // special handle job_name[index] cases, here index can be [1-3], [1,3,5],[1-3,5-7] situations - if filters[i].Field == "job_name" { - filtCdArrays := make([]utils.FilterConditionArray, 0) - jobNames := extractJobNames(filters[i].Value) - for i := range jobNames { - jobNames[i] = strings.TrimSpace(jobNames[i]) - if typeValue == "array_stats" { - if strings.Contains(jobNames[i], "[") || strings.Contains(jobNames[i], "]") { - err := errors.New("job_name with array index is not supported in statistics type") - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } - } - logger.Debug("After extractJobNames jobNames is", zap.Any("jobNames", jobNames)) - - // only "in", "between", "notin" support multiple jobnames (include array job name) - if !(filters[i].Operator == "in" || filters[i].Operator == "between" || filters[i].Operator == "notin") && len(jobNames) > 1 { - err := fmt.Errorf("operator doesn't allow to use multiple values: %v", jobNames) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - if !(strings.Contains(filters[i].Value, "[") && strings.Contains(filters[i].Value, "]")) { - continue - } - - if strings.Contains(filters[i].Value, "[") || strings.Contains(filters[i].Value, "]") { - if filters[i].Operator == "ne" { - err := fmt.Errorf("operator doesn't support %v", jobNames) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } - - for _, jobname := range jobNames { - subORFilters := make([]utils.FilterCondition, 0) - jobnameArry, err := parseArrayName(jobname) - logger.Debug("After parseArrayName jobname is ", zap.Any("jobname", jobnameArry)) - - if err != nil { - logger.Error("job name format is invalid", zap.Error(err)) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - - if err = isValidArrayName(jobnameArry.KeyID); err != nil { - err := errors.New("bad jobname: " + jobnameArry.KeyID) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - - var operator string - if filters[i].Operator == "in" { - operator = "eq" - } else { - operator = filters[i].Operator - } - // treat every section as an OR condition - mainFiltCond := utils.FilterCondition{ - Field: filters[i].Field, - Operator: operator, - Value: jobnameArry.KeyID, - } - if jobnameArry.Indices != nil { - - // array jobid: job_id:in:(101[1-3, 5-6]) - // handle every section in Indices list. - for _, jidxsec := range jobnameArry.Indices { - st, ed, err := parseArrayIndexSection(jidxsec.Index) - if err != nil { - err := errors.New("bad job array index: " + jidxsec.Index) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - if jidxsec.Step == 1 { - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "between", - Value: "(" + strconv.Itoa(st) + "," + strconv.Itoa(ed) + ")", - }) - } else { // handle step > 1 - idxList := make([]string, 0) - for idx := st; idx <= ed; idx += jidxsec.Step { - idxList = append(idxList, strconv.Itoa(idx)) - } - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "in", - Value: "(" + strings.Join(idxList, ",") + ")", - }) - } - } - } - filtCdArrays = append(filtCdArrays, utils.FilterConditionArray{ - MainFiltCond: mainFiltCond, - SubFiltConds: subORFilters, - }) - } - compfilters["job_name"] = filtCdArrays - } - - if filters[i].Field == "job_id" { - filtCdArrays := make([]utils.FilterConditionArray, 0) - jobids := extractJobNames(filters[i].Value) - for i := range jobids { - jobids[i] = strings.TrimSpace(jobids[i]) - if typeValue == "array_stats" { - if strings.Contains(jobids[i], "[") || strings.Contains(jobids[i], "]") { - err := errors.New("job_id with array index is not supported in statistics type") - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } - } - logger.Debug("After extractJobNames jobids is", zap.Any("jobids", jobids)) - - if !(filters[i].Operator == "in" || filters[i].Operator == "between" || filters[i].Operator == "notin") && len(jobids) > 1 { - err := fmt.Errorf("operator doesn't allow to use multiple values: %v", jobids) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - if !(strings.Contains(filters[i].Value, "[") && strings.Contains(filters[i].Value, "]")) { - continue - } - if strings.Contains(filters[i].Value, "[") || strings.Contains(filters[i].Value, "]") { - if filters[i].Operator == "ne" { - err := fmt.Errorf("operator doesn't support %v", jobids) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } - - for _, jobid := range jobids { - subORFilters := make([]utils.FilterCondition, 0) - jobidArry, err := parseArrayName(jobid) - logger.Debug("After parseArrayName jobid is ", zap.Any("jobid", jobidArry)) - - if err != nil { - logger.Error("job id format is invalid", zap.Error(err)) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - - if !utils.IsNumeric(jobidArry.KeyID) { - err := errors.New("bad jobid: " + jobidArry.KeyID) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - var operator string - if filters[i].Operator == "in" { - operator = "eq" - } else { - // for other operator, only one job is supported. - // means job_id:eq:100[1-3,6-10]. - // if job_id:eq:(100[1-3],101[1-2]) should be rejected - // TODO: need check if already handle this case ??? - operator = filters[i].Operator - } - - // single jobid: job_id:in:(100, 101[1-3, 5-6]) - // here, handle the job_id = 100 - mainFiltCond := utils.FilterCondition{ - Field: filters[i].Field, - Operator: operator, - Value: jobidArry.KeyID, - } - // treat every section as an OR condition - if jobidArry.Indices != nil { - // handle every section in Indices list. - for _, jidxsec := range jobidArry.Indices { - st, ed, err := parseArrayIndexSection(jidxsec.Index) - if err != nil { - err := errors.New("bad job array index: " + jidxsec.Index) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - - if jidxsec.Step == 1 { - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "between", - Value: "(" + strconv.Itoa(st) + "," + strconv.Itoa(ed) + ")", - }) - } else { // handle step > 1 - idxList := make([]string, 0) - for idx := st; idx <= ed; idx += jidxsec.Step { - idxList = append(idxList, strconv.Itoa(idx)) - } - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "in", - Value: "(" + strings.Join(idxList, ",") + ")", - }) - } - } - } - filtCdArrays = append(filtCdArrays, utils.FilterConditionArray{ - MainFiltCond: mainFiltCond, - SubFiltConds: subORFilters, - }) - } // end of for jobids - compfilters["job_id"] = filtCdArrays - } // filter with arrayjob[] - // ==================== exec_hosts ================ // filter is exec_hosts need special handling // the exec_hosts query request is array, eg: "{exec_hosts:eq:h1,h2,hg1} @@ -491,25 +264,6 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { } // end of for { filters } - // remove filter with jobarray[] from filters, for filter in compfilters need special handling. - if len(compfilters) > 0 { - var filtersNew []utils.FilterCondition - for _, filter := range filters { - found := false - for k, _ := range compfilters { - if filter.Field == k { - found = true - break - } - } - if found { - continue - } - filtersNew = append(filtersNew, filter) - } - filters = filtersNew - } - var job interface{} if typeValue == "array_stats" { job = &models.ArrayJobInfo{} @@ -529,15 +283,6 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { return } - // convert filterConditionArray to compound conditions - logger.Debug("before call ConvertToNewFilterArray for all array jobs", zap.Any("filtersarray", compfilters)) - compfiltercd, err := utils.ConvertToNewFilterArray(compfilters, job) - if err != nil { - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - // ---- check fields ---- // get fields and do check, if not set, use default display columns if typeValue == "array_stats" { @@ -678,14 +423,17 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { } } - queryOpts, err := utils.BuildQueryOptions(filtercds, compfiltercd, sortOrders, limit, pagetokenStr) + queryOpts, err := utils.BuildQueryOptions(filtercds, nil, sortOrders, limit, pagetokenStr) if err != nil { logger.Error(err.Error()) ReturnError(ctx, common.ERROR_CODE, common.ERROR_QUERY_JOBINFO_MSG+": "+err.Error(), nil) return } + // job selections are evaluated as row predicates, before sorting and + // pagination (bug-1898) + queryOpts.Predicates = selPreds - logger.Debug("the job query request:", zap.Any("jquery request", filtercds), zap.Any("jquery request arrayjob:", compfiltercd), zap.Any("sortOrders", sortOrders), zap.Any("limit", limit), + logger.Debug("the job query request:", zap.Any("jquery request", filtercds), zap.Int("selection predicates", len(selPreds)), zap.Any("sortOrders", sortOrders), zap.Any("limit", limit), zap.String("pagetoken", pagetokenStr), zap.Any("fields", fields)) var jqueryReply []utils.OrderMap // Declare jqueryReply @@ -708,6 +456,81 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { ReturnSuccess(ctx, common.SUCCESS_CODE, common.SUCCESS_MSG, queryResult) } +// validateJobQueryFilters is GetJobsInfo's validate-all-first pass. It +// checks every filter entry, in request order and before any store access: +// the filter field is supported for the query type, job_id/job_name values +// carry no index under type=array_stats, single-value operators have one +// value, and job selections parse and suit their operator +// (BuildSelectionPredicates). On success it returns the plain filters (the +// converted bracketed entries removed, written bare identity keys normalised +// to decimal) and the selection predicates; on the first rejection it writes +// the 400 reply and returns ok == false. +func validateJobQueryFilters(ctx *gin.Context, filters []utils.FilterCondition, + typeValue string) ([]utils.FilterCondition, []func(utils.OrderMap) bool, bool) { + supported := models.JOB_SUPPORT_FILTERS + if typeValue == "array_stats" { + supported = models.ARRAY_JOB_SUPPORT_FILTERS + } + + plain := make([]utils.FilterCondition, 0, len(filters)) + var preds []func(utils.OrderMap) bool + for _, filter := range filters { + if !utils.ContainsField(supported, filter.Field) { + err := errors.New("unsupported filter: " + filter.Field) + logger.Error(err.Error()) + ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) + return nil, nil, false + } + + if filter.Field == "job_id" || filter.Field == "job_name" { + values := extractJobNames(filter.Value) + for i := range values { + values[i] = strings.TrimSpace(values[i]) + } + // statistics rows are arrays, not elements: no index allowed + // (checked before the selection parsers) + if typeValue == "array_stats" && strings.ContainsAny(filter.Value, "[]") { + err := errors.New(filter.Field + " with array index is not supported in statistics type") + logger.Error(err.Error()) + ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) + return nil, nil, false + } + // only "in", "between", "notin" take multiple values + if !(filter.Operator == "in" || filter.Operator == "between" || filter.Operator == "notin") && len(values) > 1 { + err := fmt.Errorf("operator doesn't allow to use multiple values: %v", values) + logger.Error(err.Error()) + ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) + return nil, nil, false + } + } + + entryPlain, entryPreds, err := BuildSelectionPredicates([]utils.FilterCondition{filter}) + if err != nil { + returnSelectionError(ctx, filter.Value, err) + return nil, nil, false + } + plain = append(plain, entryPlain...) + preds = append(preds, entryPreds...) + } + return plain, preds, true +} + +// returnSelectionError writes the 400 reply of a rejected job selection or +// key. A selection-parser rejection (*SelError) passes its message through +// unchanged (it names the selection as written); any other rejection keeps +// today's "Invalid parameters: " text. +func returnSelectionError(ctx *gin.Context, raw string, err error) { + var se *SelError + if errors.As(err, &se) { + logger.Debug("rejected job selection", zap.String("selection", raw), + zap.String("rule", se.Rule), zap.String("reason", se.Msg)) + ReturnError(ctx, common.INVALID_PARAMS_CODE, se.Msg, nil) + return + } + logger.Debug("rejected job selection", zap.String("selection", raw), zap.Error(err)) + ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) +} + // GetJobByIDs godoc // @Summary Get multiple jobs by their IDs // @Description Retrieve information for multiple jobs in a single request with optional field selection @@ -773,120 +596,47 @@ func (j JobController) GetJobByIDs(ctx *gin.Context) { jobids[i] = strings.TrimSpace(jobids[i]) } logger.Debug("The input jobids", zap.Any("jobids", jobids)) + + // Validate every key before any store query (validate-all-first): the + // first rejected key, in request order, rejects the whole request. + sels := make([]Selection, 0, len(jobids)) + for _, jobid := range jobids { + sel, se := ParseIdentitySelection(jobid) + if se != nil { + returnSelectionError(ctx, jobid, se) + return + } + sels = append(sels, sel) + } + mergeJqueryReply := make([]utils.OrderMap, 0) var page_token = "" - for _, jobid := range jobids { - - compfilters := make(utils.CompFilterCondition) - - filtCdArrays := make([]utils.FilterConditionArray, 0) - subORFilters := make([]utils.FilterCondition, 0) - - jobid = strings.TrimSpace(jobid) - filters := make([]utils.FilterCondition, 0) + for _, sel := range sels { sortOrders := make([]utils.SortOrder, 0) - var err error - // parse the jobid to support array list format - jobid, err := parseArrayName(jobid) - if err != nil { - logger.Error("job id format is invalid", zap.Error(err)) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - logger.Debug("GetJobByIDs after parseArrayName", zap.Any("jobid", jobid)) - - if !utils.IsNumeric(jobid.KeyID) { - err := errors.New("bad jobid: " + jobid.KeyID) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - - filters = append(filters, utils.FilterCondition{ - Field: "job_id", - Operator: "eq", - Value: jobid.KeyID, - }) - - for _, filter := range filters { - // check the filter is supported. - if typeValue == "array_stats" { - if !utils.ContainsField(models.ARRAY_JOB_SUPPORT_FILTERS, filter.Field) { - err := errors.New("unsupported filter: " + filter.Field) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } else { - if !utils.ContainsField(models.JOB_SUPPORT_FILTERS, filter.Field) { - err := errors.New("unsupported filter: " + filter.Field) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - } - - } - - mainFiltCond := utils.FilterCondition{ - Field: filters[0].Field, - Operator: filters[0].Operator, - Value: jobid.KeyID, - } - - // treat every section as an OR condition - if jobid.Indices != nil && typeValue != "array_stats" { - // handle every section in Indices list. - for _, jidxsec := range jobid.Indices { - st, ed, err := parseArrayIndexSection(jidxsec.Index) - if err != nil { - err := errors.New("bad job array index: " + jidxsec.Index) - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - - if jidxsec.Step == 1 { - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "between", - Value: "(" + strconv.Itoa(st) + "," + strconv.Itoa(ed) + ")", - }) - } else { // handle step > 1 - idxList := make([]string, 0) - for idx := st; idx <= ed; idx += jidxsec.Step { - idxList = append(idxList, strconv.Itoa(idx)) - } - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "in", - Value: "(" + strings.Join(idxList, ",") + ")", - }) - } - } - } - filtCdArrays = append(filtCdArrays, utils.FilterConditionArray{ - MainFiltCond: mainFiltCond, - SubFiltConds: subORFilters, - }) - - compfilters["job_id"] = filtCdArrays - - // convert filterConditionArray to compound conditions - logger.Debug("before call ConvertToNewFilterArray for all jobs", zap.Any("compfilters", compfilters)) var job interface{} if typeValue == "array_stats" { job = &models.ArrayJobInfo{} } else { job = &models.JobInfo{} } - compfiltercd, err := utils.ConvertToNewFilterArray(compfilters, job) + filtercds, err := utils.ConvertToNewFilter([]utils.FilterCondition{{ + Field: "job_id", + Operator: "eq", + Value: strconv.FormatUint(sel.Key, 10), + }}, job) if err != nil { logger.Error(err.Error()) ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) return } + // An element selection matches only that array's elements in its + // range (5.2, 5.11); statistics rows are whole arrays, so there the + // index part selects the array as before. + var preds []func(utils.OrderMap) bool + if sel.Form != SelBare && typeValue != "array_stats" { + preds = append(preds, identityMatcher(sel)) + } var fields []string if typeValue == "array_stats" { @@ -958,13 +708,14 @@ func (j JobController) GetJobByIDs(ctx *gin.Context) { var bookmark string = "" var limit int = utils.PER_PAGE_LIMIT_MAX for { - queryOpts, err := utils.BuildQueryOptions(nil, compfiltercd, sortOrders, limit, bookmark) + queryOpts, err := utils.BuildQueryOptions(filtercds, nil, sortOrders, limit, bookmark) if err != nil { logger.Error(err.Error()) ReturnError(ctx, common.ERROR_CODE, common.ERROR_QUERY_JOBINFO_MSG+": "+err.Error(), nil) return } - logger.Debug("the job query request:", zap.Any("jquery request arrayjob:", compfiltercd), zap.Any("sortOrders", sortOrders), zap.Any("limit", limit), + queryOpts.Predicates = preds + logger.Debug("the job query request:", zap.Any("jquery request", filtercds), zap.String("selection", sel.Raw), zap.Any("sortOrders", sortOrders), zap.Any("limit", limit), zap.String("pagetoken", bookmark), zap.Any("fields", fields)) var jqueryReply []utils.OrderMap // Declare jqueryReply diff --git a/src/frontend/apiserver/controllers/jobs_handler_test.go b/src/frontend/apiserver/controllers/jobs_handler_test.go index afb165cc..53ac1d93 100644 --- a/src/frontend/apiserver/controllers/jobs_handler_test.go +++ b/src/frontend/apiserver/controllers/jobs_handler_test.go @@ -19,6 +19,8 @@ import ( "errors" "net/http" "net/http/httptest" + "net/url" + "sort" "strconv" "strings" "sync" @@ -734,3 +736,243 @@ func TestJobHandler_SubmitArrayLogsArrayIdentity(t *testing.T) { } } } + +// ========== Query by selection (task 7.3, bug-1898) ========== + +// querySeed is the store content of the query tests: arrays X ("foo", +// elements 1-4), Y ("foobar", elements 1-2), Z (unnamed, element 1) and W +// ("baz", elements 1-2), and the ordinary job O ("bar"). +type querySeed struct { + X, Y, Z, W, O uint64 +} + +func (s querySeed) elem(key uint64, k int) uint64 { return backend.MakeElementID(key, k) } + +func seedQueryJobs(f *handlerFixture) querySeed { + f.t.Helper() + s := querySeed{ + X: handlerTestArrayBase + 10<<15, + Y: handlerTestArrayBase + 11<<15, + Z: handlerTestArrayBase + 12<<15, + W: handlerTestArrayBase + 13<<15, + // array-capable (low 15 bits clear) but ordinary: element-only + // matching must not select it for O[0] (5.11) + O: handlerTestOrdinaryBase + 20<<15, + } + f.seedArrayElements(s.X, "foo", "RUN", 1, 2, 3, 4) + f.seedArrayElements(s.Y, "foobar", "RUN", 1, 2) + f.seedArrayElements(s.Z, "", "RUN", 1) + f.seedArrayElements(s.W, "baz", "RUN", 1, 2) + f.seedOrdinaryJob(s.O, "bar", "RUN") + return s +} + +// allQueryIDs is every unique_job_id seedQueryJobs stores. +func (s querySeed) all() []uint64 { + return []uint64{ + s.elem(s.X, 1), s.elem(s.X, 2), s.elem(s.X, 3), s.elem(s.X, 4), + s.elem(s.Y, 1), s.elem(s.Y, 2), s.elem(s.Z, 1), + s.elem(s.W, 1), s.elem(s.W, 2), s.O, + } +} + +// listJobs calls GetJobsInfo with filter (URL-escaped here). +func (f *handlerFixture) listJobs(filter string) *httptest.ResponseRecorder { + f.t.Helper() + return f.do(http.MethodGet, "/xce/v1/jobs?filter="+url.QueryEscape(filter), nil) +} + +// getJobs calls GetJobByIDs with the path segment jobids (URL-escaped here). +func (f *handlerFixture) getJobs(jobids string) *httptest.ResponseRecorder { + f.t.Helper() + return f.do(http.MethodGet, "/xce/v1/jobs/"+url.PathEscape(jobids), nil) +} + +// rowIDs returns the sorted unique_job_id of every row of a 200 list reply. +func rowIDs(t *testing.T, w *httptest.ResponseRecorder) []uint64 { + t.Helper() + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + dec := json.NewDecoder(bytes.NewReader(w.Body.Bytes())) + dec.UseNumber() + var raw struct { + Data struct { + Count int `json:"count"` + Rows []struct { + Data map[string]interface{} `json:"Data"` + } `json:"rows"` + } `json:"data"` + } + if err := dec.Decode(&raw); err != nil { + t.Fatalf("decode list reply: %v; body: %s", err, w.Body.String()) + } + ids := make([]uint64, 0, len(raw.Data.Rows)) + for _, r := range raw.Data.Rows { + n, ok := r.Data["unique_job_id"].(json.Number) + if !ok { + t.Fatalf("row without unique_job_id: %v", r.Data) + } + id, err := strconv.ParseUint(n.String(), 10, 64) + if err != nil { + t.Fatalf("unique_job_id %q: %v", n, err) + } + ids = append(ids, id) + } + if raw.Data.Count != len(ids) { + t.Errorf("count = %d, rows = %d", raw.Data.Count, len(ids)) + } + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) + return ids +} + +func sortedIDs(ids ...uint64) []uint64 { + out := append([]uint64{}, ids...) + sort.Slice(out, func(i, j int) bool { return out[i] < out[j] }) + return out +} + +func assertIDs(t *testing.T, name string, got, want []uint64) { + t.Helper() + want = sortedIDs(want...) + if len(got) != len(want) { + t.Errorf("%s: rows %v, want %v", name, got, want) + return + } + for i := range got { + if got[i] != want[i] { + t.Errorf("%s: rows %v, want %v", name, got, want) + return + } + } +} + +// assert400 checks a 400 reply whose message contains want. +func assert400(t *testing.T, name string, w *httptest.ResponseRecorder, want string) { + t.Helper() + if w.Code != http.StatusBadRequest { + t.Errorf("%s: status = %d, want 400; body: %s", name, w.Code, w.Body.String()) + return + } + r := decodeHandlerResponse(t, w) + msg, _ := r.Msg.(string) + if !strings.Contains(msg, want) { + t.Errorf("%s: msg = %q, want it to contain %q", name, msg, want) + } + if len(r.Data) != 0 && string(r.Data) != "null" { + t.Errorf("%s: a rejected query carries data: %s", name, r.Data) + } +} + +// TestJobHandler_QuerySelections covers the GetJobsInfo rows of the design's +// Testing Strategy (Query). +func TestJobHandler_QuerySelections(t *testing.T) { + f := newHandlerFixture(t) + s := seedQueryJobs(f) + x, y, z, w := s.X, s.Y, s.Z, s.W + X := jobIDString(x) + Y := jobIDString(y) + O := jobIDString(s.O) + writtenX := backend.FormatJobID(int64(x)) + + cases := []struct { + name string + filter string + want []uint64 + }{ + {"eq single element (bug-1898)", "{job_id:eq:" + X + "[2]}", []uint64{s.elem(x, 2)}}, + {"eq range", "{job_id:eq:" + X + "[2-3]}", []uint64{s.elem(x, 2), s.elem(x, 3)}}, + {"ordinary-id[0] lists nothing (5.11)", "{job_id:eq:" + O + "[0]}", nil}, + {"array-id[0] without element 0 lists nothing (5.11)", "{job_id:eq:" + X + "[0]}", nil}, + {"unnamed: eq [1] lists nothing", "{job_name:eq:[1]}", nil}, + {"unnamed: contains [1] lists element 1 of every array", + "{job_name:contains:[1]}", []uint64{s.elem(x, 1), s.elem(y, 1), s.elem(z, 1), s.elem(w, 1)}}, + {"notcontains foo[1]", "{job_name:notcontains:foo[1]}", []uint64{s.elem(z, 1), s.elem(w, 1)}}, + {"in + ne entries on the same field are AND'ed", + "{job_id:in:(" + X + "[1]," + O + ")},{job_id:ne:" + O + "}", []uint64{s.elem(x, 1)}}, + {"job_id and job_name selections intersect", + "{job_id:in:(" + X + "[1]," + X + "[2])},{job_name:eq:foo[2]}", []uint64{s.elem(x, 2)}}, + {"in with a bare and a bracketed value matches both", + "{job_id:in:(" + O + "," + X + "[1])}", []uint64{s.O, s.elem(x, 1)}}, + {"notin excludes the element", "{job_id:notin:(" + X + "[1])}", + []uint64{s.elem(x, 2), s.elem(x, 3), s.elem(x, 4), s.elem(y, 1), s.elem(y, 2), + s.elem(z, 1), s.elem(w, 1), s.elem(w, 2), s.O}}, + {"startwith foo[1]", "{job_name:startwith:foo[1]}", []uint64{s.elem(x, 1), s.elem(y, 1)}}, + {"written bare key alone (5.6)", "{job_id:eq:" + writtenX + "}", + []uint64{s.elem(x, 1), s.elem(x, 2), s.elem(x, 3), s.elem(x, 4)}}, + {"written bare key mixed with a bracketed value (5.6)", + "{job_id:in:(" + writtenX + "," + Y + "[1])}", + []uint64{s.elem(x, 1), s.elem(x, 2), s.elem(x, 3), s.elem(x, 4), s.elem(y, 1)}}, + {"between on job_id evaluates like in", "{job_id:between:(" + X + "[1]," + Y + "[2])}", + []uint64{s.elem(x, 1), s.elem(y, 2)}}, + } + for _, c := range cases { + assertIDs(t, c.name, rowIDs(t, f.listJobs(c.filter)), c.want) + } + + // sanity: an unfiltered (default submit_user) listing sees every seeded row + assertIDs(t, "all rows", rowIDs(t, f.listJobs("{submit_user:eq:"+handlerTestUser+"}")), s.all()) +} + +// TestJobHandler_QuerySelectionRejections: rejected selections reject the +// whole query with 400 and list nothing. +func TestJobHandler_QuerySelectionRejections(t *testing.T) { + f := newHandlerFixture(t) + s := seedQueryJobs(f) + X := jobIDString(s.X) + O := jobIDString(s.O) + // a host exists (so GetHostList accepts it) but runs no job, so the + // exec_hosts lookup alone would answer count 0 + store.Hosts.Put("h1", models.HostInfo{HostName: "h1"}) + + cases := []struct { + name, filter, want string + }{ + {"5.7 with an exec_hosts filter matching no job", + "{exec_hosts:eq:h1},{job_id:eq:" + X + "[40000]}", "invalid"}, + {"null operator on a bracketed name", "{job_name:notnull:foo[1]}", "operator doesn't support"}, + {"ne on a bracketed identity", "{job_id:ne:" + X + "[1]}", "operator doesn't support"}, + {"valid selection followed by an invalid one", + "{job_id:eq:" + X + "[1]},{job_name:eq:foo[5-3]}", "reversed"}, + {"5.8: element of a non-array identity", "{job_id:eq:" + jobIDString(s.O+1) + "[1]}", "not an array identity"}, + {"5.16 syntax", "{job_id:eq:" + X + "[a]}", "invalid job identity or element selection"}, + {"5.17 marked decimal without index", + "{job_id:eq:" + jobIDString(s.elem(s.X, 1)) + "}", "must be named as"}, + {"5.14 too-wide written key", "{job_id:in:(" + O + ",64-0-1-0[1])}", "wider than allowed"}, + } + for _, c := range cases { + assert400(t, c.name, f.listJobs(c.filter), c.want) + } + + // the exec_hosts row's message names the selection (5.7) and is not + // replaced by "bad job array index" + w := f.listJobs("{exec_hosts:eq:h1},{job_id:eq:" + X + "[40000]}") + if strings.Contains(w.Body.String(), "bad job array index") { + t.Errorf("message replaced by the old text: %s", w.Body.String()) + } + if !strings.Contains(w.Body.String(), X+"[40000]") { + t.Errorf("message does not name the selection: %s", w.Body.String()) + } +} + +// TestJobHandler_GetJobByIDsSelections covers GET /jobs/{jobids}. +func TestJobHandler_GetJobByIDsSelections(t *testing.T) { + f := newHandlerFixture(t) + s := seedQueryJobs(f) + X := jobIDString(s.X) + O := jobIDString(s.O) + + assertIDs(t, "X[1-3]", rowIDs(t, f.getJobs(X+"[1-3]")), + []uint64{s.elem(s.X, 1), s.elem(s.X, 2), s.elem(s.X, 3)}) + assertIDs(t, "X[2],Y[1]", rowIDs(t, f.getJobs(X+"[2],"+jobIDString(s.Y)+"[1]")), + []uint64{s.elem(s.X, 2), s.elem(s.Y, 1)}) + assertIDs(t, "bare X lists every element (5.6)", rowIDs(t, f.getJobs(X)), + []uint64{s.elem(s.X, 1), s.elem(s.X, 2), s.elem(s.X, 3), s.elem(s.X, 4)}) + assertIDs(t, "written bare O", rowIDs(t, f.getJobs(backend.FormatJobID(int64(s.O)))), []uint64{s.O}) + assertIDs(t, "O[0] lists nothing (5.11)", rowIDs(t, f.getJobs(O+"[0]")), nil) + + assert400(t, "valid then invalid", f.getJobs(X+"[1],"+X+"[40000]"), X+"[40000]") + assert400(t, "5.17", f.getJobs(jobIDString(s.elem(s.X, 1))), "must be named as") + assert400(t, "5.14", f.getJobs("64-0-1-0"), "wider than allowed") + assert400(t, "5.5", f.getJobs(X+"[3-1]"), "reversed") +}