OPS-442: Batch variable set fetching in ListTicketSchemas - #45
Conversation
|
Claude finished @arreyder's task in 1m 23s —— View job PR Review: OPS-442 - Batch variable set fetching
Overall: Good optimization — the N+1 pattern is correctly identified and the batch approach using ServiceNow's Key findings (see inline comments):
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Batch: find all variable set links for all items at once | ||
| allLinks, _, err := c.GetVariableSetLinksForItems(ctx, itemSysIDs, PaginationVars{Limit: 500}) |
There was a problem hiding this comment.
Bug: Pagination truncation risk. With a hardcoded Limit: 500, if there are more than 500 variable set links across all items, results will be silently truncated. The second page token is captured (allLinks, _, err) but discarded.
This was also an issue in the original single-item path (Limit: 200), but batching multiple items makes it much more likely to hit the limit. Consider either:
- Paginating in a loop until the next-page token is empty, or
- At minimum, logging a warning when the next-page token is non-empty so truncation is detectable.
Same concern applies to GetVariablesBySetIDs (limit 500) and GetChoicesForVariables (limit 1000) inside getSetVariablesAndChoices.
|
|
||
| var ret []*v2.TicketSchema | ||
| for _, catalogItem := range catalogItems { | ||
| catalogItem := catalogItem |
There was a problem hiding this comment.
Nit: Unnecessary loop variable re-declaration. Since Go 1.22, range loop variables are per-iteration. If this module targets Go 1.22+, catalogItem := catalogItem can be removed.
| catalogItem := catalogItem | |
| catalogItemSchema, err := s.schemaForCatalogItemWithSetVars(ctx, &catalogItem, setVarsByItem[catalogItem.Id]) |
| // schemaForCatalogItemWithSetVars builds a TicketSchema using pre-fetched set variables | ||
| // (from the batch query) plus per-item direct variables from the catalog API. | ||
| func (s *ServiceNow) schemaForCatalogItemWithSetVars(ctx context.Context, catalogItem *servicenow.CatalogItem, setVars []servicenow.CatalogItemVariable) (*v2.TicketSchema, error) { | ||
| var ticketTypes []*v2.TicketType | ||
| customFields := make(map[string]*v2.TicketCustomField) | ||
|
|
||
| // Fetch direct (non-set) variables per item — this still uses the catalog API per item | ||
| itemVars, err := s.client.GetCatalogItemVariables(ctx, catalogItem.Id) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("servicenow-connector: failed to get direct variables for catalog item %s: %w", catalogItem.Id, err) | ||
| } | ||
|
|
||
| // Merge: direct vars first, then set vars (prefer direct on collision) | ||
| allVars := make([]servicenow.CatalogItemVariable, 0, len(itemVars)+len(setVars)) | ||
| seen := make(map[string]struct{}, len(itemVars)) | ||
| for _, v := range itemVars { | ||
| allVars = append(allVars, v) | ||
| seen[v.ID] = struct{}{} | ||
| } | ||
| for _, v := range setVars { | ||
| if _, dup := seen[v.ID]; !dup { | ||
| allVars = append(allVars, v) | ||
| } | ||
| } | ||
|
|
||
| for _, v := range allVars { | ||
| vCopy := v | ||
| cf := servicenow.ConvertVariableToSchemaCustomField(ctx, &vCopy) | ||
| if cf == nil { | ||
| continue | ||
| } | ||
| customFields[vCopy.Name] = cf | ||
| } | ||
|
|
||
| return &v2.TicketSchema{ | ||
| Id: catalogItem.Id, | ||
| DisplayName: catalogItem.Name, | ||
| Types: ticketTypes, | ||
| CustomFields: customFields, | ||
| }, nil | ||
| } | ||
|
|
||
| func (s *ServiceNow) serviceCatalogRequestItemToTicket(ctx context.Context, requestedItem *servicenow.RequestedItem) (*v2.Ticket, annotations.Annotations, error) { | ||
| createdAt, err := time.Parse(time.DateTime, requestedItem.SysCreatedOn) | ||
| if err != nil { |
There was a problem hiding this comment.
Minor: Dead code potential. The old schemaForCatalogItem method (line 250) is no longer called from ListTicketSchemas. If it has no other callers, consider removing it to avoid maintaining two code paths. If it's intentionally kept for other call sites, a comment explaining that would be helpful.
|
Claude finished @arreyder's task in 2m 38s —— View job PR Review: OPS-442 - Batch variable set fetching in ListTicketSchemas
Blocking Issues: 0 | Suggestions: 2 Security IssuesNone found. Correctness IssuesNone found. The batch approach is correctly implemented — the Suggestions
Notes on prior review
OverallClean optimization. The N+1 → batch refactor is well-structured: the single-item path is preserved as a thin wrapper, the new batch path correctly groups by |
| @@ -600,7 +659,7 @@ func (c *Client) GetCatalogItemVariablesPlusSets(ctx context.Context, itemSysID | |||
| // Fetch variables that belong to those sets | |||
| setVars, _, err := c.GetVariablesBySetIDs(ctx, setIDs, PaginationVars{Limit: 500}) | |||
There was a problem hiding this comment.
🟡 Suggestion: Inner limits may need scaling too. The outer batch limit was increased to 2000, but GetVariablesBySetIDs (limit 500) and GetChoicesForVariables (limit 1000) weren't adjusted proportionally. With more items batched, the number of unique variable sets — and therefore variables and choices — also grows. Consider increasing these or adding pagination loops, especially for GetVariablesBySetIDs which has the tightest limit.
|
Claude finished @arreyder's task in 1m 24s —— View job PR Review: OPS-442 - Batch variable set fetching in ListTicketSchemas
Overall: Clean, well-structured optimization. The N+1 → batch refactor correctly identifies the problem, the Blocking Issues: 0 | Suggestions: 2 | Nits: 1 Suggestions1. Deduplicate set IDs in When multiple catalog items share the same variable set, seen := make(map[string]struct{}, len(links))
setIDs := make([]string, 0, len(links))
for _, l := range links {
if _, ok := seen[l.VariableSet]; !ok {
setIDs = append(setIDs, l.VariableSet)
seen[l.VariableSet] = struct{}{}
}
}2. Inner query limits may silently truncate — The outer batch limit was increased to 2000 (good), but
Nits1. Double mapping in
Confirmed Non-Issues
|
Replace per-catalog-item variable set queries with a single batch query using ServiceNow's IN operator. For 20 catalog items this reduces API calls from ~61 to ~24 (20 direct variable calls + 1 batch set links + 1 batch set variables + 1 batch choices + 1 states). Changes: - Add GetVariableSetLinksForItems() batch variant using sc_cat_itemIN - Add GetCatalogItemVariablesPlusSetsMulti() that fetches set data for all items at once and returns results keyed by item ID - Extract getSetVariablesAndChoices() and mergeVariables() helpers - Add CatItem field to VariableSetM2M struct (needed for grouping) - Add schemaForCatalogItemWithSetVars() that accepts pre-fetched set vars - ListTicketSchemas now batch-fetches all set data upfront The per-item GetCatalogItemVariables (catalog service API) call is retained since it uses a different API endpoint that doesn't support batch queries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unnecessary catalogItem := catalogItem (Go 1.22+ per-iteration) - Increase batch limit from 500 to 2000 to reduce truncation risk (matches concern about batching multiple items hitting the limit) - schemaForCatalogItem kept for GetTicketSchema single-item path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
0967a13 to
91eba09
Compare
|
Claude finished @arreyder's task in 1m 9s —— View job PR Review: OPS-442 - Batch variable set fetching in ListTicketSchemas
Blocking Issues: 0 | Suggestions: 2 | Nits: 0 Overall: Well-structured optimization. The N+1 → batch refactor is cleanly decomposed — Suggestions1. Deduplicate set IDs in When multiple catalog items share the same variable set, seen := make(map[string]struct{}, len(links))
setIDs := make([]string, 0, len(links))
for _, l := range links {
if _, ok := seen[l.VariableSet]; !ok {
setIDs = append(setIDs, l.VariableSet)
seen[l.VariableSet] = struct{}{}
}
}This is especially worthwhile in the batch path where many items may reference the same sets. 2. Inner query limits may silently truncate — The outer batch limit was appropriately increased to 2000, but Options, in order of robustness:
Confirmed Non-Issues
Architecture NotesThe batch approach correctly preserves behavioral parity with the single-item path: The comment on line 607-610 acknowledging the fixed-limit trade-off is a good callout — this is the same limitation as the pre-existing single-item path, just at a larger scale. |
1 participant
Summary
Replace per-catalog-item variable set queries with a single batch query using ServiceNow IN operator.
For 20 catalog items: ~61 API calls reduced to ~24.
Changes
Test plan