support variables within containers #32
Conversation
JavierCarnelli-ConductorOne
left a comment
JavierCarnelli-ConductorOne
left a comment
There was a problem hiding this comment.
I got some doubts regarding pagination but the PR looks good to me, so I approved
| func (c *Client) GetCatalogItemVariablesPlusSets(ctx context.Context, itemSysID string) ([]CatalogItemVariable, error) { | ||
| itemVars, err := c.GetCatalogItemVariables(ctx, itemSysID) | ||
| // Item-level variables from Table API | ||
| itemVarsRaw, _, err := c.GetVariablesForItem(ctx, itemSysID, PaginationVars{Limit: 500}) |
There was a problem hiding this comment.
We are cutting a page of 500 items, right?
The page token is not being stored here. Are we handling that pagination or is it a case we cannot handle?
|
|
||
| // Find attached variable sets | ||
| // Variable sets attached to this item | ||
| links, _, err := c.GetVariableSetLinksForItem(ctx, itemSysID, PaginationVars{Limit: 200}) |
There was a problem hiding this comment.
Here we could have extra pages also? Or it's ok to discard the next page token?
| // Variables inside those sets | ||
| setVarsRaw := []ItemOptionNew{} | ||
| if len(setIDs) > 0 { | ||
| setVarsRaw, _, err = c.GetVariablesBySetIDs(ctx, setIDs, PaginationVars{Limit: 1000}) |
There was a problem hiding this comment.
Don't wanna repeat myself but same question about pagination here
| cvSet = append(cvSet, MapItemOptionNewToCatalogItemVariable(v, choicesByQ[v.SysID])) | ||
| choicesByQ := map[string][]QuestionChoice{} | ||
| if len(varIDs) > 0 { | ||
| choices, _, err := c.GetChoicesForVariables(ctx, varIDs, PaginationVars{Limit: 5000}) |
There was a problem hiding this comment.
Same question about the pagination here
JavierCarnelli-ConductorOne
force-pushed
the
lauren/container-variables
branch
from
a0e95df to
6da1f43
Compare
WalkthroughRefactors GetCatalogItemVariablesPlusSets to aggregate item-level and set-level variables, fetch choices in a single pass, and unify mapping to CatalogItemVariable. Updates model types to use a strong VariableType with JSON marshal/unmarshal, adjusts related structs and helpers, and simplifies type handling in conversion/mapping functions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant SN as ServiceNow API
Note over C: GetCatalogItemVariablesPlusSets(itemID)
C->>SN: GetVariablesForItem(itemID)
SN-->>C: itemVars
C->>SN: GetVariableSetLinks(itemID)
SN-->>C: setLinks
C->>SN: GetVariablesBySetIDs(setLinks.setIDs)
SN-->>C: setVars
Note over C: Merge itemVars + setVars -> allVars
C->>SN: GetChoicesForVariables(allVars.ids)
SN-->>C: choices
Note over C: Build choicesByQuestion, map allVars -> CatalogItemVariable (dedupe)
C-->>C: Return aggregated CatalogItemVariables
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| cvSet = append(cvSet, MapItemOptionNewToCatalogItemVariable(v, choicesByQ[v.SysID])) | ||
| choicesByQ := map[string][]QuestionChoice{} | ||
| if len(varIDs) > 0 { | ||
| choices, _, err := c.GetChoicesForVariables(ctx, varIDs, PaginationVars{Limit: 5000}) |
There was a problem hiding this comment.
Bug: API Pagination Token Ignored
The GetCatalogItemVariablesPlusSets function discards pagination tokens from GetVariablesForItem, GetVariableSetLinksForItem, GetVariablesBySetIDs, and GetChoicesForVariables. When any of these APIs return more results than their limits (e.g., 500, 200, 1000, 5000), only the first page is processed. This causes silent data loss and incomplete catalog item variable information.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/servicenow/model.go (1)
401-447: UI label bug: using value as DisplayName for choices.For MultipleChoice and SelectBox branches, DisplayName is set to c.Value. That renders IDs instead of human-readable labels.
Apply:
- allowedChoices = append(allowedChoices, &v2.TicketCustomFieldObjectValue{ - Id: c.Value, - DisplayName: c.Value, - }) + allowedChoices = append(allowedChoices, &v2.TicketCustomFieldObjectValue{ + Id: c.Value, + DisplayName: c.Label, + })Repeat the same change in the SelectBox/LookupSelectBox case.
🧹 Nitpick comments (3)
pkg/servicenow/client.go (3)
544-546: Doc nit: clarify that choices are fetched for all variables, not just set variables.
660-676: Stale comment: this function is used.Comment says “Unused…”, but it’s called in GetCatalogItemVariablesPlusSets. Please update.
591-603: Optional: deterministic ordering.If downstream expects a stable variable order, consider sorting out by name or by SN’s “order” if available in item_option_new (not currently selected in sysparm_fields).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/servicenow/client.go(1 hunks)pkg/servicenow/model.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/servicenow/client.go (2)
pkg/servicenow/model.go (5)
CatalogItemVariable(192-213)VariableSet(319-323)ItemOptionNew(326-339)QuestionChoice(342-347)MapItemOptionNewToCatalogItemVariable(464-492)pkg/servicenow/request.go (1)
PaginationVars(80-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
pkg/servicenow/model.go (5)
192-213: Struct tightening to strong types looks good.Switching CatalogItemVariable fields to concrete types and VariableType is a solid improvement.
247-311: Robust VariableType (un)marshalling — LGTM.Covers null/empty/object/quoted/bare numbers and emits numeric JSON. Please confirm we never POST this field back to SN expecting a quoted string.
458-461: boolStr switch to strconv.ParseBool — good improvement.
325-339: Field tag check: reference qualifier key.RefQualifier uses
json:"reference_qual". SN sometimes exposes this asreference_qualvsref_qualifierdepending on table/Display Value settings. Please double-check the exact field name from item_option_new in your instance.
463-492: Type mapping uses strong VariableType — LGTM.pkg/servicenow/client.go (4)
548-551: Pagination missing for item-level variables (may truncate at 500).You ignore the next page token from GetVariablesForItem, so items beyond the first page are silently dropped. Please loop until next is empty. This was raised previously.
Apply:
- itemVarsRaw, _, err := c.GetVariablesForItem(ctx, itemSysID, PaginationVars{Limit: 500}) - if err != nil { - return nil, fmt.Errorf("get item-level variables: %w", err) - } + var itemVarsRaw []ItemOptionNew + pgItem := PaginationVars{Limit: 500} + for { + page, next, err := c.GetVariablesForItem(ctx, itemSysID, pgItem) + if err != nil { + return nil, fmt.Errorf("get item-level variables: %w", err) + } + itemVarsRaw = append(itemVarsRaw, page...) + if next == "" { + break + } + if off, err := strconv.Atoi(next); err == nil { + pgItem.Offset = off + } else { + break + } + }Also add import:
import ( "bytes" "context" "encoding/json" "errors" "fmt" "io" "math" "net/http" "net/url" "strings" + "strconv" )
554-561: Pagination missing for variable-set links + lack of setID de-dup.Same issue: only first page of io_set_item links is considered; duplicates can inflate queries. Loop and de-dup. Previously flagged.
- links, _, err := c.GetVariableSetLinksForItem(ctx, itemSysID, PaginationVars{Limit: 200}) - if err != nil { - return nil, fmt.Errorf("get variable set links: %w", err) - } - setIDs := make([]string, 0, len(links)) - for _, l := range links { - setIDs = append(setIDs, l.VariableSet) - } + var links []VariableSetM2M + pgLinks := PaginationVars{Limit: 200} + for { + page, next, err := c.GetVariableSetLinksForItem(ctx, itemSysID, pgLinks) + if err != nil { + return nil, fmt.Errorf("get variable set links: %w", err) + } + links = append(links, page...) + if next == "" { + break + } + if off, err := strconv.Atoi(next); err == nil { + pgLinks.Offset = off + } else { + break + } + } + // de-dup set IDs + setIDSet := make(map[string]struct{}, len(links)) + for _, l := range links { + setIDSet[l.VariableSet] = struct{}{} + } + setIDs := make([]string, 0, len(setIDSet)) + for id := range setIDSet { + setIDs = append(setIDs, id) + }
565-570: Pagination missing for set variables (across all sets).Only the first page from GetVariablesBySetIDs is processed. Loop over pages. Previously raised.
- setVarsRaw := []ItemOptionNew{} - if len(setIDs) > 0 { - setVarsRaw, _, err = c.GetVariablesBySetIDs(ctx, setIDs, PaginationVars{Limit: 1000}) - if err != nil { - return nil, fmt.Errorf("get variables by set ids: %w", err) - } - } + setVarsRaw := []ItemOptionNew{} + if len(setIDs) > 0 { + pgSet := PaginationVars{Limit: 1000} + for { + page, next, err := c.GetVariablesBySetIDs(ctx, setIDs, pgSet) + if err != nil { + return nil, fmt.Errorf("get variables by set ids: %w", err) + } + setVarsRaw = append(setVarsRaw, page...) + if next == "" { + break + } + if off, err := strconv.Atoi(next); err == nil { + pgSet.Offset = off + } else { + break + } + } + }
575-589: Choices fetch: de-dup variable IDs, chunk the IN query, and paginate choices.
- varIDs may contain duplicates; de-dup first.
- questionIN with thousands of IDs risks URL/query-size limits; chunk.
- You also ignore next page tokens from GetChoicesForVariables. Previously flagged.
- varIDs := make([]string, 0, len(allRaw)) - for _, v := range allRaw { - varIDs = append(varIDs, v.SysID) - } + idSet := make(map[string]struct{}, len(allRaw)) + for _, v := range allRaw { + idSet[v.SysID] = struct{}{} + } + varIDs := make([]string, 0, len(idSet)) + for id := range idSet { + varIDs = append(varIDs, id) + } choicesByQ := map[string][]QuestionChoice{} if len(varIDs) > 0 { - choices, _, err := c.GetChoicesForVariables(ctx, varIDs, PaginationVars{Limit: 5000}) - if err != nil { - return nil, fmt.Errorf("get choices for variables: %w", err) - } - for _, ch := range choices { - choicesByQ[ch.Question] = append(choicesByQ[ch.Question], ch) - } + const chunk = 200 + for i := 0; i < len(varIDs); i += chunk { + end := i + chunk + if end > len(varIDs) { + end = len(varIDs) + } + pgChoices := PaginationVars{Limit: 1000} + for { + page, next, err := c.GetChoicesForVariables(ctx, varIDs[i:end], pgChoices) + if err != nil { + return nil, fmt.Errorf("get choices for variables: %w", err) + } + for _, ch := range page { + choicesByQ[ch.Question] = append(choicesByQ[ch.Question], ch) + } + if next == "" { + break + } + if off, err := strconv.Atoi(next); err == nil { + pgChoices.Offset = off + } else { + break + } + } + } }
Note
Adds robust VariableType JSON (de)serialization, updates models to typed variables, and reworks catalog item variable fetching to include item and set variables with choices and de-duplication.
pkg/servicenow/client.go):GetCatalogItemVariablesPlusSetsto fetch item-level variables and set variables, load choices for all, map toCatalogItemVariable, and de-duplicate; increased limits and clearer errors.pkg/servicenow/model.go):VariableTypeNewand ad-hoc parsing with typedVariableTypeplus custom JSONUnmarshalJSON/MarshalJSONsupporting multiple incoming formats; updateCatalogItemVariable.TypeandItemOptionNew.TypetoVariableType.ConvertVariableToSchemaCustomFieldto use typedVariableType; ignore container start/end variables.strconv.ParseBool; removeparseVariableType; map variables using typedv.Type.Written by Cursor Bugbot for commit 6da1f43. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor