Conversation
Move trend query result models into application to remove infrastructure type leakage. Improve input validation and return a 404 when a trend detail is missing.
📝 WalkthroughWalkthroughRefactors the trend module to use new application-level domain models and validated request DTOs. Ports, adapter, usecase, controller, and response DTOs were updated to the new types; error codes and validation handlers were adjusted accordingly. Some legacy infrastructure DTOs were removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendRow.java (1)
14-14:⚠️ Potential issue | 🟠 MajorRemove
TrendRowandTrendWithUrlsentirely instead of adding this constructor.
TrendAdapterhas fully migrated toTrendItem(application layer) for all instantiations, andTrendRowis completely unused in the codebase.TrendWithUrlsalso has no usages. Adding a second constructor to a dead infrastructure DTO only defers necessary cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendRow.java` at line 14, TrendRow and TrendWithUrls are dead infrastructure DTOs; remove both classes entirely (delete the TrendRow class including its extra constructor and delete the TrendWithUrls class), then remove any imports/usages or references to TrendRow/TrendWithUrls (search for TrendRow, TrendWithUrls, and the constructor call patterns) and ensure TrendAdapter and all callers use TrendItem only; finally run the build/tests and remove any leftover mapping/tests that referenced these DTOs.src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendAdapter.java (1)
214-249:⚠️ Potential issue | 🟠 MajorEscape SQL wildcards before building the search pattern.
%and_in user input are treated asLIKEwildcards here. Inputs like%can therefore match almost the whole table and turn this endpoint into a very broad scan.🛠️ One way to treat the keyword literally
- WHERE keyword LIKE :keyword + WHERE keyword LIKE :keyword ESCAPE '\\'- WHERE title LIKE :keyword + WHERE title LIKE :keyword ESCAPE '\\'+ String escapedKeyword = normalized + .replace("\\", "\\\\") + .replace("%", "\\%") + .replace("_", "\\_"); + MapSqlParameterSource params = new MapSqlParameterSource() - .addValue("keyword", "%" + normalized + "%") + .addValue("keyword", "%" + escapedKeyword + "%") .addValue("limit",50);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendAdapter.java` around lines 214 - 249, The SQL uses LIKE with user input (variables used in keywordSql and urlSql) so escape SQL wildcard characters in the normalized input before building the pattern and add an explicit ESCAPE clause to the queries; e.g., transform normalized by replacing backslash, percent and underscore with escaped forms (backslash -> double-backslash, % -> \%, _ -> \_) and then set the parameter in MapSqlParameterSource as "%" + escapedNormalized + "%" and update both keywordSql and urlSql to use LIKE :keyword ESCAPE '\\' so literal %/_ in user input won’t act as wildcards.
🧹 Nitpick comments (5)
src/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/TrendUrlResponse.java (1)
10-15: Renamerowto match the new abstraction.Line 10 now maps an application model, not an infrastructure row. Renaming the parameter to
trendUrlwould make the boundary clearer after this refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/TrendUrlResponse.java` around lines 10 - 15, Rename the parameter "row" in the static factory method TrendUrlResponse.from to a name reflecting the application model (e.g., "trendUrl"): update the method signature and all uses inside the method to accept and call trendUrl.title(), trendUrl.url(), and trendUrl.viewCount() so the parameter name matches the new abstraction in TrendUrlResponse.from.src/main/java/tave/crezipsa/crezipsa/global/exception/handler/GlobalExceptionHandler.java (1)
41-54: Extract the shared validation-message formatter.
handleValidationExceptionsandhandleBindExceptionnow duplicate the same mapping logic. A small private helper would keep both paths aligned the next time the response format changes.♻️ Small extraction
public ResponseEntity<GlobalResponseDto<Void>> handleValidationExceptions(MethodArgumentNotValidException ex) { - String errorMessage = ex.getBindingResult().getFieldErrors().stream() - .map(err -> err.getDefaultMessage()) - .collect(Collectors.joining(", ")); - return ResponseEntity.badRequest().body(GlobalResponseDto.error(errorMessage)); + return ResponseEntity.badRequest().body(GlobalResponseDto.error(extractErrorMessage(ex.getBindingResult()))); } `@ExceptionHandler`(BindException.class) public ResponseEntity<GlobalResponseDto<Void>> handleBindException(BindException ex) { - String errorMessage = ex.getBindingResult().getFieldErrors().stream() - .map(err -> err.getDefaultMessage()) - .collect(Collectors.joining(", ")); - return ResponseEntity.badRequest().body(GlobalResponseDto.error(errorMessage)); + return ResponseEntity.badRequest().body(GlobalResponseDto.error(extractErrorMessage(ex.getBindingResult()))); } + + private String extractErrorMessage(BindingResult bindingResult) { + return bindingResult.getFieldErrors().stream() + .map(err -> err.getDefaultMessage()) + .collect(Collectors.joining(", ")); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/global/exception/handler/GlobalExceptionHandler.java` around lines 41 - 54, Extract the duplicated mapping/joining logic in GlobalExceptionHandler into a single private helper (e.g., private String formatValidationMessages(BindingResult bindingResult) or formatValidationMessages(MethodArgumentNotValidException|BindException) that accepts a BindingResult), implement the stream -> map(err->err.getDefaultMessage())->collect(joining(", ")) there, and update both handleValidationExceptions(MethodArgumentNotValidException) and handleBindException(BindException) to call that helper and return ResponseEntity.badRequest().body(GlobalResponseDto.error(...)) so both handlers share the same formatter.src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecase.java (1)
3-4: Replace the wildcard response import.
TrendUsecaseis part of the public application contract, sodto.response.*makes the surface harder to audit. Please import the response DTOs explicitly here.As per coding guidelines, "Avoid wildcard imports (
import *); explicitly import each class individually".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecase.java` around lines 3 - 4, Replace the wildcard response import in TrendUsecase: remove "import tave.crezipsa.crezipsa.application.trend.dto.response.*" and add explicit imports for each response DTO referenced by TrendUsecase (i.e., import each concrete response class used by methods in TrendUsecase). Keep the existing explicit import for TrendSaveRequest and ensure all referenced response DTO class names are individually imported so no wildcard imports remain.src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecaseImpl.java (1)
7-8: Replace the wildcard response import.This file still uses
dto.response.*. Please switch to explicit DTO imports so the service dependencies stay obvious.As per coding guidelines, "Avoid wildcard imports (
import *); explicitly import each class individually".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecaseImpl.java` around lines 7 - 8, The file TrendUsecaseImpl currently uses a wildcard import for tave.crezipsa.crezipsa.application.trend.dto.response.*; replace that wildcard with explicit imports for each response DTO actually referenced in the TrendUsecaseImpl class (i.e., inspect method signatures, fields and return types and add individual imports for those DTO classes from the dto.response package), remove the wildcard import, and keep the existing TrendItem import as-is; use your IDE’s organize-imports or manually add the exact DTO class names used in TrendUsecaseImpl so dependencies are explicit.src/main/java/tave/crezipsa/crezipsa/application/trend/port/TrendQueryPort.java (1)
15-15: Split writes out ofTrendQueryPort.
saveTrend(...)makes this interface mixed read/write again. Extracting a dedicated command port would keep the boundary clearer after this refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/application/trend/port/TrendQueryPort.java` at line 15, TrendQueryPort currently mixes reads and writes because it declares saveTrend(long userId, TrendCommand trendCommand); extract that write into a new TrendCommandPort interface: declare saveTrend(...) there, remove the method from TrendQueryPort, and update all callers/implementations to depend on/implement TrendCommandPort for mutations while leaving TrendQueryPort as read-only; search for implementations and injection sites referencing saveTrend and switch them to the new TrendCommandPort (e.g., adapters, services, constructors) so read/write boundaries remain clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/tave/crezipsa/crezipsa/application/trend/dto/request/TrendSearchRequest.java`:
- Around line 12-13: The `@Size`(max = 20, message = "검색어는 20자 이하만 가능합니다.") in
TrendSearchRequest conflicts with ErrorCode.SEARCH_KEYWORD_TOO_LONG which states
30; align them by updating the `@Size` annotation in TrendSearchRequest to use max
= 30 and adjust the validation message to "검색어는 30자 이하만 가능합니다." (or
alternatively update ErrorCode.SEARCH_KEYWORD_TOO_LONG to mention 20 if you
intend the new limit), ensuring both the annotation on class TrendSearchRequest
and the ErrorCode constant describe the same limit.
- Around line 8-18: TrendSearchRequest's trend field cannot be bound by Spring's
`@ModelAttribute` because the class only has a no-args constructor and no setter,
causing `@Valid/`@NotBlank to always fail; fix by either adding a setter for the
trend field (e.g., add `@Setter` or an explicit setTrend method on
TrendSearchRequest) so Spring can populate the field, or remove
`@NoArgsConstructor` and use constructor binding by keeping the existing String
constructor so Spring can bind via that constructor.
In
`@src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecaseImpl.java`:
- Around line 51-61: The searchTrend method in TrendUsecaseImpl is marked
`@Transactional`(readOnly = true) but calls userHistoryPort.saveHistory(...) which
performs a write; change the transaction behavior so writes are not executed in
a read-only transaction: either remove readOnly (use `@Transactional`) on
searchTrend, or better extract the write into a separate method (e.g.,
saveUserHistory) annotated `@Transactional` (no readOnly) and call that from
searchTrend; ensure the read query still runs in a read-only transaction
(trendQueryPort.findKeywordByKeyword) and the save is executed in a
non-read-only transactional context.
In `@src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendAdapter.java`:
- Around line 87-114: The code is hardcoding rank and platform when mapping DB
rows to TrendItem; update the SQL in TrendAdapter.findTopKeywordsByCategory to
select the actual rank and platform columns (e.g., include category_rank and
platform from analytics_keyword_virality) and map them into the TrendItem
constructor (use rs.getInt("category_rank") or rs.getLong if appropriate for
rank, and rs.getString("platform") for platform) so
TrendResponse.from(TrendItem) exposes real values; keep using the existing
lambda mapper and parameter names (params.addValue("category", category),
"limit") when making these changes.
In
`@src/main/java/tave/crezipsa/crezipsa/presentation/trend/controller/TrendController.java`:
- Around line 51-53: The searchTrend method in TrendController currently accepts
a DTO via `@ModelAttribute` (`@ModelAttribute` `@Valid` TrendSearchRequest
trendSearchRequest) for a single query parameter; replace this with a simple
`@RequestParam` to match other endpoints: change the signature to
searchTrend(`@AuthenticationPrincipal` User user, `@RequestParam`("trend") String
trend) and pass trend into trendUsecase.searchTrend(user.getUserId(), trend);
remove or stop using TrendSearchRequest in this controller (and its import) if
no longer needed and ensure any validation or defaulting is handled via
`@RequestParam` attributes.
---
Outside diff comments:
In `@src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendAdapter.java`:
- Around line 214-249: The SQL uses LIKE with user input (variables used in
keywordSql and urlSql) so escape SQL wildcard characters in the normalized input
before building the pattern and add an explicit ESCAPE clause to the queries;
e.g., transform normalized by replacing backslash, percent and underscore with
escaped forms (backslash -> double-backslash, % -> \%, _ -> \_) and then set the
parameter in MapSqlParameterSource as "%" + escapedNormalized + "%" and update
both keywordSql and urlSql to use LIKE :keyword ESCAPE '\\' so literal %/_ in
user input won’t act as wildcards.
In `@src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendRow.java`:
- Line 14: TrendRow and TrendWithUrls are dead infrastructure DTOs; remove both
classes entirely (delete the TrendRow class including its extra constructor and
delete the TrendWithUrls class), then remove any imports/usages or references to
TrendRow/TrendWithUrls (search for TrendRow, TrendWithUrls, and the constructor
call patterns) and ensure TrendAdapter and all callers use TrendItem only;
finally run the build/tests and remove any leftover mapping/tests that
referenced these DTOs.
---
Nitpick comments:
In
`@src/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/TrendUrlResponse.java`:
- Around line 10-15: Rename the parameter "row" in the static factory method
TrendUrlResponse.from to a name reflecting the application model (e.g.,
"trendUrl"): update the method signature and all uses inside the method to
accept and call trendUrl.title(), trendUrl.url(), and trendUrl.viewCount() so
the parameter name matches the new abstraction in TrendUrlResponse.from.
In
`@src/main/java/tave/crezipsa/crezipsa/application/trend/port/TrendQueryPort.java`:
- Line 15: TrendQueryPort currently mixes reads and writes because it declares
saveTrend(long userId, TrendCommand trendCommand); extract that write into a new
TrendCommandPort interface: declare saveTrend(...) there, remove the method from
TrendQueryPort, and update all callers/implementations to depend on/implement
TrendCommandPort for mutations while leaving TrendQueryPort as read-only; search
for implementations and injection sites referencing saveTrend and switch them to
the new TrendCommandPort (e.g., adapters, services, constructors) so read/write
boundaries remain clear.
In
`@src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecase.java`:
- Around line 3-4: Replace the wildcard response import in TrendUsecase: remove
"import tave.crezipsa.crezipsa.application.trend.dto.response.*" and add
explicit imports for each response DTO referenced by TrendUsecase (i.e., import
each concrete response class used by methods in TrendUsecase). Keep the existing
explicit import for TrendSaveRequest and ensure all referenced response DTO
class names are individually imported so no wildcard imports remain.
In
`@src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecaseImpl.java`:
- Around line 7-8: The file TrendUsecaseImpl currently uses a wildcard import
for tave.crezipsa.crezipsa.application.trend.dto.response.*; replace that
wildcard with explicit imports for each response DTO actually referenced in the
TrendUsecaseImpl class (i.e., inspect method signatures, fields and return types
and add individual imports for those DTO classes from the dto.response package),
remove the wildcard import, and keep the existing TrendItem import as-is; use
your IDE’s organize-imports or manually add the exact DTO class names used in
TrendUsecaseImpl so dependencies are explicit.
In
`@src/main/java/tave/crezipsa/crezipsa/global/exception/handler/GlobalExceptionHandler.java`:
- Around line 41-54: Extract the duplicated mapping/joining logic in
GlobalExceptionHandler into a single private helper (e.g., private String
formatValidationMessages(BindingResult bindingResult) or
formatValidationMessages(MethodArgumentNotValidException|BindException) that
accepts a BindingResult), implement the stream ->
map(err->err.getDefaultMessage())->collect(joining(", ")) there, and update both
handleValidationExceptions(MethodArgumentNotValidException) and
handleBindException(BindException) to call that helper and return
ResponseEntity.badRequest().body(GlobalResponseDto.error(...)) so both handlers
share the same formatter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea32b434-c5d4-4566-a3b2-f43223a221df
📒 Files selected for processing (21)
src/main/java/tave/crezipsa/crezipsa/application/trend/dto/request/TrendSaveRequest.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/dto/request/TrendSearchRequest.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/TrendDetailResponse.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/TrendResponse.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/TrendSearchResponse.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/TrendUrlResponse.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/request/TrendRequest.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/model/TrendDetail.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/model/TrendDetailResult.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/model/TrendItem.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/model/TrendSearchResult.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/model/TrendUrl.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/port/TrendQueryPort.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecase.javasrc/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecaseImpl.javasrc/main/java/tave/crezipsa/crezipsa/domain/trend/entity/command/TrendCommand.javasrc/main/java/tave/crezipsa/crezipsa/global/exception/code/ErrorCode.javasrc/main/java/tave/crezipsa/crezipsa/global/exception/handler/GlobalExceptionHandler.javasrc/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendAdapter.javasrc/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendRow.javasrc/main/java/tave/crezipsa/crezipsa/presentation/trend/controller/TrendController.java
💤 Files with no reviewable changes (1)
- src/main/java/tave/crezipsa/crezipsa/application/trend/dto/response/request/TrendRequest.java
Comment on lines
+51
to
+61
| @Transactional(readOnly = true) | ||
| public TrendSearchResponse searchTrend(long userId, String keyword) { | ||
| if (keyword == null) { | ||
| throw new CommonException(ErrorCode.SEARCH_KEYWORD_REQUIRED); | ||
| } | ||
| String normalized = keyword.trim(); | ||
| if (normalized.isBlank()) { | ||
| throw new CommonException(ErrorCode.SEARCH_KEYWORD_REQUIRED); | ||
| } | ||
| userHistoryPort.saveHistory(userId, normalized); | ||
| return TrendSearchResponse.from(trendQueryPort.findKeywordByKeyword(normalized)); |
There was a problem hiding this comment.
Don't persist history inside a read-only transaction.
Line 60 performs a write via userHistoryPort.saveHistory(...), but the method is marked @Transactional(readOnly = true). That makes history persistence unreliable on JPA/read-replica setups and can silently skip or misroute the write.
Suggested fix
`@Override`
- `@Transactional`(readOnly = true)
+ `@Transactional`
public TrendSearchResponse searchTrend(long userId, String keyword) {
if (keyword == null) {
throw new CommonException(ErrorCode.SEARCH_KEYWORD_REQUIRED);
}As per coding guidelines, "Use @Transactional on usecases; @Transactional(readOnly = true) for query-only operations".
📝 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
| @Transactional(readOnly = true) | |
| public TrendSearchResponse searchTrend(long userId, String keyword) { | |
| if (keyword == null) { | |
| throw new CommonException(ErrorCode.SEARCH_KEYWORD_REQUIRED); | |
| } | |
| String normalized = keyword.trim(); | |
| if (normalized.isBlank()) { | |
| throw new CommonException(ErrorCode.SEARCH_KEYWORD_REQUIRED); | |
| } | |
| userHistoryPort.saveHistory(userId, normalized); | |
| return TrendSearchResponse.from(trendQueryPort.findKeywordByKeyword(normalized)); | |
| `@Transactional` | |
| public TrendSearchResponse searchTrend(long userId, String keyword) { | |
| if (keyword == null) { | |
| throw new CommonException(ErrorCode.SEARCH_KEYWORD_REQUIRED); | |
| } | |
| String normalized = keyword.trim(); | |
| if (normalized.isBlank()) { | |
| throw new CommonException(ErrorCode.SEARCH_KEYWORD_REQUIRED); | |
| } | |
| userHistoryPort.saveHistory(userId, normalized); | |
| return TrendSearchResponse.from(trendQueryPort.findKeywordByKeyword(normalized)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/tave/crezipsa/crezipsa/application/trend/usecase/TrendUsecaseImpl.java`
around lines 51 - 61, The searchTrend method in TrendUsecaseImpl is marked
`@Transactional`(readOnly = true) but calls userHistoryPort.saveHistory(...) which
performs a write; change the transaction behavior so writes are not executed in
a read-only transaction: either remove readOnly (use `@Transactional`) on
searchTrend, or better extract the write into a separate method (e.g.,
saveUserHistory) annotated `@Transactional` (no readOnly) and call that from
searchTrend; ensure the read query still runs in a read-only transaction
(trendQueryPort.findKeywordByKeyword) and the save is executed in a
non-read-only transactional context.
Comment on lines
+87
to
+114
| String sql = """ | ||
| SELECT id, keyword,category_name | ||
| FROM analytics_keyword_virality | ||
| WHERE category_name = :category | ||
| ORDER BY category_rank ASC | ||
| LIMIT :limit | ||
| """; | ||
|
|
||
| List<TrendRow> result = new ArrayList<>(); | ||
| List<TrendItem> result = new ArrayList<>(); | ||
|
|
||
| for (String category : categories) { | ||
| MapSqlParameterSource params = new MapSqlParameterSource() | ||
| .addValue("category", category) | ||
| .addValue("limit", 10); | ||
| for (String category : categories) { | ||
| MapSqlParameterSource params = new MapSqlParameterSource() | ||
| .addValue("category", category) | ||
| .addValue("limit", 10); | ||
|
|
||
| List<TrendRow> rows = analyticsJdbc.query(sql, params, (rs, rowNum) -> new TrendRow( | ||
| rs.getLong("id"), | ||
| rs.getString("keyword"), | ||
| rs.getString("category_name") | ||
| )); | ||
| List<TrendItem> rows = analyticsJdbc.query(sql, params, (rs, rowNum) -> new TrendItem( | ||
| rs.getLong("id"), | ||
| 0, | ||
| rs.getString("keyword"), | ||
| null, | ||
| rs.getString("category_name"), | ||
| null | ||
| )); | ||
|
|
||
| result.addAll(rows); | ||
| } | ||
| result.addAll(rows); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
TREND_RESPONSE_FILE=$(fd 'TrendResponse.java$' src/main/java | head -n 1)
TREND_USECASE_FILE=$(fd 'TrendUsecaseImpl.java$' src/main/java | head -n 1)
[ -n "$TREND_RESPONSE_FILE" ] && sed -n '1,220p' "$TREND_RESPONSE_FILE"
printf '\n--- TrendUsecaseImpl ---\n'
[ -n "$TREND_USECASE_FILE" ] && rg -n -C3 'recommendTrendsByInterests\s*\(|findTopKeywordsByCategory\s*\(|TrendResponse::from' "$TREND_USECASE_FILE"Repository: CREJIPSA/CreJipsa-BE
Length of output: 1436
Map real rank and platform values from the database in findTopKeywordsByCategory.
The hardcoded rank=0 and platform=null in lines 87–114 flow directly into TrendResponse objects via TrendResponse.from(TrendItem row), which exposes both fields in the /recommendations/by-interests API response. Replace placeholder values with actual database columns or appropriate defaults if not available in the query.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/tave/crezipsa/crezipsa/infrastructure/trend/TrendAdapter.java`
around lines 87 - 114, The code is hardcoding rank and platform when mapping DB
rows to TrendItem; update the SQL in TrendAdapter.findTopKeywordsByCategory to
select the actual rank and platform columns (e.g., include category_rank and
platform from analytics_keyword_virality) and map them into the TrendItem
constructor (use rs.getInt("category_rank") or rs.getLong if appropriate for
rank, and rs.getString("platform") for platform) so
TrendResponse.from(TrendItem) exposes real values; keep using the existing
lambda mapper and parameter names (params.addValue("category", category),
"limit") when making these changes.
Comment on lines
+51
to
+53
| @GetMapping("/search") | ||
| public GlobalResponseDto<TrendSearchResponse> searchTrend(@AuthenticationPrincipal User user, @ModelAttribute @Valid TrendSearchRequest trendSearchRequest) { | ||
| return GlobalResponseDto.success(trendUsecase.searchTrend(user.getUserId(), trendSearchRequest.getTrend())); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the TrendSearchRequest class
find . -type f -name "TrendSearchRequest.java" | head -20Repository: CREJIPSA/CreJipsa-BE
Length of output: 156
🏁 Script executed:
# Search for TrendController.java
find . -type f -name "TrendController.java" | head -20Repository: CREJIPSA/CreJipsa-BE
Length of output: 153
🏁 Script executed:
# Check for Spring version in pom.xml
find . -type f -name "pom.xml" | head -5Repository: CREJIPSA/CreJipsa-BE
Length of output: 46
🏁 Script executed:
cat -n ./src/main/java/tave/crezipsa/crezipsa/application/trend/dto/request/TrendSearchRequest.javaRepository: CREJIPSA/CreJipsa-BE
Length of output: 649
🏁 Script executed:
cat -n ./src/main/java/tave/crezipsa/crezipsa/presentation/trend/controller/TrendController.javaRepository: CREJIPSA/CreJipsa-BE
Length of output: 4595
🏁 Script executed:
# Find and read pom.xml
fd "pom.xml" --max-results 1 | xargs cat | head -100Repository: CREJIPSA/CreJipsa-BE
Length of output: 46
🏁 Script executed:
# Search for Spring version more carefully
find . -name "pom.xml" -o -name "build.gradle" | head -10Repository: CREJIPSA/CreJipsa-BE
Length of output: 78
🏁 Script executed:
# Try reading the pom.xml directly from root
cat pom.xml | grep -A 5 "spring.boot.version\|spring-boot-starter-web\|<version>" | head -50Repository: CREJIPSA/CreJipsa-BE
Length of output: 103
🏁 Script executed:
# Check other examples of `@ModelAttribute` usage in the codebase
rg "@ModelAttribute" -A 2 -B 2 | head -100Repository: CREJIPSA/CreJipsa-BE
Length of output: 1413
🏁 Script executed:
cat build.gradle | head -80Repository: CREJIPSA/CreJipsa-BE
Length of output: 2264
🏁 Script executed:
# Check for Spring version in build.gradle
rg "spring.*version|springBootVersion|org.springframework.boot" build.gradle -A 2 -B 2Repository: CREJIPSA/CreJipsa-BE
Length of output: 1481
🏁 Script executed:
# Check if there are any test cases or actual usage that might reveal issues
rg "searchTrend" -A 3 -B 3 | head -50Repository: CREJIPSA/CreJipsa-BE
Length of output: 2908
🏁 Script executed:
# Look for other similar single-parameter GET endpoints to check consistency
rg "@GetMapping.*@RequestParam.*String" -A 1 | head -50Repository: CREJIPSA/CreJipsa-BE
Length of output: 46
Use @RequestParam for single query parameter instead of @ModelAttribute with DTO.
This endpoint is inconsistent with similar endpoints in the same controller (see line 31 which uses @RequestParam). For a single query parameter, using @RequestParam is more idiomatic and explicit. While Spring 6.x has stable constructor binding, wrapping a single parameter in a DTO unnecessarily complicates the signature.
🛠️ Recommended refactor
- `@GetMapping`("/search")
- public GlobalResponseDto<TrendSearchResponse> searchTrend(`@AuthenticationPrincipal` User user, `@ModelAttribute` `@Valid` TrendSearchRequest trendSearchRequest) {
- return GlobalResponseDto.success(trendUsecase.searchTrend(user.getUserId(), trendSearchRequest.getTrend()));
+ `@GetMapping`("/search")
+ public GlobalResponseDto<TrendSearchResponse> searchTrend(
+ `@AuthenticationPrincipal` User user,
+ `@RequestParam`("trend") `@NotBlank` `@Size`(max = 20) String trend) {
+ return GlobalResponseDto.success(trendUsecase.searchTrend(user.getUserId(), trend.trim()));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/tave/crezipsa/crezipsa/presentation/trend/controller/TrendController.java`
around lines 51 - 53, The searchTrend method in TrendController currently
accepts a DTO via `@ModelAttribute` (`@ModelAttribute` `@Valid` TrendSearchRequest
trendSearchRequest) for a single query parameter; replace this with a simple
`@RequestParam` to match other endpoints: change the signature to
searchTrend(`@AuthenticationPrincipal` User user, `@RequestParam`("trend") String
trend) and pass trend into trendUsecase.searchTrend(user.getUserId(), trend);
remove or stop using TrendSearchRequest in this controller (and its import) if
no longer needed and ensure any validation or defaulting is handled via
`@RequestParam` attributes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/tave/crezipsa/crezipsa/global/exception/code/ErrorCode.java`:
- Line 61: Update the user-facing message for the enum constant
SEARCH_KEYWORD_TOO_LONG in ErrorCode by removing the extra period and fixing
spacing; change the message string from "검색어는 최대 20자까지 입니다.." to a normalized
form like "검색어는 최대 20자까지입니다." so the enum constant SEARCH_KEYWORD_TOO_LONG
carries the corrected text.
- Around line 63-64: The TREND_NOT_FOUND enum entry in ErrorCode.java uses the
unapproved prefix "T40401"; change its error-code string to use an approved
prefix (e.g., "SE40401" if trends belong to Search) or, if introducing a new
domain prefix is intentional, add that prefix to the project's documented set
and update any docs/contracts. Update the ErrorCode enum constant
TREND_NOT_FOUND to the new code (and adjust any references/tests that assert
this exact code) so the prefix is consistent with the documented prefixes
(reference: TREND_NOT_FOUND in ErrorCode.java).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72dafc3d-70f0-4d3f-9927-ca3ba57bff75
📒 Files selected for processing (2)
src/main/java/tave/crezipsa/crezipsa/application/trend/dto/request/TrendSearchRequest.javasrc/main/java/tave/crezipsa/crezipsa/global/exception/code/ErrorCode.java
| // 검색 관련 | ||
| SEARCH_KEYWORD_REQUIRED(400, "SE40001", "검색어는 필수입니다."), | ||
| SEARCH_KEYWORD_TOO_LONG(400, "SE40002", "검색어는 최대 30자까지 입니다.."), | ||
| SEARCH_KEYWORD_TOO_LONG(400, "SE40002", "검색어는 최대 20자까지 입니다.."), |
There was a problem hiding this comment.
Fix typo in the error message text.
Line 61 has an extra period (..) and awkward spacing in a user-facing message. Please normalize it (e.g., 검색어는 최대 20자까지입니다.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/tave/crezipsa/crezipsa/global/exception/code/ErrorCode.java` at
line 61, Update the user-facing message for the enum constant
SEARCH_KEYWORD_TOO_LONG in ErrorCode by removing the extra period and fixing
spacing; change the message string from "검색어는 최대 20자까지 입니다.." to a normalized
form like "검색어는 최대 20자까지입니다." so the enum constant SEARCH_KEYWORD_TOO_LONG
carries the corrected text.
Comment on lines
+63
to
+64
| // 트렌드 관련 | ||
| TREND_NOT_FOUND(404, "T40401", "해당 트렌드를 찾을 수 없습니다."), |
There was a problem hiding this comment.
T40401 prefix is outside the current documented error-code prefix set.
Line 64 introduces T40401, but the current project convention only documents U, A, I, S, G, C, CH, SE. Please either align this code to an approved prefix or explicitly extend the convention before merging to keep error taxonomy/contracts consistent.
As per coding guidelines: error codes are defined ... with domain-specific prefixes (U=User, A=Auth, I=Interest, S=Storyboard, G=Gemini, C=Community/Comment, CH=Chat, SE=Search). Based on learnings: If adding new error codes, update ErrorCode and keep codes/status consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/tave/crezipsa/crezipsa/global/exception/code/ErrorCode.java`
around lines 63 - 64, The TREND_NOT_FOUND enum entry in ErrorCode.java uses the
unapproved prefix "T40401"; change its error-code string to use an approved
prefix (e.g., "SE40401" if trends belong to Search) or, if introducing a new
domain prefix is intentional, add that prefix to the project's documented set
and update any docs/contracts. Update the ErrorCode enum constant
TREND_NOT_FOUND to the new code (and adjust any references/tests that assert
this exact code) so the prefix is consistent with the documented prefixes
(reference: TREND_NOT_FOUND in ErrorCode.java).
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
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.
📌 작업한 내용
트렌드 계층 관련 의존성 리팩토링
🔍 참고 사항
🖼️ 스크린샷
🔗 관련 이슈
✅ 체크리스트
Summary by CodeRabbit
New Features
Bug Fixes
Improvements