Nc fix/attachments #8478
Nc fix/attachments #8478
Conversation
WalkthroughWalkthroughThe recent updates focus on improving attachment error handling in the NocoDB SDK. This includes introducing a new error type, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (2 hunks)
- packages/nocodb/src/helpers/catchError.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/nocodb-sdk/src/lib/globals.ts
Additional comments not posted (2)
packages/nocodb/src/helpers/catchError.ts (2)
528-532: AddINVALID_ATTACHMENT_JSONerror type toerrorHelpers.The addition of the
INVALID_ATTACHMENT_JSONerror type to theerrorHelpersobject looks good. The message function and the code 400 are appropriate for this error type.
696-701: AddinvalidAttachmentJsonstatic method toNcErrorclass.The new
invalidAttachmentJsonstatic method in theNcErrorclass is correctly implemented. It throws anNcBaseErrorv2with the appropriate error type and parameters.
| for (let i = 0; i < d[col.id].length; i++) { | ||
| if (typeof d[col.id][i] === 'string') { | ||
| d[col.id][i] = JSON.parse(d[col.id][i]); | ||
| } | ||
|
|
||
| const attachment = d[col.id][i]; | ||
|
|
There was a problem hiding this comment.
Ensure safe JSON parsing.
The current code assumes that the JSON string is always valid. To prevent potential crashes due to malformed JSON, consider wrapping the JSON.parse call in a try-catch block.
- if (typeof d[col.id][i] === 'string') {
- d[col.id][i] = JSON.parse(d[col.id][i]);
+ try {
+ if (typeof d[col.id][i] === 'string') {
+ d[col.id][i] = JSON.parse(d[col.id][i]);
+ }
+ } catch (e) {
+ NcError.invalidAttachmentJson(d[col.id][i]);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| for (let i = 0; i < d[col.id].length; i++) { | |
| if (typeof d[col.id][i] === 'string') { | |
| d[col.id][i] = JSON.parse(d[col.id][i]); | |
| } | |
| const attachment = d[col.id][i]; | |
| for (let i = 0; i < d[col.id].length; i++) { | |
| try { | |
| if (typeof d[col.id][i] === 'string') { | |
| d[col.id][i] = JSON.parse(d[col.id][i]); | |
| } | |
| } catch (e) { | |
| NcError.invalidAttachmentJson(d[col.id][i]); | |
| } | |
| const attachment = d[col.id][i]; | |
| try { | ||
| if (typeof data[column.column_name] === 'string') { | ||
| data[column.column_name] = JSON.parse(data[column.column_name]); | ||
| } | ||
| } catch (e) { | ||
| NcError.invalidAttachmentJson(data[column.column_name]); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure safe JSON parsing.
The current code correctly wraps the JSON.parse call in a try-catch block. However, it would be beneficial to log the error for debugging purposes.
- } catch (e) {
- NcError.invalidAttachmentJson(data[column.column_name]);
+ } catch (e) {
+ console.error('Failed to parse attachment JSON:', e);
+ NcError.invalidAttachmentJson(data[column.column_name]);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| try { | |
| if (typeof data[column.column_name] === 'string') { | |
| data[column.column_name] = JSON.parse(data[column.column_name]); | |
| } | |
| } catch (e) { | |
| NcError.invalidAttachmentJson(data[column.column_name]); | |
| } | |
| try { | |
| if (typeof data[column.column_name] === 'string') { | |
| data[column.column_name] = JSON.parse(data[column.column_name]); | |
| } | |
| } catch (e) { | |
| console.error('Failed to parse attachment JSON:', e); | |
| NcError.invalidAttachmentJson(data[column.column_name]); | |
| } | |
| if (!('url' in attachment) && !('path' in attachment)) { | ||
| NcError.unprocessableEntity( | ||
| 'Attachment object must contain either url or path', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Clarify error message for missing URL or path.
The error message could be more descriptive to help users understand what went wrong.
- 'Attachment object must contain either url or path',
+ 'Each attachment object must contain either a "url" or a "path" property',Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (!('url' in attachment) && !('path' in attachment)) { | |
| NcError.unprocessableEntity( | |
| 'Attachment object must contain either url or path', | |
| ); | |
| } | |
| if (!('url' in attachment) && !('path' in attachment)) { | |
| NcError.unprocessableEntity( | |
| 'Each attachment object must contain either a "url" or a "path" property', | |
| ); | |
| } |
|
Uffizzi Preview |
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (4)
packages/nocodb/src/services/attachments.service.ts (4)
Line range hint
121-121: Ensure consistent error handling for all attachment processing steps.The
attachmentPathassignment block should be wrapped in a try-catch to handle potential errors fromPresignedUrl.getSignedUrl.- attachmentPath = `download/${filePath.join('/')}/${fileName}`; + try { + attachmentPath = `download/${filePath.join('/')}/${fileName}`; + } catch (e) { + errors.push(e); + }
Line range hint
162-162: Optimize the mime type extraction.The mime type extraction can be simplified by using
path.extnamedirectly.- const type = - mimetypes[path.extname(param.path).split('/').pop().slice(1)] || - 'text/plain'; + const type = mimetypes[path.extname(param.path).slice(1)] || 'text/plain';
Line range hint
170-174: Simplify the preview availability check.The
previewAvailablemethod can be simplified by usingArray.includes.- const available = ['image', 'pdf', 'text/plain']; - if (available.some((type) => mimetype.includes(type))) { - return true; - } - return false; + return ['image', 'pdf', 'text/plain'].some((type) => mimetype.includes(type));
Line range hint
177-179: Ensure thesanitizeUrlPathmethod handles edge cases.The
sanitizeUrlPathmethod should handle empty strings and null values to avoid potential errors.- return paths.map((url) => url.replace(/[/.?#]+/g, '_')); + return paths.map((url) => (url ? url.replace(/[/.?#]+/g, '_') : ''));
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (2 hunks)
- packages/nocodb/src/helpers/catchError.ts (2 hunks)
- packages/nocodb/src/services/attachments.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/nocodb-sdk/src/lib/globals.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/catchError.ts
| attachment.path = path.join( | ||
| 'download', | ||
| filePath.join('/'), | ||
| fileName, | ||
| ); |
There was a problem hiding this comment.
Ensure consistent error handling for all attachment processing steps.
The attachment.path assignment block should be wrapped in a try-catch to handle potential errors from PresignedUrl.getSignedUrl.
- attachment.path = path.join(
- 'download',
- filePath.join('/'),
- fileName,
- );
- attachment.signedPath = await PresignedUrl.getSignedUrl({
- path: attachment.path.replace(/^download\//, ''),
- });
+ try {
+ attachment.path = path.join(
+ 'download',
+ filePath.join('/'),
+ fileName,
+ );
+ attachment.signedPath = await PresignedUrl.getSignedUrl({
+ path: attachment.path.replace(/^download\//, ''),
+ });
+ } catch (e) {
+ errors.push(e);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| attachment.path = path.join( | |
| 'download', | |
| filePath.join('/'), | |
| fileName, | |
| ); | |
| try { | |
| attachment.path = path.join( | |
| 'download', | |
| filePath.join('/'), | |
| fileName, | |
| ); | |
| attachment.signedPath = await PresignedUrl.getSignedUrl({ | |
| path: attachment.path.replace(/^download\//, ''), | |
| }); | |
| } catch (e) { | |
| errors.push(e); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (2 hunks)
- packages/nocodb/src/helpers/catchError.ts (2 hunks)
- packages/nocodb/src/services/attachments.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/nocodb-sdk/src/lib/globals.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/catchError.ts
- packages/nocodb/src/services/attachments.service.ts
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of