fix: Handle missing searchPath value while merging config #10722
Conversation
📝 WalkthroughWalkthroughThis PR updates two files. In the configuration source module, a new variable Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant S as Source.getConfig
participant D as deepMerge
C->>S: Call getConfig
S->>D: Call deepMerge(integrationConfig, configPartial)
D-->>S: Return merged configuration
S->>S: Validate mergedConfig.searchPath (set to undefined if invalid)
S-->>C: Return mergedConfig
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
packages/nocodb/src/utils/dataUtils.ts (1)
79-81: Excellent improvement for proper type initialization in the deepMerge function.The change properly initializes nested target objects based on the source data type (array or object). This prevents potential issues when merging arrays into objects and vice versa, ensuring more accurate deep merging behavior.
For even more robust implementation, consider adding a check for the
nullcase:- if (!target[key]) target[key] = Array.isArray(source[key]) ? [] : {}; + if (!target[key]) target[key] = Array.isArray(source[key]) ? [] : (source[key] === null ? null : {});packages/nocodb/src/models/Source.ts (1)
378-385: Consider explicit handling for different empty searchPath scenarios.The current implementation treats empty strings and empty arrays the same way by setting them to
undefined. This is generally fine, but it might be worth considering if these cases should be handled differently.You could consider making the handling more explicit if there are different expectations for empty strings vs. empty arrays:
if ( (!Array.isArray(mergedConfig.searchPath) && typeof mergedConfig.searchPath !== 'string') || - !mergedConfig.searchPath?.length + (Array.isArray(mergedConfig.searchPath) && mergedConfig.searchPath.length === 0) || + (typeof mergedConfig.searchPath === 'string' && mergedConfig.searchPath.length === 0) ) { mergedConfig = { ...mergedConfig, searchPath: undefined }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nocodb/src/models/Source.ts(1 hunks)packages/nocodb/src/utils/dataUtils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (3)
packages/nocodb/src/models/Source.ts (3)
370-376: Improved code organization with a named variable.Storing the result of
deepMergein a named variablemergedConfigenhances code readability and maintainability. This also allows for further manipulation of the merged configuration.
387-387: LGTM!The updated return statement is aligned with the previous changes, now returning the properly validated merged configuration.
378-385:✅ Verification successful
Great fix for handling invalid searchPath values.
The added validation properly handles cases where
searchPathis missing, invalid, or empty by explicitly setting it toundefined. This prevents potential issues downstream when using this configuration.Let's verify our handling of various
searchPathvalues to ensure all edge cases are covered:
🏁 Script executed:
#!/bin/bash # Check how searchPath is handled in the codebase # Find other places in the codebase that use searchPath echo "Searching for searchPath usage in the codebase..." rg "searchPath" --context 5 | grep -v "Source.ts" # Check for any existing value validations echo "Checking for existing searchPath validation in the codebase..." rg "searchPath.*(!|\?|isArray|typeof)" --context 3Length of output: 90340
SearchPath validation confirmed across the codebase.
The changes in Source.ts properly reset an invalid or emptysearchPathtoundefined, which is in line with similar validations in other parts of the code (for example, in EditBase.vue and CreateBase.vue). This consistency helps prevent downstream issues when the value is used. No further modifications appear necessary.
|
Uffizzi Preview |
Change Summary
Re - https://discord.com/channels/661905455894888490/1346768933562224661
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of