feat: impl dal for standalone tegg #197
Conversation
|
Warning Rate Limit Exceeded@killagu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 24 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 focus on the migration from Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (6)
plugin/dal/package.jsonis excluded by:!**/*.jsonstandalone/standalone/package.jsonis excluded by:!**/*.jsonstandalone/standalone/test/fixtures/dal-module/dal/structure/Foo.jsonis excluded by:!**/*.jsonstandalone/standalone/test/fixtures/dal-module/module.ymlis excluded by:!**/*.ymlstandalone/standalone/test/fixtures/dal-module/package.jsonis excluded by:!**/*.jsonstandalone/standalone/test/fixtures/dal-module/tsconfig.jsonis excluded by:!**/*.json
Files selected for processing (13)
- core/dal-runtime/src/BaseSqlMap.ts (2 hunks)
- core/dal-runtime/src/SqlMapLoader.ts (1 hunks)
- core/tegg/index.ts (1 hunks)
- plugin/dal/app.ts (1 hunks)
- plugin/dal/lib/DalTableEggPrototypeHook.ts (1 hunks)
- standalone/standalone/src/Runner.ts (5 hunks)
- standalone/standalone/test/fixtures/dal-module/Foo.ts (1 hunks)
- standalone/standalone/test/fixtures/dal-module/dal/dao/FooDAO.ts (1 hunks)
- standalone/standalone/test/fixtures/dal-module/dal/dao/base/BaseFooDAO.ts (1 hunks)
- standalone/standalone/test/fixtures/dal-module/dal/extension/FooExtension.ts (1 hunks)
- standalone/standalone/test/fixtures/dal-module/dal/structure/Foo.sql (1 hunks)
- standalone/standalone/test/fixtures/dal-module/main.ts (1 hunks)
- standalone/standalone/test/index.test.ts (2 hunks)
Additional comments: 24
standalone/standalone/test/fixtures/dal-module/dal/extension/FooExtension.ts (1)
- 15-20: The SQL mapping for
findByNameis correctly defined. Ensure that the placeholders are properly sanitized and handled to prevent SQL injection vulnerabilities.standalone/standalone/test/fixtures/dal-module/dal/dao/FooDAO.ts (1)
- 11-19: The
FooDAOclass is well-implemented, making good use of dependency injection and the singleton pattern. ThefindByNamemethod is correctly defined.plugin/dal/lib/DalTableEggPrototypeHook.ts (1)
- 4-12: The replacement of
EggLoggerwith the newLoggerinterface is correctly implemented. Ensure that the lifecycle hook functionality is thoroughly tested, especially the management of table models and SQL maps.core/dal-runtime/src/SqlMapLoader.ts (1)
- 3-12: The update to use the new
Loggerinterface is correctly implemented. Consider adding more detailed error logging within theloadmethod to aid in troubleshooting SQL map loading issues.standalone/standalone/test/fixtures/dal-module/dal/structure/Foo.sql (1)
- 1-44: The SQL schema for the
egg_footable is well-defined, covering a wide range of column types and indexes. Consider reviewing the defined indexes for necessity and performance impact, especially in production environments.standalone/standalone/test/fixtures/dal-module/main.ts (1)
- 7-92: The
FooRunnerclass demonstrates the practical use of the DAL for database operations. Ensure that error handling is implemented for database operations and that input validation is performed for the properties set on theFooobject.standalone/standalone/test/fixtures/dal-module/Foo.ts (7)
- 13-18: The
@Tabledecorator is correctly used to define table properties such as name, comment, character set, and collation. This is a good practice for ensuring that the table's metadata is clearly specified.- 19-24: The use of the
@Indexdecorator to define a unique index on thenameandcol1columns is appropriate. Specifying the index type and store type enhances the database's performance and search capabilities.- 31-37: The primary key column
idis correctly defined with the@Columndecorator, including properties likeautoIncrementand a comment. This is essential for database integrity and identification of records.- 40-45: Using the
uniqueKey: trueoption for thenamecolumn ensures that each record has a unique name. This is a good practice for data uniqueness and integrity.- 56-60: Consider adding a comment to the
bitColumndefinition to explain its purpose or usage within the application. While the column definition is correct, additional documentation can improve code maintainability.- 231-237: The definition of the
enumColumnwith specific allowed values ('A', 'B') is a good use of theENUMtype. This restricts the column to a set of predefined values, enhancing data integrity.- 247-250: The inclusion of a
geometryColumnof typeGEOMETRYdemonstrates the capability to handle spatial data types. This is particularly useful for applications that require geographic or spatial data.standalone/standalone/test/index.test.ts (1)
- 8-8: The import statement for
Foois correctly added to support the new test case in thedal runnersection. This ensures that theFooclass is available for testing.core/dal-runtime/src/BaseSqlMap.ts (2)
- 3-3: The update to import
Loggerfrom@eggjs/tegginstead ofEggLoggeraligns with the shift towards a more unified logging system tailored for Tegg. This is a positive change for consistency across the framework.- 14-16: The modification of the
BaseSqlMapGeneratorconstructor to accept aLoggerinstance is correctly implemented. This ensures that the class uses the updated logging interface.standalone/standalone/src/Runner.ts (3)
- 33-37: The addition of imports for DAL-related classes (
DalTableEggPrototypeHook,DalModuleLoadUnitHook,MysqlDataSourceManager,SqlMapManager,TableModelManager) is correctly done. These classes are essential for the DAL's lifecycle management within the standalone runner.- 157-162: The instantiation and registration of
DalTableEggPrototypeHookandDalModuleLoadUnitHookwithin theRunnerclass are correctly implemented. This ensures that DAL-related hooks are properly integrated into the lifecycle management of the application.- 253-262: The cleanup process in the
destroymethod, which clears instances ofMysqlDataSourceManager,SqlMapManager, andTableModelManager, is a good practice. It ensures that resources are properly released when the application is shutting down.standalone/standalone/test/fixtures/dal-module/dal/dao/base/BaseFooDAO.ts (5)
- 10-10: The comment warns against manual modifications as the file is auto-generated. This is good practice for generated code to prevent accidental edits. However, ensure there's a clear process or documentation on how to regenerate this file if the model or requirements change.
- 14-16: Using
@SingletonProtowithAccessLevel.PUBLICis appropriate for a DAO class that needs to be widely accessible within the application. This setup ensures that the class is treated as a singleton, which is a common pattern for DAOs to ensure efficient resource use and consistent access to the data source.- 26-230: The
insertmethod dynamically constructs a data object for insertion based on the providedrawobject. This approach is flexible but requires careful validation of input data to prevent SQL injection or other security vulnerabilities. Ensure that all data passed to this method is properly sanitized upstream and that thedataSource.executeRawScalarmethod includes adequate protection against injection attacks.- 233-442: Similar to the
insertmethod, theupdatemethod constructs a new data object for updating records. It's crucial to ensure that theidparameter and thedataobject are properly validated and sanitized to prevent SQL injection and ensure data integrity. Additionally, consider implementing optimistic locking to handle concurrent updates safely.- 457-493: The various
findmethods demonstrate a consistent pattern for querying the database. Ensure that these methods are optimized for performance, especially for large datasets. Consider adding pagination support or query optimization techniques as needed.
| @Inject({ | ||
| name: DataSourceInjectName, | ||
| }) | ||
| @DataSourceQualifier('dal.foo.Foo') | ||
| protected readonly dataSource: DataSource<Foo>; |
There was a problem hiding this comment.
Tip
Codebase Verification
The search conducted across the repository for 'dal.foo.Foo' only found occurrences within @DataSourceQualifier annotations and did not reveal any actual configuration files or entries defining 'dal.foo.Foo'. This suggests that the data source configuration for 'dal.foo.Foo' might not exist or is not correctly set up, supporting the concerns raised in the review comment about potential misconfiguration leading to runtime errors or incorrect data source usage.
Analysis chain
The use of @Inject and @DataSourceQualifier to inject the DataSource is a clean way to manage dependencies. However, ensure that the qualifier string 'dal.foo.Foo' correctly matches the intended data source configuration. Misconfiguration here could lead to runtime errors or incorrect data source usage.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the data source configuration for 'dal.foo.Foo' exists and is correctly set up.
grep -R "dal.foo.Foo" ./config
Length of output: 72
Script:
#!/bin/bash
# Perform a broader search for 'dal.foo.Foo' across the repository to find the data source configuration.
grep -R "dal.foo.Foo" .
Length of output: 254
| public async delete(id: number): Promise<DeleteResult> { | ||
| return this.dataSource.executeRawScalar('delete', { | ||
| id, | ||
| }); | ||
| } | ||
|
|
||
| public async del(id: number): Promise<DeleteResult> { | ||
| return this.dataSource.executeRawScalar('delete', { | ||
| id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The delete and del methods are identical, providing two ways to delete a record by its id. Having duplicate functionality under different method names can lead to confusion and maintenance challenges. Consider removing one of the methods or clearly documenting the reason for the duplication.
- public async del(id: number): Promise<DeleteResult> {
- return this.dataSource.executeRawScalar('delete', {
- id,
- });
- }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.
| public async delete(id: number): Promise<DeleteResult> { | |
| return this.dataSource.executeRawScalar('delete', { | |
| id, | |
| }); | |
| } | |
| public async del(id: number): Promise<DeleteResult> { | |
| return this.dataSource.executeRawScalar('delete', { | |
| id, | |
| }); | |
| } | |
| public async delete(id: number): Promise<DeleteResult> { | |
| return this.dataSource.executeRawScalar('delete', { | |
| id, | |
| }); | |
| } |
| import _ from 'lodash'; | ||
| import { ColumnType, IndexType, SqlType, SqlMap, TableModel } from '@eggjs/dal-decorator'; | ||
| import { type EggLogger } from 'egg'; | ||
| import { Logger } from '@eggjs/tegg'; | ||
| import { TemplateUtil } from './TemplateUtil'; | ||
|
|
||
| export interface GenerateSqlMap { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [56-84]
Given the learning note about a GitHub issue opened to track the implementation of index left matching in the BaseSqlMapGenerator class, it's important to ensure that this feature is either implemented or tracked appropriately. If not already addressed, consider adding a comment or TODO in the code to remind of this pending enhancement.
Would you like me to open a follow-up GitHub issue to track the implementation of index left matching, or is there already an issue for this?
| export interface Logger { | ||
| debug(message?: any, ...optionalParams: any[]): void; | ||
| log(message?: any, ...optionalParams: any[]): void; | ||
| info(message?: any, ...optionalParams: any[]): void; | ||
| warn(message?: any, ...optionalParams: any[]): void; | ||
| error(message?: any, ...optionalParams: any[]): void; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The findings indicate that EggLogger is still being used in various parts of the codebase, including core functionalities, plugins, and test fixtures. This suggests that not all components have been updated to use the newly introduced Logger interface. It's important for the development team to manually verify these occurrences to ensure they align with the intended architectural and compatibility goals, especially considering the potential for deliberate maintenance of EggLogger usage in certain scenarios.
Analysis chain
The Logger interface is well-defined, covering standard logging levels. Ensure that this interface is consistently implemented across the application and that existing logging calls are updated accordingly.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for files that might still be using the old `EggLogger` to ensure they've been updated.
rg --type ts "EggLogger"
Length of output: 2190
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- core/dal-runtime/test/DataSource.test.ts (1 hunks)
- plugin/controller/README.md (1 hunks)
- plugin/dal/README.md (3 hunks)
- standalone/standalone/test/index.test.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- core/dal-runtime/test/DataSource.test.ts
Files skipped from review as they are similar to previous changes (1)
- standalone/standalone/test/index.test.ts
Additional comments: 1
plugin/controller/README.md (1)
- 20-20: The modification to use the
"extends": "@eggjs/tsconfig"directive intsconfig.jsonaligns with best practices for maintaining consistent TypeScript configurations across projects. This change simplifies the configuration and leverages shared settings, which is beneficial for project maintainability.
|
| @@ -1,3 +1,5 @@ | |||
| /// <reference path='./typings/index.d.ts'/> | |||
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit