feat: implement model-driven data API with dynamic SQL operations #290
+1,210
−13
Conversation
This file contains hidden or 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
WalkthroughThis pull request introduces a dynamic data management system for TinyEngine. It expands the MyBatis mapper scanning scope, creates a new REST API controller with CRUD endpoints for dynamic model data, implements a SQL provider for dynamic query generation, and adds complementary services and DAOs to manage dynamic table operations and data persistence across multiple application layers. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as ModelDataController
participant Service as DynamicService
participant DAO as ModelDataDao
participant DB as Database
Client->>Controller: POST /model-data/api/queryApi<br/>(DynamicQuery)
activate Controller
Controller->>Service: queryWithPage(dto)
activate Service
Service->>Service: Validate table exists
Service->>DAO: select(params)
activate DAO
DAO->>DB: Execute dynamic SELECT<br/>(via DynamicSqlProvider)
DB-->>DAO: List<JSONObject>
deactivate DAO
Service->>DAO: count(tableName, conditions)
activate DAO
DAO->>DB: Execute COUNT(*)
DB-->>DAO: Long totalCount
deactivate DAO
Service-->>Controller: Map{list, total, page}
deactivate Service
Controller-->>Client: Result<Map<String,Object>>
deactivate Controller
Client->>Controller: POST /model-data/api/insertApi<br/>(DynamicInsert)
activate Controller
Controller->>Service: insert(dto)
activate Service
Service->>Service: Inject system fields<br/>(created_by, updated_by)
Service->>DAO: insert(params)
activate DAO
DAO->>DB: Execute dynamic INSERT<br/>(with generated key)
DB-->>DAO: Long insertedId
deactivate DAO
Service-->>Controller: Map{insertCount, id}
deactivate Service
Controller-->>Client: Result<Map<String,Object>>
deactivate Controller
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In
`@base/src/main/java/com/tinyengine/it/dynamic/controller/ModelDataController.java`:
- Around line 43-47: The controller currently returns raw exception messages
(e.getMessage()) to clients; instead, in ModelDataController wrap each
endpoint's try/catch (e.g., the method calling
dynamicService.queryWithPage(dto)) to log the full exception server-side (using
your logger) and return a sanitized, non-sensitive message via Result.failed
(e.g., "Internal server error" or a brief contextual message). Update the catch
blocks across all endpoint methods in ModelDataController to log the exception
(including stack trace) and replace e.getMessage() with a generic error string
while preserving any error codes or flags your Result type supports.
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/DynamicSqlProvider.java`:
- Around line 69-90: The update method in DynamicSqlProvider currently
interpolates tableName and column names from the data and conditions maps
directly (variables: tableName, data, conditions in method update of class
DynamicSqlProvider), creating an SQL injection risk; fix by
validating/whitelisting the tableName and each column key before building the
SQL (e.g., check against an allow-list or strict identifier regex like
/^[A-Za-z_][A-Za-z0-9_]*$/) and throw an IllegalArgumentException for any
invalid identifier, or resolve identifiers via a known mapping of
logical->actual column names; keep using parameter placeholders (#{data.xxx},
#{conditions.xxx}) for values but do not concatenate unchecked identifiers into
sql.SET/sql.WHERE.
- Around line 92-106: The delete(Map<String, Object> params) method currently
issues DELETE_FROM(tableName) even when conditions is null/empty; add a guard
that checks the conditions Map (from params.get("conditions")) and if it is null
or empty throw an IllegalArgumentException (or another explicit runtime
exception) instead of building the SQL, so no SQL is generated without a WHERE;
update the method that builds SQL (uses SQL sql = new SQL();
sql.DELETE_FROM(tableName); sql.WHERE(...)) to only perform DELETE_FROM and add
WHERE clauses after the non-empty check (or return/throw before calling
sql.DELETE_FROM) to ensure you never produce a DELETE FROM without conditions.
- Around line 10-51: The select method in DynamicSqlProvider currently
concatenates untrusted inputs (tableName, fields, orderBy, orderType, and
condition keys) into SQL, enabling injection; update select to validate and
whitelist identifiers before using them: ensure tableName and each field in
fields match a safe identifier whitelist or strict regex (letters, digits,
underscore) or against allowed table/column lists, validate orderBy is a single
whitelisted column and orderType is only "ASC" or "DESC", and validate
pageNum/pageSize are non-negative integers; only after validation pass the
sanitized identifiers to SQL.SELECT, SQL.FROM and SQL.ORDER_BY, and keep
parameter binding (#{conditions.xxx}) for values rather than concatenation to
eliminate injection vectors in condition values and LIMIT.
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/ModelDataDao.java`:
- Around line 17-19: The DAO insert method has the wrong return type: change
ModelDataDao.insert's signature from Long insert(Map<String,Object> params) to
int (or Integer) insert(Map<String,Object> params) because MyBatis returns the
number of affected rows while generated key is populated into params via
`@Options`; then update callers (e.g., DynamicService.insert) to treat the return
value as rowsAffected (rename insertRow -> rowsAffected) and continue to read
the generated id from params.get("id"); leave DynamicSqlProvider.insert and the
`@Options` annotation as-is.
In
`@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java`:
- Around line 459-470: The getEnumOptions method is wrapping enum values in
single quotes without escaping, which breaks SQL when a value contains a single
quote; update getEnumOptions so each extracted value from JSON (in method
getEnumOptions) has single quotes escaped (replace ' with '') before you wrap it
in surrounding single quotes and join them, ensuring the map/stream step uses
the escaped string to build the final comma-separated list.
- Around line 132-144: The loop in DynamicModelService is overwriting each
ParametersDto default with the hardcoded "1" (param.setDefaultValue("1")), which
must be removed; instead read the existing default with param.getDefaultValue()
and only throw IllegalArgumentException if that value is null for a required
parameter (Boolean.TRUE.equals(param.getRequired())). Update the for-loop that
iterates over parameters (ParametersDto param) to stop setting a hardcoded
default and use convertValueByType(value, fieldType, columnName) with the actual
configured default value.
- Around line 387-405: The Date type mapping is inconsistent between
mapJavaTypeToSQL and generateColumnDefinition causing spurious schema-change
detection; fix by unifying the mapping: either change mapJavaTypeToSQL to return
"DATE" for "Date" (instead of "TIMESTAMP") or refactor generateColumnDefinition
to call mapJavaTypeToSQL (and use its result) so both use the same canonical
mapping; update references in modifyTableStructure-related logic to rely on that
single mapping function (mapJavaTypeToSQL) to avoid mismatches.
- Around line 237-249: The getWhereCondition method in DynamicModelService
concatenates raw map keys into SQL (entry.getKey()), which allows SQL injection;
fix by validating/whitelisting keys before using them: create or use an
allowedColumns set (or table metadata) and only build clauses for keys present
in that set, mapping validated keys to safe column identifiers (or
pre-quoted/escaped identifiers) and throw/log on unknown keys; continue to use
named parameters for values and append to sql and whereClauses only after
validation so no raw keys are injected.
- Around line 574-607: The updateDateById method is vulnerable because
updateFields' keys are concatenated into SQL; validate each field name against
an allowlist or a safe identifier pattern before appending (e.g., only
[a-zA-Z0-9_]+) or map DTO keys to known column names, and reject or ignore
invalid keys; build the SET clause using only validated/whitelisted column names
(still using "column = ?" placeholders for values) and use
jdbcTemplate.update(sql, params.toArray()) as before; reference updateDateById,
updateFields, tableName, the sql StringBuilder, and jdbcTemplate.update when
making this change.
- Around line 171-211: The dynamicQuery method currently concatenates
user-controlled identifiers (tableName, fields, orderBy) into SQL; fix by
validating/whitelisting or safely quoting these identifiers before building the
SQL: ensure tableName matches an allowed table name pattern or a maintained
whitelist, ensure each entry in fields is an allowed column name or matches a
safe identifier regex (reject/escape anything else), and validate orderBy to
only allow "column [ASC|DESC]" patterns or map it from a safe whitelist;
continue to use parameter binding for values via
getWhereCondition/namedParameterJdbcTemplate but do not pass unvalidated
identifiers directly into SQL—apply these checks in dynamicQuery (and any helper
like getWhereCondition if it touches identifiers) before appending to the
StringBuilder.
- Around line 309-366: modifyTableStructure: when querying
INFORMATION_SCHEMA.COLUMNS (fetchColumnsSql) include the schema filter (e.g.,
AND TABLE_SCHEMA = DATABASE()) to avoid cross-schema matches; normalize type
comparison by comparing a canonical form of types — call
mapJavaTypeToSQL(param.getType()) and compare it against the existing DATA_TYPE
+ length (or use startsWith/regex on existingColumnMap values) instead of raw
equalsIgnoreCase so VARCHAR(255) vs VARCHAR matches; and avoid mutating the
caller's list by making a defensive copy of parameters before calling
addCommonFields (e.g., copy to a new List and operate on that) so the original
Model.parameters is not changed.
- Around line 502-546: createData currently builds column list from
record.keySet() and then iterates record.values() to set PreparedStatement
parameters, which can misalign columns because HashMap iteration order is not
stable; fix by producing a stable iteration order (e.g., convert record to a
LinkedHashMap or build a List<String> keys = new ArrayList<>(record.keySet()) /
List<Map.Entry<String,Object>> entries = new ArrayList<>(record.entrySet())) and
use that same keys/entries sequence both to construct the columns/placeholders
and to set ps.setObject(...) in the PreparedStatementCreator; update references
in createData where columns/placeholders and the parameter-setting loop are
built so they use the same ordered keys/entries (keeping jdbcTemplate.update,
keyHolder usage and adding generatedId to record unchanged).
- Around line 261-286: queryWithPage currently ignores pageNum so pagination has
no OFFSET; compute an offset when pageNum and pageSize are provided (e.g., int
offset = (pageNum - 1) * pageSize), set limit = pageSize, and pass the offset
into dynamicQuery instead of the current single-argument limit; update the
dynamicQuery method signature (and its callers/implementation) to accept and
apply an offset parameter to the generated SQL and leave count(...) unchanged.
- Around line 411-457: In generateColumnDefinition(ParametersDto field,String
type) ensure null-safety and escape SQL-injectable values: first guard
field.getType() for null (e.g., treat null as default type "Text" or route to
default case) before the switch to avoid NPE; when appending default value and
description escape single quotes (and any other DB-specific special chars) by
replacing ' with '' (or the DB-appropriate escape) instead of directly embedding
field.getDefaultValue() or field.getDescription(); keep the existing structure
in generateColumnDefinition but perform the null check (for field and
field.getType()) and string escaping for getDefaultValue() and getDescription()
before appending to sb.
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`:
- Around line 185-190: The null-check risk comes from
modelService.getAllModelName() returning null and causing validateTableExists
(method validateTableExists) to NPE when calling tables.contains(...); fix the
data source method instead by updating ModelServiceImpl.getAllModelName to never
return null—return Collections.emptyList() or a new ArrayList<>() when no models
are found (or when internal value is null)—so validateTableExists can safely
call modelService.getAllModelName().contains(tableName).
- Line 9: The class DynamicService currently imports
jakarta.transaction.Transactional; replace that import with Spring's
org.springframework.transaction.annotation.Transactional so Spring's transaction
management is used; update any annotations or fully-qualified references in
DynamicService to use org.springframework.transaction.annotation.Transactional
(and remove the jakarta import) to ensure proper Spring-managed transactions.
- Around line 51-59: The count method in DynamicService calls
dynamicDao.select(params) and then blindly accesses result.get(0), which can
throw NPE or IndexOutOfBounds if result is null or empty; update count(String
tableName, Map<String,Object> conditions) to check that the List<JSONObject>
returned by dynamicDao.select(params) is non-null and non-empty before accessing
index 0, returning 0 (or a safe default) when no rows are found, and defensively
parsing the "count" field (handle null or non-numeric values) to avoid
NumberFormatException.
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java`:
- Around line 128-130: The exception construction in ModelServiceImpl uses
ExceptionEnum.CM001.getResultCode() for both code and message; change the second
argument to ExceptionEnum.CM001.getResultMsg() so the thrown ServiceException
carries the proper human-readable message (locate the throw in ModelServiceImpl
where the if (result != 1) block constructs the ServiceException).
- Around line 253-261: The getAllModelName method in ModelServiceImpl should
return an empty list instead of null; modify getAllModelName so that when
modelList is empty or null it returns Collections.emptyList() (or new
ArrayList<>()) and otherwise returns the mapped list
(modelList.stream().map(Model::getNameEn)...); ensure the method never returns
null so callers like DynamicService.validateTableExists can safely call
contains() without NPE.
🧹 Nitpick comments (18)
base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicUpdate.java (1)
1-11: Consider naming consistency with sibling DTOs.The field naming differs from
DynamicInsert:
DynamicInsertusesparamsfor insert dataDynamicUpdateusesdatafor update values andparamsfor WHERE conditionsThis inconsistency could cause confusion. Consider aligning the naming convention, e.g., rename
datatoupdateDataorvaluesto make the distinction clearer, or updateDynamicInsert.paramsto match.Additionally, consider adding JSR-303 validation annotations (e.g.,
@NotBlankonnameEn) for required fields to ensure input validation at the controller layer.base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicDelete.java (1)
5-5: Remove unused import.
java.util.Mapis imported but not used in this class.Proposed fix
package com.tinyengine.it.dynamic.dto; import lombok.Data; -import java.util.Map; - `@Data` public class DynamicDelete {base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicInsert.java (1)
9-9: Clarify the comment.The comment says "插入/更新数据" (insert/update data), but this DTO is specifically for insert operations.
DynamicUpdatehandles updates separately. Consider updating the comment to avoid confusion:Proposed fix
- private Map<String, Object> params; // 插入/更新数据 + private Map<String, Object> params; // 插入数据base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java (3)
155-160: Inconsistent exception handling pattern.This block catches an exception and rethrows a
RuntimeException, but elsewhere in this class (e.g., lines 119, 129, 153) aServiceExceptionis thrown. Additionally,dropDynamicTablealready throwsRuntimeExceptionon failure per the summary, so catching and re-wrapping it is redundant. Consider usingServiceExceptionfor consistency or letting the original exception propagate if the transactional rollback is the desired behavior.Proposed fix
- try { - dynamicModelService.dropDynamicTable(model); - } catch (Exception e) { - log.error("刪除动态表失败", e); - throw new RuntimeException("刪除动态表失败: " + e.getMessage()); - } + dynamicModelService.dropDynamicTable(model);
131-135: Remove or address commented-out code.Line 134 contains commented-out code for
initializeDynamicTable. If this functionality is not yet needed, remove it. If it's planned for future implementation, track it via a TODO comment with a ticket reference or open an issue.
191-196: Same inconsistent exception handling as deleteModelById.Apply consistent exception handling here as suggested for the delete operation.
base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicQuery.java (1)
8-19: Add input validation annotations.This DTO lacks validation constraints that are important for security and correctness:
nameEnshould be@NotBlanksince it's requiredorderTypeshould be validated to only allow "ASC" or "DESC" to prevent SQL injection via the order clausepageSizeshould have a@Maxconstraint to prevent DoS via excessive page sizesProposed enhancement
package com.tinyengine.it.dynamic.dto; import lombok.Data; +import jakarta.validation.constraints.Max; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Pattern; import java.util.List; import java.util.Map; `@Data` public class DynamicQuery { + `@NotBlank`(message = "nameEn is required") private String nameEn; // 表名 private String nameCh; // 表中文名 private List<String> fields; // 查询字段 private Map<String, Object> params; // 查询条件 private Integer currentPage = 1; // 页码 + `@Max`(value = 1000, message = "pageSize cannot exceed 1000") private Integer pageSize = 10; // 每页大小 private String orderBy; // 排序字段 + `@Pattern`(regexp = "^(ASC|DESC)$", message = "orderType must be ASC or DESC") private String orderType = "ASC"; // 排序方式 }base/src/main/java/com/tinyengine/it/dynamic/dao/ModelDataDao.java (1)
10-12: Redundant annotation.
@Repositoryis unnecessary when@Mapperis present. MyBatis-Spring automatically registers mapper interfaces as beans.Proposed fix
-@Repository `@Mapper` public interface ModelDataDao {base/src/main/java/com/tinyengine/it/dynamic/controller/ModelDataController.java (2)
40-48: Add@Validannotation to enable DTO validation.The controller is annotated with
@Validated, but without@Validon the@RequestBodyparameter, Bean Validation constraints onDynamicQuerywon't be enforced.Proposed fix
`@PostMapping`("/queryApi") - public Result<Map<String, Object>> query(`@RequestBody` DynamicQuery dto) { + public Result<Map<String, Object>> query(`@RequestBody` `@Valid` DynamicQuery dto) {Apply the same change to
insert,update, anddeletemethods.
107-108: Consider using HTTP DELETE method for delete operation.Using
@PostMappingfor delete deviates from RESTful conventions. Consider@DeleteMappingfor semantic clarity and proper HTTP method usage.base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java (3)
4-4: Remove unused import.
com.google.gson.JsonObjectis imported but never used in this file.
88-94: System fields added after params map is built.Lines 93-94 add
created_byandupdated_bytodto.getParams(), butparams.put("data", dto.getParams())was already called on line 90. Sincedto.getParams()is a reference, this works, but the order is confusing and mutates the input DTO which is a side effect. Consider building the data map explicitly.Proposed fix
public Map<String, Object> insert(DynamicInsert dto) { validateTableExists(dto.getNameEn()); validateTableAndData(dto.getNameEn(), dto.getParams()); String tableName = getTableName(dto.getNameEn()); + + // Build data map with system fields + Map<String, Object> data = new HashMap<>(dto.getParams()); + String userId = loginUserContext.getLoginUserId(); + data.put("created_by", userId); + data.put("updated_by", userId); + Map<String, Object> params = new HashMap<>(); params.put("tableName", tableName); - params.put("data", dto.getParams()); - String userId = loginUserContext.getLoginUserId(); - // 添加系统字段 - dto.getParams().put("created_by", userId); - dto.getParams().put("updated_by", userId); + params.put("data", data); // ... }
161-180: Validation validates nameEn but actual table uses "dynamic_" prefix.
validateTableAndDatavalidates thattableNamematches^[a-zA-Z_][a-zA-Z0-9_]*$, but it's called withdto.getNameEn(), not the actual table name (dynamic_+ nameEn). This is fine for validation purposes, but the method name and parameter name are misleading. Consider renaming for clarity.base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java (5)
561-572: ValidatemodelIdbefore building SQL.While
modelIdis used internally, it should be validated to prevent SQL injection if this method ever receives untrusted input.🔧 Proposed fix
public Map<String, Object> getDataById(String modelId, Long id) { + if (modelId == null || !modelId.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) { + throw new IllegalArgumentException("Invalid model ID: " + modelId); + } String tableName = getTableName(modelId);
609-625: Consider soft delete instead of hard delete.The table schema includes a
deleted_atcolumn (line 110), suggesting soft delete was intended. However,deleteDataByIdperforms a hardDELETE. Consider using soft delete for data recovery capability.♻️ Soft delete implementation
public Map<String,Object> deleteDataById(DynamicDelete dto) { // ... validation ... String tableName = getTableName(modelId); - String sql = "DELETE FROM " + tableName + " WHERE id = ?"; + String sql = "UPDATE " + tableName + " SET deleted_at = CURRENT_TIMESTAMP WHERE id = ? AND deleted_at IS NULL"; int update = jdbcTemplate.update(sql, id);Also update query methods to exclude soft-deleted records by adding
WHERE deleted_at IS NULL.
476-496: Validation method exists but is not consistently used.This validation method is well-implemented but isn't called consistently before SQL operations (e.g.,
dynamicQuery,updateDateById, etc.). Consider calling it at the entry points of all public methods that construct dynamic SQL.
254-257: Add defensive null check for count result.While
COUNT(*)typically returns a single row, consider adding a defensive check to avoid potentialIndexOutOfBoundsException.♻️ Defensive fix
public Long count(String tableName, Map<String, Object> conditions) { List<Map<String, Object>> result = dynamicCount(tableName, conditions); - return Long.parseLong(result.get(0).get("count").toString()); + if (result == null || result.isEmpty()) { + return 0L; + } + return Long.parseLong(result.get(0).get("count").toString()); }
287-308: Limited number type handling and fragile date parsing.
"Number"(line 293) always parses asInteger, but values could beLong,BigDecimal, etc.Date.valueOf(line 297) assumes strict ISO format (yyyy-MM-dd) without explicit validation or error messaging.♻️ Improved number handling
case "Number": - return value != null ? Integer.parseInt(value.toString()) : null; + if (value == null) return null; + String numStr = value.toString(); + if (numStr.contains(".")) { + return new BigDecimal(numStr); + } + return Long.parseLong(numStr);
Comment on lines
+43
to
+47
| try { | ||
| return Result.success(dynamicService.queryWithPage(dto)); | ||
| } catch (Exception e) { | ||
| return Result.failed(e.getMessage()); | ||
| } |
There was a problem hiding this comment.
Avoid exposing raw exception messages to clients.
Returning e.getMessage() directly can leak internal implementation details (stack traces, SQL errors, etc.) to API consumers. Log the full exception server-side and return a sanitized error message.
Proposed pattern
try {
return Result.success(dynamicService.queryWithPage(dto));
} catch (Exception e) {
- return Result.failed(e.getMessage());
+ log.error("Query failed for table: {}", dto.getNameEn(), e);
+ return Result.failed("Query operation failed");
}Note: This pattern should be applied to all endpoint methods in this controller.
🤖 Prompt for AI Agents
In
`@base/src/main/java/com/tinyengine/it/dynamic/controller/ModelDataController.java`
around lines 43 - 47, The controller currently returns raw exception messages
(e.getMessage()) to clients; instead, in ModelDataController wrap each
endpoint's try/catch (e.g., the method calling
dynamicService.queryWithPage(dto)) to log the full exception server-side (using
your logger) and return a sanitized, non-sensitive message via Result.failed
(e.g., "Internal server error" or a brief contextual message). Update the catch
blocks across all endpoint methods in ModelDataController to log the exception
(including stack trace) and replace e.getMessage() with a generic error string
while preserving any error codes or flags your Result type supports.
Comment on lines
+10
to
+51
| public String select(Map<String, Object> params) { | ||
| String tableName = (String) params.get("tableName"); | ||
| List<String> fields = (List<String>) params.get("fields"); | ||
| Map<String, Object> conditions = (Map<String, Object>) params.get("conditions"); | ||
| Integer pageNum = (Integer) params.get("pageNum"); | ||
| Integer pageSize = (Integer) params.get("pageSize"); | ||
| String orderBy = (String) params.get("orderBy"); | ||
| String orderType = (String) params.get("orderType"); | ||
|
|
||
| SQL sql = new SQL(); | ||
|
|
||
| // 选择字段 | ||
| if (fields != null && !fields.isEmpty()) { | ||
| for (String field : fields) { | ||
| sql.SELECT(field); | ||
| } | ||
| } else { | ||
| sql.SELECT("*"); | ||
| } | ||
|
|
||
| sql.FROM(tableName); | ||
|
|
||
| // 条件 | ||
| if (conditions != null && !conditions.isEmpty()) { | ||
| for (Map.Entry<String, Object> entry : conditions.entrySet()) { | ||
| if (entry.getValue() != null) { | ||
| sql.WHERE(entry.getKey() + " = #{conditions." + entry.getKey() + "}"); | ||
| } | ||
| } | ||
| } | ||
| // 排序 | ||
| if (orderBy != null && !orderBy.isEmpty()) { | ||
| sql.ORDER_BY(orderBy + " " + orderType); | ||
| } | ||
|
|
||
| // 分页 | ||
| if (pageNum != null && pageSize != null) { | ||
| return sql.toString() + " LIMIT " + (pageNum - 1) * pageSize + ", " + pageSize; | ||
| } | ||
|
|
||
| return sql.toString(); | ||
| } |
There was a problem hiding this comment.
Critical: SQL injection vulnerability in select method.
Multiple inputs are directly interpolated into the SQL string without parameterization:
tableName(line 30) - directly passed tosql.FROM()fields(lines 23-24) - directly passed tosql.SELECT()orderByandorderType(line 42) - directly concatenated- Column names in WHERE clause (line 36) - directly concatenated
While DynamicService.validateTableAndData() validates some inputs, this provider can be called independently and the validation doesn't cover orderBy, orderType, or field names. An attacker could inject malicious SQL through these vectors.
Proposed mitigation
Add validation within the provider itself for defense in depth:
public String select(Map<String, Object> params) {
String tableName = (String) params.get("tableName");
+ validateIdentifier(tableName, "tableName");
List<String> fields = (List<String>) params.get("fields");
Map<String, Object> conditions = (Map<String, Object>) params.get("conditions");
Integer pageNum = (Integer) params.get("pageNum");
Integer pageSize = (Integer) params.get("pageSize");
String orderBy = (String) params.get("orderBy");
String orderType = (String) params.get("orderType");
SQL sql = new SQL();
// 选择字段
if (fields != null && !fields.isEmpty()) {
for (String field : fields) {
+ validateIdentifier(field, "field");
sql.SELECT(field);
}
} else {
sql.SELECT("*");
}
// ...
// 排序
if (orderBy != null && !orderBy.isEmpty()) {
+ validateIdentifier(orderBy, "orderBy");
+ if (!"ASC".equalsIgnoreCase(orderType) && !"DESC".equalsIgnoreCase(orderType)) {
+ throw new IllegalArgumentException("Invalid orderType: " + orderType);
+ }
sql.ORDER_BY(orderBy + " " + orderType);
}
// ...
}
+private void validateIdentifier(String identifier, String paramName) {
+ if (identifier == null || !identifier.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) {
+ throw new IllegalArgumentException("Invalid " + paramName + ": " + identifier);
+ }
+}🤖 Prompt for AI Agents
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/DynamicSqlProvider.java`
around lines 10 - 51, The select method in DynamicSqlProvider currently
concatenates untrusted inputs (tableName, fields, orderBy, orderType, and
condition keys) into SQL, enabling injection; update select to validate and
whitelist identifiers before using them: ensure tableName and each field in
fields match a safe identifier whitelist or strict regex (letters, digits,
underscore) or against allowed table/column lists, validate orderBy is a single
whitelisted column and orderType is only "ASC" or "DESC", and validate
pageNum/pageSize are non-negative integers; only after validation pass the
sanitized identifiers to SQL.SELECT, SQL.FROM and SQL.ORDER_BY, and keep
parameter binding (#{conditions.xxx}) for values rather than concatenation to
eliminate injection vectors in condition values and LIMIT.
Comment on lines
+69
to
+90
| public String update(Map<String, Object> params) { | ||
| String tableName = (String) params.get("tableName"); | ||
| Map<String, Object> data = (Map<String, Object>) params.get("data"); | ||
| Map<String, Object> conditions = (Map<String, Object>) params.get("conditions"); | ||
|
|
||
| SQL sql = new SQL(); | ||
| sql.UPDATE(tableName); | ||
|
|
||
| if (data != null && !data.isEmpty()) { | ||
| for (Map.Entry<String, Object> entry : data.entrySet()) { | ||
| sql.SET(entry.getKey() + " = #{data." + entry.getKey() + "}"); | ||
| } | ||
| } | ||
|
|
||
| if (conditions != null && !conditions.isEmpty()) { | ||
| for (Map.Entry<String, Object> entry : conditions.entrySet()) { | ||
| sql.WHERE(entry.getKey() + " = #{conditions." + entry.getKey() + "}"); | ||
| } | ||
| } | ||
|
|
||
| return sql.toString(); | ||
| } |
There was a problem hiding this comment.
SQL injection risk in update method.
Same vulnerability pattern: tableName and column names from data and conditions maps are directly interpolated. Ensure all identifiers are validated before SQL construction.
🤖 Prompt for AI Agents
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/DynamicSqlProvider.java`
around lines 69 - 90, The update method in DynamicSqlProvider currently
interpolates tableName and column names from the data and conditions maps
directly (variables: tableName, data, conditions in method update of class
DynamicSqlProvider), creating an SQL injection risk; fix by
validating/whitelisting the tableName and each column key before building the
SQL (e.g., check against an allow-list or strict identifier regex like
/^[A-Za-z_][A-Za-z0-9_]*$/) and throw an IllegalArgumentException for any
invalid identifier, or resolve identifiers via a known mapping of
logical->actual column names; keep using parameter placeholders (#{data.xxx},
#{conditions.xxx}) for values but do not concatenate unchecked identifiers into
sql.SET/sql.WHERE.
Comment on lines
+92
to
+106
| public String delete(Map<String, Object> params) { | ||
| String tableName = (String) params.get("tableName"); | ||
| Map<String, Object> conditions = (Map<String, Object>) params.get("conditions"); | ||
|
|
||
| SQL sql = new SQL(); | ||
| sql.DELETE_FROM(tableName); | ||
|
|
||
| if (conditions != null && !conditions.isEmpty()) { | ||
| for (Map.Entry<String, Object> entry : conditions.entrySet()) { | ||
| sql.WHERE(entry.getKey() + " = #{conditions." + entry.getKey() + "}"); | ||
| } | ||
| } | ||
|
|
||
| return sql.toString(); | ||
| } |
There was a problem hiding this comment.
SQL injection risk in delete method - potential for mass deletion.
The delete method has no safeguard against empty conditions. If conditions is null or empty, this generates DELETE FROM tableName without a WHERE clause, potentially deleting all rows.
Proposed safeguard
public String delete(Map<String, Object> params) {
String tableName = (String) params.get("tableName");
Map<String, Object> conditions = (Map<String, Object>) params.get("conditions");
+ if (conditions == null || conditions.isEmpty()) {
+ throw new IllegalArgumentException("Delete operation requires conditions");
+ }
+
SQL sql = new SQL();
sql.DELETE_FROM(tableName);
- if (conditions != null && !conditions.isEmpty()) {
- for (Map.Entry<String, Object> entry : conditions.entrySet()) {
- sql.WHERE(entry.getKey() + " = #{conditions." + entry.getKey() + "}");
- }
+ for (Map.Entry<String, Object> entry : conditions.entrySet()) {
+ sql.WHERE(entry.getKey() + " = #{conditions." + entry.getKey() + "}");
}
return sql.toString();
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public String delete(Map<String, Object> params) { | |
| String tableName = (String) params.get("tableName"); | |
| Map<String, Object> conditions = (Map<String, Object>) params.get("conditions"); | |
| SQL sql = new SQL(); | |
| sql.DELETE_FROM(tableName); | |
| if (conditions != null && !conditions.isEmpty()) { | |
| for (Map.Entry<String, Object> entry : conditions.entrySet()) { | |
| sql.WHERE(entry.getKey() + " = #{conditions." + entry.getKey() + "}"); | |
| } | |
| } | |
| return sql.toString(); | |
| } | |
| public String delete(Map<String, Object> params) { | |
| String tableName = (String) params.get("tableName"); | |
| Map<String, Object> conditions = (Map<String, Object>) params.get("conditions"); | |
| if (conditions == null || conditions.isEmpty()) { | |
| throw new IllegalArgumentException("Delete operation requires conditions"); | |
| } | |
| SQL sql = new SQL(); | |
| sql.DELETE_FROM(tableName); | |
| for (Map.Entry<String, Object> entry : conditions.entrySet()) { | |
| sql.WHERE(entry.getKey() + " = #{conditions." + entry.getKey() + "}"); | |
| } | |
| return sql.toString(); | |
| } |
🤖 Prompt for AI Agents
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/DynamicSqlProvider.java`
around lines 92 - 106, The delete(Map<String, Object> params) method currently
issues DELETE_FROM(tableName) even when conditions is null/empty; add a guard
that checks the conditions Map (from params.get("conditions")) and if it is null
or empty throw an IllegalArgumentException (or another explicit runtime
exception) instead of building the SQL, so no SQL is generated without a WHERE;
update the method that builds SQL (uses SQL sql = new SQL();
sql.DELETE_FROM(tableName); sql.WHERE(...)) to only perform DELETE_FROM and add
WHERE clauses after the non-empty check (or return/throw before calling
sql.DELETE_FROM) to ensure you never produce a DELETE FROM without conditions.
Comment on lines
+17
to
+19
| @InsertProvider(type = DynamicSqlProvider.class, method = "insert") | ||
| @Options(useGeneratedKeys = true, keyProperty = "id", keyColumn = "id") | ||
| Long insert(Map<String, Object> params); |
There was a problem hiding this comment.
Incorrect return type for insert method.
The @Options(useGeneratedKeys = true, keyProperty = "id") annotation populates the generated key into the params map, but the method return type is Long. MyBatis insert methods return the number of affected rows (typically 1), not the generated key. The caller in DynamicService.insert() correctly retrieves the ID from params.get("id"), but the return value insertRow is misleadingly named and typed.
Proposed fix
`@InsertProvider`(type = DynamicSqlProvider.class, method = "insert")
`@Options`(useGeneratedKeys = true, keyProperty = "id", keyColumn = "id")
- Long insert(Map<String, Object> params);
+ int insert(Map<String, Object> params);📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @InsertProvider(type = DynamicSqlProvider.class, method = "insert") | |
| @Options(useGeneratedKeys = true, keyProperty = "id", keyColumn = "id") | |
| Long insert(Map<String, Object> params); | |
| `@InsertProvider`(type = DynamicSqlProvider.class, method = "insert") | |
| `@Options`(useGeneratedKeys = true, keyProperty = "id", keyColumn = "id") | |
| int insert(Map<String, Object> params); |
🤖 Prompt for AI Agents
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/ModelDataDao.java` around
lines 17 - 19, The DAO insert method has the wrong return type: change
ModelDataDao.insert's signature from Long insert(Map<String,Object> params) to
int (or Integer) insert(Map<String,Object> params) because MyBatis returns the
number of affected rows while generated key is populated into params via
`@Options`; then update callers (e.g., DynamicService.insert) to treat the return
value as rowsAffected (rename insertRow -> rowsAffected) and continue to read
the generated id from params.get("id"); leave DynamicSqlProvider.insert and the
`@Options` annotation as-is.
| import com.tinyengine.it.dynamic.dao.ModelDataDao; | ||
| import com.tinyengine.it.dynamic.dto.*; | ||
| import com.tinyengine.it.service.material.ModelService; | ||
| import jakarta.transaction.Transactional; |
There was a problem hiding this comment.
Use Spring's @Transactional annotation.
The import uses jakarta.transaction.Transactional which is JTA-based. For Spring-managed transactions, use org.springframework.transaction.annotation.Transactional to ensure proper integration with Spring's transaction management.
Proposed fix
-import jakarta.transaction.Transactional;
+import org.springframework.transaction.annotation.Transactional;📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import jakarta.transaction.Transactional; | |
| import org.springframework.transaction.annotation.Transactional; |
🤖 Prompt for AI Agents
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java` at
line 9, The class DynamicService currently imports
jakarta.transaction.Transactional; replace that import with Spring's
org.springframework.transaction.annotation.Transactional so Spring's transaction
management is used; update any annotations or fully-qualified references in
DynamicService to use org.springframework.transaction.annotation.Transactional
(and remove the jakarta import) to ensure proper Spring-managed transactions.
Comment on lines
+51
to
+59
| public Long count(String tableName, Map<String, Object> conditions) { | ||
| Map<String, Object> params = new HashMap<>(); | ||
| params.put("tableName", tableName); | ||
| params.put("fields", Arrays.asList("COUNT(*) as count")); | ||
| params.put("conditions", conditions); | ||
|
|
||
| List<JSONObject> result = dynamicDao.select(params); | ||
| return Long.parseLong(result.get(0).get("count").toString()); | ||
| } |
There was a problem hiding this comment.
Potential NPE or IndexOutOfBoundsException in count method.
If the query returns an empty result or null, accessing result.get(0) will throw an exception.
Proposed fix
public Long count(String tableName, Map<String, Object> conditions) {
Map<String, Object> params = new HashMap<>();
params.put("tableName", tableName);
params.put("fields", Arrays.asList("COUNT(*) as count"));
params.put("conditions", conditions);
List<JSONObject> result = dynamicDao.select(params);
+ if (result == null || result.isEmpty()) {
+ return 0L;
+ }
return Long.parseLong(result.get(0).get("count").toString());
}🤖 Prompt for AI Agents
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`
around lines 51 - 59, The count method in DynamicService calls
dynamicDao.select(params) and then blindly accesses result.get(0), which can
throw NPE or IndexOutOfBounds if result is null or empty; update count(String
tableName, Map<String,Object> conditions) to check that the List<JSONObject>
returned by dynamicDao.select(params) is non-null and non-empty before accessing
index 0, returning 0 (or a safe default) when no rows are found, and defensively
parsing the "count" field (handle null or non-numeric values) to avoid
NumberFormatException.
Comment on lines
+185
to
+190
| public void validateTableExists(String tableName) { | ||
| List<String> tables = modelService.getAllModelName(); | ||
| if (!tables.contains(tableName)) { | ||
| throw new IllegalArgumentException("模型不存在: " + tableName); | ||
| } | ||
| } |
There was a problem hiding this comment.
NPE risk when validating table existence.
modelService.getAllModelName() can return null (as implemented in ModelServiceImpl), causing tables.contains(tableName) to throw NPE.
This is related to the issue flagged in ModelServiceImpl.getAllModelName(). Fixing that method to return an empty list instead of null will resolve this.
🤖 Prompt for AI Agents
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`
around lines 185 - 190, The null-check risk comes from
modelService.getAllModelName() returning null and causing validateTableExists
(method validateTableExists) to NPE when calling tables.contains(...); fix the
data source method instead by updating ModelServiceImpl.getAllModelName to never
return null—return Collections.emptyList() or a new ArrayList<>() when no models
are found (or when internal value is null)—so validateTableExists can safely
call modelService.getAllModelName().contains(tableName).
Comment on lines
128
to
130
| if (result != 1) { | ||
| throw new ServiceException(ExceptionEnum.CM001.getResultCode(), ExceptionEnum.CM001.getResultCode()); | ||
| } |
There was a problem hiding this comment.
Incorrect exception message construction.
Line 129 passes getResultCode() for both the code and message parameters. This appears to be a copy-paste error.
Proposed fix
if (result != 1) {
- throw new ServiceException(ExceptionEnum.CM001.getResultCode(), ExceptionEnum.CM001.getResultCode());
+ throw new ServiceException(ExceptionEnum.CM001.getResultCode(), ExceptionEnum.CM001.getResultMsg());
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (result != 1) { | |
| throw new ServiceException(ExceptionEnum.CM001.getResultCode(), ExceptionEnum.CM001.getResultCode()); | |
| } | |
| if (result != 1) { | |
| throw new ServiceException(ExceptionEnum.CM001.getResultCode(), ExceptionEnum.CM001.getResultMsg()); | |
| } |
🤖 Prompt for AI Agents
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java`
around lines 128 - 130, The exception construction in ModelServiceImpl uses
ExceptionEnum.CM001.getResultCode() for both code and message; change the second
argument to ExceptionEnum.CM001.getResultMsg() so the thrown ServiceException
carries the proper human-readable message (locate the throw in ModelServiceImpl
where the if (result != 1) block constructs the ServiceException).
Comment on lines
+253
to
+261
| public List<String> getAllModelName() { | ||
| List<Model> modelList = this.baseMapper.selectList(null); | ||
| if (!CollectionUtils.isEmpty(modelList)) { | ||
| return modelList.stream() | ||
| .map(Model::getNameEn) | ||
| .collect(Collectors.toList()); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Return empty list instead of null to avoid NPE in callers.
Returning null when no models exist forces callers to perform null checks. The validateTableExists method in DynamicService calls contains() on this result without null checking, which would throw NPE.
Proposed fix
`@Override`
public List<String> getAllModelName() {
List<Model> modelList = this.baseMapper.selectList(null);
if (!CollectionUtils.isEmpty(modelList)) {
return modelList.stream()
.map(Model::getNameEn)
.collect(Collectors.toList());
}
- return null;
+ return Collections.emptyList();
}🤖 Prompt for AI Agents
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java`
around lines 253 - 261, The getAllModelName method in ModelServiceImpl should
return an empty list instead of null; modify getAllModelName so that when
modelList is empty or null it returns Collections.emptyList() (or new
ArrayList<>()) and otherwise returns the mapped list
(modelList.stream().map(Model::getNameEn)...); ensure the method never returns
null so callers like DynamicService.validateTableExists can safely call
contains() without NPE.
msslulu
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
English | 简体中文
PR
模型驱动
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.