feat: query result mapping to typed DTOs - #37
Open
sant1ago-da-hanoi wants to merge 3 commits into
Open
Conversation
- Add ResultMapper with record + POJO support, type coercion - Add @column annotation for explicit column name mapping - Add TypeCoercer (numbers, temporals, UUID, enum, boolean) - Add TypeMetadata with Jackson SnakeCaseStrategy for camelCase->snake_case - Genericize Page<T> (breaking: consumers need Page<Map<String, Object>>) - Add typed overloads to ReadService: findAll/findOne/findPage with Class<T> - Add @builder to JdbcReadService - Wire ResultMapper bean in DbAutoConfiguration - 432 tests green, 95%+ coverage on mapping package
- Remove dead Number branch in coerceBoolean (unreachable via public API) - Make coerceNumber package-private for direct testing - Add wrapper type tests (Integer.class, Long.class, etc.) - Add Timestamp/Instant toLong with Long.class branch - Add synthetic field test (Java 21: no longer generates synthetic fields) - Add record instantiation failure test - Add POJO MappingException propagation test - TypeCoercer: 100% instruction + 100% branch - TypeMetadata: 95%/90% (remaining: unreachable in Java 21) - ResultMapper: 95%/100% (remaining: unreachable defensive catch)
sant1ago-da-hanoi
left a comment
Contributor
Author
There was a problem hiding this comment.
Code Review
Files changed: 14 | Lines: +1617 / -27
Summary
Thêm ResultMapper để map Map<String, Object> sang typed records/POJOs. Genericize Page<T>, thêm typed overloads cho ReadService. Code chất lượng tốt, test coverage cao, architecture hợp lý. Có vài vấn đề performance và hardcode timezone cần fix.
Findings
Phải sửa
| # | File | Category | Issue | Suggestion |
|---|---|---|---|---|
| 1 | core/src/main/java/dev/suprim/query/mapping/ResultMapper.java:241-244 |
Code Quality | resolveValue() dùng linear scan O(n) cho case-insensitive fallback. Mỗi row map N fields × M columns = O(N×M) per row. Với result set lớn (1000+ rows, 20+ columns) → performance hit đáng kể. |
Build một Map<String, String> (lowercased key → original key) một lần khi bắt đầu map batch, hoặc cache normalized keys trong TypeMetadata. Ít nhất thêm code comment giải thích trade-off. |
| 2 | core/src/main/java/dev/suprim/query/mapping/TypeCoercer.java:419,452-453,469-470 |
Code Quality | Hardcode ZoneOffset.UTC ở mọi temporal conversion. Consumer ở timezone khác (VN = +07:00) sẽ bị sai data khi convert Instant ↔ LocalDateTime. Không có cách override. |
Inject ZoneId vào TypeCoercer (hoặc qua ResultMapper constructor). Default UTC nhưng cho phép consumer override. Thêm code comment giải thích tại sao default UTC. |
| 3 | core/src/main/java/dev/suprim/query/mapping/ResultMapper.java:199,220 |
Code Quality | constructor.setAccessible(true) gọi mỗi lần map row. Với 1000 rows = 1000 lần gọi redundant (constructor đã accessible từ lần đầu, và metadata được cache). |
Move setAccessible(true) vào TypeMetadata.resolve() — gọi 1 lần duy nhất khi resolve metadata. Tương tự cho field.setAccessible(true). |
Nên sửa
| # | File | Category | Issue | Suggestion |
|---|---|---|---|---|
| 4 | jdbc/src/main/java/dev/suprim/query/jdbc/executor/read/ReadService.java:17-21 |
Standards | Typed overloads thiếu Javadoc. Đây là public API interface — consumer cần biết behavior khi row null, khi type không có no-arg constructor, etc. | Thêm Javadoc cho 3 method mới. |
| 5 | core/src/main/java/dev/suprim/query/mapping/TypeCoercer.java:326-329 |
Code Quality | java.sql.Date chỉ handle LocalDate target rồi fall-through im lặng. Nếu target là LocalDateTime hay Instant → throw generic error message khó debug. |
Thêm explicit handling hoặc throw rõ ràng: "Cannot coerce java.sql.Date to X, convert to Timestamp first". |
| 6 | jdbc/src/main/java/dev/suprim/query/jdbc/executor/read/JdbcReadService.java:172-177 |
Code Quality | findAll(ctx, type) gọi findAll(ctx) rồi map toàn bộ list. Nếu query trả 10k rows, allocate 2 lists (raw + mapped). Không có streaming option. |
Chấp nhận cho v1, nhưng thêm code comment: // TODO: consider streaming for large result sets hoặc note trong Javadoc. |
Nhỏ
| # | File | Category | Issue | Suggestion |
|---|---|---|---|---|
| 7 | core/src/main/java/dev/suprim/query/mapping/TypeMetadata.java:539 |
Standards | Import tools.jackson.databind.PropertyNamingStrategies — hợp lệ với Jackson 3.x, nhưng thêm code comment giải thích tại sao depend vào Jackson chỉ cho snake_case conversion. |
Thêm comment: // Reuse Jackson's battle-tested snake_case logic instead of rolling our own |
| 8 | core/src/main/java/dev/suprim/query/mapping/TypeCoercer.java:277 |
Standards | coerceNumber là package-private static cho testability — thiếu comment giải thích visibility choice. |
Thêm // package-private for direct unit testing |
Positive Patterns
ConcurrentHashMapcache choTypeMetadata— thread-safe, lazy init, đúng pattern.List.copyOf(data)trongfindPage— defensive copy tốt.- Test coverage rất kỹ: edge cases, error paths, coercion boundaries. 884 lines test cho 600 lines code.
- Breaking change (
Page<T>) được document rõ trong PR description + migration path.
Đánh giá: Chưa sẵn sàng merge
3 phải sửa (performance O(N×M), hardcode UTC, redundant setAccessible). Fix xong 3 cái đó rồi merge.
- perf(ResultMapper): replace O(N×M) linear scan with O(1) normalized key map - feat(TypeCoercer): inject ZoneId, default UTC, allow consumer override - perf(TypeMetadata): move setAccessible(true) into resolve() — call once - docs(ReadService): add Javadoc to typed overloads - fix(TypeCoercer): explicit java.sql.Date handling for non-LocalDate targets - docs(JdbcReadService): add TODO comment for streaming large results - docs(TypeMetadata): comment explaining Jackson snake_case dependency - style(TypeCoercer): add visibility comment on coerceNumber
sant1ago-da-hanoi
left a comment
Contributor
Author
There was a problem hiding this comment.
Re-review sau commit a6dccec
Tất cả 8 findings đã fix:
- ✅
resolveValue()→buildNormalizedKeyMap()O(1) lookup - ✅
ZoneIdinjectable, default UTC - ✅
setAccessible(true)move vàoTypeMetadata.resolve() - ✅ Javadoc đầy đủ trên
ReadServicetyped overloads - ✅
coerceSqlDate()explicit handling + clear error message - ✅ TODO comment cho streaming
- ✅ Jackson dependency comment
- ✅ Visibility comment trên
coerceNumber
CI green. Không có finding mới. OK merge.
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
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.
Summary
Closes #7. Maps
Map<String, Object>query results to typed Java records/POJOs automatically.Changes
Core module —
dev.suprim.query.mapping@Column— annotation for explicit column name mappingResultMapper— facade:mapOne,mapOptional,mapListTypeCoercer— type coercion (numbers, temporals, UUID, enum, boolean, BigDecimal/BigInteger)TypeMetadata— reflection metadata with caching, uses JacksonSnakeCaseStrategyfor camelCase→snake_caseComponentMapping— field/component descriptor recordMappingException— runtime exception for mapping failuresPage<T>— genericized (breaking: wasPagewith hardcodedList<Map<String, Object>>)JDBC module
ReadService— added typed overloads:<T> List<T> findAll(ctx, Class<T>),<T> Optional<T> findOne(ctx, Class<T>),<T> Page<T> findPage(ctx, Class<T>)JdbcReadService— implements typed methods viaResultMapper, added@BuilderSpring Boot Starter
DbAutoConfiguration— wiresResultMapperbean, uses builder forReadServiceConsumer API
Breaking Changes
Page→Page<T>: existing code using rawPageneedsPage<Map<String, Object>>JdbcReadServiceconstructor has newResultMapperparameter (use builder)Tests
ResultMapperTestcovering records, POJOs, all coercion paths, error cases