Added validation to vtex skuid #1488
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughAdds a sanitizer that trims and retains only numeric IDs for SKU and Product ID selection, applies it when building queries and cache keys (sorted), and updates where sanitized ID lists are used in query construction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vtex/loaders/legacy/productList.ts (1)
135-143: SKU ID validation is solid; consider behavior when everything gets filtered outThe new filtering logic for
skuIds(trimming and enforcing^\d+$) is a good match for VTEX’s Int64 expectation and should eliminate the 400s from invalid SKUs.Two small follow‑ups you may want to consider:
- When all provided SKU IDs are invalid/whitespace,
skuIdsbecomes empty but we still:
- Leave
params.fqempty, and- Call VTEX with
_from = 0,_to = 0.
That likely returns some arbitrary first product instead of “no results”. If you’d rather treat “no valid SKUs” as “no products”, you could short‑circuit before making the VTEX call (e.g., returnnull/[]from the loader) or encode a sentinel range that the caller interprets as “skip request”.- You’re trimming twice (
skuId.trim()in the filter and again when buildingfq). A tiny DRY/readability improvement is to normalize once and reuse:const sanitizeNumericIds = (ids?: string[]) => (ids ?? []) .map((id) => id.trim()) .filter((id) => id && /^\d+$/.test(id)); if (isSKUIDProps(props)) { const skuIds = sanitizeNumericIds(props.ids); skuIds.forEach((skuId) => params.fq.push(`skuId:${skuId}`)); params._from = 0; params._to = Math.max(skuIds.length - 1, 0); return params; }You could also reuse
sanitizeNumericIdsincacheKeyso different spellings of the same IDs (whitespace, leading zeros, etc.) map to the same cache entry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/loaders/legacy/productList.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
vtex/loaders/legacy/productList.ts
[error] 150-152: Deno fmt --check failed: Found 1 not formatted file in 2081 files. Apply formatting (deno fmt) and re-run the CI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vtex/loaders/legacy/productList.ts (2)
141-148: Consider handling the empty sanitized IDs case.If all SKU IDs are invalid and
sanitizeNumericIdsreturns an empty array, the code will still make a VTEX API request with nofqfilters and_to = 0. This might return unexpected results.Consider adding an early return or empty result when no valid IDs remain:
if (isSKUIDProps(props)) { const skuIds = sanitizeNumericIds(props.ids); + + if (skuIds.length === 0) { + // Return params that will result in no products + params._from = 0; + params._to = 0; + return params; + } skuIds.forEach((skuId) => params.fq.push(`skuId:${skuId}`));Alternatively, the loader could return an empty array immediately when no valid IDs are provided, avoiding the API call entirely.
127-130: Inconsistency: Raw IDs used for SKU selection and sorting after sanitization.The
fromPropsfunction sanitizes IDs before querying VTEX (lines 142, 152), butpreferredSKU(line 128) andsortProducts(lines 228, 234) still use the raw, unsanitized IDs from props. This creates an inconsistency:
- VTEX returns products matching sanitized IDs
preferredSKUlooks for SKUs using raw IDs (including invalid ones)sortProductsattempts to sort using raw IDs (including invalid ones)Impact: If invalid IDs are provided,
preferredSKUwon't find matches and will fall back toitems[0]. Invalid IDs passed tosortProductswon't match any products, potentially affecting the sort order.Recommendation: Store the sanitized IDs at a broader scope and use them consistently:
const loader = async ( expandedProps: Props, req: Request, ctx: AppContext, ): Promise<Product[] | null> => { const props = expandedProps.props ?? (expandedProps as unknown as Props["props"]); // Store sanitized IDs for consistent use let sanitizedIds: string[] | undefined; if (isSKUIDProps(props)) { sanitizedIds = sanitizeNumericIds(props.ids); } else if (isProductIDProps(props)) { sanitizedIds = sanitizeNumericIds(props.productIds); } // ... rest of loader // Update preferredSKU to accept sanitized IDs const fetchedSkus = new Set(sanitizedIds ?? []); // Update sortProducts calls to use sanitized IDs if (isSKUIDProps(props)) { products = sortProducts(products, sanitizedIds || [], "sku"); } if (isProductIDProps(props)) { products = sortProducts(products, sanitizedIds || [], "inProductGroupWithID"); }Also applies to: 228-229, 232-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/loaders/legacy/productList.ts(4 hunks)
🔇 Additional comments (2)
vtex/loaders/legacy/productList.ts (2)
121-125: LGTM! Clean helper for ID sanitization.The
sanitizeNumericIdshelper correctly filters out invalid IDs and trims whitespace, which should prevent the VTEX "Invalid Parameter, skuId should be Int64" errors mentioned in the PR objectives.
311-319: LGTM! Cache key now consistent with sanitized IDs.The cache key generation now uses
sanitizeNumericIdsand sorts the results, ensuring that cache entries match the actual IDs sent to VTEX. This addresses the inconsistency flagged in previous reviews.
What is this Contribution About?
Some stores are receiving many VTEX timeout errors (HttpError 400: "Can't create search criteria! Error: Invalid Parameter, skuId should be Int64.)
Possibly invalid skuids were being passed to the VTEX API.
Added validation to filter out invalid IDs before sending to the VTEX API.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.