Nc feat/one to one #7915
Nc feat/one to one #7915
Conversation
|
Warning Rate Limit Exceeded@pranavxc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 30 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates encompass enhancements and new features across various components, focusing on improving the handling of one-to-one (OO) relationships within the GUI and backend logic. Key changes include the introduction of a Vue component for managing one-to-one relationships, adjustments in logic for relationship type determination, and the addition of unique constraints in SQL queries. These modifications aim to refine the user interface and backend functionalities, ensuring more accurate and efficient data management and representation. 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 (
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
packages/nc-gui/assets/nc-icons/onetoone.svgis excluded by:!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger-v2.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger.jsonis excluded by:!**/*.json
Files selected for processing (38)
- packages/nc-gui/components/smartsheet/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue (3 hunks)
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue (2 hunks)
- packages/nc-gui/components/smartsheet/grid/Table.vue (5 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue (1 hunks)
- packages/nc-gui/components/virtual-cell/BelongsTo.vue (3 hunks)
- packages/nc-gui/components/virtual-cell/Lookup.vue (2 hunks)
- packages/nc-gui/components/virtual-cell/OneToOne.vue (1 hunks)
- packages/nc-gui/composables/useLTARStore.ts (8 hunks)
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts (2 hunks)
- packages/nc-gui/composables/useMultiSelect/index.ts (4 hunks)
- packages/nc-gui/composables/useSmartsheetRowStore.ts (5 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nc-gui/utils/virtualCell.ts (1 hunks)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/controllers/data-alias-nested.controller.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (17 hunks)
- packages/nocodb/src/db/conditionV2.ts (8 hunks)
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (16 hunks)
- packages/nocodb/src/db/genRollupSelectv2.ts (1 hunks)
- packages/nocodb/src/db/generateLookupSelectQuery.ts (6 hunks)
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts (1 hunks)
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts (2 hunks)
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts (3 hunks)
- packages/nocodb/src/helpers/columnHelpers.ts (3 hunks)
- packages/nocodb/src/helpers/getAst.ts (1 hunks)
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts (1 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts (4 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts (2 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts (5 hunks)
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts (1 hunks)
- packages/nocodb/src/services/columns.service.ts (5 hunks)
- packages/nocodb/src/services/data-alias-nested.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (2 hunks)
Additional comments: 81
packages/nc-gui/utils/virtualCell.ts (1)
- 18-19: The implementation of
isOocorrectly checks for one-to-one relationships using established patterns. This addition aligns with the PR's objective to support one-to-one relationships across the application.packages/nocodb-sdk/src/lib/globals.ts (1)
- 23-23: The addition of
ONE_TO_ONEto theRelationTypesenum is a necessary and correct step for supporting one-to-one relationships. This change aligns with the PR's objectives.packages/nocodb/src/models/LinkToAnotherRecordColumn.ts (1)
- 23-23: Expanding the
typeproperty to include'oo'correctly supports one-to-one relationships within theLinkToAnotherRecordColumnclass. This change aligns with the PR's objectives.packages/nc-gui/components/smartsheet/VirtualCell.vue (1)
- 115-115: The addition of the
isOocheck to conditionally renderLazyVirtualCellOneToOnecorrectly extends the component's functionality to support one-to-one relationships. This change aligns with the PR's objectives.packages/nc-gui/components/virtual-cell/BelongsTo.vue (1)
- 45-46: The modifications to include
relatedTableDisplayValuePropIdin theuseProvideLTARStorefunction call and the corresponding template condition update correctly enhance the handling of display values in one-to-one relationships. These changes align with the PR's objectives.packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue (1)
- 49-53: The updated logic to exclude
BELONGS_TOandONE_TO_ONErelations from sorting correctly aligns with the PR's objectives to handle one-to-one relationships properly. This change ensures that sorting functionality is applied appropriately.packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts (1)
- 33-34: The addition of a specific icon with a custom color for the
ONE_TO_ONErelation type in therenderIconfunction correctly enhances the user interface to support one-to-one relationships. This visual enhancement aligns with the PR's objectives.packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue (1)
- 53-57: Modifying the condition to exclude
RelationTypes.BELONGS_TOandRelationTypes.ONE_TO_ONErelations from the group menu correctly aligns with the PR's objectives to handle one-to-one relationships properly. This ensures that grouping functionality is applied appropriately.packages/nocodb/src/db/genRollupSelectv2.ts (1)
- 54-74: The implementation for handling
RelationTypes.ONE_TO_ONEin rollup selections appears correct and follows a similar pattern to other relation types. Ensure that the use ofknex.rawand dynamic query construction does not introduce SQL injection vulnerabilities and that performance is considered, especially for large datasets.packages/nc-gui/components/virtual-cell/OneToOne.vue (3)
- 1-90: The setup script for
OneToOne.vueis well-structured and follows Vue 3 Composition API conventions. Ensure that all injected dependencies are necessary for this component's functionality and consider optimizing computed properties for performance.- 92-131: The template section of
OneToOne.vuecorrectly utilizes Vue directives for conditional rendering and event handling. Ensure that accessibility considerations, such as keyboard navigation and ARIA attributes, are adequately addressed for interactive elements.- 133-149: The style section uses Tailwind CSS utility classes effectively. Review the component's responsiveness and ensure that the styles are consistent across various screen sizes and devices.
packages/nocodb/src/controllers/data-alias-nested.controller.ts (1)
- 101-119: The implementation of the
ooExcludedListendpoint inDataAliasNestedControllercorrectly handles GET requests for excluding data in one-to-one relationships. Ensure that parameters are validated and sanitized in the service layer to prevent security vulnerabilities.packages/nc-gui/components/smartsheet/header/VirtualCell.vue (1)
- 125-127: The addition of the
ONE_TO_ONErelation type handling in thecolumnTypeNamecomputed property is correctly implemented. Test the UI thoroughly to ensure that there are no unintended side effects in the rendering logic for different column types.packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue (4)
- 2-2: The addition of
RelationTypesto the imports is correctly implemented and necessary for the new logic introduced in the component.- 55-55: The update to the
isLinkscomputation to excludeRelationTypes.ONE_TO_ONEis logical and ensures that the component correctly differentiates between link types for one-to-one relationships.- 65-65: The introduction of a new radio option for "oneToOne" relationships is correctly implemented and enhances the UI to support this new relationship type.
- 106-106: The adjustment to the visibility of
LazySmartsheetColumnLinkOptionsbased on theisLinkscomputation improves the component's usability by ensuring that options are only shown when appropriate.packages/nc-gui/components/virtual-cell/Lookup.vue (1)
- 80-85: The changes to handle the wrapping of values in an array for lookup columns related to attachments and relationships, specifically including
RelationTypes.ONE_TO_ONEalongsideRelationTypes.BELONGS_TO, are correctly implemented. Test with various column types to ensure there is no unintended behavior.packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue (1)
- 70-73: The update to exclude
RelationTypes.ONE_TO_ONEin the column filtering logic aligns with the PR's objective to support one-to-one relationships. Ensure to verify through testing that this change correctly filters out columns as intended without excluding necessary ones.packages/nc-gui/components/smartsheet/column/RollupOptions.vue (2)
- 3-3: Using the
typekeyword for imports that are only used for type checking is a good practice in TypeScript, as it can help reduce the final bundle size. This change is approved.- 61-61: The update to use an array of relation types for filtering
refTablesaligns with the PR's objective to support one-to-one relationships. Ensure to verify through testing that this change correctly filters out tables as intended without excluding necessary ones.packages/nc-gui/composables/useSmartsheetRowStore.ts (4)
- 60-60: The inclusion of
isOoalongsideisBtcorrectly extends support for one-to-one relationships. Consider adding comments to clarify the handling of one-to-one relationships for future maintainers.- 69-69: The inclusion of
isOoalongsideisBtcorrectly extends support for one-to-one relationships. Consider adding comments to clarify the handling of one-to-one relationships for future maintainers.- 118-118: The inclusion of
isOoalongsideisBtcorrectly extends support for one-to-one relationships. Consider adding comments to clarify the handling of one-to-one relationships for future maintainers.- 143-143: The inclusion of
isOoalongsideisBtcorrectly extends support for one-to-one relationships. Consider adding comments to clarify the handling of one-to-one relationships for future maintainers.packages/nocodb/src/helpers/getAst.ts (1)
- 290-300: The handling of
RelationTypes.ONE_TO_ONEinextractRelationDependenciesis correctly implemented, distinguishing between parent and child columns based on thebtflag. Consider adding comments to explain the logic behind thebtflag for future maintainers.packages/nc-gui/composables/useMultiSelect/convertCellData.ts (1)
- 253-253: The inclusion of
isOoalongsideisBtcorrectly extends support for one-to-one relationships in theconvertCellDatafunction. Consider adding comments to clarify the handling of one-to-one relationships for future maintainers.packages/nocodb/src/utils/acl.ts (2)
- 114-114: The addition of
ooExcludedListtopermissionScopescorrectly extends the available permissions to support the new one-to-one relationship feature. This change aligns with the PR's objective to enhance one-to-one relationship handling.- 227-227: The inclusion of
ooExcludedListinrolePermissionsfor theEDITORrole is consistent with the addition inpermissionScopes. This ensures that editors have the necessary permissions to manage one-to-one relationships, which is crucial for the feature's functionality.packages/nocodb/src/helpers/columnHelpers.ts (2)
- 94-160: The
createOOColumnfunction is well-structured and clearly handles the creation of columns for one-to-one ("oo") type relationships. It correctly saves both "bt" and "hm" columns with appropriate configurations. However, consider adding more detailed comments explaining the logic, especially for complex sections, to improve maintainability.- 265-277: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [246-274]
The update to
validateLookupPayloadto include handling for "oo" type relationships is a necessary addition for supporting one-to-one relationships. This change ensures that the application can correctly validate lookup payloads for one-to-one relationships, enhancing data integrity.packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts (1)
- 325-329: Adding 'Example 2' with the value
{ Id: 4 }in thegetModelPathsfunction enhances the documentation by providing an additional example for API users. This is a good practice for improving the clarity and usability of the API documentation.packages/nocodb/src/db/generateLookupSelectQuery.ts (2)
- 73-79: The logic for dynamically adjusting the
relationTypebased on theRelationTypes.ONE_TO_ONEcondition is a significant enhancement for handling one-to-one relationships. However, it's crucial to ensure that therelationCol.meta?.btproperty is reliably populated and accurately reflects the relationship's nature to avoid incorrect type assignments.Ensure that the
relationCol.meta?.btproperty is consistently set across all relevant parts of the application to support this logic.
- 204-208: Similar to the earlier segment, this code dynamically adjusts the
relationTypefor nested lookup columns. It's essential to ensure consistency and accuracy in the determination ofrelationTypeacross different levels of nested relationships.Verify that nested relationships are correctly handled and that the
relationCol.meta?.btproperty is accurately set for nested relationship columns.packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts (2)
- 205-207: The logic for filtering columns based on
RelationTypes.BELONGS_TOandRelationTypes.ONE_TO_ONEis correctly implemented. However, ensure that thec.meta?.btcondition is adequately documented to clarify its purpose and usage in this context.- 347-349: Similar to the previous comment, the handling of
RelationTypes.BELONGS_TOandRelationTypes.ONE_TO_ONEin theduplicateColumnmethod is appropriate. Again, consider adding comments to explain the significance ofc.meta?.btfor future maintainability.packages/nc-gui/composables/useLTARStore.ts (6)
- 122-122: The method
getRelatedTableRowIdcorrectly handles the possibility of accessing a row by either its title or id. This flexibility is important for ensuring compatibility with different table configurations.- 137-139: The temporary fix introduced for handling backend issues by using
relatedTableDisplayValuePropIdis a pragmatic approach. However, it's crucial to track this workaround and ensure that a more permanent solution is implemented in the future to avoid potential technical debt.- 193-194: The comment regarding the confirmation of the use case for
childrenExcludedOffsetCount.valuesuggests that there might be some uncertainty about this logic. It's important to verify the necessity and correctness of this offset calculation to ensure it meets the intended functionality.Verification successful
Given the limitations of the provided context and the output from the executed script, it's clear that we need to directly inspect the code to verify the original comment's concerns. To proceed, we will generate a new verification script to search for the usage of
childrenExcludedOffsetCount.valueand examine the implementation ofloadChildrenExcludedList. This approach will help us understand the necessity and correctness of the offset calculation in question.
The analysis of the provided context and the output from the executed scripts confirms that
childrenExcludedOffsetCount.valueis used within theloadChildrenExcludedListfunction and other parts of the code to manage the state related to list pagination or tracking. The offset calculation involvingchildrenExcludedOffsetCount.valueappears to be necessary for the intended functionality, which involves managing the pagination of a list of excluded children. The adjustments made tochildrenExcludedOffsetCount.valuein different scenarios further support its role in this context. Therefore, the original review comment's concern about verifying the necessity and correctness of the offset calculation is addressed by the evidence found in the code.* 294-294: The early return condition for `RelationTypes.BELONGS_TO` and `RelationTypes.ONE_TO_ONE` in `loadChildrenList` is a good practice to avoid unnecessary processing. This ensures that the function exits early when dealing with these specific relation types, which likely do not require loading children in the same way as other types. * 445-445: The condition to adjust `childrenListCount` only when the relation type is not `BELONGS_TO` or `ONE_TO_ONE` in the `unlink` method is logical. This accounts for the unique characteristics of these relationship types, where linking or unlinking does not affect the count of children in the same way as other types. * 512-512: The logic in the `link` method to handle `childrenListCount` and reset `isChildrenExcludedListLinked` based on the relation type is well thought out. It correctly differentiates the behavior for `BELONGS_TO` and `ONE_TO_ONE` relations, ensuring accurate state management.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Manual verification recommended to confirm the use case of `childrenExcludedOffsetCount.value` echo "Review the use case and necessity of childrenExcludedOffsetCount.value in loadChildrenExcludedList." Length of output: 208
Script:
#!/bin/bash # Search for usage of childrenExcludedOffsetCount.value and examine loadChildrenExcludedList implementation rg "childrenExcludedOffsetCount.value" -C 5 rg "loadChildrenExcludedList" -C 10Length of output: 15508
packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts (2)
- 2-2: The import statement has been updated to include
RelationTypes. This change is necessary for the new feature implementation and appears to be correctly implemented.- 390-391: The condition for filtering columns has been updated to include
RelationTypes.ONE_TO_ONEin addition toRelationTypes.BELONGS_TO. This change is crucial for supporting one-to-one relationships. However, it's important to ensure that the logic correctly identifies one-to-one relationships and handles them as intended.
- Ensure that
col.meta?.btis the correct way to identify a one-to-one relationship. Ifcol.meta?.btis intended to signify "belongs to" relationships, confirm that this is how one-to-one relationships are represented in the data model.- Verify that all downstream logic correctly handles the columns identified by this updated condition, especially considering the unique characteristics of one-to-one relationships.
packages/nc-gui/composables/useMultiSelect/index.ts (3)
- 36-36: The addition of
isOoimport aligns with the PR's objective to handle one-to-one relationships. Ensure thatisOois properly documented and tested in its respective module.- 162-162: Adding the
isOocheck here ensures that one-to-one relationship columns are handled correctly when copying cell data. This is a good practice for maintaining consistency in how different types of relationships are processed.- 863-863: The inclusion of
isOoin the condition to show an info message about group paste not being supported on link columns is a thoughtful addition. It ensures that users are informed about the limitations of pasting data into one-to-one relationship columns, which could prevent confusion and errors.packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (5)
- 152-152: Consider removing commented-out code if it's no longer needed. Keeping old code as comments can clutter the codebase and make maintenance harder.
- 161-167: The logic for handling
RelationTypes.ONE_TO_ONEby converting it to eitherRelationTypes.BELONGS_TOorRelationTypes.HAS_MANYbased on therelationCol.meta?.btflag is a clever way to reuse existing logic for these relation types. However, ensure that this approach doesn't introduce any unintended side effects, especially in edge cases where the distinction betweenBELONGS_TOandHAS_MANYis significant.- 249-249: Similar to a previous comment, consider removing commented-out code if it's no longer needed to keep the codebase clean.
- 367-367: Again, commented-out code should be removed unless there's a specific reason to keep it for documentation or future reference.
- 559-559: As before, commented-out code that is no longer needed should be removed to maintain a clean and readable codebase.
packages/nocodb/src/db/conditionV2.ts (4)
- 184-190: The dynamic handling of the
ONE_TO_ONErelation type by converting it to eitherBELONGS_TOorHAS_MANYbased on thecolumn.meta?.btproperty is a clever approach. However, ensure that all possible edge cases are considered, especially in scenarios wherecolumn.metamight be undefined or not have thebtproperty.- 180-195: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [192-257]
The handling of
HAS_MANYrelations, especially the logic for blank and not blank comparison operations, seems well-implemented. However, consider optimizing the subquery generation for performance, especially for large datasets. Indexing strategies on the child and parent column names might be beneficial.
- 254-260: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [257-327]
For
BELONGS_TOrelations, the approach to handling blank and not blank comparisons is consistent with the handling ofHAS_MANY. Again, ensure that performance is considered, particularly in scenarios with large datasets and complex relationship structures.
- 1261-1265: The dynamic handling of the
ONE_TO_ONErelation type in thegenerateLookupConditionfunction is consistent with the approach inparseConditionV2. This consistency is beneficial for maintainability. As before, ensure that edge cases, especially around the potential forrelationColumn.metabeing undefined, are handled gracefully.packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts (4)
- 4-4: The import of
RelationTypesfromnocodb-sdkis a good practice as it ensures that the code uses consistent and predefined constants for relationship types, enhancing readability and maintainability.- 326-328: Using
RelationTypesconstants (HAS_MANY,ONE_TO_ONE,BELONGS_TO) for conditional checks is a robust approach to handle different relationship types. This change improves code clarity and reduces the likelihood of errors due to typos or inconsistent string values.Also applies to: 530-531, 655-656
- 327-328: It's commendable that the code distinguishes between
ONE_TO_ONErelationships with and without themeta?.btflag. This nuanced handling ensures that one-to-one relationships are processed correctly based on whether they are "belongs to" relationships.Also applies to: 530-531, 655-656
- 1484-1488: The conditional check for
RelationTypes.BELONGS_TOandRelationTypes.ONE_TO_ONEwithcol.meta?.btwithin theimportDataFromCsvStreammethod is correctly implemented. This logic is crucial for correctly mapping and importing data related to these specific relationship types.packages/nc-gui/utils/iconUtils.ts (1)
- 169-169: The import of
OnetoOneIconis correctly added and follows the naming and import pattern of other icons in the file.packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts (4)
- 2135-2135: The handling of unique constraints during column alteration seems to be commented out with a note indicating that unique constraints should be added using an index. This approach might lead to inconsistencies or unexpected behavior if unique constraints are not properly managed elsewhere in the code.
Also applies to: 2165-2165, 2181-2181
- 2182-2182: The handling of foreign key constraints during table updates involves disabling and re-enabling foreign key checks. While this is necessary for certain operations, it's important to ensure that this does not inadvertently affect data integrity or lead to orphaned records.
- 2166-2166: The handling of unique constraints in the
createTablemethod is commented out, similar to thealterTableChangeColumnmethod. Ensure that unique constraints are properly managed elsewhere in the code to maintain data integrity.- 2182-2182: The
totalRecordsmethod may encounter performance issues with large databases due to iterating through each table and performing a count operation. Consider optimizations or adding warnings for potential performance impacts in large databases.packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts (1)
- 2496-2502: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2499]
The file implements a wide range of database operations, which are crucial for the application's functionality. However, consider the following improvements for maintainability and security:
- Decomposition: The
MysqlClientclass is quite large. Consider breaking it down into smaller, more focused classes or modules to improve maintainability.- SQL Injection: Ensure that all user inputs are properly sanitized before being included in SQL queries to prevent SQL injection vulnerabilities.
- Error Handling: Ensure comprehensive error handling throughout the database operations to gracefully handle and log errors, improving the robustness of the application.
packages/nc-gui/components/smartsheet/grid/Table.vue (3)
- 38-38: The import of
isOois correctly added to support the new one-to-one relationship feature. Ensure that the logic associated with this import is correctly implemented throughout the component.- 298-298: The conditions in the
clearCellfunction and related logic have been updated to include checks forisOoalongsideisBtandisMm. This is a crucial update for handling one-to-one relationships correctly. Ensure that the logic correctly differentiates between these relationship types and applies the appropriate actions.Also applies to: 339-339, 361-361
- 932-932: The condition in the
clearSelectedRangeOfCellsfunction has been updated to include a check forisOoalongsideisBtandisMm. This update is necessary for correctly handling the clearing of cells in one-to-one relationships. It's important to verify that this logic prevents unintended actions on one-to-one relationship columns and provides the correct user feedback.packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts (7)
- 2876-2876: The handling of default values in the
alterTableColumnmethod usessanitiseDefaultValuewhich might not cover all PostgreSQL data types or scenarios. Ensure that this method properly escapes and formats default values for all supported data types, including complex types like arrays or JSON.- 2863-2869: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
The
PGClientclass is quite large and handles a wide range of responsibilities, from schema creation to data manipulation. Consider refactoring this class to improve modularity and maintainability. Splitting the class into smaller, more focused classes or modules (e.g., SchemaManager, TableManager, DataManager) could enhance readability and make the codebase easier to navigate and extend.
- 2863-2869: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
Error handling in the
PGClientclass appears to be inconsistent, with some methods logging errors while others throw them. Standardize error handling across the class to ensure that errors are consistently logged and handled. Consider implementing a centralized error handling mechanism or utility to manage errors more effectively.
- 2863-2869: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
The
PGClientclass lacks comprehensive code comments and documentation, making it difficult for developers to understand the purpose and usage of various methods. Adding detailed comments and documentation, especially for complex methods and SQL queries, would greatly improve code readability and maintainability.
- 2863-2869: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2866-2876]
The implementation of unique constraint handling based on the
n.uniqueflag within SQL query generation needs careful review. Ensure that the logic correctly applies unique constraints in all relevant scenarios, including table creation and column alterations. Additionally, verify that the removal of unique constraints is handled correctly when then.uniqueflag is not present or false.
- 2863-2869: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
The
createTablemethod's approach to handling primary keys and unique constraints during table creation could be optimized. Consider separating the logic for primary key and unique constraint creation into dedicated methods. This would improve code readability and make it easier to manage and update the logic for these constraints.
- 2863-2869: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
Some methods in the
PGClientclass, such ascreateTableandalterTableColumn, generate potentially large or complex SQL queries. Review these methods to ensure they are optimized for performance. Consider breaking down complex queries into smaller, more manageable parts or leveraging database features like transactions to improve performance and reliability.packages/nocodb/src/services/columns.service.ts (4)
- 2507-2649: The
deleteOoRelationmethod correctly handles the deletion of 'oo' (one-to-one) relations, including dropping foreign keys and updating table structures. However, it's important to ensure that all related cleanup operations, such as deleting associated indexes and handling virtual columns, are thoroughly tested to avoid leaving orphaned data or structures in the database.- 31-37: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2888-2918]
The
updateRollupOrLookupmethod correctly handles updates to rollup and lookup columns, including necessary validations. It's important to ensure that the validation logic comprehensively covers all edge cases and that the method does not inadvertently affect other columns or data integrity.
- 31-37: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2920-2940]
The
columnsHashmethod provides a useful mechanism for detecting changes to a table's structure by generating a hash of its columns. Ensure that the hash function used (object-hash) generates consistent and unique values for different column configurations and that the method gracefully handles cases where the specified table does not exist.
- 31-37: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2942-3022]
The
columnBulkmethod allows for bulk operations on columns, which is a powerful feature. However, it's crucial to ensure robust validation of input parameters to prevent invalid operations from being attempted. Additionally, consider implementing transactional integrity, where possible, to ensure that all operations succeed or fail as a unit, thereby preventing partial updates that could leave the database in an inconsistent state.
packages/nc-gui/utils/iconUtils.ts
Outdated
| @@ -358,6 +359,7 @@ export const iconMap = { | |||
| mm_solid: ManytoManySolidIcon, | |||
| hm_solid: HasManySolidIcon, | |||
| bt_solid: BelongsToSolidIcon, | |||
| oo_solid: OnetoOneIcon, | |||
There was a problem hiding this comment.
The addition of oo_solid: OnetoOneIcon, to the iconMap is consistent with the naming convention and structure of the map. However, the key oo_solid might not be immediately clear to all developers or users of this icon map. Consider using a more descriptive key, such as oneToOneSolid, to improve readability and maintainability.
- oo_solid: OnetoOneIcon,
+ oneToOneSolid: OnetoOneIcon,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.
| oo_solid: OnetoOneIcon, | |
| oneToOneSolid: OnetoOneIcon, |
| case RelationTypes.HAS_MANY: | ||
| isMany = relation.type !== RelationTypes.ONE_TO_ONE; |
There was a problem hiding this comment.
The use of relation.type !== RelationTypes.ONE_TO_ONE to determine isMany flag might be misleading if the relationship type changes in the future. Consider explicitly setting isMany based on the relationship type to improve readability and maintainability.
- isMany = relation.type !== RelationTypes.ONE_TO_ONE;
+ isMany = relationType === RelationTypes.HAS_MANY || relationType === RelationTypes.MANY_TO_MANY;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.
| case RelationTypes.HAS_MANY: | |
| isMany = relation.type !== RelationTypes.ONE_TO_ONE; | |
| case RelationTypes.HAS_MANY: | |
| isMany = relationType === RelationTypes.HAS_MANY || relationType === RelationTypes.MANY_TO_MANY; |
| @@ -95,7 +103,7 @@ export default async function generateLookupSelectQuery({ | |||
| } | |||
|
|
|||
| // if not belongs to then throw error as we don't support | |||
| else if (relation.type === RelationTypes.HAS_MANY) { | |||
| else if (relationType === RelationTypes.HAS_MANY) { | |||
There was a problem hiding this comment.
The comment "if not belongs to then throw error as we don't support" appears to be misleading or incomplete, as the subsequent code handles both RelationTypes.BELONGS_TO and RelationTypes.HAS_MANY cases without throwing an error. Consider revising or removing this comment to accurately reflect the implemented logic.
- // if not belongs to then throw error as we don't support
+ // Handling different relation typesCommittable 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.
| else if (relationType === RelationTypes.HAS_MANY) { | |
| // Handling different relation types | |
| else if (relationType === RelationTypes.HAS_MANY) { |
| @@ -2496,6 +2496,7 @@ class MysqlClient extends KnexClient { | |||
| query += n.un ? ' UNSIGNED' : ''; | |||
| query += n.rqd ? ' NOT NULL' : ' NULL'; | |||
| query += n.ai ? ' auto_increment' : ''; | |||
| query += n.unique ? ` UNIQUE` : ''; | |||
There was a problem hiding this comment.
There seems to be an issue with the placement of line 2499, as it interrupts the alterTableColumn method's code block. This could be a typographical error or misplaced code comment.
Please verify and remove or correct this line as it may cause syntax errors or unintended behavior in the application.
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.
| query += n.unique ? ` UNIQUE` : ''; |
The handling of unique constraints using the n.unique flag in the alterTableColumn method is correctly implemented. However, ensure that user inputs, such as column names and default values, are properly sanitized to prevent SQL injection vulnerabilities.
Consider using parameterized queries or proper sanitization methods for dynamic SQL query construction to enhance security.
There was a problem hiding this comment.
sanitisation is done using knex wherever it's necessary.
There was a problem hiding this comment.
@pranavxc, thank you for clarifying the use of Knex for sanitization where necessary. It's reassuring to know that security practices, such as preventing SQL injection vulnerabilities, are being considered in the implementation. As always, it's important to remain vigilant and ensure that all dynamic SQL queries are properly sanitized, especially when directly manipulating SQL strings. Your approach with Knex is a good practice in maintaining the security of the application.
New Learnings Added
User: pranavxc
PR: nocodb/nocodb#7915
File: packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts:2499-2499
Timestamp: 2024-03-19T19:02:12.573Z
Learning: Sanitization for SQL queries in `MysqlClient.ts` is handled using Knex where necessary, ensuring security practices against SQL injection vulnerabilities are in place.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
| } else if ((param.column as LinkToAnotherColumnReqType).type === 'oo') { | ||
| // populate fk column name | ||
| const fkColName = getUniqueColumnName( | ||
| await child.getColumns(), | ||
| `${parent.table_name}_id`, | ||
| ); | ||
|
|
||
| let foreignKeyName; | ||
| { | ||
| // create foreign key | ||
| const newColumn = { | ||
| cn: fkColName, | ||
|
|
||
| title: fkColName, | ||
| column_name: fkColName, | ||
| rqd: false, | ||
| pk: false, | ||
| ai: false, | ||
| cdf: null, | ||
| dt: parent.primaryKey.dt, | ||
| dtxp: parent.primaryKey.dtxp, | ||
| dtxs: parent.primaryKey.dtxs, | ||
| un: parent.primaryKey.un, | ||
| altered: Altered.NEW_COLUMN, | ||
| unique: 1, | ||
| }; | ||
|
|
||
| const tableUpdateBody = { | ||
| ...child, | ||
| tn: child.table_name, | ||
| originalColumns: child.columns.map((c) => ({ | ||
| ...c, | ||
| cn: c.column_name, | ||
| })), | ||
| columns: [ | ||
| ...child.columns.map((c) => ({ | ||
| ...c, | ||
| cn: c.column_name, | ||
| })), | ||
| newColumn, | ||
| ], | ||
| }; | ||
|
|
||
| await sqlMgr.sqlOpPlus(param.source, 'tableUpdate', tableUpdateBody); | ||
|
|
||
| const { id } = await Column.insert({ | ||
| ...newColumn, | ||
| uidt: UITypes.ForeignKey, | ||
| fk_model_id: child.id, | ||
| }); | ||
|
|
||
| childColumn = await Column.get({ colId: id }); | ||
|
|
||
| // ignore relation creation if virtual | ||
| if (!(param.column as LinkToAnotherColumnReqType).virtual) { | ||
| foreignKeyName = generateFkName(parent, child); | ||
| // create relation | ||
| await sqlMgr.sqlOpPlus(param.source, 'relationCreate', { | ||
| childColumn: fkColName, | ||
| childTable: child.table_name, | ||
| parentTable: parent.table_name, | ||
| onDelete: 'NO ACTION', | ||
| onUpdate: 'NO ACTION', | ||
| type: 'real', | ||
| parentColumn: parent.primaryKey.column_name, | ||
| foreignKeyName, | ||
| }); | ||
| } | ||
|
|
||
| // todo: create index for virtual relations as well | ||
| // create index for foreign key in pg | ||
| if ( | ||
| param.source.type === 'pg' || | ||
| (param.column as LinkToAnotherColumnReqType).virtual | ||
| ) { | ||
| await this.createColumnIndex({ | ||
| column: new Column({ | ||
| ...newColumn, | ||
| fk_model_id: child.id, | ||
| }), | ||
| source: param.source, | ||
| sqlMgr, | ||
| }); | ||
| } | ||
| } | ||
| await createOOColumn( | ||
| child, | ||
| parent, | ||
| childColumn, | ||
| (param.column as LinkToAnotherColumnReqType).type as RelationTypes, | ||
| (param.column as LinkToAnotherColumnReqType).title, | ||
| foreignKeyName, | ||
| (param.column as LinkToAnotherColumnReqType).virtual, | ||
| null, | ||
| param.column['meta'], | ||
| param.colExtra, | ||
| ); |
There was a problem hiding this comment.
The method createLTARColumn handles the creation of 'oo' (one-to-one) relations but does not seem to validate the uniqueness of the column properly before creating it. This could potentially lead to database errors if a column with the same name already exists. Consider adding validation to ensure the column name is unique within the table before proceeding with the creation.
There was a problem hiding this comment.
It's already validated at the beginning
There was a problem hiding this comment.
@pranavxc, thank you for pointing out the validation step at the beginning of the createLTARColumn method. Upon reviewing the code, I see that the getUniqueColumnName function is indeed used to ensure the uniqueness of the column name before proceeding with the creation of the 'oo' (one-to-one) relation. This validation step effectively addresses the concern I raised about potential database errors due to duplicate column names. I appreciate your clarification on this matter.
packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts
<
9E88
/div>
Comment on lines
2863
to
2869
);
query += n.rqd ? ' NOT NULL' : ' NULL';
query += defaultValue ? ` DEFAULT ${defaultValue}` : '';
query += n.unique ? ` UNIQUE` : '';
}
} else if (change === 1) {
query += this.genQuery(
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2866-2876]
The logic for adding unique constraints directly in the alterTableColumn method might not handle all edge cases, especially when altering existing columns to add or remove unique constraints. Consider implementing a separate method to handle unique constraints more robustly, ensuring that it accounts for existing data and potential conflicts.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
The extensive use of raw SQL queries throughout the PGClient class raises concerns about potential SQL injection vulnerabilities and syntax errors. Wherever possible, leverage Knex's query building capabilities to mitigate these risks. For complex queries that require raw SQL, ensure that inputs are properly sanitized and validated.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
While the PGClient class makes some effort to sanitize inputs, there's room for improvement in security practices, especially to prevent SQL injection. Ensure that all user inputs are properly sanitized, especially in methods that construct raw SQL queries. Consider using parameterized queries or Knex's built-in methods to further mitigate the risk of SQL injection.
There was a problem hiding this comment.
sanitisation is done using knex wherever it's necessary.
There was a problem hiding this comment.
@pranavxc, thank you for clarifying the use of Knex for sanitization in your implementation. It's great to hear that you're taking steps to ensure the security of your SQL queries. Your attention to these practices is commendable. If there's anything else you need, feel free to ask! 🐇
New Learnings Added
User: pranavxc
PR: nocodb/nocodb#7915
File: packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts:2863-2869
Timestamp: 2024-03-19T19:03:15.953Z
Learning: Sanitization in the `PGClient` class is handled using Knex wherever necessary to mitigate the risks of SQL injection.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
There was a problem hiding this comment.
We are using genQuery method from base class for sanitisation of user input.
There was a problem hiding this comment.
@pranavxc, thank you for providing more details on your sanitization practices, specifically the use of the genQuery method from the base class. It's helpful to understand the specific mechanisms you've implemented to ensure the security of your SQL queries. Your proactive approach to sanitization is commendable. If there are any more aspects of your implementation you'd like to discuss or if you have further questions, feel free to share! 🐇
New Learnings Added
User: pranavxc
PR: nocodb/nocodb#7915
File: packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts:2863-2869
Timestamp: 2024-03-20T09:20:30.035Z
Learning: The `genQuery` method from the base class is used for sanitization of user input in the `PGClient` class to mitigate the risks of SQL injection.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
Comment on lines
189
to
229
async ooExcludedList(
param: PathParams & {
query: any;
columnName: string;
rowId: string;
},
) {
const { model, view } = await getViewAndModelByAliasOrId(param);
if (!model) NcError.notFound('Table not found');
const source = await Source.get(model.source_id);
const baseModel = await Model.getBaseModelSQL({
id: model.id,
viewId: view?.id,
dbDriver: await NcConnectionMgrv2.get(source),
});
const column = await getColumnByIdOrName(param.columnName, model);
const data = await baseModel.getOoChildrenExcludedList(
{
colId: column.id,
cid: param.rowId,
},
param.query,
);
const count = await baseModel.getOoChildrenExcludedListCount(
{
colId: column.id,
cid: param.rowId,
},
param.query,
);
return new PagedResponseImpl(data, {
count,
...param.query,
});
}
There was a problem hiding this comment.
The implementation of ooExcludedList method looks correct and follows the established pattern for fetching exclusion list data. However, it would be beneficial to add error handling for scenarios where the column is not found or is not of the expected type, similar to the checks performed in other methods like mmList.
There was a problem hiding this comment.
Any unhandled errors are captured globally using Nestjs filter.
There was a problem hiding this comment.
@pranavxc, thank you for clarifying the use of global error handling with NestJS filters in your application. It's a good practice to leverage such mechanisms for cleaner and more maintainable code. If you haven't already, it might be beneficial to review the global error handling setup to ensure it's configured to appropriately handle various error scenarios, including those related to not found or incorrect column types. This can help maintain robustness and clarity in error responses across the application.
Comment on lines
1808
to
1856
// todo: naming & optimizing
public async getOoChildrenExcludedListCount(
{ colId, cid = null },
args,
): Promise<any> {
const { where } = this._getListArgs(args as any);
const relColumn = (await this.model.getColumns()).find(
(c) => c.id === colId,
);
const relColOptions =
(await relColumn.getColOptions()) as LinkToAnotherRecordColumn;
const rcn = (await relColOptions.getParentColumn()).column_name;
const parentTable = await (
await relColOptions.getParentColumn()
).getModel();
const cn = (await relColOptions.getChildColumn()).column_name;
const childTable = await (await relColOptions.getChildColumn()).getModel();
const childTn = this.getTnPath(childTable);
const parentTn = this.getTnPath(parentTable);
const rtn = parentTn;
const tn = childTn;
await childTable.getColumns();
const isBt = relColumn.meta?.bt;
const qb = this.dbDriver(isBt ? rtn : tn)
.where((qb) => {
qb.whereNotIn(
isBt ? rcn : cn,
this.dbDriver(isBt ? tn : rtn)
.select(isBt ? cn : rcn)
.where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid))
.whereNotNull(isBt ? cn : rcn),
).orWhereNull(isBt ? rcn : cn);
})
.count(`*`, { as: 'count' });
const aliasColObjMap = await parentTable.getAliasColObjMap();
const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
await conditionV2(this, filterObj, qb);
return (await this.execAndParse(qb, null, { raw: true, first: true }))
?.count;
}
There was a problem hiding this comment.
The method getOoChildrenExcludedListCount is designed to count the number of children excluded in a one-to-one (1:1) relationship. However, there are a few areas that could be improved for clarity and maintainability:
- The method name could be more descriptive. Consider renaming it to more clearly reflect its purpose, such as
countExcludedOneToOneChildren.
- The use of
isBt to determine if the relationship is "belongs to" within a method specifically for one-to-one relationships might be confusing. If this method is intended to handle both one-to-one and belongs-to relationships, consider updating the method documentation to clarify this.
- The method performs a database query to count excluded children. Ensure that indexes are properly used on the relevant columns to optimize query performance, especially if the tables involved have a large number of rows.
- public async getOoChildrenExcludedListCount(
+ public async countExcludedOneToOneChildren(
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.
Suggested change
// todo: naming & optimizing
public async getOoChildrenExcludedListCount(
{ colId, cid = null },
args,
): Promise<any> {
const { where } = this._getListArgs(args as any);
const relColumn = (await this.model.getColumns()).find(
(c) => c.id === colId,
);
const relColOptions =
(await relColumn.getColOptions()) as LinkToAnotherRecordColumn;
const rcn = (await relColOptions.getParentColumn()).column_name;
const parentTable = await (
await relColOptions.getParentColumn()
).getModel();
const cn = (await relColOptions.getChildColumn()).column_name;
const childTable = await (await relColOptions.getChildColumn()).getModel();
const childTn = this.getTnPath(childTable);
const parentTn = this.getTnPath(parentTable);
const rtn = parentTn;
const tn = childTn;
await childTable.getColumns();
const isBt = relColumn.meta?.bt;
const qb = this.dbDriver(isBt ? rtn : tn)
.where((qb) => {
qb.whereNotIn(
isBt ? rcn : cn,
this.dbDriver(isBt ? tn : rtn)
.select(isBt ? cn : rcn)
.where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid))
.whereNotNull(isBt ? cn : rcn),
).orWhereNull(isBt ? rcn : cn);
})
.count(`*`, { as: 'count' });
const aliasColObjMap = await parentTable.getAliasColObjMap();
const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
await conditionV2(this, filterObj, qb);
return (await this.execAndParse(qb, null, { raw: true, first: true }))
?.count;
}
// todo: naming & optimizing
public async countExcludedOneToOneChildren(
{ colId, cid = null },
args,
): Promise<any> {
const { where } = this._getListArgs(args as any);
const relColumn = (await this.model.getColumns()).find(
(c) => c.id === colId,
);
const relColOptions =
(await relColumn.getColOptions()) as LinkToAnotherRecordColumn;
const rcn = (await relColOptions.getParentColumn()).column_name;
const parentTable = await (
await relColOptions.getParentColumn()
).getModel();
const cn = (await relColOptions.getChildColumn()).column_name;
const childTable = await (await relColOptions.getChildColumn()).getModel();
const childTn = this.getTnPath(childTable);
const parentTn = this.getTnPath(parentTable);
const rtn = parentTn;
const tn = childTn;
await childTable.getColumns();
const isBt = relColumn.meta?.bt;
const qb = this.dbDriver(isBt ? rtn : tn)
.where((qb) => {
qb.whereNotIn(
isBt ? rcn : cn,
this.dbDriver(isBt ? tn : rtn)
.select(isBt ? cn : rcn)
.where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid))
.whereNotNull(isBt ? cn : rcn),
).orWhereNull(isBt ? rcn : cn);
})
.count(`*`, { as: 'count' });
const aliasColObjMap = await parentTable.getAliasColObjMap();
const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
await conditionV2(this, filterObj, qb);
return (await this.execAndParse(qb, null, { raw: true, first: true }))
?.count;
}
The method getOoChildrenExcludedListCount and similar methods in this class perform direct database operations. Ensure that:
- Inputs used in database queries are sanitized or parameterized to prevent SQL injection vulnerabilities.
- Error handling is robust, especially for database operations that might fail due to constraints, connectivity issues, or other unexpected conditions.
- Consider the impact of these operations on database performance, particularly for large datasets. Use indexing where appropriate and review query execution plans for potential optimizations.
In methods like getOoChildrenExcludedListCount, there's a potential for improvement in how database errors are handled. Consider adding more specific error handling for different types of database errors (e.g., constraint violations, connection issues) to provide clearer feedback to the caller or to take corrective actions.
Comment on lines
1928
to
2008
// todo: naming & optimizing
public async getOoChildrenExcludedList(
{ colId, cid = null },
args,
): Promise<any> {
const { where, ...rest } = this._getListArgs(args as any);
const relColumn = (await this.model.getColumns()).find(
(c) => c.id === colId,
);
const relColOptions =
(await relColumn.getColOptions()) as LinkToAnotherRecordColumn;
const rcn = (await relColOptions.getParentColumn()).column_name;
const parentTable = await (
await relColOptions.getParentColumn()
).getModel();
const cn = (await relColOptions.getChildColumn()).column_name;
const childTable = await (await relColOptions.getChildColumn()).getModel();
const parentModel = await Model.getBaseModelSQL({
dbDriver: this.dbDriver,
model: parentTable,
});
const childModel = await Model.getBaseModelSQL({
dbDriver: this.dbDriver,
model: childTable,
});
const rtn = this.getTnPath(parentTable);
const tn = this.getTnPath(childTable);
await childTable.getColumns();
const isBt = relColumn.meta?.bt;
const qb = this.dbDriver(isBt ? rtn : tn).where((qb) => {
qb.whereNotIn(
isBt ? rcn : cn,
this.dbDriver(isBt ? tn : rtn)
.select(isBt ? cn : rcn)
.where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid))
.whereNotNull(isBt ? cn : rcn),
).orWhereNull(isBt ? rcn : cn);
});
if (+rest?.shuffle) {
await this.shuffle({ qb });
}
await (isBt ? parentModel : childModel).selectObject({ qb });
const aliasColObjMap = await parentTable.getAliasColObjMap();
const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
await conditionV2(this, filterObj, qb);
// sort by primary key if not autogenerated string
// if autogenerated string sort by created_at column if present
if (parentTable.primaryKey && parentTable.primaryKey.ai) {
qb.orderBy(parentTable.primaryKey.column_name);
} else if (
parentTable.columns.find((c) => c.column_name === 'created_at')
) {
qb.orderBy('created_at');
}
applyPaginate(qb, rest);
const proto = await (isBt ? parentModel : childModel).getProto();
const data = await this.execAndParse(
qb,
await (isBt ? parentTable : childTable).getColumns(),
);
return data.map((c) => {
c.__proto__ = proto;
return c;
});
}
There was a problem hiding this comment.
The method getOoChildrenExcludedList retrieves a list of children excluded in a one-to-one (1:1) relationship. Similar to the previous comment, consider the following improvements:
- The method name could be more descriptive. Consider renaming it to more clearly reflect its purpose, such as
getExcludedOneToOneChildrenList.
- The logic within this method is quite complex, involving conditional checks and database queries. Consider adding more detailed comments within the method to explain the logic, especially the purpose of the
isBt check and how it affects the query.
- As with the count method, ensure that database queries are optimized for performance.
- public async getOoChildrenExcludedList(
+ public async getExcludedOneToOneChildrenList(
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.
Suggested change
// todo: naming & optimizing
public async getOoChildrenExcludedList(
{ colId, cid = null },
args,
): Promise<any> {
const { where, ...rest } = this._getListArgs(args as any);
const relColumn = (await this.model.getColumns()).find(
(c) => c.id === colId,
);
const relColOptions =
(await relColumn.getColOptions()) as LinkToAnotherRecordColumn;
const rcn = (await relColOptions.getParentColumn()).column_name;
const parentTable = await (
await relColOptions.getParentColumn()
).getModel();
const cn = (await relColOptions.getChildColumn()).column_name;
const childTable = await (await relColOptions.getChildColumn()).getModel();
const parentModel = await Model.getBaseModelSQL({
dbDriver: this.dbDriver,
model: parentTable,
});
const childModel = await Model.getBaseModelSQL({
dbDriver: this.dbDriver,
model: childTable,
});
const rtn = this.getTnPath(parentTable);
const tn = this.getTnPath(childTable);
await childTable.getColumns();
const isBt = relColumn.meta?.bt;
const qb = this.dbDriver(isBt ? rtn : tn).where((qb) => {
qb.whereNotIn(
isBt ? rcn : cn,
this.dbDriver(isBt ? tn : rtn)
.select(isBt ? cn : rcn)
.where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid))
.whereNotNull(isBt ? cn : rcn),
).orWhereNull(isBt ? rcn : cn);
});
if (+rest?.shuffle) {
await this.shuffle({ qb });
}
await (isBt ? parentModel : childModel).selectObject({ qb });
const aliasColObjMap = await parentTable.getAliasColObjMap();
const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
await conditionV2(this, filterObj, qb);
// sort by primary key if not autogenerated string
// if autogenerated string sort by created_at column if present
if (parentTable.primaryKey && parentTable.primaryKey.ai) {
qb.orderBy(parentTable.primaryKey.column_name);
} else if (
parentTable.columns.find((c) => c.column_name === 'created_at')
) {
qb.orderBy('created_at');
}
applyPaginate(qb, rest);
const proto = await (isBt ? parentModel : childModel).getProto();
const data = await this.execAndParse(
qb,
await (isBt ? parentTable : childTable).getColumns(),
);
return data.map((c) => {
c.__proto__ = proto;
return c;
});
}
// todo: naming & optimizing
public async getExcludedOneToOneChildrenList(
{ colId, cid = null },
args,
): Promise<any> {
const { where, ...rest } = this._getListArgs(args as any);
const relColumn = (await this.model.getColumns()).find(
(c) => c.id === colId,
);
const relColOptions =
(await relColumn.getColOptions()) as LinkToAnotherRecordColumn;
const rcn = (await relColOptions.getParentColumn()).column_name;
const parentTable = await (
await relColOptions.getParentColumn()
).getModel();
const cn = (await relColOptions.getChildColumn()).column_name;
const childTable = await (await relColOptions.getChildColumn()).getModel();
const parentModel = await Model.getBaseModelSQL({
dbDriver: this.dbDriver,
model: parentTable,
});
const childModel = await Model.getBaseModelSQL({
dbDriver: this.dbDriver,
model: childTable,
});
const rtn = this.getTnPath(parentTable);
const tn = this.getTnPath(childTable);
await childTable.getColumns();
const isBt = relColumn.meta?.bt;
const qb = this.dbDriver(isBt ? rtn : tn).where((qb) => {
qb.whereNotIn(
isBt ? rcn : cn,
this.dbDriver(isBt ? tn : rtn)
.select(isBt ? cn : rcn)
.where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid))
.whereNotNull(isBt ? cn : rcn),
).orWhereNull(isBt ? rcn : cn);
});
if (+rest?.shuffle) {
await this.shuffle({ qb });
}
await (isBt ? parentModel : childModel).selectObject({ qb });
const aliasColObjMap = await parentTable.getAliasColObjMap();
const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
await conditionV2(this, filterObj, qb);
// sort by primary key if not autogenerated string
// if autogenerated string sort by created_at column if present
if (parentTable.primaryKey && parentTable.primaryKey.ai) {
qb.orderBy(parentTable.primaryKey.column_name);
} else if (
parentTable.columns.find((c) => c.column_name === 'created_at')
) {
qb.orderBy('created_at');
}
applyPaginate(qb, rest);
const proto = await (isBt ? parentModel : childModel).getProto();
const data = await this.execAndParse(
qb,
await (isBt ? parentTable : childTable).getColumns(),
);
return data.map((c) => {
c.__proto__ = proto;
return c;
});
}
| ); | ||
| query += n.rqd ? ' NOT NULL' : ' NULL'; | ||
| query += defaultValue ? ` DEFAULT ${defaultValue}` : ''; | ||
| query += n.unique ? ` UNIQUE` : ''; | ||
| } | ||
| } else if (change === 1) { | ||
| query += this.genQuery( |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2866-2876]
The logic for adding unique constraints directly in the alterTableColumn method might not handle all edge cases, especially when altering existing columns to add or remove unique constraints. Consider implementing a separate method to handle unique constraints more robustly, ensuring that it accounts for existing data and potential conflicts.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
The extensive use of raw SQL queries throughout the PGClient class raises concerns about potential SQL injection vulnerabilities and syntax errors. Wherever possible, leverage Knex's query building capabilities to mitigate these risks. For complex queries that require raw SQL, ensure that inputs are properly sanitized and validated.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2866]
While the PGClient class makes some effort to sanitize inputs, there's room for improvement in security practices, especially to prevent SQL injection. Ensure that all user inputs are properly sanitized, especially in methods that construct raw SQL queries. Consider using parameterized queries or Knex's built-in methods to further mitigate the risk of SQL injection.
There was a problem hiding this comment.
sanitisation is done using knex wherever it's necessary.
There was a problem hiding this comment.
@pranavxc, thank you for clarifying the use of Knex for sanitization in your implementation. It's great to hear that you're taking steps to ensure the security of your SQL queries. Your attention to these practices is commendable. If there's anything else you need, feel free to ask! 🐇
New Learnings Added
User: pranavxc
PR: nocodb/nocodb#7915
File: packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts:2863-2869
Timestamp: 2024-03-19T19:03:15.953Z
Learning: Sanitization in the `PGClient` class is handled using Knex wherever necessary to mitigate the risks of SQL injection.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
There was a problem hiding this comment.
We are using genQuery method from base class for sanitisation of user input.
There was a problem hiding this comment.
@pranavxc, thank you for providing more details on your sanitization practices, specifically the use of the genQuery method from the base class. It's helpful to understand the specific mechanisms you've implemented to ensure the security of your SQL queries. Your proactive approach to sanitization is commendable. If there are any more aspects of your implementation you'd like to discuss or if you have further questions, feel free to share! 🐇
New Learnings Added
User: pranavxc
PR: nocodb/nocodb#7915
File: packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts:2863-2869
Timestamp: 2024-03-20T09:20:30.035Z
Learning: The `genQuery` method from the base class is used for sanitization of user input in the `PGClient` class to mitigate the risks of SQL injection.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
| async ooExcludedList( | ||
| param: PathParams & { | ||
| query: any; | ||
| columnName: string; | ||
| rowId: string; | ||
| }, | ||
| ) { | ||
| const { model, view } = await getViewAndModelByAliasOrId(param); | ||
| if (!model) NcError.notFound('Table not found'); | ||
|
|
||
| const source = await Source.get(model.source_id); | ||
|
|
||
| const baseModel = await Model.getBaseModelSQL({ | ||
| id: model.id, | ||
| viewId: view?.id, | ||
| dbDriver: await NcConnectionMgrv2.get(source), | ||
| }); | ||
|
|
||
| const column = await getColumnByIdOrName(param.columnName, model); | ||
|
|
||
| const data = await baseModel.getOoChildrenExcludedList( | ||
| { | ||
| colId: column.id, | ||
| cid: param.rowId, | ||
| }, | ||
| param.query, | ||
| ); | ||
|
|
||
| const count = await baseModel.getOoChildrenExcludedListCount( | ||
| { | ||
| colId: column.id, | ||
| cid: param.rowId, | ||
| }, | ||
| param.query, | ||
| ); | ||
|
|
||
| return new PagedResponseImpl(data, { | ||
| count, | ||
| ...param.query, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The implementation of ooExcludedList method looks correct and follows the established pattern for fetching exclusion list data. However, it would be beneficial to add error handling for scenarios where the column is not found or is not of the expected type, similar to the checks performed in other methods like mmList.
There was a problem hiding this comment.
Any unhandled errors are captured globally using Nestjs filter.
There was a problem hiding this comment.
@pranavxc, thank you for clarifying the use of global error handling with NestJS filters in your application. It's a good practice to leverage such mechanisms for cleaner and more maintainable code. If you haven't already, it might be beneficial to review the global error handling setup to ensure it's configured to appropriately handle various error scenarios, including those related to not found or incorrect column types. This can help maintain robustness and clarity in error responses across the application.
| // todo: naming & optimizing | ||
| public async getOoChildrenExcludedListCount( | ||
| { colId, cid = null }, | ||
| args, | ||
| ): Promise<any> { | ||
| const { where } = this._getListArgs(args as any); | ||
| const relColumn = (await this.model.getColumns()).find( | ||
| (c) => c.id === colId, | ||
| ); | ||
| const relColOptions = | ||
| (await relColumn.getColOptions()) as LinkToAnotherRecordColumn; | ||
|
|
||
| const rcn = (await relColOptions.getParentColumn()).column_name; | ||
| const parentTable = await ( | ||
| await relColOptions.getParentColumn() | ||
| ).getModel(); | ||
| const cn = (await relColOptions.getChildColumn()).column_name; | ||
| const childTable = await (await relColOptions.getChildColumn()).getModel(); | ||
|
|
||
| const childTn = this.getTnPath(childTable); | ||
| const parentTn = this.getTnPath(parentTable); | ||
|
|
||
| const rtn = parentTn; | ||
| const tn = childTn; | ||
| await childTable.getColumns(); | ||
|
|
||
| const isBt = relColumn.meta?.bt; | ||
|
|
||
| const qb = this.dbDriver(isBt ? rtn : tn) | ||
| .where((qb) => { | ||
| qb.whereNotIn( | ||
| isBt ? rcn : cn, | ||
| this.dbDriver(isBt ? tn : rtn) | ||
| .select(isBt ? cn : rcn) | ||
| .where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid)) | ||
| .whereNotNull(isBt ? cn : rcn), | ||
| ).orWhereNull(isBt ? rcn : cn); | ||
| }) | ||
| .count(`*`, { as: 'count' }); | ||
|
|
||
| const aliasColObjMap = await parentTable.getAliasColObjMap(); | ||
| const filterObj = extractFilterFromXwhere(where, aliasColObjMap); | ||
|
|
||
| await conditionV2(this, filterObj, qb); | ||
| return (await this.execAndParse(qb, null, { raw: true, first: true })) | ||
| ?.count; | ||
| } |
There was a problem hiding this comment.
The method getOoChildrenExcludedListCount is designed to count the number of children excluded in a one-to-one (1:1) relationship. However, there are a few areas that could be improved for clarity and maintainability:
- The method name could be more descriptive. Consider renaming it to more clearly reflect its purpose, such as
countExcludedOneToOneChildren. - The use of
isBtto determine if the relationship is "belongs to" within a method specifically for one-to-one relationships might be confusing. If this method is intended to handle both one-to-one and belongs-to relationships, consider updating the method documentation to clarify this. - The method performs a database query to count excluded children. Ensure that indexes are properly used on the relevant columns to optimize query performance, especially if the tables involved have a large number of rows.
- public async getOoChildrenExcludedListCount(
+ public async countExcludedOneToOneChildren(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.
| // todo: naming & optimizing | |
| public async getOoChildrenExcludedListCount( | |
| { colId, cid = null }, | |
| args, | |
| ): Promise<any> { | |
| const { where } = this._getListArgs(args as any); | |
| const relColumn = (await this.model.getColumns()).find( | |
| (c) => c.id === colId, | |
| ); | |
| const relColOptions = | |
| (await relColumn.getColOptions()) as LinkToAnotherRecordColumn; | |
| const rcn = (await relColOptions.getParentColumn()).column_name; | |
| const parentTable = await ( | |
| await relColOptions.getParentColumn() | |
| ).getModel(); | |
| const cn = (await relColOptions.getChildColumn()).column_name; | |
| const childTable = await (await relColOptions.getChildColumn()).getModel(); | |
| const childTn = this.getTnPath(childTable); | |
| const parentTn = this.getTnPath(parentTable); | |
| const rtn = parentTn; | |
| const tn = childTn; | |
| await childTable.getColumns(); | |
| const isBt = relColumn.meta?.bt; | |
| const qb = this.dbDriver(isBt ? rtn : tn) | |
| .where((qb) => { | |
| qb.whereNotIn( | |
| isBt ? rcn : cn, | |
| this.dbDriver(isBt ? tn : rtn) | |
| .select(isBt ? cn : rcn) | |
| .where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid)) | |
| .whereNotNull(isBt ? cn : rcn), | |
| ).orWhereNull(isBt ? rcn : cn); | |
| }) | |
| .count(`*`, { as: 'count' }); | |
| const aliasColObjMap = await parentTable.getAliasColObjMap(); | |
| const filterObj = extractFilterFromXwhere(where, aliasColObjMap); | |
| await conditionV2(this, filterObj, qb); | |
| return (await this.execAndParse(qb, null, { raw: true, first: true })) | |
| ?.count; | |
| } | |
| // todo: naming & optimizing | |
| public async countExcludedOneToOneChildren( | |
| { colId, cid = null }, | |
| args, | |
| ): Promise<any> { | |
| const { where } = this._getListArgs(args as any); | |
| const relColumn = (await this.model.getColumns()).find( | |
| (c) => c.id === colId, | |
| ); | |
| const relColOptions = | |
| (await relColumn.getColOptions()) as LinkToAnotherRecordColumn; | |
| const rcn = (await relColOptions.getParentColumn()).column_name; | |
| const parentTable = await ( | |
| await relColOptions.getParentColumn() | |
| ).getModel(); | |
| const cn = (await relColOptions.getChildColumn()).column_name; | |
| const childTable = await (await relColOptions.getChildColumn()).getModel(); | |
| const childTn = this.getTnPath(childTable); | |
| const parentTn = this.getTnPath(parentTable); | |
| const rtn = parentTn; | |
| const tn = childTn; | |
| await childTable.getColumns(); | |
| const isBt = relColumn.meta?.bt; | |
| const qb = this.dbDriver(isBt ? rtn : tn) | |
| .where((qb) => { | |
| qb.whereNotIn( | |
| isBt ? rcn : cn, | |
| this.dbDriver(isBt ? tn : rtn) | |
| .select(isBt ? cn : rcn) | |
| .where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid)) | |
| .whereNotNull(isBt ? cn : rcn), | |
| ).orWhereNull(isBt ? rcn : cn); | |
| }) | |
| .count(`*`, { as: 'count' }); | |
| const aliasColObjMap = await parentTable.getAliasColObjMap(); | |
| const filterObj = extractFilterFromXwhere(where, aliasColObjMap); | |
| await conditionV2(this, filterObj, qb); | |
| return (await this.execAndParse(qb, null, { raw: true, first: true })) | |
| ?.count; | |
| } |
The method getOoChildrenExcludedListCount and similar methods in this class perform direct database operations. Ensure that:
- Inputs used in database queries are sanitized or parameterized to prevent SQL injection vulnerabilities.
- Error handling is robust, especially for database operations that might fail due to constraints, connectivity issues, or other unexpected conditions.
- Consider the impact of these operations on database performance, particularly for large datasets. Use indexing where appropriate and review query execution plans for potential optimizations.
In methods like getOoChildrenExcludedListCount, there's a potential for improvement in how database errors are handled. Consider adding more specific error handling for different types of database errors (e.g., constraint violations, connection issues) to provide clearer feedback to the caller or to take corrective actions.
| // todo: naming & optimizing | ||
| public async getOoChildrenExcludedList( | ||
| { colId, cid = null }, | ||
| args, | ||
| ): Promise<any> { | ||
| const { where, ...rest } = this._getListArgs(args as any); | ||
| const relColumn = (await this.model.getColumns()).find( | ||
| (c) => c.id === colId, | ||
| ); | ||
| const relColOptions = | ||
| (await relColumn.getColOptions()) as LinkToAnotherRecordColumn; | ||
|
|
||
| const rcn = (await relColOptions.getParentColumn()).column_name; | ||
| const parentTable = await ( | ||
| await relColOptions.getParentColumn() | ||
| ).getModel(); | ||
| const cn = (await relColOptions.getChildColumn()).column_name; | ||
| const childTable = await (await relColOptions.getChildColumn()).getModel(); | ||
| const parentModel = await Model.getBaseModelSQL({ | ||
| dbDriver: this.dbDriver, | ||
| model: parentTable, | ||
| }); | ||
| const childModel = await Model.getBaseModelSQL({ | ||
| dbDriver: this.dbDriver, | ||
| model: childTable, | ||
| }); | ||
|
|
||
| const rtn = this.getTnPath(parentTable); | ||
| const tn = this.getTnPath(childTable); | ||
| await childTable.getColumns(); | ||
|
|
||
| const isBt = relColumn.meta?.bt; | ||
|
|
||
| const qb = this.dbDriver(isBt ? rtn : tn).where((qb) => { | ||
| qb.whereNotIn( | ||
| isBt ? rcn : cn, | ||
| this.dbDriver(isBt ? tn : rtn) | ||
| .select(isBt ? cn : rcn) | ||
| .where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid)) | ||
| .whereNotNull(isBt ? cn : rcn), | ||
| ).orWhereNull(isBt ? rcn : cn); | ||
| }); | ||
|
|
||
| if (+rest?.shuffle) { | ||
| await this.shuffle({ qb }); | ||
| } | ||
|
|
||
| await (isBt ? parentModel : childModel).selectObject({ qb }); | ||
|
|
||
| const aliasColObjMap = await parentTable.getAliasColObjMap(); | ||
| const filterObj = extractFilterFromXwhere(where, aliasColObjMap); | ||
| await conditionV2(this, filterObj, qb); | ||
|
|
||
| // sort by primary key if not autogenerated string | ||
| // if autogenerated string sort by created_at column if present | ||
| if (parentTable.primaryKey && parentTable.primaryKey.ai) { | ||
| qb.orderBy(parentTable.primaryKey.column_name); | ||
| } else if ( | ||
| parentTable.columns.find((c) => c.column_name === 'created_at') | ||
| ) { | ||
| qb.orderBy('created_at'); | ||
| } | ||
|
|
||
| applyPaginate(qb, rest); | ||
|
|
||
| const proto = await (isBt ? parentModel : childModel).getProto(); | ||
| const data = await this.execAndParse( | ||
| qb, | ||
| await (isBt ? parentTable : childTable).getColumns(), | ||
| ); | ||
|
|
||
| return data.map((c) => { | ||
| c.__proto__ = proto; | ||
| return c; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
The method getOoChildrenExcludedList retrieves a list of children excluded in a one-to-one (1:1) relationship. Similar to the previous comment, consider the following improvements:
- The method name could be more descriptive. Consider renaming it to more clearly reflect its purpose, such as
getExcludedOneToOneChildrenList. - The logic within this method is quite complex, involving conditional checks and database queries. Consider adding more detailed comments within the method to explain the logic, especially the purpose of the
isBtcheck and how it affects the query. - As with the count method, ensure that database queries are optimized for performance.
- public async getOoChildrenExcludedList(
+ public async getExcludedOneToOneChildrenList(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.
| // todo: naming & optimizing | |
| public async getOoChildrenExcludedList( | |
| { colId, cid = null }, | |
| args, | |
| ): Promise<any> { | |
| const { where, ...rest } = this._getListArgs(args as any); | |
| const relColumn = (await this.model.getColumns()).find( | |
| (c) => c.id === colId, | |
| ); | |
| const relColOptions = | |
| (await relColumn.getColOptions()) as LinkToAnotherRecordColumn; | |
| const rcn = (await relColOptions.getParentColumn()).column_name; | |
| const parentTable = await ( | |
| await relColOptions.getParentColumn() | |
| ).getModel(); | |
| const cn = (await relColOptions.getChildColumn()).column_name; | |
| const childTable = await (await relColOptions.getChildColumn()).getModel(); | |
| const parentModel = await Model.getBaseModelSQL({ | |
| dbDriver: this.dbDriver, | |
| model: parentTable, | |
| }); | |
| const childModel = await Model.getBaseModelSQL({ | |
| dbDriver: this.dbDriver, | |
| model: childTable, | |
| }); | |
| const rtn = this.getTnPath(parentTable); | |
| const tn = this.getTnPath(childTable); | |
| await childTable.getColumns(); | |
| const isBt = relColumn.meta?.bt; | |
| const qb = this.dbDriver(isBt ? rtn : tn).where((qb) => { | |
| qb.whereNotIn( | |
| isBt ? rcn : cn, | |
| this.dbDriver(isBt ? tn : rtn) | |
| .select(isBt ? cn : rcn) | |
| .where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid)) | |
| .whereNotNull(isBt ? cn : rcn), | |
| ).orWhereNull(isBt ? rcn : cn); | |
| }); | |
| if (+rest?.shuffle) { | |
| await this.shuffle({ qb }); | |
| } | |
| await (isBt ? parentModel : childModel).selectObject({ qb }); | |
| const aliasColObjMap = await parentTable.getAliasColObjMap(); | |
| const filterObj = extractFilterFromXwhere(where, aliasColObjMap); | |
| await conditionV2(this, filterObj, qb); | |
| // sort by primary key if not autogenerated string | |
| // if autogenerated string sort by created_at column if present | |
| if (parentTable.primaryKey && parentTable.primaryKey.ai) { | |
| qb.orderBy(parentTable.primaryKey.column_name); | |
| } else if ( | |
| parentTable.columns.find((c) => c.column_name === 'created_at') | |
| ) { | |
| qb.orderBy('created_at'); | |
| } | |
| applyPaginate(qb, rest); | |
| const proto = await (isBt ? parentModel : childModel).getProto(); | |
| const data = await this.execAndParse( | |
| qb, | |
| await (isBt ? parentTable : childTable).getColumns(), | |
| ); | |
| return data.map((c) => { | |
| c.__proto__ = proto; | |
| return c; | |
| }); | |
| } | |
| // todo: naming & optimizing | |
| public async getExcludedOneToOneChildrenList( | |
| { colId, cid = null }, | |
| args, | |
| ): Promise<any> { | |
| const { where, ...rest } = this._getListArgs(args as any); | |
| const relColumn = (await this.model.getColumns()).find( | |
| (c) => c.id === colId, | |
| ); | |
| const relColOptions = | |
| (await relColumn.getColOptions()) as LinkToAnotherRecordColumn; | |
| const rcn = (await relColOptions.getParentColumn()).column_name; | |
| const parentTable = await ( | |
| await relColOptions.getParentColumn() | |
| ).getModel(); | |
| const cn = (await relColOptions.getChildColumn()).column_name; | |
| const childTable = await (await relColOptions.getChildColumn()).getModel(); | |
| const parentModel = await Model.getBaseModelSQL({ | |
| dbDriver: this.dbDriver, | |
| model: parentTable, | |
| }); | |
| const childModel = await Model.getBaseModelSQL({ | |
| dbDriver: this.dbDriver, | |
| model: childTable, | |
| }); | |
| const rtn = this.getTnPath(parentTable); | |
| const tn = this.getTnPath(childTable); | |
| await childTable.getColumns(); | |
| const isBt = relColumn.meta?.bt; | |
| const qb = this.dbDriver(isBt ? rtn : tn).where((qb) => { | |
| qb.whereNotIn( | |
| isBt ? rcn : cn, | |
| this.dbDriver(isBt ? tn : rtn) | |
| .select(isBt ? cn : rcn) | |
| .where(_wherePk((isBt ? childTable : parentTable).primaryKeys, cid)) | |
| .whereNotNull(isBt ? cn : rcn), | |
| ).orWhereNull(isBt ? rcn : cn); | |
| }); | |
| if (+rest?.shuffle) { | |
| await this.shuffle({ qb }); | |
| } | |
| await (isBt ? parentModel : childModel).selectObject({ qb }); | |
| const aliasColObjMap = await parentTable.getAliasColObjMap(); | |
| const filterObj = extractFilterFromXwhere(where, aliasColObjMap); | |
| await conditionV2(this, filterObj, qb |
|
| // sort by primary key if not autogenerated string | |
| // if autogenerated string sort by created_at column if present | |
| if (parentTable.primaryKey && parentTable.primaryKey.ai) { | |
| qb.orderBy(parentTable.primaryKey.column_name); | |
| } else if ( | |
| parentTable.columns.find((c) => c.column_name === 'created_at') | |
| ) { | |
| qb.orderBy('created_at'); | |
| } | |
| applyPaginate(qb, rest); | |
| const proto = await (isBt ? parentModel : childModel).getProto(); | |
| const data = await this.execAndParse( | |
| qb, | |
| await (isBt ? parentTable : childTable).getColumns(), | |
| ); | |
| return data.map((c) => { | |
| c.__proto__ = proto; | |
| return c; | |
| }); | |
| } |
|
Uffizzi Preview |
b617494 to
37c671b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
packages/nc-gui/assets/nc-icons/onetoone.svgis excluded by:!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger-v2.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger.jsonis excluded by:!**/*.json
Files selected for processing (38)
- packages/nc-gui/components/smartsheet/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue (3 hunks)
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue (2 hunks)
- packages/nc-gui/components/smartsheet/grid/Table.vue (5 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue (1 hunks)
- packages/nc-gui/components/virtual-cell/BelongsTo.vue (3 hunks)
- packages/nc-gui/components/virtual-cell/Lookup.vue (2 hunks)
- packages/nc-gui/components/virtual-cell/OneToOne.vue (1 hunks)
- packages/nc-gui/composables/useLTARStore.ts (9 hunks)
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts (2 hunks)
- packages/nc-gui/composables/useMultiSelect/index.ts (4 hunks)
- packages/nc-gui/composables/useSmartsheetRowStore.ts (5 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nc-gui/utils/virtualCell.ts (1 hunks)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/controllers/data-alias-nested.controller.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (17 hunks)
- packages/nocodb/src/db/conditionV2.ts (8 hunks)
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (16 hunks)
- packages/nocodb/src/db/genRollupSelectv2.ts (1 hunks)
- packages/nocodb/src/db/generateLookupSelectQuery.ts (6 hunks)
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts (1 hunks)
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts (2 hunks)
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts (3 hunks)
- packages/nocodb/src/helpers/columnHelpers.ts (3 hunks)
- packages/nocodb/src/helpers/getAst.ts (1 hunks)
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts (1 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts (4 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts (2 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts (5 hunks)
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts (1 hunks)
- packages/nocodb/src/services/columns.service.ts (5 hunks)
- packages/nocodb/src/services/data-alias-nested.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (2 hunks)
Files skipped from review as they are similar to previous changes (38)
- packages/nc-gui/components/smartsheet/VirtualCell.vue
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue
- packages/nc-gui/components/virtual-cell/BelongsTo.vue
- packages/nc-gui/components/virtual-cell/Lookup.vue
- packages/nc-gui/components/virtual-cell/OneToOne.vue
- packages/nc-gui/composables/useLTARStore.ts
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts
- packages/nc-gui/composables/useMultiSelect/index.ts
- packages/nc-gui/composables/useSmartsheetRowStore.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nc-gui/utils/virtualCell.ts
- packages/nocodb-sdk/src/lib/globals.ts
- packages/nocodb/src/controllers/data-alias-nested.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/db/conditionV2.ts
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts
- packages/nocodb/src/db/genRollupSelectv2.ts
- packages/nocodb/src/db/generateLookupSelectQuery.ts
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts
- packages/nocodb/src/helpers/columnHelpers.ts
- packages/nocodb/src/helpers/getAst.ts
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts
- packages/nocodb/src/services/columns.service.ts
- packages/nocodb/src/services/data-alias-nested.service.ts
- packages/nocodb/src/utils/acl.ts
37c671b to
a3b65cc
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
packages/nc-gui/assets/nc-icons/onetoone.svgis excluded by:!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger-v2.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger.jsonis excluded by:!**/*.json
Files selected for processing (38)
- packages/nc-gui/components/smartsheet/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue (3 hunks)
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue (2 hunks)
- packages/nc-gui/components/smartsheet/grid/Table.vue (5 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue (1 hunks)
- packages/nc-gui/components/virtual-cell/BelongsTo.vue (3 hunks)
- packages/nc-gui/components/virtual-cell/Lookup.vue (2 hunks)
- packages/nc-gui/components/virtual-cell/OneToOne.vue (1 hunks)
- packages/nc-gui/composables/useLTARStore.ts (9 hunks)
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts (2 hunks)
- packages/nc-gui/composables/useMultiSelect/index.ts (4 hunks)
- packages/nc-gui/composables/useSmartsheetRowStore.ts (5 hunks)
- packages/nc-gui/utils/iconUtils.ts 10000 (2 hunks)
- packages/nc-gui/utils/virtualCell.ts (1 hunks)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/controllers/data-alias-nested.controller.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (17 hunks)
- packages/nocodb/src/db/conditionV2.ts (8 hunks)
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (20 hunks)
- packages/nocodb/src/db/genRollupSelectv2.ts (1 hunks)
- packages/nocodb/src/db/generateLookupSelectQuery.ts (6 hunks)
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts (1 hunks)
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts (2 hunks)
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts (3 hunks)
- packages/nocodb/src/helpers/columnHelpers.ts (3 hunks)
- packages/nocodb/src/helpers/getAst.ts (1 hunks)
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts (1 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts (4 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts (2 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts (5 hunks)
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts (1 hunks)
- packages/nocodb/src/services/columns.service.ts (5 hunks)
- packages/nocodb/src/services/data-alias-nested.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (2 hunks)
Files skipped from review as they are similar to previous changes (36)
- packages/nc-gui/components/smartsheet/VirtualCell.vue
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue
- packages/nc-gui/components/virtual-cell/BelongsTo.vue
- packages/nc-gui/components/virtual-cell/Lookup.vue
- packages/nc-gui/components/virtual-cell/OneToOne.vue
- packages/nc-gui/composables/useLTARStore.ts
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts
- packages/nc-gui/composables/useMultiSelect/index.ts
- packages/nc-gui/composables/useSmartsheetRowStore.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nc-gui/utils/virtualCell.ts
- packages/nocodb-sdk/src/lib/globals.ts
- packages/nocodb/src/controllers/data-alias-nested.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/db/conditionV2.ts
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts
- packages/nocodb/src/db/genRollupSelectv2.ts
- packages/nocodb/src/db/generateLookupSelectQuery.ts
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts
- packages/nocodb/src/helpers/getAst.ts
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts
- packages/nocodb/src/services/data-alias-nested.service.ts
- packages/nocodb/src/utils/acl.ts
Additional comments: 1
packages/nocodb/src/helpers/columnHelpers.ts (1)
- 265-277: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [246-274]
Enhancements to
validateLookupPayloadinclude handling for one-to-one (1:1) relationships. A few points to consider:
Error Messages: The error messages thrown in this function are generic (e.g., "Relation column not found"). Including more context in the error messages, such as the column ID or type, can help with debugging and user feedback.
Code Duplication: Similar to the
createOOColumnfunction, there's a pattern of code duplication in handling different relationship types. Consider abstracting common logic to reduce duplication.Handling of
ooCase: The handling of theoocase introduces a conditional check based oncolumn.meta?.bt. It's important to ensure that this property is always correctly set and documented, as its presence significantly alters the logic flow.- throw new Error('Relation column not found'); + throw new Error(`Relation column not found for column ID: ${columnId}`);
| export async function createOOColumn( | ||
| child: Model, | ||
| parent: Model, | ||
| childColumn: Column, | ||
| type?: RelationTypes, | ||
| alias?: string, | ||
| fkColName?: string, | ||
| virtual: BoolType = false, | ||
| isSystemCol = false, | ||
| columnMeta = null, | ||
| colExtra?: any, | ||
| ) { | ||
| // save bt column | ||
| { | ||
| const title = getUniqueColumnAliasName( | ||
| await child.getColumns(), | ||
| `${parent.title}`, | ||
| ); | ||
| await Column.insert<LinkToAnotherRecordColumn>({ | ||
| title, | ||
|
|
||
| fk_model_id: child.id, | ||
| // ref_db_alias | ||
| uidt: UITypes.LinkToAnotherRecord, | ||
| type: 'oo', | ||
| // db_type: | ||
|
|
||
| fk_child_column_id: childColumn.id, | ||
| fk_parent_column_id: parent.primaryKey.id, | ||
| fk_related_model_id: parent.id, | ||
| virtual, | ||
| // if self referencing treat it as system field to hide from ui | ||
| system: isSystemCol || parent.id === child.id, | ||
| fk_col_name: fkColName, | ||
| fk_index_name: fkColName, | ||
| // ...(colExtra || {}), | ||
| meta: { ...(colExtra?.meta || {}), bt: true }, | ||
| }); | ||
| } | ||
| // save hm column | ||
| { | ||
| const title = getUniqueColumnAliasName( | ||
| await parent.getColumns(), | ||
| alias || child.title, | ||
| ); | ||
| const meta = { | ||
| plural: columnMeta?.plural || pluralize(child.title), | ||
| singular: columnMeta?.singular || singularize(child.title), | ||
| }; | ||
|
|
||
| await Column.insert({ | ||
| title, | ||
| fk_model_id: parent.id, | ||
| uidt: UITypes.LinkToAnotherRecord, | ||
| type: 'oo', | ||
| fk_child_column_id: childColumn.id, | ||
| fk_parent_column_id: parent.primaryKey.id, | ||
| fk_related_model_id: child.id, | ||
| virtual, | ||
| system: isSystemCol, | ||
| fk_col_name: fkColName, | ||
| fk_index_name: fkColName, | ||
| meta, | ||
| ...(colExtra || {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The createOOColumn function introduces support for creating columns with one-to-one (1:1) relationships. Here are a few observations and suggestions:
-
Parameter Documentation: It's important to document the parameters of this function, especially since it handles a complex feature like 1:1 relationships. Consider adding JSDoc comments to explain each parameter's role and expected values.
-
Error Handling: The function performs asynchronous operations (
await Column.insert) but does not include any error handling mechanisms. Consider wrapping these operations in try-catch blocks to handle potential errors gracefully. -
Code Duplication: The structure of the code for saving the "bt" and "hm" columns is quite similar. While the types and some parameters differ, consider abstracting common logic into a separate function to reduce duplication and improve maintainability.
-
Use of
metain Line 130: Themetaobject is being created with a propertybt: true. It's unclear why this specific property is needed and how it's used downstream. Clarification in the code comments or documentation would be helpful. -
Consistency with Existing Functions: Ensure that the implementation of
createOOColumnis consistent with similar functions in the file, such ascreateHmAndBtColumn. This includes naming conventions, parameter handling, and error management.
+ /**
+ * Creates a column with a one-to-one (1:1) relationship.
+ * @param {Model} child - The child model.
+ * @param {Model} parent - The parent model.
+ * @param {Column} childColumn - The child column.
+ * @param {RelationTypes} [type] - The type of relationship.
+ * @param {string} [alias] - The alias for the column.
+ * @param {string} [fkColName] - The foreign key column name.
+ * @param {BoolType} [virtual=false] - Whether the column is virtual.
+ * @param {boolean} [isSystemCol=false] - Whether the column is a system column.
+ * @param {any} [columnMeta=null] - Metadata for the column.
+ * @param {any} [colExtra] - Additional column parameters.
+ */
- // save bt column
+ try {
+ // save bt column
+ ...
+ } catch (error) {
+ console.error(`Error creating OO column: ${error}`);
+ throw error; // Or handle it according to your application's error handling policy
+ }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.
| export async function createOOColumn( | |
| child: Model, | |
| parent: Model, | |
| childColumn: Column, | |
| type?: RelationTypes, | |
| alias?: string, | |
| fkColName?: string, | |
| virtual: BoolType = false, | |
| isSystemCol = false, | |
| columnMeta = null, | |
| colExtra?: any, | |
| ) { | |
| // save bt column | |
| { | |
| const title = getUniqueColumnAliasName( | |
| await child.getColumns(), | |
| `${parent.title}`, | |
| ); | |
| await Column.insert<LinkToAnotherRecordColumn>({ | |
| title, | |
| fk_model_id: child.id, | |
| // ref_db_alias | |
| uidt: UITypes.LinkToAnotherRecord, | |
| type: 'oo', | |
| // db_type: | |
| fk_child_column_id: childColumn.id, | |
| fk_parent_column_id: parent.primaryKey.id, | |
| fk_related_model_id: parent.id, | |
| virtual, | |
| // if self referencing treat it as system field to hide from ui | |
| system: isSystemCol || parent.id === child.id, | |
| fk_col_name: fkColName, | |
| fk_index_name: fkColName, | |
| // ...(colExtra || {}), | |
| meta: { ...(colExtra?.meta || {}), bt: true }, | |
| }); | |
| } | |
| // save hm column | |
| { | |
| const title = getUniqueColumnAliasName( | |
| await parent.getColumns(), | |
| alias || child.title, | |
| ); | |
| const meta = { | |
| plural: columnMeta?.plural || pluralize(child.title), | |
| singular: columnMeta?.singular || singularize(child.title), | |
| }; | |
| await Column.insert({ | |
| title, | |
| fk_model_id: parent.id, | |
| uidt: UITypes.LinkToAnotherRecord, | |
| type: 'oo', | |
| fk_child_column_id: childColumn.id, | |
| fk_parent_column_id: parent.primaryKey.id, | |
| fk_related_model_id: child.id, | |
| virtual, | |
| system: isSystemCol, | |
| fk_col_name: fkColName, | |
| fk_index_name: fkColName, | |
| meta, | |
| ...(colExtra || {}), | |
| }); | |
| } | |
| /** | |
| * Creates a column with a one-to-one (1:1) relationship. | |
| * @param {Model} child - The child model. | |
| * @param {Model} parent - The parent model. | |
| * @param {Column} childColumn - The child column. | |
| * @param {RelationTypes} [type] - The type of relationship. | |
| * @param {string} [alias] - The alias for the column. | |
| * @param {string} [fkColName] - The foreign key column name. | |
| * @param {BoolType} [virtual=false] - Whether the column is virtual. | |
| * @param {boolean} [isSystemCol=false] - Whether the column is a system column. | |
| * @param {any} [columnMeta=null] - Metadata for the column. | |
| * @param {any} [colExtra] - Additional column parameters. | |
| */ | |
| try { | |
| // save bt column | |
| { | |
| const title = getUniqueColumnAliasName( | |
| await child.getColumns(), | |
| `${parent.title}`, | |
| ); | |
| await Column.insert<LinkToAnotherRecordColumn>({ | |
| title, | |
| fk_model_id: child.id, | |
| // ref_db_alias | |
| uidt: UITypes.LinkToAnotherRecord, | |
| type: 'oo', | |
| // db_type: | |
| fk_child_column_id: childColumn.id, | |
| fk_parent_column_id: parent.primaryKey.id, | |
| fk_related 93C6 _model_id: parent.id, | |
| virtual, | |
| // if self referencing treat it as system field to hide from ui | |
| system: isSystemCol || parent.id === child.id, | |
| fk_col_name: fkColName, | |
| fk_index_name: fkColName, | |
| // ...(colExtra || {}), | |
| meta: { ...(colExtra?.meta || {}), bt: true }, | |
| }); | |
| } | |
| // save hm column | |
| { | |
| const title = getUniqueColumnAliasName( | |
| await parent.getColumns(), | |
| alias || child.title, | |
| ); | |
| const meta = { | |
| plural: columnMeta?.plural || pluralize(child.title), | |
| singular: columnMeta?.singular || singularize(child.title), | |
| }; | |
| await Column.insert({ | |
| title, | |
| fk_model_id: parent.id, | |
| uidt: UITypes.LinkToAnotherRecord, | |
| type: 'oo', | |
| fk_child_column_id: childColumn.id, | |
| fk_parent_column_id: parent.primaryKey.id, | |
| fk_related_model_id: child.id, | |
| virtual, | |
| system: isSystemCol, | |
| fk_col_name: fkColName, | |
| fk_index_name: fkColName, | |
| meta, | |
| ...(colExtra || {}), | |
| }); | |
| } | |
| } catch (error) { | |
| console.error(`Error creating OO column: ${error}`); | |
| throw error; // Or handle it according to your application's error handling policy | |
| } |
There was a problem hiding this comment.
Error Handling - is done globally in nestjs global exception filter.
Code Duplication - planning to refactor in a followup PR.
There was a problem hiding this comment.
@pranavxc, thank you for the clarifications. It's great to hear that error handling is managed globally through NestJS's global exception filter, which is a neat approach. Also, understanding your plan to address the code duplication in a follow-up PR is reassuring. Looking forward to seeing the improvements in the future. If there's anything else you need, feel free to ask!
| } else if ((param.column as LinkToAnotherColumnReqType).type === 'oo') { | ||
| // populate fk column name | ||
| const fkColName = getUniqueColumnName( | ||
| await child.getColumns(), | ||
| `${parent.table_name}_id`, | ||
| ); | ||
|
|
||
| let foreignKeyName; | ||
| { | ||
| // create foreign key | ||
| const newColumn = { | ||
| cn: fkColName, | ||
|
|
||
| title: fkColName, | ||
| column_name: fkColName, | ||
| rqd: false, | ||
| pk: false, | ||
| ai: false, | ||
| cdf: null, | ||
| dt: parent.primaryKey.dt, | ||
| dtxp: parent.primaryKey.dtxp, | ||
| dtxs: parent.primaryKey.dtxs, | ||
| un: parent.primaryKey.un, | ||
| altered: Altered.NEW_COLUMN, | ||
| unique: 1, | ||
| }; | ||
|
|
||
| const tableUpdateBody = { | ||
| ...child, | ||
| tn: child.table_name, | ||
| originalColumns: child.columns.map((c) => ({ | ||
| ...c, | ||
| cn: c.column_name, | ||
| })), | ||
| columns: [ | ||
| ...child.columns.map((c) => ({ | ||
| ...c, | ||
| cn: c.column_name, | ||
| })), | ||
| newColumn, | ||
| ], | ||
| }; | ||
|
|
||
| await sqlMgr.sqlOpPlus(param.source, 'tableUpdate', tableUpdateBody); | ||
|
|
||
| const { id } = await Column.insert({ | ||
| ...newColumn, | ||
| uidt: UITypes.ForeignKey, | ||
| fk_model_id: child.id, | ||
| }); | ||
|
|
||
| childColumn = await Column.get({ colId: id }); | ||
|
|
||
| // ignore relation creation if virtual | ||
| if (!(param.column as LinkToAnotherColumnReqType).virtual) { | ||
| foreignKeyName = generateFkName(parent, child); | ||
| // create relation | ||
| await sqlMgr.sqlOpPlus(param.source, 'relationCreate', { | ||
| childColumn: fkColName, | ||
| childTable: child.table_name, | ||
| parentTable: parent.table_name, | ||
| onDelete: 'NO ACTION', | ||
| onUpdate: 'NO ACTION', | ||
| type: 'real', | ||
| parentColumn: parent.primaryKey.column_name, | ||
| foreignKeyName, | ||
| }); | ||
| } | ||
|
|
||
| // todo: create index for virtual relations as well | ||
| // create index for foreign key in pg | ||
| if ( | ||
| param.source.type === 'pg' || | ||
| (param.column as LinkToAnotherColumnReqType).virtual | ||
| ) { | ||
| await this.createColumnIndex({ | ||
| column: new Column({ | ||
| ...newColumn, | ||
| fk_model_id: child.id, | ||
| }), | ||
| source: param.source, | ||
| sqlMgr, | ||
| }); | ||
| } | ||
| } | ||
| await createOOColumn( | ||
| child, | ||
| parent, | ||
| childColumn, | ||
| (param.column as LinkToAnotherColumnReqType).type as RelationTypes, | ||
| (param.column as LinkToAnotherColumnReqType).title, | ||
| foreignKeyName, | ||
| (param.column as LinkToAnotherColumnReqType).virtual, | ||
| null, | ||
| param.column['meta'], | ||
| param.colExtra, | ||
| ); |
There was a problem hiding this comment.
The implementation of the createLTARColumn method for handling one-to-one ('oo') relationships appears to correctly follow the logic for creating a foreign key column, generating a unique column name, and inserting the new column into the database. However, there are a few areas that could be improved for clarity and maintainability:
- The use of hardcoded strings for column names and types could be replaced with constants to avoid typos and make the code more maintainable.
- The method could benefit from more detailed comments explaining the logic, especially for complex operations like relation creation and handling virtual relations.
+ // Create foreign key column for one-to-one relationship
const newColumn = {
- cn: fkColName,
- title: fkColName,
+ cn: fkColName, // Column name in the database
+ title: fkColName, // Human-readable title for the column
rqd: false,
pk: false,
ai: false,
cdf: null,
dt: parent.primaryKey.dt, // Data type of the foreign key column matches the primary key of the parent
dtxp: parent.primaryKey.dtxp,
dtxs: parent.primaryKey.dtxs,
un: parent.primaryKey.un,
altered: Altered.NEW_COLUMN,
+ unique: 1, // Ensure the foreign key column is unique for one-to-one relationships
};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.
| } else if ((param.column as LinkToAnotherColumnReqType).type === 'oo') { | |
| // populate fk column name | |
| const fkColName = getUniqueColumnName( | |
| await child.getColumns(), | |
| `${parent.table_name}_id`, | |
| ); | |
| let foreignKeyName; | |
| { | |
| // create foreign key | |
| const newColumn = { | |
| cn: fkColName, | |
| title: fkColName, | |
| column_name: fkColName, | |
| rqd: false, | |
| pk: false, | |
| ai: false, | |
| cdf: null, | |
| dt: parent.primaryKey.dt, | |
| dtxp: parent.primaryKey.dtxp, | |
| dtxs: parent.primaryKey.dtxs, | |
| un: parent.primaryKey.un, | |
| altered: Altered.NEW_COLUMN, | |
| unique: 1, | |
| }; | |
| const tableUpdateBody = { | |
| ...child, | |
| tn: child.table_name, | |
| originalColumns: child.columns.map((c) => ({ | |
| ...c, | |
| cn: c.column_name, | |
| })), | |
| columns: [ | |
| ...child.columns.map((c) => ({ | |
| ...c, | |
| cn: c.column_name, | |
| })), | |
| newColumn, | |
| ], | |
| }; | |
| await sqlMgr.sqlOpPlus(param.source, 'tableUpdate', tableUpdateBody); | |
| const { id } = await Column.insert({ | |
| ...newColumn, | |
| uidt: UITypes.ForeignKey, | |
| fk_model_id: child.id, | |
| }); | |
| childColumn = await Column.get({ colId: id }); | |
| // ignore relation creation if virtual | |
| if (!(param.column as LinkToAnotherColumnReqType).virtual) { | |
| foreignKeyName = generateFkName(parent, child); | |
| // create relation | |
| await sqlMgr.sqlOpPlus(param.source, 'relationCreate', { | |
| childColumn: fkColName, | |
| childTable: child.table_name, | |
| parentTable: parent.table_name, | |
| onDelete: 'NO ACTION', | |
| onUpdate: 'NO ACTION', | |
| type: 'real', | |
| parentColumn: parent.primaryKey.column_name, | |
| foreignKeyName, | |
| }); | |
| } | |
| // todo: create index for virtual relations as well | |
| // create index for foreign key in pg | |
| if ( | |
| param.source.type === 'pg' || | |
| (param.column as LinkToAnotherColumnReqType).virtual | |
| ) { | |
| await this.createColumnIndex({ | |
| column: new Column({ | |
| ...newColumn, | |
| fk_model_id: child.id, | |
| }), | |
| source: param.source, | |
| sqlMgr, | |
| }); | |
| } | |
| } | |
| await createOOColumn( | |
| child, | |
| parent, | |
| childColumn, | |
| (param.column as LinkToAnotherColumnReqType).type as RelationTypes, | |
| (param.column as LinkToAnotherColumnReqType).title, | |
| foreignKeyName, | |
| (param.column as LinkToAnotherColumnReqType).virtual, | |
| null, | |
| param.column['meta'], | |
| param.colExtra, | |
| ); | |
| // Create foreign key column for one-to-one relationship | |
| const newColumn = { | |
| cn: fkColName, // Column name in the database | |
| title: fkColName, // Human-readable title for the column | |
| rqd: false, | |
| pk: false, | |
| ai: false, | |
| cdf: null, | |
| dt: parent.primaryKey.dt, // Data type of the foreign key column matches the primary key of the parent | |
| dtxp: parent.primaryKey.dtxp, | |
| dtxs: parent.primaryKey.dtxs, | |
| un: parent.primaryKey.un, | |
| altered: Altered.NEW_COLUMN, | |
| unique: 1, // Ensure the foreign key column is unique for one-to-one relationships | |
| }; |
| )?.colOptions as LinkToAnotherRecordType | ||
| ).fk_index_name; | ||
| } else { | ||
| foreignKeyName = relationColOpt.fk_index_name; | ||
| } | ||
|
|
||
| if (!relationColOpt?.virtual && !virtual) { | ||
| // todo: handle relation delete exception | ||
| try { | ||
| await sqlMgr.sqlOpPlus(source, 'relationDelete', { | ||
| childColumn: childColumn.column_name, | ||
| childTable: childTable.table_name, | ||
| parentTable: parentTable.table_name, | ||
| parentColumn: parentColumn.column_name, | ||
| foreignKeyName, | ||
| }); | ||
| } catch (e) { | ||
| console.log(e.message); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!relationColOpt) return; | ||
| const columnsInRelatedTable: Column[] = await relationColOpt | ||
| .getRelatedTable(ncMeta) | ||
| .then((m) => m.getColumns(ncMeta)); | ||
| const relType = relationColOpt.type === 'bt' ? 'hm' : 'bt'; | ||
| for (const c of columnsInRelatedTable) { | ||
| if (c.uidt !== UITypes.LinkToAnotherRecord) continue; | ||
| const colOpt = await c.getColOptions<LinkToAnotherRecordColumn>(ncMeta); | ||
| if ( | ||
| colOpt.fk_parent_column_id === parentColumn.id && | ||
| colOpt.fk_child_column_id === childColumn.id && | ||
| colOpt.type === relType | ||
| ) { | ||
| await Column.delete(c.id, ncMeta); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // delete virtual columns | ||
| await Column.delete(relationColOpt.fk_column_id, ncMeta); | ||
|
|
||
| if (!ignoreFkDelete) { | ||
| const cTable = await Model.getWithInfo( | ||
| { | ||
| id: childTable.id, | ||
| }, | ||
| ncMeta, | ||
| ); | ||
|
|
||
| // if virtual column delete all index before deleting the column | ||
| if (relationColOpt?.virtual) { | ||
| const indexes = | ||
| ( | ||
| await sqlMgr.sqlOp(source, 'indexList', { | ||
| tn: cTable.table_name, | ||
| }) | ||
| )?.data?.list ?? []; | ||
|
|
||
| for (const index of indexes) { | ||
| if (index.cn !== childColumn.column_name) continue; | ||
|
|
||
| await sqlMgr.sqlOpPlus(source, 'indexDelete', { | ||
| ...index, | ||
| tn: cTable.table_name, | ||
| columns: [childColumn.column_name], | ||
| indexName: index.key_name, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| const tableUpdateBody = { | ||
| ...cTable, | ||
| tn: cTable.table_name, | ||
| originalColumns: cTable.columns.map((c) => ({ | ||
| ...c, | ||
| cn: c.column_name, | ||
| cno: c.column_name, | ||
| })), | ||
| columns: cTable.columns.map((c) => { | ||
| if (c.id === childColumn.id) { | ||
| return { | ||
| ...c, | ||
| cn: c.column_name, | ||
| cno: c.column_name, | ||
| altered: Altered.DELETE_COLUMN, | ||
| }; | ||
| } else { | ||
| (c as any).cn = c.column_name; | ||
| } | ||
| return c; | ||
| }), | ||
| }; | ||
|
|
||
| await sqlMgr.sqlOpPlus(source, 'tableUpdate', tableUpdateBody); | ||
| } | ||
| // delete foreign key column | ||
| await Column.delete(childColumn.id, ncMeta); | ||
| }; |
There was a problem hiding this comment.
The deleteOoRelation method for handling the deletion of one-to-one relationships seems to correctly manage the complexities involved, such as dropping foreign keys and ensuring data integrity. A few suggestions for improvement:
- Similar to the
createLTARColumnmethod, consider using constants for hardcoded strings and adding more detailed comments to explain the logic, especially for complex operations. - Verify that all necessary cleanup operations are performed, including removing any associated indexes that might have been created for the foreign key column.
+ // Ensure relation deletion is not attempted for virtual relations
try {
+ // Attempt to delete the foreign key constraint from the database
await sqlMgr.sqlOpPlus(source, 'relationDelete', {
childColumn: childColumn.column_name,
childTable: childTable.table_name,
parentTable: parentTable.table_name,
parentColumn: parentColumn.column_name,
foreignKeyName,
});
} catch (e) {
console.log(e.message);
}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.
| deleteOoRelation = async ( | |
| { | |
| relationColOpt, | |
| source, | |
| childColumn, | |
| childTable, | |
| parentColumn, | |
| parentTable, | |
| sqlMgr, | |
| ncMeta = Noco.ncMeta, | |
| virtual, | |
| }: { | |
| relationColOpt: LinkToAnotherRecordColumn; | |
| source: Source; | |
| childColumn: Column; | |
| childTable: Model; | |
| parentColumn: Column; | |
| parentTable: Model; | |
| sqlMgr: SqlMgrv2; | |
| ncMeta?: MetaService; | |
| virtual?: boolean; | |
| }, | |
| ignoreFkDelete = false, | |
| ) => { | |
| if (childTable) { | |
| let foreignKeyName; | |
| // if relationColOpt is not provided, extract it from child table | |
| // and get the foreign key name for dropping the foreign key | |
| if (!relationColOpt) { | |
| foreignKeyName = ( | |
| ( | |
| await childTable.getColumns(ncMeta).then(async (cols) => { | |
| for (const col of cols) { | |
| if (col.uidt === UITypes.LinkToAnotherRecord) { | |
| const colOptions = | |
| await col.getColOptions<LinkToAnotherRecordColumn>(ncMeta); | |
| if (colOptions.fk_related_model_id === parentTable.id) { | |
| return { colOptions }; | |
| } | |
| } | |
| } | |
| }) | |
| )?.colOptions as LinkToAnotherRecordType | |
| ).fk_index_name; | |
| } else { | |
| foreignKeyName = relationColOpt.fk_index_name; | |
| } | |
| if (!relationColOpt?.virtual && !virtual) { | |
| // todo: handle relation delete exception | |
| try { | |
| await sqlMgr.sqlOpPlus(source, 'relationDelete', { | |
| childColumn: childColumn.column_name, | |
| childTable: childTable.table_name, | |
| parentTable: parentTable.table_name, | |
| parentColumn: parentColumn.column_name, | |
| foreignKeyName, | |
| }); | |
| } catch (e) { | |
| console.log(e.message); | |
| } | |
| } | |
| } | |
| if (!relationColOpt) return; | |
| const columnsInRelatedTable: Column[] = await relationColOpt | |
| .getRelatedTable(ncMeta) | |
| .then((m) => m.getColumns(ncMeta)); | |
| const relType = relationColOpt.type === 'bt' ? 'hm' : 'bt'; | |
| for (const c of columnsInRelatedTable) { | |
| if (c.uidt !== UITypes.LinkToAnotherRecord) continue; | |
| const colOpt = await c.getColOptions<LinkToAnotherRecordColumn>(ncMeta); | |
| if ( | |
| colOpt.fk_parent_column_id === parentColumn.id && | |
| colOpt.fk_child_column_id === childColumn.id && | |
| colOpt.type === relType | |
| ) { | |
| await Column.delete(c.id, ncMeta); | |
| break; | |
| } | |
| } | |
| // delete virtual columns | |
| await Column.delete(relationColOpt.fk_column_id, ncMeta); | |
| if (!ignoreFkDelete) { | |
| const cTable = await Model.getWithInfo( | |
| { | |
| id: childTable.id, | |
| }, | |
| ncMeta, | |
| ); | |
| // if virtual column delete all index before deleting the column | |
| if (relationColOpt?.virtual) { | |
| const indexes = | |
| ( | |
| await sqlMgr.sqlOp(source, 'indexList', { | |
| tn: cTable.table_name, | |
| }) | |
| )?.data?.list ?? []; | |
| for (const index of indexes) { | |
| if (index.cn !== childColumn.column_name) continue; | |
| await sqlMgr.sqlOpPlus(source, 'indexDelete', { | |
| ...index, | |
| tn: cTable.table_name, | |
| columns: [childColumn.column_name], | |
| indexName: index.key_name, | |
| }); | |
| } | |
| } | |
| const tableUpdateBody = { | |
| ...cTable, | |
| tn: cTable.table_name, | |
| originalColumns: cTable.columns.map((c) => ({ | |
| ...c, | |
| cn: c.column_name, | |
| cno: c.column_name, | |
| })), | |
| columns: cTable.columns.map((c) => { | |
| if (c.id === childColumn.id) { | |
| return { | |
| ...c, | |
| cn: c.column_name, | |
| cno: c.column_name, | |
| altered: Altered.DELETE_COLUMN, | |
| }; | |
| } else { | |
| (c as any).cn = c.column_name; | |
| } | |
| return c; | |
| }), | |
| }; | |
| await sqlMgr.sqlOpPlus(source, 'tableUpdate', tableUpdateBody); | |
| } | |
| // delete foreign key column | |
| await Column.delete(childColumn.id, ncMeta); | |
| }; | |
| deleteOoRelation = async ( | |
| { | |
| relationColOpt, | |
| source, | |
| childColumn, | |
| childTable, | |
| parentColumn, | |
| parentTable, | |
| sqlMgr, | |
| ncMeta = Noco.ncMeta, | |
| virtual, | |
| }: { | |
| relationColOpt: LinkToAnotherRecordColumn; | |
| source: Source; | |
| childColumn: Column; | |
| childTable: Model; | |
| parentColumn: Column; | |
| parentTable: Model; | |
| sqlMgr: SqlMgrv2; | |
| ncMeta?: MetaService; | |
| virtual?: boolean; | |
| }, | |
| ignoreFkDelete = false, | |
| ) => { | |
| if (childTable) { | |
| let foreignKeyName; | |
| // if relationColOpt is not provided, extract it from child table | |
| // and get the foreign key name for dropping the foreign key | |
| if (!relationColOpt) { | |
| foreignKeyName = ( | |
| ( | |
| await childTable.getColumns(ncMeta).then(async (cols) => { | |
| for (const col of cols) { | |
| if (col.uidt === UITypes.LinkToAnotherRecord) { | |
| const colOptions = | |
| await col.getColOptions<LinkToAnotherRecordColumn>(ncMeta); | |
| if (colOptions.fk_related_model_id === parentTable.id) { | |
| return { colOptions }; | |
| } | |
| } | |
| } | |
| }) | |
| )?.colOptions as LinkToAnotherRecordType | |
| ).fk_index_name; | |
| } else { | |
| foreignKeyName = relationColOpt.fk_index_name; | |
| } | |
| if (!relationColOpt?.virtual && !virtual) { | |
| // todo: handle relation delete exception | |
| try { | |
| // Ensure relation deletion is not attempted for virtual relations | |
| // Attempt to delete the foreign key constraint from the database | |
| await sqlMgr.sqlOpPlus(source, 'relationDelete', { | |
| childColumn: childColumn.column_name, | |
| childTable: childTable.table_name, | |
| parentTable: parentTable.table_name, | |
| parentColumn: parentColumn.column_name, | |
| foreignKeyName, | |
| }); | |
| } catch (e) { | |
| console.log(e.message); | |
| } | |
| } | |
| } | |
| if (!relationColOpt) return; | |
| const columnsInRelatedTable: Column[] = await relationColOpt | |
| .getRelatedTable(ncMeta) | |
| .then((m) => m.getColumns(ncMeta)); | |
| const relType = relationColOpt.type === 'bt' ? 'hm' : 'bt'; | |
| for (const c of columnsInRelatedTable) { | |
| if (c.uidt !== UITypes.LinkToAnotherRecord) continue; | |
| const colOpt = await c.getColOptions<LinkToAnotherRecordColumn>(ncMeta); | |
| if ( | |
| colOpt.fk_parent_column_id === parentColumn.id && | |
| colOpt.fk_child_column_id === childColumn.id && | |
| colOpt.type === relType | |
| ) { | |
| await Column.delete(c.id, ncMeta); | |
| break; | |
| } | |
| } | |
| // delete virtual columns | |
| await Column.delete(relationColOpt.fk_column_id, ncMeta); | |
| if (!ignoreFkDelete) { | |
| const cTable = await Model.getWithInfo( | |
| { | |
| id: childTable.id, | |
| }, | |
| ncMeta, | |
| ); | |
| // if virtual column delete all index before deleting the column | |
| if (relationColOpt?.virtual) { | |
| const indexes = | |
| ( | |
| await sqlMgr.sqlOp(source, 'indexList', { | |
| tn: cTable.table_name, | |
| }) | |
| )?.data?.list ?? []; | |
| for (const index of indexes) { | |
| if (index.cn !== childColumn.column_name) continue; | |
| await sqlMgr.sqlOpPlus(source, 'indexDelete', { | |
| ...index, | |
| tn: cTable.table_name, | |
| columns: [childColumn.column_name], | |
| indexName: index.key_name, | |
| }); | |
| } | |
| } | |
| const tableUpdateBody = { | |
| ...cTable, | |
| tn: cTable.table_name, | |
| originalColumns: cTable.columns.map((c) => ({ | |
| ...c, | |
| cn: c.column_name, | |
| cno: c.column_name, | |
| })), | |
| columns: cTable.columns.map((c) => { | |
| if (c.id === childColumn.id) { | |
| return { | |
| ...c, | |
| cn: c.column_name, | |
| cno: c.column_name, | |
| altered: Altered.DELETE_COLUMN, | |
| }; | |
| } else { | |
| (c as any).cn = c.column_name; | |
| } | |
| return c; | |
| }), | |
| }; | |
| await sqlMgr.sqlOpPlus(source, 'tableUpdate', tableUpdateBody); | |
| } | |
| // delete foreign key column | |
| await Column.delete(childColumn.id, ncMeta); | |
| }; |
| import ProjectMgrv2 from '~/db/sql-mgr/v2/ProjectMgrv2'; | ||
| import { | ||
| createHmAndBtColumn, | ||
| createOOColumn, | ||
| generateFkName, | ||
| randomID, | ||
| sanitizeColumnName, |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3640-3657]
The createColumnIndex method correctly handles the creation of indexes for columns. It's well-structured and accounts for both unique and non-unique indexes. To further improve the code:
- Consider adding error handling to catch and log any issues that might occur during the index creation process. This would help in debugging and ensuring the robustness of the operation.
+ try {
sqlMgr.sqlOpPlus(source, 'indexCreate', indexArgs);
+ } catch (e) {
+ console.error(`Failed to create index for column ${column.column_name}: ${e.message}`);
+ }📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3620-3638]
The updateRollupOrLookup method provides a clear approach to updating rollup and lookup columns. However, it could be enhanced by:
- Adding more detailed validation for the
colBodyparameters specific to rollup and lookup columns. This would ensure that all necessary data is present and valid before attempting an update. - Improving error handling to provide more informative feedback to the caller in case of validation failures or update issues.
+ // Validate rollup or lookup payload before proceeding with the update
if (
UITypes.Lookup === column.uidt &&
validateRequiredField(colBody, [
'fk_lookup_column_id',
'fk_relation_column_id',
])
) {
+ // Perform additional validation for lookup payload
await validateLookupPayload(colBody, column.id);
await Column.update(column.id, colBody);
} else if (
UITypes.Rollup === column.uidt &&
validateRequiredField(colBody, [
'fk_relation_column_id',
'fk_rollup_column_id',
'rollup_function',
])
) {
+ // Perform additional validation for rollup payload
await validateRollupPayload(colBody);
await Column.update(column.id, colBody);
}📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3660-3702]
The columnBulk method effectively handles bulk operations on columns, including add, update, and delete operations. Suggestions for improvement include:
- Enhancing error handling to capture and report errors for each operation individually. This would provide more detailed feedback on which operations failed and why.
- Adding more detailed comments to explain the logic behind the hash check and how it ensures data consistency.
+ // Perform operations in a loop, capturing any errors for individual operations
for (const op of ops) {
const column = op.column;
if (op.op === 'add') {
try {
await this.columnAdd({
tableId,
column: column as ColumnReqType,
req,
user: req.user,
reuse,
});
} catch (e) {
failedOps.push({
...op,
error: e.message,
});
}
} else if (op.op === 'update') {
// Similar error handling for 'update' and 'delete' operations
}There was a problem hiding this comment.
Error handling is done globally most of the cases
There was a problem hiding this comment.
@pranavxc, thank you for clarifying the project's approach to error handling. It makes sense to rely on a global error handling strategy, especially in a large and complex codebase where consistency and centralization of error processing can significantly simplify maintenance and debugging. I appreciate your insight!
a3b65cc to
d442988
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
packages/nc-gui/assets/nc-icons/onetoone.svgis excluded by:!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger-v2.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger.jsonis excluded by:!**/*.json
Files selected for processing (38)
- packages/nc-gui/components/smartsheet/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue (3 hunks)
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue (2 hunks)
- packages/nc-gui/components/smartsheet/grid/Table.vue (5 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue (1 hunks)
- packages/nc-gui/components/virtual-cell/BelongsTo.vue (3 hunks)
- packages/nc-gui/components/virtual-cell/Lookup.vue (2 hunks)
- packages/nc-gui/components/virtual-cell/OneToOne.vue (1 hunks)
- packages/nc-gui/composables/useLTARStore.ts (9 hunks)
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts (2 hunks)
- packages/nc-gui/composables/useMultiSelect/index.ts (4 hunks)
- packages/nc-gui/composables/useSmartsheetRowStore.ts (5 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nc-gui/utils/virtualCell.ts (1 hunks)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/controllers/data-alias-nested.controller.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (17 hunks)
- packages/nocodb/src/db/conditionV2.ts (8 hunks)
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (20 hunks)
- packages/nocodb/src/db/genRollupSelectv2.ts (1 hunks)
- packages/nocodb/src/db/generateLookupSelectQuery.ts (6 hunks)
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts (1 hunks)
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts (2 hunks)
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts (3 hunks)
- packages/nocodb/src/helpers/columnHelpers.ts (3 hunks)
- packages/nocodb/src/helpers/getAst.ts (1 hunks)
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts (1 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts (4 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts (2 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts (5 hunks)
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts (1 hunks)
- packages/nocodb/src/services/columns.service.ts (5 hunks)
- packages/nocodb/src/services/data-alias-nested.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (2 hunks)
Files skipped from review as they are similar to previous changes (37)
- packages/nc-gui/components/smartsheet/VirtualCell.vue
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue
- packages/nc-gui/components/virtual-cell/BelongsTo.vue
- packages/nc-gui/components/virtual-cell/Lookup.vue
- packages/nc-gui/components/virtual-cell/OneToOne.vue
- packages/nc-gui/composables/useLTARStore.ts
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts
- packages/nc-gui/composables/useMultiSelect/index.ts
- packages/nc-gui/composables/useSmartsheetRowStore.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nc-gui/utils/virtualCell.ts
- packages/nocodb-sdk/src/lib/globals.ts
- packages/nocodb/src/controllers/data-alias-nested.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/db/conditionV2.ts
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts
- packages/nocodb/src/db/genRollupSelectv2.ts
- packages/nocodb/src/db/generateLookupSelectQuery.ts
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts
- packages/nocodb/src/helpers/columnHelpers.ts
- packages/nocodb/src/helpers/getAst.ts
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts
- packages/nocodb/src/services/data-alias-nested.service.ts
- packages/nocodb/src/utils/acl.ts
Additional comments: 3
packages/nocodb/src/services/columns.service.ts (3)
- 2785-2881: The implementation for handling one-to-one ('oo') relationships in the
createLTARColumnmethod is consistent with the PR's objectives. It's good to see the unique constraint being enforced at the database level for these relationships.Consider adding comments to explain the logic, especially for complex operations like relation creation and handling virtual relations, to improve clarity and maintainability.
- 2507-2649: The
deleteOoRelationmethod for handling the deletion of one-to-one relationships seems to correctly manage the complexities involved, such as dropping foreign keys and ensuring data integrity.Consider using constants for hardcoded strings and adding more detailed comments to explain the logic, especially for complex operations. Also, verify that all necessary cleanup operations are performed, including removing any associated indexes that might have been created for the foreign key column.
- 1647-1647: Emitting the
RELATION_CREATEevent after adding a column for one-to-one relationships is a good practice for enabling further asynchronous processing or notifications.
d442988 to
0ae36ed
Compare
0ae36ed to
a398521
Compare
Signed-off-by: Pranav C <pranavxc@gmail.com>
Signed-off-by: Pranav C <pranavxc@gmail.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
packages/nc-gui/assets/nc-icons/onetoone.svgis excluded by:!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger-v2.jsonis excluded by:!**/*.jsonpackages/nocodb/src/schema/swagger.jsonis excluded by:!**/*.json
Files selected for processing (38)
- packages/nc-gui/components/smartsheet/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue (3 hunks)
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue (2 hunks)
- packages/nc-gui/components/smartsheet/grid/Table.vue (5 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue (2 hunks)
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue (1 hunks)
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue (1 hunks)
- packages/nc-gui/components/virtual-cell/BelongsTo.vue (3 hunks)
- packages/nc-gui/components/virtual-cell/Lookup.vue (2 hunks)
- packages/nc-gui/components/virtual-cell/OneToOne.vue (1 hunks)
- packages/nc-gui/composables/useLTARStore.ts (9 hunks)
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts (2 hunks)
- packages/nc-gui/composables/useMultiSelect/index.ts (4 hunks)
- packages/nc-gui/composables/useSmartsheetRowStore.ts (5 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nc-gui/utils/virtualCell.ts (1 hunks)
- packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
- packages/nocodb/src/controllers/data-alias-nested.controller.ts (1 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (17 hunks)
- packages/nocodb/src/db/conditionV2.ts (8 hunks)
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (20 hunks)
- packages/nocodb/src/db/genRollupSelectv2.ts (1 hunks)
- packages/nocodb/src/db/generateLookupSelectQuery.ts (6 hunks)
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts (1 hunks)
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts (2 hunks)
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts (3 hunks)
- packages/nocodb/src/helpers/columnHelpers.ts (4 hunks)
- packages/nocodb/src/helpers/getAst.ts (1 hunks)
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts (1 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts (4 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts (2 hunks)
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts (5 hunks)
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts (1 hunks)
- packages/nocodb/src/services/columns.service.ts (6 hunks)
- packages/nocodb/src/services/data-alias-nested.service.ts (1 hunks)
- packages/nocodb/src/utils/acl.ts (2 hunks)
Files skipped from review as they are similar to previous changes (38)
- packages/nc-gui/components/smartsheet/VirtualCell.vue
- packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/smartsheet/header/VirtualCell.vue
- packages/nc-gui/components/smartsheet/header/VirtualCellIcon.ts
- packages/nc-gui/components/smartsheet/toolbar/CreateGroupBy.vue
- packages/nc-gui/components/smartsheet/toolbar/CreateSort.vue
- packages/nc-gui/components/smartsheet/toolbar/SortListMenu.vue
- packages/nc-gui/components/virtual-cell/BelongsTo.vue
- packages/nc-gui/components/virtual-cell/Lookup.vue
- packages/nc-gui/components/virtual-cell/OneToOne.vue
- packages/nc-gui/composables/useLTARStore.ts
- packages/nc-gui/composables/useMultiSelect/convertCellData.ts
- packages/nc-gui/composables/useMultiSelect/index.ts
- packages/nc-gui/composables/useSmartsheetRowStore.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nc-gui/utils/virtualCell.ts
- packages/nocodb-sdk/src/lib/globals.ts
- packages/nocodb/src/controllers/data-alias-nested.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/db/conditionV2.ts
- packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts
- packages/nocodb/src/db/genRollupSelectv2.ts
- packages/nocodb/src/db/generateLookupSelectQuery.ts
- packages/nocodb/src/db/sql-client/lib/mysql/MysqlClient.ts
- packages/nocodb/src/db/sql-client/lib/pg/PgClient.ts
- packages/nocodb/src/db/sql-client/lib/sqlite/SqliteClient.ts
- packages/nocodb/src/helpers/columnHelpers.ts
- packages/nocodb/src/helpers/getAst.ts
- packages/nocodb/src/models/LinkToAnotherRecordColumn.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/duplicate.processor.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/export.service.ts
- packages/nocodb/src/modules/jobs/jobs/export-import/import.service.ts
- packages/nocodb/src/services/api-docs/swaggerV2/templates/paths.ts
- packages/nocodb/src/services/columns.service.ts
- packages/nocodb/src/services/data-alias-nested.service.ts
- packages/nocodb/src/utils/acl.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
Summary by CodeRabbit