feat: impl dal #192
Merged
feat: impl dal #192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fengmk2
reviewed
Mar 12, 2024
fengmk2
reviewed
Mar 12, 2024
fengmk2
reviewed
Mar 18, 2024
fengmk2
reviewed
Mar 18, 2024
fengmk2
reviewed
Mar 18, 2024
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThis update introduces enhancements across different components, focusing on database abstraction layer (DAL) functionality. It adds new decorators, utilities, models, and types for managing database schema elements like tables, columns, and indexes. The changes also include guidance for using the 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: 31
Configuration used: CodeRabbit UI
Files ignored due to path filters (21)
core/dal-decorator/package.jsonis excluded by:!**/*.jsoncore/dal-decorator/test/fixtures/modules/dal/package.jsonis excluded by:!**/*.jsoncore/dal-decorator/tsconfig.jsonis excluded by:!**/*.jsoncore/dal-decorator/tsconfig.pub.jsonis excluded by:!**/*.jsoncore/dal-runtime/package.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/dal/dal/structure/Foo.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/dal/package.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/Foo.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/MultiPrimaryKey.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/generate_codes/package.jsonis excluded by:!**/*.jsoncore/dal-runtime/tsconfig.jsonis excluded by:!**/*.jsoncore/dal-runtime/tsconfig.pub.jsonis excluded by:!**/*.jsoncore/tegg/package.jsonis excluded by:!**/*.jsonplugin/dal/package.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/config/module.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/structure/Foo.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/modules/dal/module.ymlis excluded by:!**/*.ymlplugin/dal/test/fixtures/apps/dal-app/modules/dal/package.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/package.jsonis excluded by:!**/*.jsonplugin/dal/tsconfig.jsonis excluded by:!**/*.jsonplugin/dal/tsconfig.pub.jsonis excluded by:!**/*.json
Files selected for processing (88)
- .gitignore (1 hunks)
- core/core-decorator/src/model/EggMultiInstancePrototypeInfo.ts (1 hunks)
- core/core-decorator/test/decorators.test.ts (1 hunks)
- core/dal-decorator/README.md (1 hunks)
- core/dal-decorator/index.ts (1 hunks)
- core/dal-decorator/src/decorator/Column.ts (1 hunks)
- core/dal-decorator/src/decorator/DataSourceQualifier.ts (1 hunks)
- core/dal-decorator/src/decorator/Index.ts (1 hunks)
- core/dal-decorator/src/decorator/Table.ts (1 hunks)
- core/dal-decorator/src/enum/ColumnFormat.ts (1 hunks)
- core/dal-decorator/src/enum/ColumnType.ts (1 hunks)
- core/dal-decorator/src/enum/CompressionType.ts (1 hunks)
- core/dal-decorator/src/enum/IndexStoreType.ts (1 hunks)
- core/dal-decorator/src/enum/IndexType.ts (1 hunks)
- core/dal-decorator/src/enum/InsertMethod.ts (1 hunks)
- core/dal-decorator/src/enum/RowFormat.ts (1 hunks)
- core/dal-decorator/src/model/ColumnModel.ts (1 hunks)
- core/dal-decorator/src/model/IndexModel.ts (1 hunks)
- core/dal-decorator/src/model/TableModel.ts (1 hunks)
- core/dal-decorator/src/type/ColumnTsType.ts (1 hunks)
- core/dal-decorator/src/type/DateSource.ts (1 hunks)
- core/dal-decorator/src/type/MySql.ts (1 hunks)
- core/dal-decorator/src/type/Spatial.ts (1 hunks)
- core/dal-decorator/src/type/SqlMap.ts (1 hunks)
- core/dal-decorator/src/util/ColumnInfoUtil.ts (1 hunks)
- core/dal-decorator/src/util/IndexInfoUtil.ts (1 hunks)
- core/dal-decorator/src/util/TableInfoUtil.ts (1 hunks)
- core/dal-decorator/test/fixtures/modules/dal/Foo.ts (1 hunks)
- core/dal-decorator/test/index.test.ts (1 hunks)
- core/dal-runtime/README.md (1 hunks)
- core/dal-runtime/index.ts (1 hunks)
- core/dal-runtime/src/BaseSqlMap.ts (1 hunks)
- core/dal-runtime/src/CodeGenerator.ts (1 hunks)
- core/dal-runtime/src/DataSource.ts (1 hunks)
- core/dal-runtime/src/MySqlDataSource.ts (1 hunks)
- core/dal-runtime/src/NunjucksConverter.ts (1 hunks)
- core/dal-runtime/src/NunjucksUtil.ts (1 hunks)
- core/dal-runtime/src/SqlGenerator.ts (1 hunks)
- core/dal-runtime/src/SqlMapLoader.ts (1 hunks)
- core/dal-runtime/src/SqlUtil.ts (1 hunks)
- core/dal-runtime/src/TableModelInstanceBuilder.ts (1 hunks)
- core/dal-runtime/src/TableSqlMap.ts (1 hunks)
- core/dal-runtime/src/TemplateUtil.ts (1 hunks)
- core/dal-runtime/src/templates/base_dao.njk (1 hunks)
- core/dal-runtime/src/templates/dao.njk (1 hunks)
- core/dal-runtime/src/templates/extension.njk (1 hunks)
- core/dal-runtime/test/CodeGenerator.test.ts (1 hunks)
- core/dal-runtime/test/DAO.test.ts (1 hunks)
- core/dal-runtime/test/DataSource.test.ts (1 hunks)
- core/dal-runtime/test/SqlGenerator.test.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/Foo.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/dao/FooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/dao/base/BaseFooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/extension/FooExtension.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/structure/Foo.sql (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/Foo.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/MultiPrimaryKey.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/FooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/MultiPrimaryKeyDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseFooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseMultiPrimaryKeyDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/FooExtension.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/MultiPrimaryKeyExtension.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/Foo.sql (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/MultiPrimaryKey.sql (1 hunks)
- core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (5 hunks)
- core/metadata/src/model/AppGraph.ts (4 hunks)
- core/metadata/test/ModuleGraph.test.ts (1 hunks)
- core/tegg/dal.ts (1 hunks)
- plugin/dal/README.md (1 hunks)
- plugin/dal/app.ts (1 hunks)
- plugin/dal/lib/DalModuleLoadUnitHook.ts (1 hunks)
- plugin/dal/lib/DalTableEggPrototypeHook.ts (1 hunks)
- plugin/dal/lib/DataSource.ts (1 hunks)
- plugin/dal/lib/MysqlDataSourceManager.ts (1 hunks)
- plugin/dal/lib/SqlMapManager.ts (1 hunks)
- plugin/dal/lib/TableModelManager.ts (1 hunks)
- plugin/dal/test/dal.test.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/config/config.default.js (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/config/plugin.js (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/Foo.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/FooDAO.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/base/BaseFooDAO.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/extension/FooExtension.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/structure/Foo.sql (1 hunks)
- plugin/dal/typings/index.d.ts (1 hunks)
- plugin/tegg/lib/AppLoadUnit.ts (1 hunks)
Files skipped from review due to trivial changes (13)
- .gitignore
- core/dal-decorator/src/enum/ColumnFormat.ts
- core/dal-decorator/src/enum/ColumnType.ts
- core/dal-decorator/src/enum/CompressionType.ts
- core/dal-decorator/src/enum/IndexStoreType.ts
- core/dal-decorator/src/enum/IndexType.ts
- core/dal-decorator/src/enum/InsertMethod.ts
- core/dal-decorator/src/enum/RowFormat.ts
- core/dal-decorator/src/type/MySql.ts
- core/dal-runtime/test/fixtures/modules/dal/dal/extension/FooExtension.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/MultiPrimaryKeyExtension.ts
- core/tegg/dal.ts
- plugin/dal/typings/index.d.ts
Additional comments: 98
core/dal-decorator/README.md (1)
- 1-5: The README directs users to another document for usage instructions. While this is clear, consider adding a brief overview or example usage of
@eggjs/dal-decoratorhere for better standalone comprehension.core/dal-runtime/README.md (1)
- 1-5: The README directs users to another document for usage instructions. While this is clear, consider adding a brief overview or example usage of
@eggjs/dal-runtimehere for better standalone comprehension.core/dal-runtime/index.ts (1)
- 1-6: The exports in
index.tsare correctly defined and cover key components of thedal-runtimepackage. LGTM!plugin/dal/test/fixtures/apps/dal-app/config/config.default.js (1)
- 1-13: The default configuration settings in
config.default.jsare correctly defined and follow standard practices for an Egg.js application. LGTM!plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/extension/FooExtension.ts (1)
- 1-8: The SQL mapping in
FooExtension.tsis correctly defined and uses parameterized queries, which is a good practice for preventing SQL injection. LGTM!plugin/dal/test/fixtures/apps/dal-app/config/plugin.js (1)
- 1-16: The plugin configurations in
plugin.jsare correctly defined and follow standard practices for enabling Egg.js plugins. LGTM!core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/FooDAO.ts (1)
- 1-16: Skipping this file as it's a duplicate of the previously reviewed
FooDAO.ts.core/dal-decorator/test/fixtures/modules/dal/Foo.ts (1)
- 1-22: The usage of DAL decorators (
@Table,@Index, and@Column) in theFooclass is correctly implemented and follows best practices for defining database entities in the context of the Egg.js framework.core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/MultiPrimaryKeyDAO.ts (1)
- 8-8: The
@datetag in the class documentation is left empty. Consider completing this with the relevant date or removing the tag if it's not necessary.core/metadata/test/ModuleGraph.test.ts (1)
- 10-10: The addition of an extra argument
'foo'to theModuleGraphconstructor call should be verified against theModuleGraphclass definition to ensure it's correctly handled and expected.core/dal-decorator/src/decorator/DataSourceQualifier.ts (1)
- 1-11: The
DataSourceQualifierdecorator function is correctly implemented, following best practices for dependency injection in the Egg.js framework. It properly uses theQualifierUtilto associate a data source with a property.core/dal-decorator/src/type/SqlMap.ts (1)
- 1-23: The
SqlTypeenum andSqlMapinterfaces are correctly defined, providing a comprehensive framework for managing different types of SQL operations within the Egg.js framework. This design facilitates clear and structured handling of SQL queries and blocks.plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/FooDAO.ts (2)
- 12-15: The class definition and its annotation with
@SingletonProtoare correctly implemented.- 16-20: The
findByNamemethod implementation looks good and follows best practices for executing SQL queries through the data source.core/dal-decorator/src/type/DateSource.ts (1)
- 1-15: The interfaces
PaginateDataandDataSourceare well-defined, covering essential operations for executing SQL queries and managing pagination. Good job on ensuring comprehensive method signatures.core/dal-decorator/src/util/IndexInfoUtil.ts (1)
- 6-15: The utility class
IndexInfoUtiland its methodsaddIndexandgetIndexListare correctly implemented, making good use ofMetadataUtilfor metadata operations. This is a solid approach for managing index information.core/dal-runtime/src/templates/dao.njk (1)
- 1-18: The Nunjucks template for generating DAO classes is well-structured, making good use of placeholders and conditional rendering. The generated class structure, including documentation and extension of a base DAO class, follows best practices.
core/dal-decorator/src/decorator/Index.ts (1)
- 6-21: The
IndexParamsinterface and theIndexdecorator are well-defined and implemented. The interface covers various aspects of index configuration comprehensively, and the decorator correctly utilizesIndexInfoUtilfor managing index information.core/dal-runtime/test/fixtures/modules/generate_codes/MultiPrimaryKey.ts (1)
- 7-38: The class
MultiPrimaryKeyand its use of decorators for defining table and column properties, including primary keys and a unique key, are correctly implemented. This approach enhances readability and maintainability of database schema definitions.plugin/dal/lib/SqlMapManager.ts (1)
- 3-28: The singleton class
SqlMapManageris well-designed, correctly implementing the singleton pattern and providing clear methods for managing SQL maps. This design ensures efficient and centralized management of SQL maps across the application.plugin/dal/lib/TableModelManager.ts (1)
- 3-28: The singleton class
TableModelManageris well-designed, correctly implementing the singleton pattern and providing clear methods for managing table models. This design ensures efficient and centralized management of table models across the application.core/dal-decorator/index.ts (1)
- 1-24: Ensure all exported entities are used within the DAL feature and are documented appropriately. This will aid developers in understanding and utilizing the DAL capabilities effectively.
core/dal-decorator/src/type/ColumnTsType.ts (1)
- 1-42: Verify the accuracy and completeness of the
ColumnTsTypedefinitions against the SQL column types supported by the target database(s). Consider adding or refining types as necessary to ensure comprehensive coverage.core/core-decorator/src/model/EggMultiInstancePrototypeInfo.ts (1)
- 12-12: The addition of the
moduleNameproperty toMultiInstancePrototypeGetObjectsContextis clear and enhances the context for object retrieval in multi-instance scenarios. Ensure that any relevant documentation is updated to reflect this new property.plugin/dal/lib/DalModuleLoadUnitHook.ts (1)
- 10-12: The constructor implementation correctly handles the injection of module configurations. This is a good practice for dependency management.
core/dal-decorator/src/util/ColumnInfoUtil.ts (1)
- 9-27: The utility methods for managing column information and types are correctly implemented and follow best practices for metadata management in TypeScript.
plugin/dal/app.ts (2)
- 18-21: The
configWillLoadmethod correctly sets up and registers DAL-related lifecycle hooks, integrating DAL functionality into the application lifecycle effectively.- 24-34: The
beforeClosemethod correctly handles the teardown of DAL-related lifecycle hooks and managers, ensuring resources are gracefully released during application shutdown.core/dal-decorator/src/type/Spatial.ts (1)
- 3-51: The spatial data type definitions and utility methods are correctly implemented, providing a clear and effective way to work with spatial data within the DAL layer.
core/dal-decorator/src/decorator/Table.ts (1)
- 31-43: The
TableParamsinterface andTabledecorator function are correctly implemented, providing a comprehensive and effective way to define table metadata within the DAL layer.core/dal-decorator/test/index.test.ts (1)
- 6-58: The test cases for the DAL decorator functionality are correctly implemented, covering the basic functionality and verifying the expected outcomes effectively.
plugin/dal/lib/MysqlDataSourceManager.ts (4)
- 15-21: The
getmethod for retrieving data sources is correctly implemented, using a two-level map for efficient indexing and retrieval.- 23-42: The
createDataSourcemethod correctly implements logic for reusing data source configurations when possible, addressing previous concerns about connection reuse across modules. This is an important improvement for resource management.- 44-46: The
clearmethod correctly handles the cleanup of data source indices and resources, ensuring efficient resource management.- 48-60: The
createDataSourceKeyutility method is correctly implemented, providing a reliable way to generate unique keys for data source configurations based on their properties.core/dal-decorator/src/model/IndexModel.ts (1)
- 42-45: The method
buildIndexNamegenerates index names based on keys and type. Ensure that the generated names adhere to any database-specific limitations on index name lengths or character sets.core/dal-runtime/test/fixtures/modules/dal/dal/structure/Foo.sql (1)
- 1-44: The SQL script correctly defines a table with a wide range of column types and two indexes. Ensure that all column types and index configurations are relevant to the tests they are intended to support. Additionally, verify that the unique and fulltext indexes (
uk_name_col1andidx_col1) are adequately tested in the corresponding test cases.core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1)
- 23-23: Including the
moduleNamein thegetMultiInstancePropertycall is a significant change that aligns with supporting multi-instance objects in Egg.js. Ensure that all downstream usages of this method and themultiInstanceProtoInfoobject properly handle themoduleNameproperty.core/dal-runtime/src/NunjucksUtil.ts (2)
- 11-16: The custom
escapeSQLfunction provides SQL value escaping. Ensure that this escaping mechanism is thoroughly tested, especially for edge cases, to prevent SQL injection vulnerabilities.- 34-64: The
_compilefunction includes a significant modification to the Nunjucks template compilation process. Ensure that this custom compilation process is compatible with all Nunjucks features used in your templates and does not introduce unexpected behavior.plugin/tegg/lib/AppLoadUnit.ts (1)
- 45-45: The addition of the
moduleNameproperty to thegetInitTypemethod call seems appropriate for enhanced module identification or handling. Ensure that this new property is consistently used and properly handled across all relevant parts of the application.core/dal-runtime/test/SqlGenerator.test.ts (1)
- 6-56: The test case for the
SqlGeneratorclass is comprehensive, covering a wide range of column types and SQL features. However, consider a more flexible approach to validating the generated SQL, such as parsing and comparing SQL structures, to make the test less fragile to changes in the SQL generation logic or formatting.core/dal-runtime/src/TableSqlMap.ts (1)
- 13-107: The
TableSqlMapclass is well-structured and provides a clear interface for managing SQL templates and generating SQL strings using Nunjucks templates. Ensure that SQL injection vulnerabilities are properly addressed when using template-based SQL generation. Consider adding comments or documentation regarding SQL injection prevention measures.core/dal-runtime/src/TemplateUtil.ts (1)
- 14-100: The utility functions in
TemplateUtil.tsare well-implemented and serve their purpose for converting various data types to SQL-compatible strings. Ensure that user-supplied data is properly sanitized or escaped to prevent SQL injection vulnerabilities. Consider adding comments or documentation regarding the handling of user-supplied data.core/dal-runtime/src/DataSource.ts (1)
- 14-92: The
DataSourceclass is well-structured and provides a clear interface for executing SQL queries and managing database interactions. Ensure that SQL queries are properly sanitized to prevent SQL injection vulnerabilities. Consider adding comments or documentation regarding SQL query sanitization and SQL injection prevention measures.core/dal-runtime/src/NunjucksConverter.ts (1)
- 1-111: The conversion methods in
NunjucksConverter.tsare a proactive approach to preventing SQL injection vulnerabilities in template-generated SQL. Ensure that all potential injection vectors are covered and that the conversion logic does not introduce new vulnerabilities. Consider thorough testing and review to validate the security of the conversion logic.plugin/dal/test/dal.test.ts (1)
- 7-118: The test case in
dal.test.tsis comprehensive, covering CRUD operations on theFoomodel. Consider enhancing it with more assertions to validate the state of the database after each operation, ensuring the test case fully verifies the expected behavior.core/core-decorator/test/decorators.test.ts (1)
- 121-121: The addition of the
moduleNameproperty to thegetMultiInstancePropertyfunction call in the test case seems appropriate for enhanced module identification or handling. Ensure that this new property is consistently used and properly handled across all relevant parts of the test cases.plugin/dal/lib/DataSource.ts (1)
- 71-111: The
DataSourceDelegateclass methodsexecute,executeScalar,executeRaw,executeRawScalar,paginate, andcountdirectly delegate calls to thedataSourcemember without adding any additional logic. This delegation could be simplified or documented to clarify the necessity of these wrapper methods, ensuring maintainability and understanding of the codebase.core/dal-runtime/src/templates/base_dao.njk (1)
- 1-150: The Nunjucks template for generating base DAO classes uses several macros to generate repetitive logic. While this approach is generally good for code generation, ensure that the generated code follows best practices, such as avoiding unnecessary comments (
// empty-line) and ensuring type safety and clarity in the generated methods. Additionally, consider adding more comments within the template to explain the purpose of each macro and section for better maintainability.core/dal-decorator/src/model/TableModel.ts (1)
- 15-148: The
TableModelclass provides a structured way to define and work with database tables. Ensure that all properties and methods are well-documented, especially those that might not be immediately clear to other developers, such asautoExtendSize,avgRowLength, andkeyBlockSize. Adding detailed comments can improve the maintainability and usability of this class.plugin/dal/test/fixtures/apps/dal-app/modules/dal/Foo.ts (3)
- 13-18: The
@Tableannotation is well-defined, including thoughtful considerations for character encoding withutf8mb4. This setup supports a wide range of Unicode characters, which is a good practice.- 19-29: The
@Indexannotations are appropriately used, with a UNIQUE index on 'name' and 'col1', and a FULLTEXT index on 'col1'. Including comments for indexes is a good practice for maintainability.- 31-291: The
@Columnannotations are comprehensive and well-defined, covering a wide range of data types and specifying properties such as length, unsigned, zeroFill, characterSet, and collate where applicable. This thorough approach enhances the clarity and maintainability of the database schema.core/dal-runtime/src/SqlUtil.ts (1)
- 19-185: The
minifymethod in theSqlUtilclass uses a state machine approach to effectively parse and minify SQL strings, handling various contexts such as comments, block comments with hints, and quoted strings. To improve readability and maintainability, consider adding comments explaining the purpose of each state and transitions between them.core/dal-runtime/test/fixtures/modules/dal/Foo.ts (2)
- 13-31: The usage of
@Tableand@Indexdecorators along with the@Columndecorator for defining database table structure, columns, and indexes is correctly implemented.- 49-317: The class properties are well-defined with appropriate usage of
@Columndecorator to specify column types and attributes, covering a wide range of SQL data types.core/dal-runtime/test/fixtures/modules/generate_codes/Foo.ts (1)
- 13-317: The usage of
@Table,@Index, and@Columndecorators for defining database table structure, columns, and indexes, as well as the definition of class properties, are correctly implemented, identical to the previous file reviewed.core/dal-decorator/src/decorator/Column.ts (2)
- 262-272: The implementation of the
Columndecorator is correctly handled, ensuring proper registration of column information on the target class and asserting the property key is a string.- 7-260: The structured definition of interfaces for various SQL column types ensures type safety and clarity, facilitating easy extension and maintenance. The separation into base interfaces for numeric types demonstrates good practice for code reuse.
core/metadata/src/impl/ModuleLoadUnit.ts (2)
- 35-42: The addition of the
moduleNameparameter to theProtoNodeconstructor and its usage in the prototype initialization process is correctly implemented, enhancing module identification.- 69-75: The handling of the
moduleNameparameter in theModuleGraphconstructor and its consistent usage throughout the class, including when creatingProtoNodeinstances, is correctly implemented, facilitating precise module management.Also applies to: 109-112
core/metadata/src/model/AppGraph.ts (1)
- 58-64: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [61-70]
The usage of
moduleNamein theClazzMapclass to enhance the handling of multi-instance properties and access levels based on the module name is correctly implemented, improving dependency resolution and access control.Also applies to: 89-89
core/dal-runtime/src/BaseSqlMap.ts (6)
- 6-10: The
GenerateSqlMapinterface is well-defined and adheres to TypeScript best practices. It clearly specifies the structure for SQL map generation, which is crucial for the DAL's operation.- 21-24: The method
generateAllColumnscleverly uses Nunjucks templating to dynamically generate SQL columns, including a conditional for counting. This approach is flexible and maintainable.- 27-53: The
generateFindByPrimarymethod correctly handles the case where a table might not have a primary key and logs a warning. It also generates SQL for selecting by primary key, supporting both single and composite keys.- 190-245: The
generateUpdatemethod correctly generates an SQL update statement, including handling of thegmtModifiedfield to automatically update the timestamp. This is a good practice for maintaining data integrity.- 248-262: The
generateDeletemethod properly generates an SQL delete statement based on the primary key. It also correctly handles the case where a table might not have a primary key by logging a warning.- 265-321: The
loadmethod aggregates all generated SQL maps into a single record, making it easy to access and use these maps throughout the application. This approach enhances modularity and maintainability.plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/base/BaseFooDAO.ts (3)
- 26-230: The
insertmethod inBaseFooDAOis well-structured and handles the insertion of various data types correctly. It uses a pattern of checking for undefined values before adding them to the data object, which is a good practice for data integrity.- 233-442: The
updatemethod follows a similar pattern to theinsertmethod, ensuring that only defined fields are included in the update operation. This approach minimizes the risk of unintentionally overwriting data.- 457-493: The find methods (
findByCol1,findOneByCol1,findByUkNameCol1,findOneByUkNameCol1,findById,findByPrimary) are correctly implemented, providing various ways to query theFooentity. It's good practice to have both list and scalar return types for flexibility.core/dal-runtime/test/fixtures/modules/dal/dal/dao/base/BaseFooDAO.ts (3)
- 24-228: The
insertmethod in thisBaseFooDAOclass is consistent with the implementation reviewed in the plugin directory. It correctly handles the insertion of various data types and checks for undefined values.- 231-440: The
updatemethod also mirrors the implementation in the plugin directory, ensuring that only defined fields are included in the update operation. This consistency across test fixtures and actual implementation is crucial for reliable testing.- 455-491: The find methods in this class are consistent with those in the plugin directory, providing various ways to query the
Fooentity. This consistency is important for ensuring that test fixtures accurately reflect the actual implementation.core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseFooDAO.ts (5)
- 6-6: Consider using a utility type from a standard library or TypeScript itself for
Optional, if available, to reduce code redundancy and improve maintainability.- 10-10: The comment about the file being auto-generated is crucial. Ensure that there's a mechanism in place to regenerate this file when the underlying model or requirements change, to keep the DAO layer up-to-date.
- 24-228: The
insertmethod constructs a data object dynamically based on the properties ofraw. While this approach is flexible, it's essential to ensure that theexecuteRawScalarmethod properly sanitizes inputs to prevent SQL injection vulnerabilities.- 231-440: Similar to the
insertmethod, theupdatemethod dynamically constructs an update payload. Ensure that theexecuteRawScalarmethod used for executing the update operation is secure against SQL injection by properly escaping or binding parameters.- 455-491: The methods for finding records (
findByCol1,findOneByCol1,findByUkNameCol1,findOneByUkNameCol1,findById,findByPrimary) demonstrate a good use of abstraction for common database operations. Ensure that these methods are adequately tested, especially for edge cases and error handling.core/dal-runtime/src/SqlGenerator.ts (3)
- 16-71: The
generateColumnmethod constructs SQL column definitions dynamically. Ensure that inputs, such as column names and comments, are properly sanitized to prevent SQL injection vulnerabilities.- 73-253: The
generateColumnTypemethod handles various column types and their specific attributes. Ensure that the method correctly handles all edge cases, especially for types with optional parameters like length, precision, and character set.- 255-301: The
generateIndexmethod constructs SQL index definitions. Ensure that index names and comments are properly sanitized to prevent SQL injection vulnerabilities.plugin/dal/README.md (15)
- 3-3: The introduction provides a brief overview of the plugin's purpose but could benefit from a more detailed explanation of the DAL concept and how this plugin enhances data access layer development in Egg.js applications.
- 5-14: The installation instructions are clear and straightforward. However, it would be helpful to include a brief description of what each package does for newcomers to understand why they need to install these specific packages.
- 16-25: The preparation step for enabling experimental decorators in TypeScript is crucial for using this plugin. It's well-explained, but linking to TypeScript documentation for more context on "experimentalDecorators" could enhance understanding.
- 27-40: Configuration instructions are concise. It might be beneficial to explain the purpose of enabling these plugins in the
config/plugin.jsfile, especially for developers new to the Egg.js ecosystem.- 44-57: The section on configuring data sources through
module.ymlis clear. Including an example of how to reference these data sources in code (beyond the@Tableannotation) would provide a more complete picture.- 59-90: The example usage of
@Table,@Index, and@Columndecorators is informative. It would be beneficial to include a brief explanation of each decorator's purpose and how they interact with each other to define a table structure.- 92-118: The detailed parameters for the
@Tabledecorator are well-documented. A small example demonstrating the use of some of these parameters (e.g.,autoIncrement,compression) in context would make this section more practical.- 120-137: Similarly, for the
@Indexdecorator, the documentation is thorough. Including an example showcasing the use of optional parameters likenameandstoreTypecould provide clearer guidance on their usage.- 139-158: The
@Columndecorator's parameters are well-explained. An example illustrating the use of parameters likecanNull,autoIncrement, oruniqueKeywould enhance understanding.- 160-207: The enumeration of supported column types is comprehensive. A brief note on choosing the appropriate column type based on the data being stored could help developers make informed decisions.
- 209-240: The section on using
ColumnTsTypefor uncertain type mappings is helpful. Clarifying when it's best to useColumnTsTypeover directColumnTypeenums could guide developers in best practices.- 492-511: The directory structure generated by
egg-bin dal genis clearly outlined. It would be useful to include a brief description of typical use cases or scenarios where developers might need to modify or extend the generated files.- 513-528: The DAO injection example is straightforward. Expanding on how to handle more complex queries or transactions within the repository class could provide valuable insights for advanced use cases.
- 530-562: The custom SQL definition and usage instructions are clear. A note on best practices for managing complex SQL queries within extensions, such as organizing or modularizing SQL statements, could be beneficial.
- 564-581: The DataSource interface documentation is concise. Including examples of common use cases for each method (e.g.,
paginate,count) would make this section more actionable.
core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/FooExtension.ts
Outdated
Show resolved
Hide resolved
core/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/MultiPrimaryKey.sql
Show resolved
Hide resolved
Comment on lines
+21
to
+51
| constructor(params: { | ||
| columnName: string; | ||
| propertyName: string; | ||
| type: ColumnTypeParams; | ||
| canNull: boolean; | ||
| default?: string; | ||
| comment?: string; | ||
| visible?: boolean; | ||
| autoIncrement?: boolean; | ||
| uniqueKey?: boolean; | ||
| primaryKey?: boolean; | ||
| collate?: string; | ||
| columnFormat?: ColumnFormat; | ||
| engineAttribute?: string; | ||
| secondaryEngineAttribute?: string; | ||
| }) { | ||
| this.columnName = params.columnName; | ||
| this.propertyName = params.propertyName; | ||
| this.type = params.type; | ||
| this.canNull = params.canNull; | ||
| this.default = params.default; | ||
| this.comment = params.comment; | ||
| this.visible = params.visible; | ||
| this.autoIncrement = params.autoIncrement; | ||
| this.uniqueKey = params.uniqueKey; | ||
| this.primaryKey = params.primaryKey; | ||
| this.collate = params.collate; | ||
| this.columnFormat = params.columnFormat; | ||
| this.engineAttribute = params.engineAttribute; | ||
| this.secondaryEngineAttribute = params.secondaryEngineAttribute; | ||
| } |
There was a problem hiding this comment.
The constructor of ColumnModel directly assigns values from the params object to its properties. Consider validating the inputs, especially for critical properties like columnName, propertyName, and type, to ensure they meet expected formats or constraints.
Comment on lines
+53
to
+76
| static build(property: string, type: ColumnTypeParams, params?: ColumnParams) { | ||
| const columnName = params?.name ?? snakecase(property); | ||
| // TODO can null default should be false | ||
| let canNull = params?.canNull ?? true; | ||
| if (params?.primaryKey) { | ||
| canNull = false; | ||
| } | ||
| return new ColumnModel({ | ||
| columnName, | ||
| propertyName: property, | ||
| type, | ||
| canNull, | ||
| default: params?.default, | ||
| comment: params?.comment, | ||
| visible: params?.visible, | ||
| autoIncrement: params?.autoIncrement, | ||
| uniqueKey: params?.uniqueKey, | ||
| primaryKey: params?.primaryKey, | ||
| collate: params?.collate, | ||
| columnFormat: params?.columnFormat, | ||
| engineAttribute: params?.engineAttribute, | ||
| secondaryEngineAttribute: params?.secondaryEngineAttribute, | ||
| }); | ||
| } |
There was a problem hiding this comment.
In static build, the handling of canNull for primary keys is correct (line 57-58). However, consider adding a comment explaining why primary keys cannot be null, for clarity to future maintainers.
+ // Primary keys cannot be null by definition
if (params?.primaryKey) {
canNull = false;
}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
| static build(property: string, type: ColumnTypeParams, params?: ColumnParams) { | |
| const columnName = params?.name ?? snakecase(property); | |
| // TODO can null default should be false | |
| let canNull = params?.canNull ?? true; | |
| if (params?.primaryKey) { | |
| canNull = false; | |
| } | |
| return new ColumnModel({ | |
| columnName, | |
| propertyName: property, | |
| type, | |
| canNull, | |
| default: params?.default, | |
| comment: params?.comment, | |
| visible: params?.visible, | |
| autoIncrement: params?.autoIncrement, | |
| uniqueKey: params?.uniqueKey, | |
| primaryKey: params?.primaryKey, | |
| collate: params?.collate, | |
| columnFormat: params?.columnFormat, | |
| engineAttribute: params?.engineAttribute, | |
| secondaryEngineAttribute: params?.secondaryEngineAttribute, | |
| }); | |
| } | |
| static build(property: string, type: ColumnTypeParams, params?: ColumnParams) { | |
| const columnName = params?.name ?? snakecase(property); | |
| // TODO can null default should be false | |
| let canNull = params?.canNull ?? true; | |
| // Primary keys cannot be null by definition | |
| if (params?.primaryKey) { | |
| canNull = false; | |
| } | |
| return new ColumnModel({ | |
| columnName, | |
| propertyName: property, | |
| type, | |
| canNull, | |
| default: params?.default, | |
| comment: params?.comment, | |
| visible: params?.visible, | |
| autoIncrement: params?.autoIncrement, | |
| uniqueKey: params?.uniqueKey, | |
| primaryKey: params?.primaryKey, | |
| collate: params?.collate, | |
| columnFormat: params?.columnFormat, | |
| engineAttribute: params?.engineAttribute, | |
| secondaryEngineAttribute: params?.secondaryEngineAttribute, | |
| }); | |
| } |
core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseMultiPrimaryKeyDAO.ts
Show resolved
Hide resolved
Comment on lines
+81
to
+86
| public async del(primary: { | ||
| id1: ColumnTsType['INT'], | ||
| id2: ColumnTsType['INT'] | ||
| }): Promise<DeleteResult> { | ||
| return this.dataSource.executeRawScalar('delete', primary); | ||
| } |
There was a problem hiding this comment.
The del method is a duplicate of the delete method. If this is intentional (e.g., for aliasing purposes), consider adding a comment to clarify. Otherwise, remove the duplicate to avoid confusion and potential maintenance issues.
- public async del(primary: {
- id1: ColumnTsType['INT'],
- id2: ColumnTsType['INT']
- }): Promise<DeleteResult> {
- return this.dataSource.executeRawScalar('delete', primary);
- }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
| public async del(primary: { | |
| id1: ColumnTsType['INT'], | |
| id2: ColumnTsType['INT'] | |
| }): Promise<DeleteResult> { | |
| return this.dataSource.executeRawScalar('delete', primary); | |
| } |
Comment on lines
+19
to
+27
| function _replaceCodeWithSQLFeature(source) { | ||
| const funcs = [ | ||
| 'convertNormalVariableCode', // 普通变量 | ||
| 'convertTernaryCode', // 三目运算 | ||
| 'convertNestedObjectCode', // 对象中的变量，如 `user.id` | ||
| 'convertValueInsideFor', // for 中的值需要转义 | ||
| ]; | ||
|
|
||
| return funcs.reduce((source, func) => NunjucksConverter[func](source), source); | ||
| } |
There was a problem hiding this comment.
The _replaceCodeWithSQLFeature function modifies Nunjucks code to support SQL escaping. This approach is innovative but could introduce maintenance challenges. Consider documenting the specific Nunjucks patterns this function targets and any limitations or edge cases.
+ /**
+ * Modifies Nunjucks code to support SQL escaping by targeting specific patterns.
+ * Targets:
+ * - Normal variables
+ * - Ternary operations
+ * - Nested object variables
+ * - Values inside loops
+ * Note: Document any limitations or edge cases.
+ */
function _replaceCodeWithSQLFeature(source) {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
| function _replaceCodeWithSQLFeature(source) { | |
| const funcs = [ | |
| 'convertNormalVariableCode', // 普通变量 | |
| 'convertTernaryCode', // 三目运算 | |
| 'convertNestedObjectCode', // 对象中的变量，如 `user.id` | |
| 'convertValueInsideFor', // for 中的值需要转义 | |
| ]; | |
| return funcs.reduce((source, func) => NunjucksConverter[func](source), source); | |
| } | |
| /** | |
| * Modifies Nunjucks code to support SQL escaping by targeting specific patterns. | |
| * Targets: | |
| * - Normal variables | |
| * - Ternary operations | |
| * - Nested object variables | |
| * - Values inside loops | |
| * Note: Document any limitations or edge cases. | |
| */ | |
| function _replaceCodeWithSQLFeature(source) { | |
| const funcs = [ | |
| 'convertNormalVariableCode', // 普通变量 | |
| 'convertTernaryCode', // 三目运算 | |
| 'convertNestedObjectCode', // 对象中的变量，如 `user.id` | |
| 'convertValueInsideFor', // for 中的值需要转义 | |
| ]; | |
| return funcs.reduce((source, func) => NunjucksConverter[func](source), source); | |
| } |
killagu
force-pushed
the
feat/dal
branch
2 times, most recently
from
8f0ddf0 to
367c2cf
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (21)
core/dal-decorator/package.jsonis excluded by:!**/*.jsoncore/dal-decorator/test/fixtures/modules/dal/package.jsonis excluded by:!**/*.jsoncore/dal-decorator/tsconfig.jsonis excluded by:!**/*.jsoncore/dal-decorator/tsconfig.pub.jsonis excluded by:!**/*.jsoncore/dal-runtime/package.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/dal/dal/structure/Foo.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/dal/package.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/Foo.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/MultiPrimaryKey.jsonis excluded by:!**/*.jsoncore/dal-runtime/test/fixtures/modules/generate_codes/package.jsonis excluded by:!**/*.jsoncore/dal-runtime/tsconfig.jsonis excluded by:!**/*.jsoncore/dal-runtime/tsconfig.pub.jsonis excluded by:!**/*.jsoncore/tegg/package.jsonis excluded by:!**/*.jsonplugin/dal/package.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/config/module.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/structure/Foo.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/modules/dal/module.ymlis excluded by:!**/*.ymlplugin/dal/test/fixtures/apps/dal-app/modules/dal/package.jsonis excluded by:!**/*.jsonplugin/dal/test/fixtures/apps/dal-app/package.jsonis excluded by:!**/*.jsonplugin/dal/tsconfig.jsonis excluded by:!**/*.jsonplugin/dal/tsconfig.pub.jsonis excluded by:!**/*.json
Files selected for processing (89)
- .github/workflows/nodejs.yml (3 hunks)
- .gitignore (1 hunks)
- core/core-decorator/src/model/EggMultiInstancePrototypeInfo.ts (1 hunks)
- core/core-decorator/test/decorators.test.ts (1 hunks)
- core/dal-decorator/README.md (1 hunks)
- core/dal-decorator/index.ts (1 hunks)
- core/dal-decorator/src/decorator/Column.ts (1 hunks)
- core/dal-decorator/src/decorator/DataSourceQualifier.ts (1 hunks)
- core/dal-decorator/src/decorator/Index.ts (1 hunks)
- core/dal-decorator/src/decorator/Table.ts (1 hunks)
- core/dal-decorator/src/enum/ColumnFormat.ts (1 hunks)
- core/dal-decorator/src/enum/ColumnType.ts (1 hunks)
- core/dal-decorator/src/enum/CompressionType.ts (1 hunks)
- core/dal-decorator/src/enum/IndexStoreType.ts (1 hunks)
- core/dal-decorator/src/enum/IndexType.ts (1 hunks)
- core/dal-decorator/src/enum/InsertMethod.ts (1 hunks)
- core/dal-decorator/src/enum/RowFormat.ts (1 hunks)
- core/dal-decorator/src/model/ColumnModel.ts (1 hunks)
- core/dal-decorator/src/model/IndexModel.ts (1 hunks)
- core/dal-decorator/src/model/TableModel.ts (1 hunks)
- core/dal-decorator/src/type/ColumnTsType.ts (1 hunks)
- core/dal-decorator/src/type/DateSource.ts (1 hunks)
- core/dal-decorator/src/type/MySql.ts (1 hunks)
- core/dal-decorator/src/type/Spatial.ts (1 hunks)
- core/dal-decorator/src/type/SqlMap.ts (1 hunks)
- core/dal-decorator/src/util/ColumnInfoUtil.ts (1 hunks)
- core/dal-decorator/src/util/IndexInfoUtil.ts (1 hunks)
- core/dal-decorator/src/util/TableInfoUtil.ts (1 hunks)
- core/dal-decorator/test/fixtures/modules/dal/Foo.ts (1 hunks)
- core/dal-decorator/test/index.test.ts (1 hunks)
- core/dal-runtime/README.md (1 hunks)
- core/dal-runtime/index.ts (1 hunks)
- core/dal-runtime/src/BaseSqlMap.ts (1 hunks)
- core/dal-runtime/src/CodeGenerator.ts (1 hunks)
- core/dal-runtime/src/DataSource.ts (1 hunks)
- core/dal-runtime/src/MySqlDataSource.ts (1 hunks)
- core/dal-runtime/src/NunjucksConverter.ts (1 hunks)
- core/dal-runtime/src/NunjucksUtil.ts (1 hunks)
- core/dal-runtime/src/SqlGenerator.ts (1 hunks)
- core/dal-runtime/src/SqlMapLoader.ts (1 hunks)
- core/dal-runtime/src/SqlUtil.ts (1 hunks)
- core/dal-runtime/src/TableModelInstanceBuilder.ts (1 hunks)
- core/dal-runtime/src/TableSqlMap.ts (1 hunks)
- core/dal-runtime/src/TemplateUtil.ts (1 hunks)
- core/dal-runtime/src/templates/base_dao.njk (1 hunks)
- core/dal-runtime/src/templates/dao.njk (1 hunks)
- core/dal-runtime/src/templates/extension.njk (1 hunks)
- core/dal-runtime/test/CodeGenerator.test.ts (1 hunks)
- core/dal-runtime/test/DAO.test.ts (1 hunks)
- core/dal-runtime/test/DataSource.test.ts (1 hunks)
- core/dal-runtime/test/SqlGenerator.test.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/Foo.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/dao/FooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/dao/base/BaseFooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/extension/FooExtension.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/dal/dal/structure/Foo.sql (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/Foo.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/MultiPrimaryKey.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/FooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/MultiPrimaryKeyDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseFooDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseMultiPrimaryKeyDAO.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/FooExtension.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/MultiPrimaryKeyExtension.ts (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/Foo.sql (1 hunks)
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/MultiPrimaryKey.sql (1 hunks)
- core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (5 hunks)
- core/metadata/src/model/AppGraph.ts (4 hunks)
- core/metadata/test/ModuleGraph.test.ts (1 hunks)
- core/tegg/dal.ts (1 hunks)
- plugin/dal/README.md (1 hunks)
- plugin/dal/app.ts (1 hunks)
- plugin/dal/lib/DalModuleLoadUnitHook.ts (1 hunks)
- plugin/dal/lib/DalTableEggPrototypeHook.ts (1 hunks)
- plugin/dal/lib/DataSource.ts (1 hunks)
- plugin/dal/lib/MysqlDataSourceManager.ts (1 hunks)
- plugin/dal/lib/SqlMapManager.ts (1 hunks)
- plugin/dal/lib/TableModelManager.ts (1 hunks)
- plugin/dal/test/dal.test.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/config/config.default.js (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/config/plugin.js (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/Foo.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/FooDAO.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/base/BaseFooDAO.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/extension/FooExtension.ts (1 hunks)
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/structure/Foo.sql (1 hunks)
- plugin/dal/typings/index.d.ts (1 hunks)
- plugin/tegg/lib/AppLoadUnit.ts (1 hunks)
Files skipped from review as they are similar to previous changes (85)
- .gitignore
- core/core-decorator/src/model/EggMultiInstancePrototypeInfo.ts
- core/core-decorator/test/decorators.test.ts
- core/dal-decorator/index.ts
- core/dal-decorator/src/decorator/Column.ts
- core/dal-decorator/src/decorator/DataSourceQualifier.ts
- core/dal-decorator/src/decorator/Index.ts
- core/dal-decorator/src/decorator/Table.ts
- core/dal-decorator/src/enum/ColumnFormat.ts
- core/dal-decorator/src/enum/ColumnType.ts
- core/dal-decorator/src/enum/CompressionType.ts
- core/dal-decorator/src/enum/IndexStoreType.ts
- core/dal-decorator/src/enum/IndexType.ts
- core/dal-decorator/src/enum/InsertMethod.ts
- core/dal-decorator/src/enum/RowFormat.ts
- core/dal-decorator/src/model/ColumnModel.ts
- core/dal-decorator/src/model/IndexModel.ts
- core/dal-decorator/src/type/ColumnTsType.ts
- core/dal-decorator/src/type/DateSource.ts
- core/dal-decorator/src/type/MySql.ts
- core/dal-decorator/src/type/Spatial.ts
- core/dal-decorator/src/type/SqlMap.ts
- core/dal-decorator/src/util/ColumnInfoUtil.ts
- core/dal-decorator/src/util/IndexInfoUtil.ts
- core/dal-decorator/src/util/TableInfoUtil.ts
- core/dal-decorator/test/fixtures/modules/dal/Foo.ts
- core/dal-decorator/test/index.test.ts
- core/dal-runtime/index.ts
- core/dal-runtime/src/BaseSqlMap.ts
- core/dal-runtime/src/CodeGenerator.ts
- core/dal-runtime/src/DataSource.ts
- core/dal-runtime/src/MySqlDataSource.ts
- core/dal-runtime/src/NunjucksConverter.ts
- core/dal-runtime/src/NunjucksUtil.ts
- core/dal-runtime/src/SqlGenerator.ts
- core/dal-runtime/src/SqlMapLoader.ts
- core/dal-runtime/src/SqlUtil.ts
- core/dal-runtime/src/TableModelInstanceBuilder.ts
- core/dal-runtime/src/TableSqlMap.ts
- core/dal-runtime/src/TemplateUtil.ts
- core/dal-runtime/src/templates/base_dao.njk
- core/dal-runtime/src/templates/dao.njk
- core/dal-runtime/src/templates/extension.njk
- core/dal-runtime/test/CodeGenerator.test.ts
- core/dal-runtime/test/DAO.test.ts
- core/dal-runtime/test/DataSource.test.ts
- core/dal-runtime/test/SqlGenerator.test.ts
- core/dal-runtime/test/fixtures/modules/dal/Foo.ts
- core/dal-runtime/test/fixtures/modules/dal/dal/dao/FooDAO.ts
- core/dal-runtime/test/fixtures/modules/dal/dal/dao/base/BaseFooDAO.ts
- core/dal-runtime/test/fixtures/modules/dal/dal/extension/FooExtension.ts
- core/dal-runtime/test/fixtures/modules/dal/dal/structure/Foo.sql
- core/dal-runtime/test/fixtures/modules/generate_codes/Foo.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/MultiPrimaryKey.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/FooDAO.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/MultiPrimaryKeyDAO.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseFooDAO.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/dao/base/BaseMultiPrimaryKeyDAO.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/FooExtension.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/extension/MultiPrimaryKeyExtension.ts
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/Foo.sql
- core/dal-runtime/test/fixtures/modules/generate_codes/dal/structure/MultiPrimaryKey.sql
- core/metadata/src/factory/EggPrototypeCreatorFactory.ts
- core/metadata/src/impl/ModuleLoadUnit.ts
- core/metadata/src/model/AppGraph.ts
- core/metadata/test/ModuleGraph.test.ts
- core/tegg/dal.ts
- plugin/dal/README.md
- plugin/dal/app.ts
- plugin/dal/lib/DalModuleLoadUnitHook.ts
- plugin/dal/lib/DalTableEggPrototypeHook.ts
- plugin/dal/lib/DataSource.ts
- plugin/dal/lib/MysqlDataSourceManager.ts
- plugin/dal/lib/SqlMapManager.ts
- plugin/dal/lib/TableModelManager.ts
- plugin/dal/test/dal.test.ts
- plugin/dal/test/fixtures/apps/dal-app/config/config.default.js
- plugin/dal/test/fixtures/apps/dal-app/config/plugin.js
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/Foo.ts
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/FooDAO.ts
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/base/BaseFooDAO.ts
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/extension/FooExtension.ts
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/structure/Foo.sql
- plugin/dal/typings/index.d.ts
- plugin/tegg/lib/AppLoadUnit.ts
Additional comments: 4
core/dal-decorator/README.md (1)
- 1-5: The README provides clear direction for users to find more detailed information. Ensure the link to
@eggjs/tegg-dal-pluginis functional and accessible from the repository.core/dal-runtime/README.md (1)
- 1-5: Ensure the link to
@eggjs/tegg-dal-pluginis functional and accessible from the repository. Additionally, verify that the content is relevant to thedal-runtimepackage..github/workflows/nodejs.yml (2)
- 23-23: Updating the
node-versionmatrix to exclude version 14 aligns with Node.js versioning policies. Consider adding a note in the documentation or changelog to inform users about dropping support for Node.js version 14.Also applies to: 54-54
- 83-112: The removal of the Windows runner due to GitHub MySQL service limitations is understandable. Ensure that this limitation and its implications are clearly documented for contributors who might want to run tests on Windows environments.
| columns.push(ColumnModel.build(property, columnType, columnParam)); | ||
| } | ||
|
|
||
| // TODO 补充 PRIMARY 主键 |
There was a problem hiding this comment.
There's a TODO comment about supplementing the PRIMARY key. Ensure this is addressed or tracked in a project management tool to avoid missing this critical functionality.
Would you like assistance in implementing the PRIMARY key supplementation?
There was a problem hiding this comment.
@fengmk2, to address the supplementation of the PRIMARY key in the TableModel class, you can enhance the getPrimary method to ensure it automatically creates a PRIMARY key index if one is not explicitly defined among the indices. Here's a suggested approach:
- The
getPrimarymethod already attempts to find a PRIMARY index among the defined indices. If it finds one, it returns it. - If no PRIMARY index is found, it creates one based on columns marked as primary keys. This is a good approach, but ensure that all columns intended to be part of the PRIMARY key are correctly marked with
primaryKey: truein their definitions.
Here's a refined implementation snippet for the getPrimary method:
getPrimary(): IndexModel | undefined {
// Attempt to find an explicitly defined PRIMARY index
const index = this.indices.find(t => t.type === IndexType.PRIMARY);
if (index) {
return index;
}
// If no PRIMARY index is found, create one based on columns marked as primary keys
const primaryColumn = this.columns.filter(t => t.primaryKey === true);
if (primaryColumn.length > 0) {
return new IndexModel({
name: 'PRIMARY',
type: IndexType.PRIMARY,
keys: primaryColumn.map(t => ({
columnName: t.columnName,
propertyName: t.propertyName,
})),
});
}
// If no columns are marked as primary keys, consider logging a warning or throwing an error
console.warn('No PRIMARY key defined for table:', this.name);
return undefined;
}This implementation ensures that if a PRIMARY index is not explicitly defined among the indices, the class will attempt to create one based on columns marked as primary keys. If no columns are marked as primary keys, it logs a warning, which you can replace with an error throw if your application's logic requires a PRIMARY key to be defined.
Please review this approach and adjust it according to your specific requirements and coding standards.
Comment on lines
1
to
147
| collate?: string; | ||
| compression?: CompressionType; | ||
| encryption?: boolean; | ||
| engine?: string; | ||
| engineAttribute?: string; | ||
| insertMethod?: InsertMethod; | ||
| keyBlockSize?: number; | ||
| maxRows?: number; | ||
| minRows?: number; | ||
| rowFormat?: RowFormat; | ||
| secondaryEngineAttribute?: string; | ||
| }) { | ||
| this.clazz = params.clazz; | ||
| this.name = params.name; | ||
| this.dataSourceName = params.dataSourceName; | ||
| this.columns = params.columns; | ||
| this.indices = params.indices; | ||
| this.comment = params.comment; | ||
| this.autoExtendSize = params.autoExtendSize; | ||
| this.autoIncrement = params.autoIncrement; | ||
| this.avgRowLength = params.avgRowLength; | ||
| this.characterSet = params.characterSet; | ||
| this.collate = params.collate; | ||
| this.compression = params.compression; | ||
| this.encryption = params.encryption; | ||
| this.engine = params.engine; | ||
| this.engineAttribute = params.engineAttribute; | ||
| this.insertMethod = params.insertMethod; | ||
| this.keyBlockSize = params.keyBlockSize; | ||
| this.maxRows = params.maxRows; | ||
| this.minRows = params.minRows; | ||
| this.rowFormat = params.rowFormat; | ||
| this.secondaryEngineAttribute = params.secondaryEngineAttribute; | ||
| } | ||
|
|
||
| getPrimary(): IndexModel | undefined { | ||
| const index = this.indices.find(t => t.type === IndexType.PRIMARY); | ||
| if (index) { | ||
| return index; | ||
| } | ||
| const primaryColumn = this.columns.filter(t => t.primaryKey === true); | ||
| return new IndexModel({ | ||
| name: 'PRIMARY', | ||
| type: IndexType.PRIMARY, | ||
| keys: primaryColumn.map(t => { | ||
| return { | ||
| columnName: t.columnName, | ||
| propertyName: t.propertyName, | ||
| }; | ||
| }), | ||
| }); | ||
| } | ||
|
|
||
| static build<T>(clazz: EggProtoImplClass<T>): TableModel<T> { | ||
| const params = TableInfoUtil.getTableParams(clazz as EggProtoImplClass); | ||
| const name = params?.name ?? snakecase(pluralize(clazz.name)); | ||
| const columnInfoMap = ColumnInfoUtil.getColumnInfoMap(clazz as EggProtoImplClass); | ||
| const columnTypeMap = ColumnInfoUtil.getColumnTypeMap(clazz as EggProtoImplClass); | ||
| // TODO set to default name | ||
| const dataSourceName = params?.dataSourceName ?? 'default'; | ||
| assert(TableInfoUtil.getIsTable(clazz as EggProtoImplClass), `${name} is not Table`); | ||
| assert(columnTypeMap, `${name} has no columns`); | ||
| const columns: Array<ColumnModel> = []; | ||
| const indices: Array<IndexModel> = []; | ||
| for (const [ property, columnType ] of columnTypeMap?.entries()) { | ||
| const columnParam = columnInfoMap?.get(property); | ||
| columns.push(ColumnModel.build(property, columnType, columnParam)); | ||
| } | ||
|
|
||
| // TODO 补充 PRIMARY 主键 | ||
| const indexList = IndexInfoUtil.getIndexList(clazz as EggProtoImplClass); | ||
| for (const index of indexList) { | ||
| indices.push(IndexModel.build(index, columns)); | ||
| } | ||
|
|
||
| return new TableModel({ | ||
| clazz, | ||
| name, | ||
| columns, | ||
| indices, | ||
| dataSourceName, | ||
|
|
||
| comment: params?.comment, | ||
| autoExtendSize: params?.autoExtendSize, | ||
| autoIncrement: params?.autoIncrement, | ||
| avgRowLength: params?.avgRowLength, | ||
| characterSet: params?.characterSet, | ||
| collate: params?.collate, | ||
| compression: params?.compression, | ||
| encryption: params?.encryption, | ||
| engine: params?.engine, | ||
| engineAttribute: params?.engineAttribute, | ||
| insertMethod: params?.insertMethod, | ||
| keyBlockSize: params?.keyBlockSize, | ||
| maxRows: params?.maxRows, | ||
| minRows: params?.minRows, | ||
| rowFormat: params?.rowFormat, | ||
| secondaryEngineAttribute: params?.secondaryEngineAttribute, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The TableModel class is well-structured and provides a comprehensive representation of database table metadata. Ensure that all properties and methods are adequately documented, especially for complex or non-obvious functionality.
Consider adding detailed comments or documentation within the code to explain the purpose and usage of each property and method, especially for the build method, which performs significant logic to construct a TableModel instance.
This was referenced Mar 21, 2024
|
@coderabbitai pause |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #192 +/- ##
==========================================
- Coverage 94.02% 92.20% -1.82%
==========================================
Files 236 276 +40
Lines 5036 6478 +1442
Branches 545 969 +424
==========================================
+ Hits 4735 5973 +1238
- Misses 301 505 +204 ☔ View full report in Codecov by Sentry. |
|
+1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
.gitignoreto exclude.nodefiles.