Conversation
- Add param filter in TableSqlMap for parameterized queries - Update MysqlDataSource.query() to accept optional params array - Modify DataSource to pass params to query methods - Update BaseSqlMap to use param filter in all generated SQLs - generateFindByPrimary: primary key queries - generateFindByIndexes: index queries - generateInsert: insert statements - generateUpdate: update statements - generateDelete: delete statements - Add test case for param filter functionality 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add param filter description in custom filters section - Update custom SQL example to use param filter - Add detailed usage example and security notes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR implements parameterized SQL templates via a new Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant DS as DataSource
participant TSM as TableSqlMap
participant MDS as MySqlDataSource
participant DB as MySQL Client
App->>DS: execute(sqlName, data, options?)
DS->>TSM: generate(sqlName, data, timezone)
Note over TSM: Render template with `param` filter\ncollecting values into params[]
TSM-->>DS: { sql, params }
DS->>MDS: query(sql, params, options)
alt options.executeType or default === 'execute'
MDS->>DB: client.execute(sql, params, options)
else
MDS->>DB: client.query(sql, params, options)
end
DB-->>MDS: results
MDS-->>DS: results
DS-->>App: mapped results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @killagu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security of database operations by implementing robust support for parameterized queries. By introducing a 'param' filter and integrating it throughout the SQL generation and execution layers, it ensures that all dynamically generated SQL statements are safe from SQL injection attacks. This change provides a more secure and reliable data access layer without altering the core logic of how data is retrieved or manipulated. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces parameterized queries to prevent SQL injection, which is a great security improvement. The implementation correctly uses a nunjucks filter to replace values with '?' and collect the parameters. However, I've found a critical concurrency issue in how the parameters are collected. The current implementation uses a shared environment state, which will lead to race conditions and incorrect query parameters under concurrent requests. I've provided suggestions to fix this by using the per-render context instead. I also found a minor simplification that can be made in the MySqlDataSource.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/dal-runtime/src/BaseSqlMap.ts (2)
152-189: Critical: SQL injection vulnerability in INSERT statement for timestamp fields.The
gmtCreateandgmtModifiedfields in the INSERT statement (line 186) do not use theparamfilter when a value is provided by the user:{{ $${propertyName} if $${propertyName} !== undefined else '${now}' }}This means if a user provides custom values for these fields, they will be directly interpolated into the SQL without parameterization, creating a SQL injection vulnerability.
Compare this with the UPDATE statement (line 244) which correctly uses the param filter:
{{ ($${propertyName} | param) if $${propertyName} !== undefined else '${now}' }}Apply this diff to fix the vulnerability:
values.push((` {% if ___first %} {% set ___first = false %} {% else %} , {% endif %} - {{ $${propertyName} if $${propertyName} !== undefined else '${now}' }} + {{ ($${propertyName} | param) if $${propertyName} !== undefined else '${now}' }} `).trim());
34-267: Add parameterization filter to gmtModified handling in INSERT method (line 186).Verification found an inconsistency in parameterization. Line 186 in the
generateInsert()method lacks aparamfilter for thepropertyNamevariable when handling gmtModified/gmtCreate columns, while line 244 in thegenerateUpdate()method correctly applies it for the same column types.Change line 186 from:
{{ $${propertyName} if $${propertyName} !== undefined else '${now}' }}to:
{{ ($${propertyName} | param) if $${propertyName} !== undefined else '${now}' }}All other template variables across primary key queries, index queries, update statements, and delete statements are consistently parameterized.
🧹 Nitpick comments (2)
core/dal-runtime/src/TableSqlMap.ts (1)
50-56: Add error handling to the param filter.The param filter silently skips parameter collection if
$$paramsis undefined. While defensive, this could mask bugs where the filter is used outside thegeneratecontext, leading to SQL with literal'?'strings instead of parameterized queries.Consider throwing an error if
$$paramsis not available:// Add param filter for parameterized queries env.addFilter('param', function(this: Template, value: any) { - if ((this as any).env.$$params) { - (this as any).env.$$params.push(value); + const params = (this as any).env.$$params; + if (!params) { + throw new Error('param filter can only be used within DataSource.generate() context'); } + params.push(value); return '?'; });plugin/dal/README.md (1)
613-613: Use a proper heading instead of bold text.The bold text
**param 过滤器**should be a proper Markdown heading for better document structure and accessibility.Apply this diff:
-**param 过滤器** +#### param 过滤器Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/dal-runtime/src/BaseSqlMap.ts(6 hunks)core/dal-runtime/src/DataSource.ts(4 hunks)core/dal-runtime/src/MySqlDataSource.ts(1 hunks)core/dal-runtime/src/TableSqlMap.ts(3 hunks)core/dal-runtime/test/TableSqlMap.test.ts(1 hunks)core/dal-runtime/test/fixtures/modules/dal/dal/extension/FooExtension.ts(1 hunks)plugin/dal/README.md(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/dal-runtime/test/TableSqlMap.test.ts (2)
core/dal-decorator/src/model/TableModel.ts (1)
TableModel(12-144)core/dal-runtime/src/SqlMapLoader.ts (1)
SqlMapLoader(6-26)
core/dal-runtime/src/DataSource.ts (1)
core/dal-runtime/src/TableModelInstanceBuilder.ts (1)
TableModelInstanceBuilder(3-25)
🪛 markdownlint-cli2 (0.18.1)
plugin/dal/README.md
613-613: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
🔇 Additional comments (5)
core/dal-runtime/test/fixtures/modules/dal/dal/extension/FooExtension.ts (1)
8-17: LGTM! Good example of parameterized query usage.The
customFindextension demonstrates proper usage of theparamfilter for SQL parameterization. This serves as a good fixture for testing the new functionality.plugin/dal/README.md (1)
575-637: LGTM! Comprehensive documentation for the param filter.The documentation clearly explains the purpose, usage, and security benefits of the param filter with practical examples. The warning about using it for all user inputs is especially important.
core/dal-runtime/test/TableSqlMap.test.ts (1)
14-26: LGTM! Well-structured tests for parameterized queries.The tests properly validate both aspects of the new functionality:
- Existing test updated to verify the GeneratedSql structure
- New test validates parameterized query generation with placeholder and params array
core/dal-runtime/src/MySqlDataSource.ts (1)
65-70: LGTM! Clean implementation of parameterized query support.The method signature change properly supports optional parameters while maintaining backward compatibility. The conditional logic ensures params are only passed when provided and non-empty.
core/dal-runtime/src/DataSource.ts (1)
7-100: LGTM! Comprehensive parameter propagation through all query paths.The changes systematically propagate parameters through all query execution methods:
ExecuteSqlinterface properly includes paramsgenerateSqlextracts and returns params from TableSqlMap- All execution methods (
count,execute,executeRaw,paginate) pass params to MySqlDataSource- Pagination correctly handles separate params for data and count queries
The implementation is thorough and maintains consistency across all code paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugin/dal/README.md (2)
613-613: Consider using a proper heading for consistency.The markdown uses bold text instead of a heading, which is flagged by markdownlint. Consider using
###for consistency with the document structure.Apply this diff:
-**param 过滤器** +### param 过滤器
615-615: Clarify the scope of SQL injection prevention.Based on learnings, the documentation states that the param filter is used "用于防止 SQL 注入" (to prevent SQL injection), but past comments indicate that dal already performs SQL escaping internally. The primary benefits are described as "小幅的性能优化和 db 侧的可观测" (minor performance optimization and database-side observability). Consider clarifying that this provides defense-in-depth and observability benefits, rather than being the primary SQL injection prevention mechanism.
core/dal-runtime/src/TableSqlMap.ts (1)
53-53: Type hint is incorrect but not breaking.The type hint
this: Templateis incorrect. In Nunjucks filter functions,thisrefers to the render context object, not the Template. The correct type would be something likethis: { ctx?: Record<string, any> }. However, given theas anycast in the implementation, this is a pedantic type correctness issue that doesn't affect runtime behavior.Apply this diff if you want to correct the type hint:
- env.addFilter('param', function(this: Template, value: any) { + env.addFilter('param', function(this: { ctx?: Record<string, any> }, value: any) { if ((this as any).ctx[SQL_PARAMS]) { (this as any).ctx[SQL_PARAMS].push(value); } return '?'; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/dal-runtime/src/TableSqlMap.ts(3 hunks)plugin/dal/README.md(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/dal-runtime/src/TableSqlMap.ts (1)
core/dal-runtime/src/SqlGenerator.ts (1)
SqlGenerator(10-393)
🪛 markdownlint-cli2 (0.18.1)
plugin/dal/README.md
613-613: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
plugin/dal/README.md (2)
575-575: LGTM!The documentation correctly demonstrates the new
paramfilter usage, replacing direct value interpolation with parameterized queries.
602-602: LGTM!The param filter is correctly added to the list of supported filters.
core/dal-runtime/src/TableSqlMap.ts (5)
8-8: LGTM!The constant provides a clear, collision-resistant key for passing parameters through the render context.
16-19: LGTM!The interface clearly defines the return structure for parameterized SQL generation with appropriate types.
52-58: Excellent fix for the concurrency issue!The implementation now correctly uses the render context (
this.ctx[SQL_PARAMS]) instead of the shared environment, ensuring each concurrent request maintains its own parameter array. This addresses the critical concurrency issue flagged in previous reviews.
102-114: Params handling correctly implemented!The implementation now properly passes the params array through the render context instead of attaching it to the shared environment. This ensures each concurrent request maintains its own parameter collection, resolving the critical concurrency issue from previous reviews.
105-105: Proposed fix is incomplete and does not address the concurrency issue correctly.The review's suggested diff will not work as written. The
escapeSQLfunction in NunjucksUtil.ts is part of a custom RUNTIME object that replaces nunjucks.runtime during template rendering. WhenescapeSQLis called from compiled template code, itsthiscontext refers to the RUNTIME object, which has access tothis.env(the nunjucks Environment), not the render context passed totemplate.render().Passing
$$timezone: timezonethrough the render context won't make it accessible toescapeSQL, since that function only accessesthis.env.timezone. The current approach of setting timezone on the shared env object does create a race condition under concurrent load, but the proposed solution doesn't actually fix it.To properly resolve the concurrency issue, timezone would need to either:
- Be set on the custom RUNTIME object itself (passed through during template render)
- Be passed through the environment in a request-local manner
- Require a different mechanism to pass data from the render context to the runtime functions
The original concern about the race condition is valid, but the suggested fix requires more careful implementation.
killagu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/dal-runtime/src/BaseSqlMap.ts (3)
57-66: Indexed WHERE conditions correctly parameterized; consider a null-case testThe expression
`col` {{ "IS" if $prop == null else "=" }} {{$prop | param}}will yield
col IS ?for nulls andcol = ?otherwise, which should work fine with MySQL prepared statements and the new param filter.To guard against regressions in null handling, I’d suggest adding a small test case that exercises an index query where the indexed value is
null, verifying both the generated SQL and params (e.g., SQL containsIS ?and params array contains a singlenull).
82-196: Insert values are now parameterized; confirm behavior for JSON/spatial filtersThe changes to:
- Use
{{$prop | param}}for non-spatial / non-JSON columns, and- Use
{{ ($prop | param) if $prop !== undefined else '${now}' }}forgmtCreate/gmtModifiedcorrectly route normal column values and explicit timestamp overrides through the param collector while keeping the default
nowexpression inline.One thing to double‑check: spatial values (
TemplateUtil.getSpatialFilter(...)) and JSON values (| toJson) are still going through their own filters without an explicit| paramin this file. If those filters have not been updated to delegate to the param collector (i.e., return'?'and push intoparams), their values will continue to be inlined instead of parameterized. If you want full parameterization, consider either:
- Updating the spatial/JSON filters to integrate with the param mechanism internally, or
- Chaining
| paramafter them here, once you confirm that doing so doesn’t break existing SQL generation.
198-254: Update SET and primary-key predicates fully parameterized; minor shape inconsistencySwitching both branches of the SET logic to:
\col` = {{$prop | param}}` for normal columns, and\gmtModified` = {{ ($prop | param) if$prop !== undefined else '$ {now}' }}`plus parameterizing the primary-key WHERE (
\pk` = {{primary.prop | param}}) bringsgenerateUpdate` in line with the parameterized flow used elsewhere.Not for this PR, but note that primary-key variables are referenced as
$prop(findByPrimary),prop(delete), andprimary.prop(update). If there’s no strong reason for these different shapes, unifying the template variable convention across operations would reduce cognitive overhead for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/dal-runtime/src/BaseSqlMap.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (16)
🔇 Additional comments (2)
core/dal-runtime/src/BaseSqlMap.ts (2)
30-49: Primary-key WHERE clause now correctly parameterizedUsing
| paramfor all primary-key predicates (\col` = {{$prop | param}}`) aligns with the new param collector and avoids inlined literals. Looks good and consistent with the rest of the param flow.
256-271: Delete by primary-key now parameterizedUsing
{{prop | param}}for the delete WHERE clause ensures primary-key deletes participate in the bound‑parameter flow rather than inlining values, consistent with the other generators.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package.json (1)
21-21: Clarify the "20-" naming convention for new scripts.The new scripts (
test:20-,cov:20-,ci:20-) exclude thelangchain-decoratorworkspace, but the "20-" suffix is cryptic without documentation. Is this related to Node.js version constraints on that workspace?Consider adding an inline comment explaining the naming convention or documenting this in a contributing guide.
Also applies to: 23-23, 35-35
.github/workflows/nodejs.yml (1)
8-8: Rename workflow jobs to clarify Node.js version matrix.The job naming is misleading:
Runner-ubuntu-20-actually runs on Node [16, 18], while the newRunner-ubunturuns on Node [20]. This contradicts the-20-suffix, which suggests version 20 compatibility testing.Consider renaming for clarity:
Runner-ubuntu-20-→Runner-ubuntu-legacyorRunner-ubuntu-16-18Runner-ubuntu→Runner-ubuntu-20(orRunner-ubuntu-latest)- Apply the same pattern to macOS jobs
Alternatively, if the
-20-suffix has org-wide convention, document it prominently.Also applies to: 48-48, 84-84, 124-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/nodejs.yml(3 hunks)package.json(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/nodejs.yml
45-45: the runner of "codecov/codecov-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
60-60: the runner of "actions/setup-node@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
81-81: the runner of "codecov/codecov-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
105-105: the runner of "actions/setup-node@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
121-121: the runner of "codecov/codecov-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos-20- (18)
- GitHub Check: Runner-ubuntu-20- (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu-20- (16)
- GitHub Check: Runner-macos-20- (16)
- GitHub Check: Runner-ubuntu (20)
🔇 Additional comments (1)
.github/workflows/nodejs.yml (1)
17-17: Verify alignment between package.json CI scripts and workflow jobs.The workflow now uses
npm run ci:20-(line 41, 78) for older Node versions andnpm run cifor Node 20. Confirm that:
- The
langchain-decoratorworkspace exclusion inci:20-doesn't mask failures that should be caught.- Node 20 is sufficiently tested via the full
ciscript on the new jobs.- This split is intentional and documented somewhere (e.g., in CONTRIBUTING.md).
Also applies to: 39-42
| npm run tsc:pub | ||
|
|
||
| - name: Code Coverage | ||
| uses: codecov/codecov-action@v1 |
There was a problem hiding this comment.
Update deprecated GitHub Actions to current versions.
Static analysis flagged outdated action versions:
codecov/codecov-action@v1→ use@v3or@v4(current stable)actions/setup-node@v1→ use@v3or@v4(current stable)
These versions are no longer maintained and may lack security patches or have changed behavior on modern GitHub Actions runners.
Apply this diff to update to current versions:
- uses: actions/setup-node@v1
+ uses: actions/setup-node@v4
- uses: codecov/codecov-action@v1
+ uses: codecov/codecov-action@v4This change applies to all occurrences (lines 29, 60, 105, 136; lines 45, 81, 121, 157).
Also applies to: 60-60, 81-81, 105-105, 121-121
🧰 Tools
🪛 actionlint (1.7.8)
45-45: the runner of "codecov/codecov-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/nodejs.yml around lines 29, 45, 60, 81, 105, 121, 136, and
157, update deprecated action versions: replace all occurrences of
codecov/codecov-action@v1 with a current stable tag (e.g. @v3 or @v4) and
replace actions/setup-node@v1 with a current stable tag (e.g. @v3 or @v4); apply
the same substitution for every listed line so the workflow uses maintained
action versions and then run a quick workflow lint/test to confirm no behavior
changes.
killagu
force-pushed
the
feat/sql_param
branch
from
095fe60 to
f9cba6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/dal-runtime/test/DataSource.test.ts (1)
201-331: Forward all execute arguments in mock and consider de-duping Foo setupThe
mmwrapper forclient.executeonly accepts(sql, params), so any future thirdoptionsargument would be silently dropped; using a rest parameter andReflect.apply(originExecute, client, args)would keep the test robust if callers start passing options. Also, the Foo construction here is nearly identical to the earlierexecute should worktest; a small helper (e.g.buildTestFoo()) would reduce duplication, though it’s purely cosmetic.plugin/dal/README.md (1)
625-658: Clarifyparamfilter benefits vs existing SQL escaping and optional markdown tweakThe bullet says
paramprevents SQL injection, while the dedicated section emphasizes parameterized queries, perf, and observability; to avoid confusion with DAL’s built‑in SQL escaping, you could add a note that template values are already escaped by default andparamadditionally leverages server‑side parameters for better performance and DB observability. If you care about markdownlint MD036, turning**param 过滤器**into a proper heading (for example,#### param 过滤器) would also silence that warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/nodejs.yml(1 hunks)core/dal-runtime/src/DataSource.ts(3 hunks)core/dal-runtime/src/MySqlDataSource.ts(5 hunks)core/dal-runtime/test/DataSource.test.ts(1 hunks)plugin/dal/README.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/nodejs.yml
🧰 Additional context used
🧬 Code graph analysis (1)
core/dal-runtime/src/DataSource.ts (2)
core/dal-runtime/src/MySqlDataSource.ts (1)
EggQueryOptions(22-24)core/dal-runtime/src/TableModelInstanceBuilder.ts (1)
TableModelInstanceBuilder(3-25)
🪛 markdownlint-cli2 (0.18.1)
plugin/dal/README.md
636-636: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (20)
🔇 Additional comments (2)
core/dal-runtime/src/MySqlDataSource.ts (1)
2-79: executeType and EggQueryOptions wiring looks consistentThe new
executeTypeoption (defaulting to'query') and the branching inquery()betweenclient.executeandclient.query, withEggQueryOptionspassed through as the third argument, are coherent and align with the new tests and docs; no functional issues stand out.core/dal-runtime/src/DataSource.ts (1)
3-95: Param propagation and pagination/count plumbing look correctThreading
paramsthroughExecuteSql, all execute/executeRaw/count methods, andpaginate(including the separate count SQL and options propagation intomysqlDataSource.query) matches the new parameterized SQL design; behavior looks consistent and I don’t see issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/dal-runtime/src/DataSource.ts (1)
7-12: Consider stricter typing for params.The
params: any[]type is very permissive. While SQL parameters can be various types, consider using a union type likeparams: (string | number | Date | Buffer | null | undefined)[]for better type safety.Apply this diff for stricter typing:
export interface ExecuteSql { sql: string; template: string; sqlType: SqlType; - params: any[]; + params: (string | number | Date | Buffer | null | undefined)[]; }plugin/dal/README.md (1)
625-658: Excellent documentation for the param filter.The param filter documentation is comprehensive, with clear examples showing both the template syntax and the generated SQL output. This will help developers understand and use the feature correctly.
Optional: Consider using a proper heading for "param 过滤器".
Line 636 uses bold text (
**param 过滤器**) which triggers a markdownlint warning (MD036). Consider using a proper heading for better document structure.Apply this diff to use a proper heading:
-**param 过滤器** +#### param 过滤器
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/dal-runtime/src/DataSource.ts(3 hunks)core/dal-runtime/src/MySqlDataSource.ts(5 hunks)core/dal-runtime/test/DataSource.test.ts(1 hunks)plugin/dal/README.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/dal-runtime/test/DataSource.test.ts
- core/dal-runtime/src/MySqlDataSource.ts
🧰 Additional context used
🧬 Code graph analysis (1)
core/dal-runtime/src/DataSource.ts (1)
core/dal-runtime/src/MySqlDataSource.ts (1)
EggQueryOptions(22-24)
🪛 markdownlint-cli2 (0.18.1)
plugin/dal/README.md
636-636: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (16)
🔇 Additional comments (10)
core/dal-runtime/src/DataSource.ts (8)
3-3: LGTM!The import correctly adds the necessary types for parameterized query support and options handling.
32-42: LGTM!The
generateSqlmethod correctly destructures and returns theparamsarray alongside the SQL string, properly implementing the parameterized query support.
44-48: LGTM!The
countmethod correctly propagates bothparamsandoptionsto the underlying query execution, maintaining backward compatibility with the optional parameter.
49-55: LGTM!The
executemethod correctly passes SQL, params, and options to the MySQL data source while maintaining the expected return type.
56-59: LGTM!The
executeRawmethod correctly implements parameterized query support with the same pattern asexecute.
60-69: LGTM!Both
executeScalarandexecuteRawScalarcorrectly accept and forward theoptionsparameter to their underlying methods, maintaining consistency across the API.
70-87: LGTM!The
paginatemethod correctly handles two separate SQL generations (main query and count query), each with their own params array, and properly propagates both params and options to the underlying query executions.
89-95: LGTM!The private
#paginateCountmethod correctly accepts and uses theparamsarray andoptions, completing the consistent parameter propagation throughout the class.plugin/dal/README.md (2)
80-102: LGTM!The
executeTypeconfiguration documentation is comprehensive and includes important warnings about precision issues with the execute mode. The YAML example and mode descriptions are clear and helpful.
598-598: LGTM!The SQL example correctly demonstrates the
paramfilter usage in the WHERE clause, aligning with the new parameterized query feature.
4 participants
🤖 Generated with Claude Code
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.