Fix findRecords filter merge to preserve caller boolean logic.#592
Fix findRecords filter merge to preserve caller boolean logic.#592
Conversation
Previous behavior flattened caller rules into an and group when caller passed match/rules, which could silently convert caller or intent into and. New behavior nests caller filters as a single sub-rule under the base equality filter, preserving caller match semantics. Improve paged read resilience in getAllRecords and getAllChildRecords. Replace Promise.all batch fetch with allSettled handling so successful pages are retained even if one page fails. Add optional failOnPageError to support strict fail-fast behavior when required. Add warning logs with page context for failed pages. Route uploadAsset through the shared request pipeline. Remove standalone fetch path and use common request logic for retries, backoff, timeout, and 429 handling consistency. Maintain write queue integration and existing return contract. Add configurable asset-size policy in uploadAsset. New create-level and per-call controls: warnAssetSizeBytes maxAssetSizeBytes enforceAssetSize Warn when size exceeds warning threshold. Reject early with clear error when enforcement is enabled and max size is exceeded. Add human-readable byte formatting helper for diagnostics. Simplify request error construction. Remove dead response-like shape branch that was never hit by current call paths. Keep structured error contract with status and body. Harden updateRecords input-shape detection. Validate all elements before entering per-record mode. Throw clear error for mixed arrays instead of failing later during execution. Documentation updates. Expand KTL API docs with new options, defaults, behavior changes, and read-path error semantics. Add AI workflow rule to check existing docs first, update when present, and only create docs for complex features.
Co-authored-by: CSWinnall <10554243+CSWinnall@users.noreply.github.com>
Honor `options.refreshViews` in `updateRecords` per-record invocation
Prevents use of nested filter groups in filter parameters by explicitly rejecting them with clear error messages, due to Knack API limitations. Also disallows merging filters with top-level 'or' logic to avoid unreliable behavior, instructing users to run separate queries and merge results client-side. Improves documentation to clarify these restrictions and error handling.
There was a problem hiding this comment.
Pull request overview
This PR enhances the KTL API module's reliability and consistency by improving filter handling, adding resilience to paged reads, standardizing the upload pipeline, and hardening input validation.
Changes:
- Validates filter groups in
findRecordsto prevent silent OR-to-AND conversion, rejecting unsupported nested groups and non-AND logic with clear errors - Replaces
Promise.allwithPromise.allSettledin paged reads to retain successful pages when batch members fail, with optionalfailOnPageErrorfor strict behavior - Routes
uploadAssetthrough the shared_requestpipeline for consistent retry/backoff/timeout handling, adding configurable size policy controls (warn threshold, max size, enforcement toggle) - Hardens
updateRecordsto validate input shape upfront and throw clear errors for mixed arrays instead of failing during execution - Adds documentation maintenance rules and updates API docs to reflect new options, defaults, and error semantics
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| KTL_AI_Instructions.md | Adds documentation maintenance workflow rules |
| KTL.js | Implements filter validation, Promise.allSettled resilience, unified upload pipeline, asset size policy, input validation hardening, and helper utilities |
| Docs/KTL_API.md | Documents new options, defaults, behavior changes, and error handling semantics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _formatByteSize(bytes) { | ||
| if (!Number.isFinite(bytes) || bytes < 0) return '0 B'; | ||
| const units = ['B', 'KB', 'MB', 'GB', 'TB']; | ||
| if (bytes === 0) return '0 B'; |
There was a problem hiding this comment.
The check on line 5893 already returns '0 B' when bytes is 0 or negative, making the explicit if (bytes === 0) check on line 5895 unreachable. This line can be removed as it's redundant.
KTL.js
Outdated
| const fileSizeBytes = Number.isFinite(file?.size) ? file.size : 0; | ||
| const warnAssetSizeBytes = Number.isFinite(opts.warnAssetSizeBytes) && opts.warnAssetSizeBytes >= 0 | ||
| ? opts.warnAssetSizeBytes | ||
| : this.options.warnAssetSizeBytes; | ||
| const maxAssetSizeBytes = (opts.maxAssetSizeBytes === null) | ||
| ? null | ||
| : (Number.isFinite(opts.maxAssetSizeBytes) && opts.maxAssetSizeBytes > 0 | ||
| ? opts.maxAssetSizeBytes | ||
| : this.options.maxAssetSizeBytes); | ||
| const enforceAssetSize = opts.enforceAssetSize === true | ||
| ? true | ||
| : (opts.enforceAssetSize === false ? false : this.options.enforceAssetSize); | ||
|
|
||
| if (Number.isFinite(warnAssetSizeBytes) && warnAssetSizeBytes >= 0 && fileSizeBytes > warnAssetSizeBytes) { | ||
| this._log('Asset upload exceeds warning threshold', { | ||
| fileName: typeof file?.name === 'string' ? file.name : 'upload.bin', | ||
| fileSizeBytes, | ||
| warnAssetSizeBytes, | ||
| fileSizeLabel: this._formatByteSize(fileSizeBytes), | ||
| warnSizeLabel: this._formatByteSize(warnAssetSizeBytes) | ||
| }, 'warn'); | ||
| } | ||
|
|
||
| if (Number.isFinite(maxAssetSizeBytes) && maxAssetSizeBytes > 0 && fileSizeBytes > maxAssetSizeBytes) { | ||
| const sizeMessage = `KTL API error: uploadAsset file size ${this._formatByteSize(fileSizeBytes)} exceeds configured max ${this._formatByteSize(maxAssetSizeBytes)}.`; | ||
| if (enforceAssetSize) { | ||
| throw new Error(sizeMessage); | ||
| } | ||
|
|
||
| this._log('Asset upload exceeds configured max size but enforcement is disabled', { | ||
| fileName: typeof file?.name === 'string' ? file.name : 'upload.bin', | ||
| fileSizeBytes, | ||
| maxAssetSizeBytes, | ||
| fileSizeLabel: this._formatByteSize(fileSizeBytes), | ||
| maxSizeLabel: this._formatByteSize(maxAssetSizeBytes) | ||
| }, 'warn'); | ||
| } |
There was a problem hiding this comment.
When file.size is not finite, fileSizeBytes defaults to 0 (line 4786), which bypasses all size checks since 0 is never greater than any threshold. This means files without valid size information will silently skip warning and enforcement checks. Consider whether files with unknown size should be: (1) allowed without size checks (current behavior), (2) rejected when enforcement is enabled, or (3) at minimum logged as a warning. This could be relevant for edge cases where the File object doesn't properly expose its size property.
Improves asset upload logic by detecting unknown file sizes and bypassing size warning/enforcement for such files, logging a warning instead. Prevents false positives and errors when file size metadata is unavailable.
Refines file size handling for asset uploads, ensuring enforcement is robust when file size is unknown and clarifying warning/validation flows. Adds detailed documentation for exposed API methods to enhance developer clarity and usability.
Previous behavior flattened caller rules into an and group when caller passed match/rules, which could silently convert caller or intent into and. New behavior nests caller filters as a single sub-rule under the base equality filter, preserving caller match semantics. Improve paged read resilience in getAllRecords and getAllChildRecords.
Replace Promise.all batch fetch with allSettled handling so successful pages are retained even if one page fails. Add optional failOnPageError to support strict fail-fast behavior when required. Add warning logs with page context for failed pages. Route uploadAsset through the shared request pipeline.
Remove standalone fetch path and use common request logic for retries, backoff, timeout, and 429 handling consistency. Maintain write queue integration and existing return contract. Add configurable asset-size policy in uploadAsset.
New create-level and per-call controls: warnAssetSizeBytes maxAssetSizeBytes enforceAssetSize Warn when size exceeds warning threshold. Reject early with clear error when enforcement is enabled and max size is exceeded. Add human-readable byte formatting helper for diagnostics. Simplify request error construction.
Remove dead response-like shape branch that was never hit by current call paths. Keep structured error contract with status and body. Harden updateRecords input-shape detection.
Validate all elements before entering per-record mode. Throw clear error for mixed arrays instead of failing later during execution. Documentation updates.
Expand KTL API docs with new options, defaults, behavior changes, and read-path error semantics. Add AI workflow rule to check existing docs first, update when present, and only create docs for complex features.