Conversation
src/main/java/org/runimo/runimo/runimo/controller/dto/response/GetRunimoTypeListResponse.java
Outdated
Show resolved
Hide resolved
@ekgns33 슨상님 요거는 어떠셨나요 |
MADANG으로 줬던것같아요. reward api였던것같네요. 수정하겠습니다! 한글이름으로 가죠! REALNAME |
WalkthroughThis pull request updates multiple components across the application. Swagger annotations have been added to controllers to improve OpenAPI documentation, and endpoint mappings have been updated. New endpoints and DTOs for runimo type listings have been introduced while existing DTOs and models have been restructured to emphasize run statistics. Repository queries have been adjusted accordingly, and the use case implementation now features additional user validation. Test cases and SQL scripts were also updated to reflect the schema and endpoint changes. Changes
Sequence Diagram(s)Runimo Type List EndpointsequenceDiagram
participant C as Client
participant RC as RunimoController
participant RU as RunimoUsecase
participant RDef as RunimoDefinitionRepository
participant M as Model Converter
C->>RC: GET /api/v1/runimos/types/all
RC->>RU: getRunimoTypeList()
RU->>RDef: findAllToSimpleModel()
RDef-->>RU: List<RunimoTypeSimpleModel>
RU->>M: RunimoTypeSimpleModel.toDtoList(modelList)
M-->>RU: List<RunimoTypeInfo>
RU-->>RC: GetRunimoTypeListResponse DTO
RC-->>C: 200 OK with runimo type list
My Runimo List Endpoint with User ValidationsequenceDiagram
participant C as Client
participant RC as RunimoController
participant UF as UserFinder
participant RU as RunimoUsecaseImpl
participant R as RunimoRepository
participant M as Model Converter
C->>RC: GET /api/v1/users/me/runimos
RC->>UF: findUserById(userId)
alt User Found
UF-->>RU: User object (including mainRunimoId)
RU->>R: Fetch runimo models for user
R-->>RU: List<RunimoSimpleModel>
RU->>M: RunimoSimpleModel.toDtoList(modelList, mainRunimoId)
M-->>RU: List<RunimoInfo>
RU-->>RC: SuccessResponse payload
RC-->>C: 200 OK with my runimo list
else User Not Found
UF-->>RC: Throws HatchException (HATCH_USER_NOT_FOUND)
RC-->>C: 404 Not Found
end
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/runimo/runimo/runimo/controller/dto/response/GetRunimoTypeListResponse.java (1)
1-11: 🛠️ Refactor suggestionNew DTO for Runimo type list response.
The response structure looks simple and appropriate for returning a list of Runimo types. However, based on past review comments, there was a discussion about:
- Ensuring proper ordering of the returned types
- Grouping Runimos by egg type instead of using a flat list
Consider implementing the suggestion to group Runimos by egg type using a Map structure as discussed in previous comments:
Map<EggType, List<RunimoTypeInfo>> runimoListGroupByEggType;This would provide more structure to the response and make it easier for clients to process the data.
src/main/java/org/runimo/runimo/runimo/controller/RunimoController.java (1)
48-48: Consider future extensibility with filtering options.As noted in the previous review, if you anticipate needing to filter or provide options for this endpoint in the future, consider adding optional parameters.
🧹 Nitpick comments (3)
src/test/resources/sql/get_all_runimo_type_test_data.sql (1)
1-8: Test data setup is focused on users but not runimo typesThis SQL script sets up test data for the
userstable, but the filename suggests it should be related to runimo types. Consider either:
- Adding runimo type test data to this file
- Renaming the file to better reflect its contents
- Adding comments to explain how this user data relates to runimo type testing
Consider adding explanatory comments about how this test data is used for testing runimo type functionality:
+~ -- 사용자 (Users for runimo type testing)src/test/resources/sql/get_my_runimo_list_test_data.sql (1)
11-16: Consider cleaning up commented code if no longer needed.There are several commented-out lines of SQL related to the
runimo_definitiontable. If these are no longer needed, consider removing them to improve readability. If they're kept as reference, add a comment explaining why they're preserved.src/main/java/org/runimo/runimo/runimo/service/model/RunimoSimpleModel.java (1)
13-17: Added mainRunimoId parameter needs null handlingThe updated toDtoList method now identifies a main Runimo among the list. Consider adding null safety handling for mainRunimoId to prevent potential NullPointerException if it's null.
public static List<RunimoInfo> toDtoList(List<RunimoSimpleModel> modelList, Long mainRunimoId) { return modelList.stream().map(i -> - i.toDto(i.id.equals(mainRunimoId)) + i.toDto(mainRunimoId != null && i.id.equals(mainRunimoId)) ).toList(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/org/runimo/runimo/hatch/controller/HatchController.java(2 hunks)src/main/java/org/runimo/runimo/hatch/exception/HatchHttpResponseCode.java(1 hunks)src/main/java/org/runimo/runimo/runimo/controller/RunimoController.java(4 hunks)src/main/java/org/runimo/runimo/runimo/controller/dto/response/GetRunimoTypeListResponse.java(1 hunks)src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoInfo.java(1 hunks)src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoTypeInfo.java(1 hunks)src/main/java/org/runimo/runimo/runimo/exception/RunimoHttpResponseCode.java(1 hunks)src/main/java/org/runimo/runimo/runimo/repository/RunimoDefinitionRepository.java(1 hunks)src/main/java/org/runimo/runimo/runimo/repository/RunimoRepository.java(1 hunks)src/main/java/org/runimo/runimo/runimo/service/model/RunimoSimpleModel.java(1 hunks)src/main/java/org/runimo/runimo/runimo/service/model/RunimoTypeSimpleModel.java(1 hunks)src/main/java/org/runimo/runimo/runimo/service/usecase/RunimoUsecase.java(1 hunks)src/main/java/org/runimo/runimo/runimo/service/usecase/RunimoUsecaseImpl.java(3 hunks)src/test/java/org/runimo/runimo/runimo/controller/RunimoControllerTest.java(2 hunks)src/test/resources/sql/get_all_runimo_type_test_data.sql(1 hunks)src/test/resources/sql/get_my_runimo_list_test_data.sql(2 hunks)src/test/resources/sql/set_main_runimo_test_data.sql(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/runimo/runimo/hatch/controller/HatchController.java (1)
src/main/java/org/runimo/runimo/runimo/controller/RunimoController.java (1)
Tag(21-79)
🔇 Additional comments (31)
src/main/java/org/runimo/runimo/hatch/exception/HatchHttpResponseCode.java (1)
12-13: New error code added for user not found scenarios.The addition of
HATCH_USER_NOT_FOUNDfollows the existing pattern for error codes in the application. The code "HEH4042" is consistent with the naming convention (HEH prefix, 4xxx for client errors), and the HTTP status of NOT_FOUND is appropriate for this scenario.src/main/java/org/runimo/runimo/runimo/service/usecase/RunimoUsecase.java (2)
4-4: Imported GetRunimoTypeListResponse for new method.The import for the new response DTO is appropriate for the new method being added.
13-13: New method added to fetch all Runimo types.The addition of the
getRunimoTypeList()method aligns with the PR objective to provide an API for fetching all types of Runimos. The method doesn't take any parameters which is appropriate for retrieving a complete list of types.src/main/java/org/runimo/runimo/hatch/controller/HatchController.java (2)
6-6: Added import for Swagger Tag annotation.The import for the
Tagannotation from the Swagger package is appropriate for documenting the API endpoint.
19-19: Added Swagger documentation tag for the controller.The
@Tagannotation improves OpenAPI documentation by categorizing endpoints under the "HATCH" tag. This is consistent with the approach used in other controllers likeRunimoController.src/main/java/org/runimo/runimo/runimo/controller/dto/response/GetRunimoTypeListResponse.java (1)
8-8: Consider standardizing naming conventions for response fields.Based on the PR objective, there's a question about using "MADANG" instead of "마당" in the response. For consistency across APIs, standardize whether you'll use English/romanized versions (like "MADANG") or Korean names (like "마당") for these type values.
If you're using enum values that are in uppercase English, consider documenting this decision or providing a translation mapping if needed for the frontend.
src/main/java/org/runimo/runimo/runimo/exception/RunimoHttpResponseCode.java (2)
9-10: LGTM: New response code for runimo type listThe new response code follows the existing pattern and provides clear messaging.
13-14: Good catch: Added missing semicolonFixed a syntax error by adding the required semicolon after the enum constant.
src/main/java/org/runimo/runimo/runimo/repository/RunimoDefinitionRepository.java (1)
14-18: LGTM: Well-structured query methodThe new query method is well-implemented using a clean JPQL query with text blocks for better readability.
Verify that the
typefield fromRunimoDefinitionis properly mapped toeggTypeinRunimoTypeSimpleModeland that it follows the agreed-upon format (Korean "마당" instead of "MADANG") as discussed in PR comments. You might need to check the implementation of theRunimoTypeSimpleModelconstructor and any conversion methods.src/test/java/org/runimo/runimo/runimo/controller/RunimoControllerTest.java (3)
61-61: Improved API endpoint design with resource hierarchyThe API endpoint has been updated to follow a more RESTful structure by organizing resources hierarchically (users -> me -> runimos). This is a good improvement in API design.
72-75: Updated assertions to include run statisticsThe test assertions now focus on validating run statistics (
total_run_count,total_distance_in_meters,is_main_runimo) rather than descriptive data. This aligns well with the updated API response structure.
77-102: Well-structured test for the new runimo types endpointThis new test thoroughly validates the response structure and content for the runimo types endpoint. I notice that line 100 is testing for "마당" as the egg_type value, which matches what was asked in the PR description about using Korean names instead of "MADANG".
src/test/resources/sql/set_main_runimo_test_data.sql (3)
4-7: Appropriately updated user schema to track main runimoThe SQL has been correctly modified to include the
main_runimo_idcolumn in the users table. Setting it to 1 for the first user and null for the second user provides good test coverage for both cases.
19-24: Enhanced runimo schema with run statisticsThe runimo table schema has been correctly updated to include tracking of run statistics (
total_run_countandtotal_distance_in_meters). The test data provides good variety with one runimo having activity (3 runs, 1000 meters) and others having none.
11-16: Confirm egg_type naming conventionThe commented code shows egg_type as "MADANG" while the new test in RunimoControllerTest.java uses "마당" (Korean name). This aligns with the PR description question about using Korean names instead of enum constants.
Could you confirm that all other instances of egg_type in the codebase are being updated to use Korean names ("마당") instead of enum constants ("MADANG")?
src/main/java/org/runimo/runimo/runimo/repository/RunimoRepository.java (1)
16-18: Updated query to focus on run statisticsThe repository query has been updated to focus on run statistics rather than descriptive attributes. This aligns with the changes in the controller test assertions and the overall shift towards emphasizing run performance metrics.
src/main/java/org/runimo/runimo/runimo/service/usecase/RunimoUsecaseImpl.java (2)
32-40: Enhanced user validation in getMyRunimoListThe implementation now properly validates that the user exists before returning their runimos and throws an appropriate exception with a clear message. Including the user's main runimo ID in the response is also a good enhancement.
56-60: Well-implemented getRunimoTypeList methodThe new method for retrieving all runimo types is cleanly implemented and follows the same pattern as the existing methods. It correctly uses the repository to fetch the data and transforms it into the appropriate response format.
src/main/java/org/runimo/runimo/runimo/service/model/RunimoTypeSimpleModel.java (4)
7-13: Well-structured record definition with appropriate fields.This record encapsulates the essential properties of a Runimo type with clear, descriptive field names. The implementation is concise and follows Java record best practices.
15-18: Good constructor implementation for EggType handling.The constructor properly handles the conversion from an EggType object to a string name, which simplifies the creation of RunimoTypeSimpleModel instances while maintaining clean data representation.
20-22: Efficient DTO conversion using stream operations.The static toDtoList method effectively uses Java streams to transform a list of model objects into DTOs. This approach is concise and follows functional programming best practices.
24-26: Clean private helper method for DTO conversion.The private toDto method correctly maps the record fields to the corresponding DTO fields, facilitating the model-to-DTO transformation process.
src/test/resources/sql/get_my_runimo_list_test_data.sql (2)
4-7: Database schema update correctly reflected in test data.The users table INSERT statement has been updated to include the new
main_runimo_idcolumn, which aligns with the application's data model changes. The values are appropriately set for test users.
18-24: Runimo table schema properly updated with new run statistics fields.The runimo table INSERT statements now include
total_run_countandtotal_distance_in_meterscolumns, which aligns with the changes in the RunimoInfo model. The sample data provides a good mix of values for testing different scenarios.src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoInfo.java (2)
12-16: Good addition of run statistics to the model with clear Swagger documentation.The new fields
totalRunCountandtotalDistanceInMeterswith appropriate Swagger annotations enhance the API by providing important performance metrics for each Runimo.
18-19: Useful status indicator for main Runimo.The addition of the
isMainRunimoboolean field allows clients to easily identify the user's main Runimo, improving the API's usability.src/main/java/org/runimo/runimo/runimo/controller/RunimoController.java (3)
6-7: Good use of Swagger Tag annotation for API documentation.Adding the Tag annotation improves the organization of the API documentation by grouping the Runimo-related endpoints under a specific category with a clear description.
Also applies to: 21-21
32-32: API path update follows RESTful resource hierarchy conventions.Changing the endpoint from
/api/v1/runimos/myto/api/v1/users/me/runimosbetter reflects the resource ownership relationship, making the API more intuitive and consistent with RESTful design principles.
44-57:Details
❓ Verification inconclusive
Well-implemented endpoint for retrieving all Runimo types.
The new endpoint is properly documented with Swagger annotations and follows the same pattern as other endpoints in the controller. The implementation correctly delegates to the usecase and wraps the response.
Regarding the PR question about "MADANG" vs "마당" - based on the comment from ekgns33, it appears "MADANG" was used in other APIs. Consider standardizing on either the Korean name ("마당") or the transliteration ("MADANG") across all APIs for consistency.
🏁 Script executed:
#!/bin/bash # Check for existing usage patterns of "MADANG" vs "마당" in the codebase echo "Searching for occurrences of 'MADANG' in the codebase:" rg -i "MADANG" --type java echo -e "\nSearching for occurrences of '마당' in the codebase:" rg "마당" --type javaLength of output: 2493
Standardize Naming Convention for Runimo Types
The endpoint is implemented and documented well. However, our search indicates that both "MADANG" and "마당" are being used inconsistently across the codebase—in enums, domain objects, API response examples, and tests. Please review the overall design decision (and potentially confirm with product specifications) to standardize on a single naming convention for consistency across all APIs.
src/main/java/org/runimo/runimo/runimo/service/model/RunimoSimpleModel.java (2)
6-11: Field updates focus on run statistics, aligning with API purposeThe record fields have been updated to focus on run statistics (totalRunCount, totalDistanceInMeters) rather than descriptive attributes. This change aligns well with the API's purpose of retrieving Runimos with relevant performance metrics.
19-21: Method updated to support main Runimo identificationThe toDto method has been correctly updated to include the isMainRunimo flag, which helps clients distinguish the main Runimo in the response.
src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoTypeInfo.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/runimo/runimo/runimo/service/model/RunimoTypeSimpleModel.java (1)
57-70: Consider sorting the runimo types within each groupThe current implementation groups runimos by egg type but doesn't specify any ordering for the runimos within each group. You might want to add sorting based on ID or another meaningful attribute to ensure consistent ordering in the response.
private static Map<String, List<RunimoTypeInfo>> toRunimoTypeMap( List<RunimoTypeSimpleModel> modelList) { Map<String, List<RunimoTypeInfo>> runimoTypeDtos = new HashMap<>(); for (RunimoTypeSimpleModel model : modelList) { String key = model.eggType(); if (!runimoTypeDtos.containsKey(key)) { runimoTypeDtos.put(key, new ArrayList<>()); } runimoTypeDtos.get(key).add(model.toDto()); } + // Sort runimos in each group by code for consistent ordering + for (List<RunimoTypeInfo> runimoTypes : runimoTypeDtos.values()) { + runimoTypes.sort(Comparator.comparing(RunimoTypeInfo::code)); + } return runimoTypeDtos; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/runimo/runimo/runimo/controller/dto/response/GetRunimoTypeListResponse.java(1 hunks)src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoTypeGroup.java(1 hunks)src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoTypeInfo.java(1 hunks)src/main/java/org/runimo/runimo/runimo/service/model/RunimoTypeSimpleModel.java(1 hunks)src/test/java/org/runimo/runimo/runimo/controller/RunimoControllerTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoTypeInfo.java
🔇 Additional comments (6)
src/main/java/org/runimo/runimo/runimo/controller/dto/response/GetRunimoTypeListResponse.java (1)
1-11: LGTM - Simple and well-structured DTOThe
GetRunimoTypeListResponseclass is properly defined as a record with appropriate Swagger documentation, which will help with API documentation. The structure is clean and follows good practices for DTOs.src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoTypeGroup.java (1)
8-9:Details
✅ Verification successful
Validate the egg type naming convention
The example shows
eggTypeas "마당" (Korean), which aligns with the PR discussion about using Korean names instead of "MADANG". Make sure this convention is consistently applied across the codebase.
🏁 Script executed:
#!/bin/bash # Check usage of egg type names across the codebase echo "Checking for 'MADANG' usage in code:" rg -i "MADANG" --type java echo -e "\nChecking for '마당' usage in code:" rg "마당" --type java echo -e "\nChecking EggType enum definition:" rg -A 10 "enum\s+EggType" --type javaLength of output: 3492
Egg Type Naming Convention Verified
- The DTO in
src/main/java/org/runimo/runimo/runimo/controller/dto/response/RunimoTypeGroup.javanow uses the Korean display value "마당" as expected.- The
EggTypeenum insrc/main/java/org/runimo/runimo/item/domain/EggType.javacorrectly maps the enum constantMADANGto its display name "마당," which is standard practice.- Test cases and other references confirm that user-facing values are consistently using the Korean names while the enum identifier remains in uppercase per Java conventions.
The naming convention is consistently applied across the codebase. Please continue ensuring that any new implementations follow this approach.
src/test/java/org/runimo/runimo/runimo/controller/RunimoControllerTest.java (3)
61-61: Updated API endpoint follows RESTful conventionsThe API endpoint has been changed from
/api/v1/runimos/myto/api/v1/users/me/runimos, which better follows RESTful resource naming conventions by placing the user-specific resources under the user's path.
72-75: Updated test assertions for additional runimo propertiesThe test now verifies the new runimo statistics fields (
total_run_count,total_distance_in_meters, andis_main_runimo), making sure they are properly returned in the API response.
78-106: Comprehensive test for the new API endpointThe new test method
전체_러니모_종류_조회_성공is well-structured and verifies the crucial aspects of the new API, including:
- The correct response status code and response code
- The structure of the response payload
- The egg types and their associated runimo types
- The properties of individual runimo types
The test confirms that the API returns egg types in Korean ("마당", "숲") as discussed in the PR comments.
src/main/java/org/runimo/runimo/runimo/service/model/RunimoTypeSimpleModel.java (1)
19-22: Appropriate constructor for handling EggType enumThe constructor correctly extracts the name from the EggType enum, which helps maintain consistency when converting between domain models and DTOs.
| private static List<RunimoTypeGroup> toRunimoTypeGroups( | ||
| Map<String, List<RunimoTypeInfo>> runimoDtoMap) { | ||
|
|
||
| List<RunimoTypeGroup> runimoTypeGroups = new ArrayList<>(); | ||
| for (EggType eggType : EggType.values()) { | ||
| String key = eggType.getName(); | ||
| runimoTypeGroups.add(new RunimoTypeGroup(key, runimoDtoMap.get(key))); | ||
| } | ||
|
|
||
| return runimoTypeGroups; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure order consistency for egg type groups
The method creates RunimoTypeGroup objects based on the order of EggType enum values, which is a good approach for maintaining a consistent order in the response. However, there's a potential issue if the runimoDtoMap doesn't contain an entry for one of the enum values.
🏁 Script executed:
#!/bin/bash
# Check if there's error handling for missing egg types in the map
# First, check if there's any null check when accessing runimoDtoMap.get(key)
rg "runimoDtoMap\.get\(" -A 3 -B 3
# Then check if all EggType values are expected to be present in the database
echo "Checking for SQL scripts that populate EggType data:"
fd -e sql | xargs grep -l "EggType" | xargs catLength of output: 1049
Address Null Handling for Egg Type Map Entries
The current implementation correctly preserves the order by iterating over the EggType enum but doesn’t safeguard against null values when a key is missing in runimoDtoMap. This may lead to constructing a RunimoTypeGroup with a null list, potentially resulting in unexpected behavior downstream. Consider adding a null-check or defaulting to an empty list (e.g., using runimoDtoMap.get(key) != null ? runimoDtoMap.get(key) : Collections.emptyList()) to make the code more robust.
- File Affected:
src/main/java/org/runimo/runimo/runimo/service/model/RunimoTypeSimpleModel.java(Lines 42-52)
|
[추가 작업 사항]
[응답 예시] |
2 participants
[작업 내용]
[추가 작업 사항]
러니모 전체 조회 api : 응답 러니모들 정렬 및 그룹핑
응답 예시는 코멘트 참고
** 그룹핑 로직은 Model 클래스에 일단 썼는데 나중에 Mapper 클래스로 따로 빼거나 리팩토링 해볼게요
[질문]
테스트케이스나 예시 응답을 못찾았어요
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores