Conversation
WalkthroughThe pull request introduces extensive refactoring and new functionalities throughout the codebase. It modifies CI/CD workflows and build dependencies, restructures packages by consolidating node‐related code into building and map modules, and adds new endpoints and service methods for route and revision management. Auditing, exception handling, logging, and utility classes have been updated, and database configurations now favor MySQL Testcontainers over H2. Additionally, several test classes and supporting repositories have been added or re‐organized to accommodate the new design. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MapController
participant MapService
participant MapClient
participant NodeRepository
participant BuildingRepository
Client->>MapController: POST /{univId}/building-route
MapController->>MapService: createBuildingRoute(univId, reqDTO)
MapService->>MapClient: Retrieve building node info (height, etc.)
MapService->>NodeRepository: Save new Node
MapService->>BuildingRepository: Save new Building
MapService-->>MapController: Return success
MapController-->>Client: HTTP 200 OK
sequenceDiagram
participant AdminClient
participant AdminController
participant AdminService
participant RevInfoRepository
participant NodeAuditRepository
AdminClient->>AdminController: PATCH /{univId}/revisions/{versionId} (rollback)
AdminController->>AdminService: rollbackRev(univId, versionId)
AdminService->>RevInfoRepository: Fetch revision info
AdminService->>NodeAuditRepository: Delete nodes/routes after version
AdminService-->>AdminController: Confirmation of rollback
AdminController-->>AdminClient: HTTP 200 OK
Poem
✨ 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (12)
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/exception/GlobalExceptionHandler.java (1)
24-29: 🛠️ Refactor suggestion
Enhance validation error handling.
While the basic structure is good, consider these improvements:
- Rename the method to avoid collision (e.g.,
handleValidationException)- Include field-specific validation errors in the response
- Consider using more specific error codes based on validation failure type
Here's a suggested implementation:
@ExceptionHandler(MethodArgumentNotValidException.class) -public ResponseEntity<ErrorResponse> handleCustomException(MethodArgumentNotValidException ex) { +public ResponseEntity<ErrorResponse> handleValidationException(MethodArgumentNotValidException ex) { log.error(ex.getMessage()); - ErrorResponse response = new ErrorResponse(INVALID_INPUT_VALUE); + List<FieldError> fieldErrors = ex.getBindingResult().getFieldErrors(); + Map<String, String> validationErrors = fieldErrors.stream() + .collect(Collectors.toMap( + FieldError::getField, + FieldError::getDefaultMessage, + (first, second) -> first + )); + ErrorResponse response = new ErrorResponse(INVALID_INPUT_VALUE, validationErrors); return new ResponseEntity<>(response, HttpStatus.valueOf(INVALID_INPUT_VALUE.getHttpStatus())); }Note: This assumes ErrorResponse can handle validation details. If not, you'll need to extend it.
uniro_backend/src/main/java/com/softeer5/uniro_backend/building/dto/request/CreateBuildingNodeReqDTO.java (1)
27-28: 🛠️ Refactor suggestion
Add validation for level field.
The level field is used for map scaling (1-10) but lacks range validation. Add
@Min(1)and@Max(10)annotations to ensure valid values.@Schema(description = "레벨(지도 축척에 따른 노출정도, 1~10)", example = "3") + @Min(value = 1, message = "Level must be at least 1") + @Max(value = 10, message = "Level must not exceed 10") private final int level;📝 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.@Schema(description = "레벨(지도 축척에 따른 노출정도, 1~10)", example = "3") @Min(value = 1, message = "Level must be at least 1") @Max(value = 10, message = "Level must not exceed 10") private final int level;uniro_backend/src/main/java/com/softeer5/uniro_backend/building/controller/BuildingApi.java (1)
60-61: 🛠️ Refactor suggestion
Fix incorrect response code in Swagger documentation.
For resource creation, use 201 (Created) instead of 200 (OK).
@ApiResponses(value = { - @ApiResponse(responseCode = "200", description = "건물 노드 생성 성공"), + @ApiResponse(responseCode = "201", description = "건물 노드 생성 성공"), @ApiResponse(responseCode = "400", description = "EXCEPTION(임시)", content = @Content), })📝 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.@ApiResponses(value = { @ApiResponse(responseCode = "201", description = "건물 노드 생성 성공"), @ApiResponse(responseCode = "400", description = "EXCEPTION(임시)", content = @Content), })uniro_backend/src/main/java/com/softeer5/uniro_backend/common/logging/ArgType.java (1)
26-32: 💡 Verification agent
🛠️ Refactor suggestion
🧩 Analysis chain
Improve robustness of type detection.
The current implementation has several potential issues:
- Missing null checks could cause NPE
- Case-sensitive string matching might miss variations (e.g., "DTO", "Dto")
- Simple substring matching could cause false positives
- Performance could be improved with result caching
Consider this more robust implementation:
private static boolean isCustomDto(Object arg) { + if (arg == null) return false; + String className = arg.getClass().getName().toLowerCase(); - return arg.getClass().getName().contains("dto"); + return className.contains(".dto.") || className.endsWith("dto"); } private static boolean isEntity(Object arg) { + if (arg == null) return false; + String className = arg.getClass().getName().toLowerCase(); - return arg.getClass().getName().contains("entity"); + return className.contains(".entity.") || className.endsWith("entity"); }Let's verify the class naming patterns in the codebase:
🏁 Script executed:
#!/bin/bash # Search for DTO and Entity class patterns echo "DTO class patterns:" fd -e java | xargs rg -l "dto" || true echo -e "\nEntity class patterns:" fd -e java | xargs rg -l "entity" || trueLength of output: 8970
Robust Type Detection: Address Null Safety & Precise Pattern Matching
The current implementation still poses risks:
- Null Safety: No null check on the argument can lead to a NullPointerException.
- Case Sensitivity: Using a case-sensitive substring check might miss valid DTO/Entity naming variations (e.g., "DTO", "dto", "Dto").
- Overly Broad Matching: Simple substring matching can cause false positives.
- Performance: Consider caching the computed class name if this check becomes a performance hotspot.
Recommended refactor:
private static boolean isCustomDto(Object arg) { + if (arg == null) return false; + String className = arg.getClass().getName().toLowerCase(); - return arg.getClass().getName().contains("dto"); + return className.contains(".dto.") || className.endsWith("dto"); } private static boolean isEntity(Object arg) { + if (arg == null) return false; + String className = arg.getClass().getName().toLowerCase(); - return arg.getClass().getName().contains("entity"); + return className.contains(".entity.") || className.endsWith("entity"); }We've verified that the codebase consistently uses the
.dto.pattern in DTO classes and.entity.for entities. The refined approach should cover common naming variations while mitigating potential NPEs. Please integrate these changes and consider result caching if profiling indicates a performance issue.📝 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.private static boolean isCustomDto(Object arg) { if (arg == null) return false; String className = arg.getClass().getName().toLowerCase(); return className.contains(".dto.") || className.endsWith("dto"); } private static boolean isEntity(Object arg) { if (arg == null) return false; String className = arg.getClass().getName().toLowerCase(); return className.contains(".entity.") || className.endsWith("entity"); }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/logging/ExecutionLoggingAop.java (1)
99-140: 🛠️ Refactor suggestion
Consider adding depth limit for recursive logging.
The
logDetailmethod recursively traverses object fields which could cause performance issues with deeply nested objects.private void logDetail(Object arg, String requestType, StringBuilder logMessage, int depth) { + if (depth > 3) { // Limit recursion depth + logMessage.append(" ".repeat(depth)).append("[Max depth reached]\n"); + return; + } String indent = " ".repeat(depth); ArgType argType = ArgType.getArgType(arg);📝 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.private void logDetail(Object arg, String requestType, StringBuilder logMessage, int depth) { if (depth > 3) { // Limit recursion depth logMessage.append(" ".repeat(depth)).append("[Max depth reached]\n"); return; } String indent = " ".repeat(depth); // depth 수준에 따른 들여쓰기 ArgType argType = ArgType.getArgType(arg); switch (argType) { case NULL -> logMessage.append(indent).append(requestType).append(" null\n"); case LIST -> { logMessage.append(indent) .append(requestType) .append(" ") .append(arg.getClass().getSimpleName()) .append("\n"); List<?> list = (List<?>) arg; for (int i = 0; i < list.size(); i++) { logDetail(list.get(i), "[List Element " + i + "] ", logMessage, depth + 1); } } case CUSTOM_DTO -> { logMessage.append(indent) .append(requestType) .append("DTO: ") .append(arg.getClass().getSimpleName()) .append("\n"); logObjectFields(arg, logMessage, depth + 1); } case ENTITY -> { logMessage.append(indent) .append(requestType) .append(arg.getClass().getSimpleName()) .append(" : ") .append("\n"); logObjectFields(arg, logMessage, depth + 1); } default -> logMessage.append(indent) .append(requestType) .append(" ") .append(arg.getClass().getSimpleName()) .append(": ") .append(arg) .append("\n"); } }uniro_backend/src/main/java/com/softeer5/uniro_backend/map/entity/Route.java (1)
101-108: 🛠️ Refactor suggestion
Equality check excludes path and nodes.
isEqualRoutechecks ID, cost, caution, and danger factors. If node equality or path geometry matter, consider including them here or rename the method to clarify partial equality.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/RevisionContext.java (2)
32-34:
⚠️ Potential issueFix potential memory leak in clear() method.
The clear() method should remove all ThreadLocal variables to prevent memory leaks.
Apply this diff to fix the clear() method:
public static void clear() { univIdHolder.remove(); + actionHolder.remove(); + REVISION_TYPE_THREAD_LOCAL.remove(); }📝 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.public static void clear() { univIdHolder.remove(); actionHolder.remove(); REVISION_TYPE_THREAD_LOCAL.remove(); }
32-34:
⚠️ Potential issuePrevent memory leaks by clearing all ThreadLocal variables.
The
clear()method only removesunivIdHolder, but leavesactionHolderandREVISION_TYPE_THREAD_LOCALin memory. This can lead to memory leaks, especially in thread pools where threads are reused.Apply this diff to clear all ThreadLocal variables:
public static void clear() { univIdHolder.remove(); + actionHolder.remove(); + REVISION_TYPE_THREAD_LOCAL.remove(); }📝 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.public static void clear() { univIdHolder.remove(); actionHolder.remove(); REVISION_TYPE_THREAD_LOCAL.remove(); }uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/NodeAuditRepository.java (2)
30-35:
⚠️ Potential issueReplace native query with JPQL to prevent SQL injection.
While the current implementation uses parameter binding, it's safer to use JPQL or @query annotation.
Apply this diff to fix the SQL injection vulnerability:
- public void deleteAllAfterVersionId(Long univId, Long versionId) { - entityManager.createNativeQuery("DELETE FROM node_aud WHERE univ_id = :univId AND rev > :versionId") - .setParameter("univId", univId) - .setParameter("versionId", versionId) - .executeUpdate(); - } + @Query("DELETE FROM NodeAud n WHERE n.univId = :univId AND n.rev > :versionId") + @Modifying + public void deleteAllAfterVersionId(@Param("univId") Long univId, @Param("versionId") Long versionId);
30-35:
⚠️ Potential issueAdd input validation and error handling for delete operation.
The
deleteAllAfterVersionIdmethod has several potential issues:
- No validation of input parameters
- No error handling for failed queries
- Uses native SQL which could be vulnerable to SQL injection
Consider using JPQL or Criteria API instead of native SQL, and add proper validation:
public void deleteAllAfterVersionId(Long univId, Long versionId) { + if (univId == null || versionId == null) { + throw new IllegalArgumentException("univId and versionId must not be null"); + } + try { + String jpql = "DELETE FROM NodeAud n WHERE n.univId = :univId AND n.rev > :versionId"; + entityManager.createQuery(jpql) + .setParameter("univId", univId) + .setParameter("versionId", versionId) + .executeUpdate(); + } catch (Exception e) { + throw new RuntimeException("Failed to delete node audit records", e); + } }📝 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.public void deleteAllAfterVersionId(Long univId, Long versionId) { if (univId == null || versionId == null) { throw new IllegalArgumentException("univId and versionId must not be null"); } try { String jpql = "DELETE FROM NodeAud n WHERE n.univId = :univId AND n.rev > :versionId"; entityManager.createQuery(jpql) .setParameter("univId", univId) .setParameter("versionId", versionId) .executeUpdate(); } catch (Exception e) { throw new RuntimeException("Failed to delete node audit records", e); } }uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RouteAuditRepository.java (2)
31-36: 🛠️ Refactor suggestion
⚠️ Potential issueExtract common audit functionality to a base class.
- The native query should be replaced with JPQL to prevent SQL injection.
- The code is duplicated from NodeAuditRepository and should be extracted to a base class.
Create a base class for audit repositories:
@NoArgsConstructor(access = AccessLevel.PROTECTED) public abstract class BaseAuditRepository<T> { @PersistenceContext protected EntityManager entityManager; protected abstract Class<T> getEntityClass(); public List<T> getAllEntitiesAtRevision(Long univId, Long versionId) { AuditReader auditReader = AuditReaderFactory.get(entityManager); return auditReader.createQuery() .forEntitiesAtRevision(getEntityClass(), versionId) .add(AuditEntity.property("univId").eq(univId)) .getResultList(); } @Query("DELETE FROM #{#entityName}Aud e WHERE e.univId = :univId AND e.rev > :versionId") @Modifying public abstract void deleteAllAfterVersionId(@Param("univId") Long univId, @Param("versionId") Long versionId); }Then update RouteAuditRepository:
-@Repository -@RequiredArgsConstructor -@Transactional -public class RouteAuditRepository { - private final EntityManager entityManager; +@Repository +@Transactional +public class RouteAuditRepository extends BaseAuditRepository<Route> { + @Override + protected Class<Route> getEntityClass() { + return Route.class; + } - public List<Route> getAllRoutesAtRevision(Long univId, Long versionId) { - AuditReader auditReader = AuditReaderFactory.get(entityManager); - return auditReader.createQuery() - .forEntitiesAtRevision(Route.class, versionId) - .add(AuditEntity.property("univId").eq(univId)) - .getResultList(); - } - - public void deleteAllAfterVersionId(Long univId, Long versionId) { - entityManager.createNativeQuery("DELETE FROM route_aud WHERE univ_id = :univId AND rev > :versionId") - .setParameter("univId", univId) - .setParameter("versionId", versionId) - .executeUpdate(); - } }
19-36: 🛠️ Refactor suggestion
Extract common audit functionality to a base class.
The code is duplicated between
NodeAuditRepositoryandRouteAuditRepository. Consider creating a base class to handle common audit operations.Create a base class:
@MappedSuperclass public abstract class BaseAuditRepository<T> { protected final EntityManager entityManager; protected BaseAuditRepository(EntityManager entityManager) { this.entityManager = entityManager; } public List<T> getAllAtRevision(Class<T> entityClass, Long univId, Long versionId) { AuditReader auditReader = AuditReaderFactory.get(entityManager); return auditReader.createQuery() .forEntitiesAtRevision(entityClass, versionId) .add(AuditEntity.property("univId").eq(univId)) .getResultList(); } public void deleteAllAfterVersionId(String tableName, Long univId, Long versionId) { if (univId == null || versionId == null) { throw new IllegalArgumentException("univId and versionId must not be null"); } try { String jpql = String.format("DELETE FROM %s WHERE univId = :univId AND rev > :versionId", tableName); entityManager.createQuery(jpql) .setParameter("univId", univId) .setParameter("versionId", versionId) .executeUpdate(); } catch (Exception e) { throw new RuntimeException("Failed to delete audit records", e); } } }Then simplify the repositories:
@Repository @RequiredArgsConstructor @Transactional public class RouteAuditRepository extends BaseAuditRepository<Route> { public RouteAuditRepository(EntityManager entityManager) { super(entityManager); } public List<Route> getAllRoutesAtRevision(Long univId, Long versionId) { return getAllAtRevision(Route.class, univId, versionId); } public void deleteAllAfterVersionId(Long univId, Long versionId) { super.deleteAllAfterVersionId("RouteAud", univId, versionId); } }
2 participants
📝 PR 타입
🚀 변경 사항
길 찾기 로직에서 발생하던 오류를 해결하였습니다.
Summary by CodeRabbit
New Features
Improvements
Refactors