修复 AggregationProvider 的问题 #743
Conversation
There was a problem hiding this comment.
Pull request overview
该 PR 旨在修复聚合查询在“数据库列名与实体属性名不一致”场景下的映射问题：selectAggregationByExample 返回的 List<T> 中，分组字段无法正确回填到实体属性。
Changes:
- 在
AggregationProvider的 SELECT 子句中，将聚合字段与 group-by 字段的别名从“列名”调整为“属性名”，以匹配 MyBatis 对实体属性的映射。 - 调整测试表结构与测试用例，使分组字段使用下划线列名（
ro_le）来覆盖该场景。 - 在
extra模块 POM 中新增argLine（-Dfile.encoding=UTF-8）。
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| extra/src/main/java/tk/mybatis/mapper/additional/aggregation/AggregationProvider.java | 将 SELECT 别名改为属性名以修复列名/属性名不一致时的结果映射问题 |
| extra/src/test/java/tk/mybatis/mapper/additional/aggregation/User.java | 调整测试实体字段命名以制造列名与属性名差异 |
| extra/src/test/java/tk/mybatis/mapper/additional/aggregation/CreateDB.sql | 将列从 role 改为 ro_le 并更新初始化数据 |
| extra/src/test/java/tk/mybatis/mapper/additional/aggregation/AggregationMapperTest.java | 更新 groupBy / orderBy 以匹配新列名与属性名 |
| extra/pom.xml | 增加测试 JVM 的 argLine 编码参数 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| aggregateBy("id").aliasName("aggregation").aggregateType(AggregateType.MAX).groupBy("roLe"); | ||
| Example example = new Example(User.class); | ||
| example.setOrderByClause("role desc"); | ||
| example.setOrderByClause("ro_le desc"); |
There was a problem hiding this comment.
In testMax, ordering is now done by the physical column name (ro_le). For consistency and readability, consider using the Example API that orders by property name (if available in this project) so callers don’t need to know the DB column naming; otherwise, add a brief comment explaining why raw column names are required here.
| example.setOrderByClause("ro_le desc"); | |
| example.orderBy("roLe").desc(); |
| @@ -57,12 +57,12 @@ public void setName(String name) { | |||
| this.name = name; | |||
| } | |||
|
|
|||
| public String getRole() { | |||
| return role; | |||
| public String getRoLe() { | |||
| return roLe; | |||
| } | |||
|
|
|||
| public void setRole(String role) { | |||
| this.role = role; | |||
| public void setRoLe(String roLe) { | |||
| this.roLe = roLe; | |||
There was a problem hiding this comment.
roLe is an unusual/non-camelCase Java property name and makes the test harder to understand. To model a column/property name mismatch more realistically, keep the Java field as role with standard getRole/setRole and map it to ro_le via @Column(name = "ro_le") (or the project’s column-mapping annotation), then adjust the test strings accordingly.
| AggregateCondition aggregateCondition = AggregateCondition.builder(). | ||
| aggregateBy("id").aliasName("total").aggregateType(AggregateType.COUNT).groupBy("role"); | ||
| aggregateBy("id").aliasName("total").aggregateType(AggregateType.COUNT).groupBy("roLe"); | ||
| Example example = new Example(User.class); | ||
| List<User> m = mapper.selectAggregationByExample(example, aggregateCondition); | ||
| Assert.assertEquals(2, m.size()); |
There was a problem hiding this comment.
The aggregation/group-by tests currently only assert the number of rows returned. This would still pass even if the grouped column fails to map onto the entity (the original bug). Add assertions that the grouped property (e.g., roLe/role) is populated with the expected values for each returned row, and (for COUNT) consider aliasing into an existing property so the aggregate value can be asserted too.
| for (String property : condition.getGroupByProperties()) { | ||
| selectBuilder.append(", "); | ||
| columnName = propertyMap.get(property).getColumn(); | ||
| selectBuilder.append(columnName).append(" AS ").append(wrapKeyword(wrapKeyword, columnName)); | ||
| selectBuilder.append(columnName).append(" AS ").append(wrapKeyword(wrapKeyword, property)); |
There was a problem hiding this comment.
aggregationSelectClause assumes every groupByProperties entry exists in the entity propertyMap; if a caller passes an invalid property name, this will throw a NullPointerException from propertyMap.get(property).getColumn() with little context. Consider adding an explicit validation/assertion that the property exists (and likewise for aggregateProperty) and fail with a clear message naming the invalid property.
| <properties> | ||
| <jdk.version>1.8</jdk.version> | ||
| <mapper-module.version>1.1.5</mapper-module.version> | ||
| <argLine>-Dfile.encoding=UTF-8</argLine> |
There was a problem hiding this comment.
The new <argLine> property is added only in the extra module and is not referenced anywhere in this repository’s POMs, making it hard to justify and potentially a no-op. If the goal is consistent UTF-8 behavior, prefer Maven’s standard encoding properties (e.g., project.build.sourceEncoding / project.reporting.outputEncoding) or a targeted Surefire config, and keep indentation consistent with the rest of the POM.
| <argLine>-Dfile.encoding=UTF-8</argLine> | |
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |
| <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding> |
|
参考当前pr重新实现了，使用了原有的提交者信息。 |
2 participants
在列名与entity属性不同名的情况下，AggregationMapper.selectAggregationByExample 返回的 List 对象不正确。