[BE] 스프린트 4 개발 산출물 #218
[BE] 스프린트 4 개발 산출물 #218
Conversation
* [UNI-237] fix: 대학교 및 건물 이름 순으로 조회하도록 수정 * [UNI-237] fix: 대학교 검색 기능 페이지네이션 삭제 * [UNI-237] fix: page 사이즈만큼만 불러오도록 수정 * [UNI-237] fix: 건물 이름 페이지네이션 삭제 및 page 사이즈만큼만 불러오도록 수정
* [UNI-244] feat: 대학교 데이터 추가 * [UNI-244] feat: 대학교 관련 데이터 반환 기능 구현
* [UNI-238] feat : 현재 버전과 특정 버전의 차이점 조회 로직 controller 생성 * [UNI-238] feat : 현재 버전과 특정 버전의 차이점 조회 로직 DTO 정의 * [UNI-238] feat : 현재 버전과 특정 버전의 차이점 조회 로직 구현 (리팩토링 전) * [UNI-238] feat : 코드중복 리팩토링 * [UNI-238] feat : 테스트코드 작성 * [UNI-238] feat : aud 테이블도 sequence 초기화하도록 sql문 수정 * [UNI-238] feat : 변경된 이력만 조회하도록 쿼리문 수정 * [UNI-238] feat : 테스트코드 작성 * [UNI-232] refactor : 명시적으로 rev순 정렬하는 코드 작성 * [UNI-232] refactor : 변수명 수정 * [UNI-232] refactor : 코드리뷰 반영
* [UNI-249] feat: 현재 버전에 대한 버전 반환 기능 구현 * [UNI-249] fix: DTO 이름 수정 * [UNI-249] fix: calculator 반환값과 Controller DTO 분리 * [UNI-249] fix: 현재 버전과 특정 버전의 차이점 조회 API 에 버전 정보 추가
* [UNI-264] refactor : forEntitiesAtRevision 에서 발생하는 쿼리 지연 문제를 해결하기 위해 forRevisionsOfEntity로 변경 * [UNI-264] feat : findByUnivId 구현 * [UNI-264] refactor : 쿼리문 변경으로 인한 로직 변경 (getChangedRoutes, fetchNodes) * [UNI-264] refactor : 코드중복 해결 * [UNI-264] refactor : 메서드명 변경
* [UNI-280] fix: oom 추적을 위한 추가 * [UNI-280] fix: oom 추적을 위한 추가
* [UNI-285] fix: revinfo 오류 해결 * [UNI-285] fix: 타임스탬프 주입 수정
* [UNI-294] feat: trim 데이터 추가 * [UNI-294] feat: trimmed 이름으로 검색하도록 수정 * [UNI-294] fix: not null 삭제
* [UNI-300] fix: 요청 버전 이후의 가장 최근 버전 조회 로직 수정 * [UNI-300] fix: yml 수정 * [UNI-300] chore: 에러메세지 수정
* [UNI-323] feat : Stream을 사용한 route 조회 로직 구현 * [UNI-324] feat : SSE를 사용하여 분할 데이터 제공 API 개발 * [UNI-324] feat : 비동기 프로그래밍을 위한 configuration
* [UNI-218] refactor : 방향 비교할 때 i-1, i, i+1 비교가 아닌 i-2, i, i+2를 비교하도록 변경 * [UNI-218] refactor : 메서드 2개를 하나로 병합
* [UNI-328] feat : 길 추가 요청 응답값에 방금 생긴 길 정보 제공 * [UNI-328] refactor : 요구사항 변경에 의해 GetAllRoutesResDTO -> AllRoutesDTO로 변경 * [UNI-328] refactor : GetAllRoutesResDTO 롤백
* [UNI-333] feat : 길 추가 로직에서 2000개 (약 6km) 이상의 노드를 추가하지 못하도록 Limit 제한 * [UNI-333] feat : 특정 버전 미만으로 롤백하지 못하도록 코드 작성 * [UNI-333] limitVersion을 DB에 저장하도록 수정 * [UNI-333] 컬럼명 명시 * [UNI-333] 테스트 컨테이너에 임시 학교 할당 * [UNI-333] 테스트 코드 변경 * [UNI-333] refactor : limit version을 primitive type으로 변경
* [UNI-255] feat: redis 환경 세팅 * [UNI-255] feat: heap 사이즈 500으로 조정 * [UNI-255] refactor: 경량 객체 구현 * [UNI-255] feat: 경량 객체에 사용되는 계산기 구현 * [UNI-255] feat: redis 서비스 구현 * [UNI-255] feat: 로컬 캐시 버전 구현 * [UNI-255] feat: 캐시 무효화 구현 * [UNI-255] feat: 롤백시 캐시 무효화 구현 * [UNI-255] refactor: fastJson 직렬화 구현 * [UNI-255] test * [UNI-255] fix: json 지원 * [UNI-255] refactor: 캐시 로직 적용 * [UNI-255] fix: redis 컨테이너 추가 * [UNI-255] fix: redis 컨테이너 추가 & 필요없는 설정 제거 * [UNI-255] test cd 삭제 * [UNI-255] fix: redis 컨테이너 설정 수정 * [UNI-255] fix: redis 컨테이너 설정 수정 해결 * [UNI-255] fix: 필요없는 설정 제거
* [UNI-255] chore: 패키지 이동 * [UNI-255] chore: redis 패키지 이동 * [UNI-255] feat: 스트림 기반 redis 구현 * [UNI-255] fix: FastJson 임시 처리 * [UNI-255] feat: 길 추가 되었을 때 캐시 초기화 구현 * [UNI-255] feat: 롤백 되었을 때, 캐시 초기화 구현 * [UNI-255] refactor: DB에서 경량객체만 불러오도록 개선
* [UNI-355] fix : 빌딩노드 오류 해결 * [UNI-355] fix : 빌딩노드 오류 해결 - Node
* [UNI-354] fix : HTTP요청 스레드가 아닌 경우 예외처리 * [UNI-354] feat : 길 추가 시 이벤트를 발생시키고, 이를 처리하는 코드 작성 * [UNI-354] feat : 최초 길 추가 시 height를 평균값으로 처리하도록 구현 * [UNI-354] feat : status에 따라 조회하는 repository 코드 작성 * [UNI-354] feat : Node에 status 필드 추가 * [UNI-354] feat : 배치 처리에 따라 조건 수정 * [UNI-354] feat : PROGRESS 보류 * chore : sync * [UNI-354] chore : 다른 로직에 영향가는 것 수정
…214) * [UNI-340] feat : max batch size 조정 * [UNI-340] refactor : boundedElastic Thread에게 publish하도록 로직 수정 * [UNI-340] chore : 배치사이즈 300으로 수정 * [UNI-340] parallel 스케쥴러로 변경 * chore : sync (uni-340-v3 -> be)
WalkthroughThis pull request introduces extensive modifications and new features across the backend project. The updates span configurations, dependency management, and core business logic. Updates include enhancements in the Docker and build configurations, adjustments to DTOs and repository query methods (removing cursor-based pagination), and refactoring of services and controllers to streamline API inputs and outputs. Additional asynchronous processing, event handling, caching with Redis, configuration classes, error handling enhancements, package restructuring, and comprehensive test updates are also introduced alongside changes in SQL scripts and YAML configuration files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MapController
participant MapService
participant DB
participant Redis
participant EventPublisher
participant RouteEventListener
participant TaskScheduler
participant RouteEventService
participant MapClient
Client->>MapController: createRoute(univId, request)
MapController->>MapService: createRoute(univId, request)
MapService->>DB: Save new route data
MapService->>Redis: Update route cache & delete outdated entries
MapService->>EventPublisher: Publish RouteCreatedEvent
EventPublisher->>RouteEventListener: Notify about route event
RouteEventListener->>TaskScheduler: Schedule fetchHeight task (30 min delay)
TaskScheduler->>RouteEventService: Execute fetchHeight()
RouteEventService->>Redis: Get nodes with status READY
RouteEventService->>MapClient: fetchHeights(nodes)
MapClient-->>RouteEventService: Return height data
RouteEventService->>DB: Update nodes' status to DONE
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 24
🔭 Outside diff range comments (3)
uniro_backend/src/test/java/com/softeer5/uniro_backend/map/service/MapServiceTest.java (1)
386-432: 🛠️ Refactor suggestionTest cases have duplicated setup code.
The setup code in
nearest_test2is identical tonearest_test1, which violates DRY principle.Extract common setup code to a shared method:
+private List<Node> setupTestNodes(double lat, double lon) { + Node node1 = NodeFixture.createNode(lat, lon); + Node node2 = NodeFixture.createNode(0, 0); + List<Node> savedNode = nodeRepository.saveAll(List.of(node1, node2)); + routeRepository.save(RouteFixture.createRoute(savedNode.get(0), savedNode.get(1))); + doNothing().when(mapClient).fetchHeights(anyList()); + return savedNode; +} @Test @DisplayName("인접한 노드는 중복될 수 없다. a->b->a 경우 ") void nearest_test2(){ // given List<CreateRouteReqDTO> requests = List.of(/*...*/); - Node node1 = NodeFixture.createNode(37.554533318727884, 127.040754103711); - Node node2 = NodeFixture.createNode(0, 0); - List<Node> savedNode = nodeRepository.saveAll(List.of(node1, node2)); - routeRepository.save(RouteFixture.createRoute(savedNode.get(0), savedNode.get(1))); - doNothing().when(mapClient).fetchHeights(anyList()); + List<Node> savedNode = setupTestNodes(37.554533318727884, 127.040754103711);uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/aspect/RevisionOperationAspect.java (1)
24-39: 🛠️ Refactor suggestionAdd try-finally block for cleanup in around method.
The timestamp is set but not cleaned up in the around method if an exception occurs before reaching the handler methods.
@Around("@annotation(revisionOperation)") public Object around(ProceedingJoinPoint joinPoint, RevisionOperation revisionOperation) throws Throwable { RevisionOperationType opType = revisionOperation.value(); RevisionContext.setTimeStamp(LocalDateTime.now()); - Object result; - switch (opType) { - case UPDATE_RISK -> result = updateRiskHandler(joinPoint); - case CREATE_ROUTE -> result = updateRouteHandler(joinPoint); - case CREATE_BUILDING_NODE -> result = createBuildingNodeHandler(joinPoint); - case CREATE_BUILDING_ROUTE -> result = createBuildingRouteHandler(joinPoint); - default -> result = joinPoint.proceed(); - } + try { + Object result; + switch (opType) { + case UPDATE_RISK -> result = updateRiskHandler(joinPoint); + case CREATE_ROUTE -> result = updateRouteHandler(joinPoint); + case CREATE_BUILDING_NODE -> result = createBuildingNodeHandler(joinPoint); + case CREATE_BUILDING_ROUTE -> result = createBuildingRouteHandler(joinPoint); + default -> result = joinPoint.proceed(); + } - return result; + return result; + } finally { + RevisionContext.clear(); + } }uniro_backend/build.gradle (1)
3-3: 💡 Verification agent🧩 Analysis chain
Verify Spring Boot version.
The specified Spring Boot version '3.3.8' appears to be incorrect as it doesn't exist yet. The latest stable version in the 3.x series is 3.2.x.
🌐 Web query:
What is the latest version of Spring Boot 3.x?💡 Result:
The latest version of Spring Boot in the 3.x series is Spring Boot 3.4.3, released on February 20, 2025[1][9]. This minor update includes 58 bug fixes, documentation improvements, and dependency upgrades, continuing the framework's focus on stability and incremental enhancements[1][8].
Key Details About Spring Boot 3.x:
- Java Requirement: Minimum Java 17 (upgraded from Java 8/11 in earlier versions)[2][6]
- Spring Framework Dependency: Requires Spring Framework 6.x[6][7]
- Jakarta EE Support: Uses Jakarta EE 9/10 APIs instead of legacy Java EE
javax.*packages[6][7]Recent major milestones in Spring Boot 3.x include:
Version Release Date Notable Features 3.0 Nov 2022 Initial Java 17/Jakarta EE support • GraalVM native image capability[6][7] 3.1 Jan 2025 Enhanced observability tools • @SpanTag annotations[10] 3.4 Feb 2025 Latest stable release with dependency updates and bug fixes[1][9] For developers maintaining older applications, VMware provides extended commercial support for Spring Boot 2.7 until February 2025[4]. However, new projects should adopt Spring Boot 3.x to leverage modern Java features and long-term maintenance[2][7].
When upgrading from Spring Boot 2.x, follow the official migration guide addressing changes like:
- Updated security filter dispatcher types
- Deprecated method replacements (e.g.,
ObservationConventioninstead of*TagsProviderclasses)- Jakarta namespace migration requirements[6][10]
The next planned release in this line will be Spring Boot v.x, continuing quarterly minor updates with security patches and dependency refreshes[1][9].
Citations:
- 1: https://spring.io/blog/2025/02/20/spring-boot-3-4-3-available-now
- 2: https://www.marcobehler.com/guides/spring-and-spring-boot-versions
- 3: https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0-Migration-Guide
- 4: https://spring.io/blog/2022/05/24/preparing-for-spring-boot-3-0
- 5: https://spring.io/blog/2024/05/23/spring-boot-3-3-0-available-now
- 6: https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0-Release-Notes
- 7: https://positivethinking.tech/insights/whats-new-in-spring-boot-3/
- 8: https://spring.io/projects/spring-boot
- 9: https://spring.io/blog/category/releases
- 10: https://blog.siemens.com/2025/01/upgrading-the-spring-boot-version-from-2-7-11-to-3-1-0/
Incorrect Spring Boot version detected in uniro_backend/build.gradle
- The new dependency additions for Prometheus monitoring, Redis caching, and JSON serialization are well integrated.
- However, the Spring Boot version is set to
'3.3.8', which is not a valid release. According to the latest release information, Spring Boot 3.x is at version 3.4.3 as of February 20, 2025.- Action required: Update the Spring Boot version in
uniro_backend/build.gradleto'3.4.3'(or a version compatible with your project requirements).
🧹 Nitpick comments (63)
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/response/ChangedRouteDTO.java (1)
16-17: Update Swagger description to match the renamed field.The field has been renamed from
revisiontodifferenceto better reflect its purpose, but the Swagger description still refers to it as "해당 버전 (과거)". Consider updating the description to align with the new terminology.- @Schema(description = "해당 버전 (과거)", example = "") + @Schema(description = "변경된 정보", example = "")uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RevInfoRepository.java (1)
20-22: LGTM! Well-designed revision management methods.The new methods follow Spring Data JPA naming conventions and properly use Optional for null safety. They provide a clean API for:
- Finding the latest revision for a university
- Finding the next revision after a specific version
Consider adding documentation to clarify:
- The ordering of results (DESC vs ASC)
- The meaning of the
revparameterpublic interface RevInfoRepository extends JpaRepository<RevInfo,Long> { + /** + * Finds the most recent revision for a university. + * @param univId The university ID + * @return Optional containing the latest RevInfo, or empty if none exists + */ Optional<RevInfo> findFirstByUnivIdOrderByRevDesc(Long univId); + /** + * Finds the first revision after the specified revision number for a university. + * @param univId The university ID + * @param rev The revision number to search after + * @return Optional containing the next RevInfo, or empty if none exists + */ Optional<RevInfo> findFirstByUnivIdAndRevAfter(Long univId, Long rev);uniro_backend/src/main/java/com/softeer5/uniro_backend/building/repository/BuildingCustomRepository.java (2)
9-9: Consider the scalability implications of removing cursor-based pagination.The change from cursor-based pagination to a simple list with pageSize could impact performance and scalability:
- Cursor-based pagination is more efficient for large datasets as it uses a stable reference point
- Simple offset-based pagination can lead to performance issues and inconsistencies when data changes between requests
Consider keeping cursor-based pagination or implementing keyset pagination if dealing with large datasets. This would involve:
- Maintaining a stable sorting key (e.g., creation timestamp + ID)
- Using WHERE clauses instead of OFFSET for better performance
- Returning a cursor or last seen key for subsequent requests
5-5: Remove unused import.The
CursorPageimport is no longer used after removing cursor-based pagination.-import com.softeer5.uniro_backend.common.CursorPage;uniro_backend/Dockerfile (1)
10-12: Enhanced Memory Error Handling with JVM FlagsThe added JVM options (
-XX:+HeapDumpOnOutOfMemoryError,-XX:HeapDumpPath=/tmp/heapdump.hprof, and-Xmx500m) are a positive enhancement for diagnosing memory issues and controlling heap usage. This setup should help generate useful diagnostic data if the application encounters an OutOfMemoryError.Consideration:
If the memory requirements might vary across environments (development, staging, or production), consider externalizing these values as environment variables. This allows tuning the parameters without modifying the Dockerfile.uniro_backend/src/main/java/com/softeer5/uniro_backend/common/logging/ExecutionLoggingAop.java (1)
49-56: Improve null safety while maintaining logging capabilities.While the null safety improvements are good, the current implementation completely skips logging when there's no request. Consider logging a modified message for non-request contexts (e.g., async operations, scheduled tasks).
HttpServletRequest request = null; if (RequestContextHolder.getRequestAttributes() instanceof ServletRequestAttributes) { request = ((ServletRequestAttributes) RequestContextHolder.getRequestAttributes()).getRequest(); } -if(request==null){ - return pjp.proceed(); -} -log.info("✅ [ userId = {} Start] [Call Method] {}: {}", userId, request.getMethod(), task); +String method = request != null ? request.getMethod() : "NON-HTTP"; +log.info("✅ [ userId = {} Start] [Call Method] {}: {}", userId, method, task);uniro_backend/src/main/java/com/softeer5/uniro_backend/common/error/ErrorCode.java (1)
42-42: Consider adding version management section header.The version management related errors (
ALREADY_LATEST_VERSION_ID,RECENT_REVISION_NOT_FOUND,CANT_ROLLBACK_BELOW_MINIMUM_VERSION) are currently under the "어드민" (admin) section. Consider creating a separate section for version management errors for better organization.// 어드민 ALREADY_LATEST_VERSION_ID(400, "현재 가장 최신 버전 id 입니다."), INVALID_VERSION_ID(400, "유효하지 않은 버전 id 입니다."), INVALID_ADMIN_CODE(403, "유효하지 않은 어드민 코드 입니다."), INVALID_TOKEN(401, "유효하지 않은 토큰입니다."), UNAUTHORIZED_UNIV(401, "해당 대학교의 권한이 없습니다."), INVALID_UNIV_ID(400, "유효하지 않은 대학교 id 입니다."), + + // 버전 관리 RECENT_REVISION_NOT_FOUND(404, "최신 버전을 찾을 수 없습니다."), CANT_ROLLBACK_BELOW_MINIMUM_VERSION(400, "롤백하려는 버전이 허용된 최소 버전보다 낮습니다.")Also applies to: 48-49
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightRoutes.java (2)
11-15: Consider making LightRoutes immutable for better thread safety.Since this class is used for caching (as indicated in the AI summary), consider making it immutable to ensure thread safety and prevent unexpected state changes in the cache.
Apply this diff to make the class immutable:
@Getter @NoArgsConstructor -@Setter @AllArgsConstructor public class LightRoutes implements Serializable {
15-17: Enhance serialization safety and list immutability.Consider the following improvements:
- Add a serialVersionUID for better serialization control
- Make the list field final and return an unmodifiable view to prevent external modifications
Apply this diff to implement these improvements:
public class LightRoutes implements Serializable { + private static final long serialVersionUID = 1L; - private List<LightRoute> lightRoutes; + private final List<LightRoute> lightRoutes; + + public List<LightRoute> getLightRoutes() { + return Collections.unmodifiableList(lightRoutes); + } }Don't forget to add the import:
import java.util.Collections;uniro_backend/src/test/java/com/softeer5/uniro_backend/map/service/MapServiceTest.java (3)
43-56: Redis container configuration needs additional error handling.The Redis container setup looks good, but it could benefit from additional error handling and cleanup.
Consider adding the following improvements:
@Container static final GenericContainer<?> redis = new GenericContainer<>("redis:7.0.8-alpine") - .withExposedPorts(6379); + .withExposedPorts(6379) + .withReuse(true) + .waitingFor(Wait.forLogMessage(".*Ready to accept connections.*\\n", 1)); @BeforeAll static void setup() { + Assertions.assertTrue(redis.isRunning(), "Redis container failed to start"); redis.start(); } @DynamicPropertySource static void configureProperties(DynamicPropertyRegistry registry) { + Assertions.assertTrue(redis.isRunning(), "Redis container not running"); registry.add("spring.data.redis.host", redis::getHost); registry.add("spring.data.redis.port", () -> redis.getMappedPort(6379)); }
340-384: Test case could be more descriptive and include edge cases.The test case
nearest_test1tests for duplicate adjacent nodes but could be more comprehensive.Consider:
- Adding test cases for edge cases like zero-distance nodes
- Making the test data more maintainable by extracting coordinate constants
- Adding more descriptive assertions
@Test -@DisplayName("인접한 노드는 중복될 수 없다. a->b->b 경우 ") +@DisplayName("Should throw RouteCalculationException when adjacent nodes are duplicated (a->b->b case)") void nearest_test1(){ // given + final double BASE_LAT = 37.554533318727884; + final double BASE_LON = 127.040754103711; List<CreateRouteReqDTO> requests = List.of( - new CreateRouteReqDTO(37.554533318727884, 127.040754103711), + new CreateRouteReqDTO(BASE_LAT, BASE_LON), // ... other coordinates ... ); // when, then Assertions.assertThatThrownBy(() -> mapService.createRoute(1001L, new CreateRoutesReqDTO(savedNode.get(0).getId(), null, requests))) .isInstanceOf(RouteCalculationException.class) - .hasMessageContaining("has duplicate nearest node"); + .hasMessageContaining("has duplicate nearest node") + .satisfies(e -> { + RouteCalculationException rce = (RouteCalculationException) e; + assertThat(rce.getErrorCode()).isEqualTo(ErrorCode.DUPLICATE_NODE); + });
434-493: Test case needs better assertions for core node validation.The test
nearest_test3could benefit from more specific assertions about the core node.Consider improving the assertions:
// then List<Node> savedNodes = nodeRepository.findAll(); assertThat(savedNodes).hasSize(23); -int coreCount = 0; -for(Node n : savedNodes){ - if(n.isCore()){ - coreCount++; - } -} -assertThat(coreCount).isEqualTo(1); +assertThat(savedNodes) + .filteredOn(Node::isCore) + .hasSize(1) + .first() + .satisfies(coreNode -> { + assertThat(coreNode.getLatitude()).isEqualTo(37.55437473334863); + assertThat(coreNode.getLongitude()).isEqualTo(127.04076651784956); + });uniro_backend/src/main/resources/application-dev.yml (1)
5-6: Added HikariCP configuration for datasource.The new Hikari section sets the
maximum-pool-sizeto 30, which may help improve connection pooling performance. Please ensure that this pool size meets your workload requirements and that other relevant parameters (e.g., minimum idle, connection timeout) are either intentionally left as defaults or configured elsewhere.uniro_backend/src/main/resources/application-local.yml (2)
3-4: Hikari Connection Pool Configuration Added
The addition of thehikarisub-configuration withmaximum-pool-size: 30underspring.datasourceis clear and useful for tuning your connection pool in the local environment. Consider whether this fixed value should be further parameterized (for example, via an environment variable) if there's a need to adjust it without modifying the configuration file.
22-26: Redis Configuration Integration Review
The new Redis configuration underspring.data.redisis a significant addition. Please verify that this configuration prefix aligns with your application's requirements; typically, Spring Boot auto-configuration for Redis expects properties underspring.redis. If you are intentionally usingspring.data.redisfor custom configurations or integration with Spring Data Redis, ensure that your application logic is adjusted accordingly. Also, leveraging environment variable placeholders forhostandpasswordis a good practice.uniro_backend/src/main/java/com/softeer5/uniro_backend/building/entity/Building.java (1)
38-40: Document the purpose and rules fortrimmedName.The new field appears to be used for search functionality, but its purpose and relationship with the
namefield are not documented. Please add:
- JavaDoc explaining the field's purpose and how it differs from
name- Comments describing the trimming rules applied
Additionally, consider:
- Adding validation constraints to prevent null/empty values
- Implementing a mechanism to keep
trimmedNamein sync withname(e.g., using@PrePersistand@PreUpdate)+ /** + * A trimmed version of the building name used for search functionality. + * This field is automatically generated from the name field according to the following rules: + * [Please document the trimming rules here] + */ @Column(length = 30) + @NotNull private String trimmedName;uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RouteAuditRepository.java (3)
28-33: Consider filtering only the final revision directly.
Currently, you're fetching all revisions up to the given version, then retaining only the highest revision for each route. For very large histories, this approach can be costly. A potential optimization is to use a direct subquery or a specialized Envers method to fetch only the final revision for each route.
35-39: Clarify naming or add comments about discarding older revisions.
Since the map will retain only the latest route for each ID, renaminguniqueRoutesMapto something likelatestRoutesMap(or adding a clarifying comment) could prevent confusion about the intent.
49-63: Reduce duplication of query and filter logic.
The logic here mirrorsgetAllRoutesAtRevisionaside from the comparison operator on the revision number. Consider extracting these shared steps into a common helper method to enhance maintainability.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/RevisionContext.java (1)
8-8: Consider using withInitial() for timeStampHolder.For consistency with REVISION_TYPE_THREAD_LOCAL and to prevent potential null pointer issues, consider initializing timeStampHolder with a default value.
- private static final ThreadLocal<LocalDateTime> timeStampHolder = new ThreadLocal<>(); + private static final ThreadLocal<LocalDateTime> timeStampHolder = ThreadLocal.withInitial(() -> null);uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/aspect/RevisionOperationAspect.java (1)
41-73: Consider refactoring handler methods to reduce code duplication.The handler methods share similar patterns for parameter extraction and context management.
Consider extracting common logic into a shared method:
private Object handleRevisionOperation(ProceedingJoinPoint joinPoint, String defaultAction) throws Throwable { MethodSignature signature = (MethodSignature) joinPoint.getSignature(); String[] parameterNames = signature.getParameterNames(); Object[] args = joinPoint.getArgs(); Long univId = extractUnivId(parameterNames, args); String action = defaultAction; // Special handling for PostRiskReqDTO if needed if (defaultAction == null) { action = extractActionFromRiskDTO(args); } RevisionContext.setUnivId(univId); RevisionContext.setAction(action); try { return joinPoint.proceed(); } finally { RevisionContext.clear(); } } private Long extractUnivId(String[] parameterNames, Object[] args) { for (int i = 0; i < args.length; i++) { if (args[i] instanceof Long && "univId".equals(parameterNames[i])) { return (Long) args[i]; } } return null; } private String extractActionFromRiskDTO(Object[] args) { for (Object arg : args) { if (arg instanceof PostRiskReqDTO postRiskReqDTO) { int cautionSize = postRiskReqDTO.getCautionFactors().size(); int dangerSize = postRiskReqDTO.getDangerFactors().size(); if (cautionSize > 0) { return "주의요소 업데이트"; } else if (dangerSize > 0) { return "위험요소 업데이트"; } else { return "위험/주의요소 해제"; } } } return null; }Also applies to: 75-96, 98-119, 121-142
uniro_backend/src/main/java/com/softeer5/uniro_backend/building/service/BuildingService.java (1)
49-49: Consider handling string trimming within the service layer.The parameter name
trimmedNamesuggests that string trimming is expected to be done before reaching the service layer. This might violate separation of concerns. Consider:
- Accepting raw
nameparameter- Handling the trimming operation within the service method
-public SearchBuildingResDTO searchBuildings(Long univId, String trimmedName, Integer pageSize){ +public SearchBuildingResDTO searchBuildings(Long univId, String name, Integer pageSize){ + String trimmedName = name != null ? name.strip() : null;uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/RedisConfig.java (2)
20-25: Configure hash operation serializers for complete Redis support.The current configuration only sets key and value serializers. For full Redis operations support, consider configuring hash key and hash value serializers as well.
Apply this diff to add hash operation serializers:
template.setKeySerializer(serializer); template.setValueSerializer(fastJsonRedisSerializer); +template.setHashKeySerializer(serializer); +template.setHashValueSerializer(fastJsonRedisSerializer);
9-10:❓ Verification inconclusive
Consider using a more generic serializer configuration.
The current implementation is tightly coupled to
LightRoutes. Consider creating a generic configuration that can handle multiple types of objects, or separate configurations for different domain objects.Let's check the latest FastJson2 version and any security advisories:
🏁 Script executed:
#!/bin/bash # Check for security advisories and latest versions gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.alibaba.fastjson2") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 356
Redis Serializer Configuration – Consider Decoupling Domain-Specific Dependencies
After verifying that the FastJson2 library currently in use has no known security vulnerabilities, please consider revisiting the serializer configuration for enhanced flexibility. The current setup is tightly coupled with theLightRoutesdomain object. A more generic serializer configuration would facilitate reuse with multiple object types and improve maintainability.
- Security Check: FastJson2 has no reported security advisories (verification output returned an empty list).
- Specific Concern: The dependency on
LightRoutesmay restrict future scalability or reuse.- Recommendation: Refactor the Redis configuration to either support multiple domain types through a generic serializer or isolate the domain-specific configuration from the generic Redis template setup.
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/SchedulerConfig.java (1)
8-17:❓ Verification inconclusive
LGTM! Basic scheduler configuration is correct.
The configuration provides a properly initialized ThreadPoolTaskScheduler bean.
Consider enhancing the scheduler configuration for production readiness.
While the basic setup is correct, consider the following improvements for better maintainability and debugging:
- Externalize the pool size configuration
- Add thread naming strategy
- Configure error handling
- Add graceful shutdown behavior
Here's a suggested enhancement:
@Configuration public class SchedulerConfig { + @Value("${scheduler.pool-size:5}") + private int poolSize; + @Bean public TaskScheduler taskScheduler() { ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler(); - scheduler.setPoolSize(5); + scheduler.setPoolSize(poolSize); + scheduler.setThreadNamePrefix("scheduled-task-"); + scheduler.setErrorHandler(throwable -> + log.error("Scheduled task error", throwable)); + scheduler.setWaitForTasksToCompleteOnShutdown(true); + scheduler.setAwaitTerminationSeconds(60); scheduler.initialize(); return scheduler; } }Verify thread pool configuration across async components.
Since this is used alongside AsyncConfig (mentioned in the summary), ensure the combined thread pool sizes are appropriate for your deployment environment.
🏁 Script executed:
#!/bin/bash # Search for other thread pool configurations and async components rg -A 5 "ThreadPoolTaskExecutor|ThreadPoolTaskScheduler"Length of output: 3191
SchedulerConfig: Verify and Enhance Production Scheduler Settings
LGTM on the basic configuration—the ThreadPoolTaskScheduler bean is properly set up. However, for production readiness, please consider the following enhancements:
- Externalize the Pool Size: Replace the hardcoded value with a configurable property (e.g., using
@Value("${scheduler.pool-size:5}")).- Thread Naming: Add a naming strategy via
scheduler.setThreadNamePrefix("scheduled-task-")to simplify debugging.- Error Handling: Configure an error handler to catch and log exceptions from scheduled tasks.
- Graceful Shutdown: Enable waiting for tasks to complete on shutdown using
scheduler.setWaitForTasksToCompleteOnShutdown(true)and setting an appropriate termination timeout.Below is a suggested diff:
@Configuration public class SchedulerConfig { + @Value("${scheduler.pool-size:5}") + private int poolSize; + @Bean public TaskScheduler taskScheduler() { ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler(); - scheduler.setPoolSize(5); + scheduler.setPoolSize(poolSize); + scheduler.setThreadNamePrefix("scheduled-task-"); + scheduler.setErrorHandler(throwable -> log.error("Scheduled task error", throwable)); + scheduler.setWaitForTasksToCompleteOnShutdown(true); + scheduler.setAwaitTerminationSeconds(60); scheduler.initialize(); return scheduler; } }Additionally, since the SchedulerConfig is used alongside AsyncConfig (which configures a separate ThreadPoolTaskExecutor with its own pool settings), please verify that both async components’ thread pool configurations are balanced for your deployment environment.
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/NodeAuditRepository.java (1)
27-32: Consider potential performance issues with retrieving all revisions.By fetching all revisions of
Nodeup toversionId, this logic might incur significant overhead if there's a large volume of data. If needed, optimize further by retrieving only the final node state for each ID, or implement pagination/lazy loading strategies.uniro_backend/src/main/java/com/softeer5/uniro_backend/building/repository/BuildingCustomRepositoryImpl.java (1)
28-38: Consider adding an index for the name-based ordering.The query changes look good! The ordering by name is more intuitive for users. However, since we're now ordering by
building.name, consider adding an index to optimize query performance.Add an index on the
namecolumn in the building table to improve the performance of the ORDER BY clause.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightNode.java (2)
11-15: Consider making the class immutable.Since this is a Value Object (VO) used for caching and data transfer, consider making it immutable:
- Remove
@Setterannotation- Make fields final
- Keep only
@Getter,@NoArgsConstructor(if needed for frameworks), and@AllArgsConstructor@Getter @NoArgsConstructor -@Setter @AllArgsConstructor public class LightNode {
16-21: Consider aligning coordinate field names with Node entity.The Node entity uses x/y coordinates while LightNode uses lng/lat. This inconsistency might lead to confusion. Consider either:
- Using x/y to match Node entity
- Adding a comment explaining the coordinate system difference
public class LightNode { private long id; - private double lat; - private double lng; + private double y; // latitude + private double x; // longitudeuniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java (2)
28-32: Consider adding cache management strategy.While the local cache endpoint is well-implemented, consider adding cache eviction/refresh strategies to prevent stale data. This is especially important for route data that might change frequently.
98-104: Review service responsibility separation.Consider whether this endpoint belongs in
MapControllersince it usesAdminService. It might be more appropriate to move this to anAdminControllerto maintain clear separation of concerns.uniro_backend/src/main/java/com/softeer5/uniro_backend/external/event/RouteEventListener.java (2)
13-23: Add class-level documentation to describe the component's purpose.The class structure and dependency injection are well-implemented. Consider adding Javadoc to describe:
- The purpose of this event listener
- The scheduling behavior (30-minute window)
- The relationship with
RouteEventService@Slf4j @Component +/** + * Listens for RouteCreatedEvents and schedules height fetching tasks. + * Ensures only one task is scheduled within a 30-minute window using + * atomic operations for thread safety. + */ public class RouteEventListener {
25-43: Enhance error handling and configuration.The event handler implementation is good, but consider these improvements:
- Add error handling for
fetchHeight()- Make the delay configurable
- Add a retry mechanism
- Use English comments for consistency
+ @Value("${route.height.fetch.delay-minutes:30}") + private int fetchDelayMinutes; + @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) public void handleRouteCreatedEvent(RouteCreatedEvent event) { log.info("Routes Created Event Listen"); - // 이미 스케쥴링되고있는지 판단 + // Check if a task is already scheduled if(isScheduled.compareAndSet(false, true)) { - Date scheduledTime = new Date(System.currentTimeMillis() + TimeUnit.MINUTES.toMillis(30)); + Date scheduledTime = new Date(System.currentTimeMillis() + TimeUnit.MINUTES.toMillis(fetchDelayMinutes)); taskScheduler.schedule(() -> { try { routeEventService.fetchHeight(); + } catch (Exception e) { + log.error("Failed to fetch height: {}", e.getMessage(), e); + // Consider implementing retry logic here } finally { isScheduled.set(false); } }, scheduledTime.toInstant()); } }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java (3)
12-13: Add documentation for algorithm-related constants.Please add Javadoc comments explaining:
- The purpose and unit of measurement for
LIMIT_RANGE- The significance and impact of
HEURISTIC_WEIGHT_NORMALIZATION_FACTORon route calculations+/** Maximum range limit in meters for route calculations */ public static final double LIMIT_RANGE = 200; +/** Normalization factor used in heuristic calculations for route optimization */ public static final double HEURISTIC_WEIGHT_NORMALIZATION_FACTOR = 10.0;
14-16: Consider removing redundant string constant and translate comment.
- The
STREAM_FETCH_SIZE_AS_STRINGconstant seems unnecessary as string conversion can be done where needed.- The Korean comment should be translated to English for consistency.
public static final int STREAM_FETCH_SIZE = 2500; -//컴파일 상수를 보장하기 위한 코드 -public static final String STREAM_FETCH_SIZE_AS_STRING = "" + STREAM_FETCH_SIZE; +// Code to ensure compile-time constant +// Note: If this string constant is needed, consider using String.valueOf(STREAM_FETCH_SIZE)
22-22: Consider using an enum for status codes.Instead of using a string constant for status, consider creating an enum to handle all possible status codes. This would provide type safety and make it easier to manage all status values in one place.
-public static final String SUCCESS_STATUS = "OK";Create a new enum:
public enum UniroStatus { OK("OK"), // Add other status codes as needed ; private final String value; UniroStatus(String value) { this.value = value; } public String getValue() { return value; } }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/AsyncConfig.java (1)
15-15: Translate Korean comment to English for consistency.Consider translating the Korean comment to English:
- executor.setCorePoolSize(10); //기본 스레드 개수 10개 + executor.setCorePoolSize(10); // Default thread pool size of 10uniro_backend/src/main/java/com/softeer5/uniro_backend/external/redis/RedisService.java (1)
22-31: Consider parameterizing the TTL value.The TTL is hardcoded to 1000 minutes and the comment incorrectly states "10분 TTL". Consider making it configurable and correcting the comment.
- redisTemplate.opsForValue().set(key, value, Duration.ofMinutes(1000)); // 10분 TTL + redisTemplate.opsForValue().set(key, value, Duration.ofMinutes(ttlInMinutes)); // TTL in minutesAlso, consider adding validation for the key and value parameters:
public void saveData(String key, Object value) { + if (key == null || value == null) { + throw new IllegalArgumentException("Key and value must not be null"); + } long startTime = System.nanoTime();uniro_backend/src/main/java/com/softeer5/uniro_backend/external/elevation/MapClientImpl.java (1)
98-119: Enhance error handling and logging.The error handling in
mapElevationToNodescould be improved:
- Log the actual response status
- Include more context in error messages
- Consider retrying failed requests
private void mapElevationToNodes(ElevationResponse response, List<Node> batch) { - log.info("Current Thread: {}", Thread.currentThread().getName()); + log.debug("Processing elevation response on thread: {}", Thread.currentThread().getName()); if (!response.getStatus().equals(SUCCESS_STATUS)) { + log.error("Elevation API failed with status: {}", response.getStatus()); throw new ElevationApiException("Google Elevation API Fail: " + response.getStatus(), ErrorCode.ELEVATION_API_ERROR); } if (response.results.size() != batch.size()) { - log.error("The number of responses does not match the number of requests. " + - "request size: {}, response size: {}", batch.size(), response.results.size()); + String errorMessage = String.format( + "Response size mismatch. Expected: %d, Actual: %d", + batch.size(), response.results.size() + ); + log.error(errorMessage); throw new ElevationApiException("The number of responses does not match the number of requests. " + "request size: " + batch.size() + "response size: " + response.results.size(), ErrorCode.ELEVATION_API_ERROR); } - log.info("Google Elevation API Success. batch size: " + batch.size()); + log.debug("Successfully processed {} elevation results", batch.size());uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/entity/Univ.java (1)
78-87: Polygon coordinates method is generally fine.Iterating over
areaPolygon.getCoordinates()to build a list of lat/lng mappings works well. Validate that including the closing coordinate incoordinatesdoes not create duplicates in the final list (JTS often duplicates the first coordinate at the end). If duplicates are undesirable, filter them out.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/service/AdminService.java (1)
112-114: Ensure Redis keys are completely removed.
Relying on integer division to compute batch keys (routeCount / STREAM_FETCH_SIZE + 1) may leave behind partial or leftover keys whenrouteCountis not a multiple ofSTREAM_FETCH_SIZE. Consider removing keys in a loop until all are deleted to avoid stale data.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (2)
66-89: Consider concurrency and memory usage in local cache.
Storing routes in a staticMap<Long, List<LightRoute>>without synchronization can lead to data inconsistencies under concurrent requests. Additionally, for larger deployments with many universities, this map may grow indefinitely and impact memory usage.
309-311: Ensure Redis keys are fully invalidated.
Using(routeCount / STREAM_FETCH_SIZE) + 1to delete batch keys can leave remnants when the route count is not perfectly divisible bySTREAM_FETCH_SIZE. A loop-based approach that checks the existence of each batch key and removes it accordingly would help ensure proper cleanup.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java (5)
56-100: Review the large block inassembleRoutesfor clarity and potential redundancy.
- Lines 81–83 are now commented out. If they’re not needed, consider removing them to avoid confusion.
- You're mixing building-node logic with core-node logic. Consider splitting them into smaller methods if the code grows further.
102-107: MethoddetermineStartNodesmight be slightly confusing.The filtering condition mixes core nodes and single-connection nodes. Please add a JavaDoc or comment clarifying the rationale behind combining these two categories.
242-260: Heuristic weight methods appear logically sound, but watch performance.
getRiskHeuristicWeight(lines 242–245) squares the caution count, which may drastically inflate cost.- For large height differences,
getHeightHeuristicWeightcan become significant, though it’s capped byLIMIT_RANGE.- The overhead of computing these repeatedly may be nontrivial for large graphs; consider caching partial results if necessary.
281-323:generateResultmethod is solid but quite large.
- Combining distance, caution, and danger checks in one loop is fine, but the responsibilities (distance summation, height difference, factor flags) overlap.
- Consider further splitting the calculations for clarity and testability.
413-440: Route detail logic: clarify caution/danger transitions.
- Lines 424, 434: Setting partial distances mid-route is clever, but ensure each partial detail has accurate distance from the previous checkpoint.
- Lines 429–430, 439–440: Resetting caution/danger factors to empty lists is correct, but watch out for concurrency if these lists are ever shared data structures.
- Lines 450–451, 462: Confirm that newly calculated direction does not overwrite caution/danger states inadvertently.
Also applies to: 450-451, 462-462
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/FastestRouteDTO.java (2)
11-13: Consider removing@Setterfor immutability.Since this is a DTO with
finalfields, the@Setterannotation is unnecessary and could potentially violate immutability principles.@Getter -@Setter @RequiredArgsConstructor(access = AccessLevel.PRIVATE)
18-20: Fix formatting in static factory method.Add a space after the comma for better readability.
- return new FastestRouteDTO(prevRoute,totalWeightDistance); + return new FastestRouteDTO(prevRoute, totalWeightDistance);uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightRoute.java (1)
13-17: Add class-level documentation and define serialVersionUID.Consider adding:
- Class-level documentation to explain the purpose and usage of this lightweight route representation
- A serialVersionUID for proper serialization versioning
@Getter @NoArgsConstructor @Setter @AllArgsConstructor +/** + * A lightweight representation of Route entity for efficient caching and data transfer. + */ public class LightRoute implements Serializable { + private static final long serialVersionUID = 1L;uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/GetChangedRoutesByRevisionResDTO.java (1)
16-21: Add meaningful examples to Schema annotations.The Schema annotations have empty example values which reduces API documentation clarity.
Consider adding realistic examples:
- @Schema(description = "삭제된 길&노드 정보 정보", example = "") + @Schema(description = "삭제된 길&노드 정보 정보", example = "{\"nodeIds\": [1, 2], \"routeIds\": [10, 11]}") - @Schema(description = "현재 버전과 비교하여 변경된 주의/위험 요소 정보", example = "") + @Schema(description = "현재 버전과 비교하여 변경된 주의/위험 요소 정보", example = "[{\"routeId\": 1, \"difference\": \"CAUTION\"}]")uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/AllRoutesInfo.java (1)
22-24: Consider making batchSize immutable.Having a mutable field (
batchSize) in an otherwise immutable class could lead to thread-safety issues and violates the principle of immutability.Consider making
batchSizefinal and including it in the constructor:- @Setter @Schema(description = "배치 사이즈", example = "1") - private int batchSize = 1; + private final int batchSize; public static AllRoutesInfo of(List<NodeInfoResDTO> nodeInfos, List<CoreRouteResDTO> coreRoutes, - List<BuildingRouteResDTO> buildingRoutes) { - return new AllRoutesInfo(nodeInfos, coreRoutes, buildingRoutes); + List<BuildingRouteResDTO> buildingRoutes, int batchSize) { + return new AllRoutesInfo(nodeInfos, coreRoutes, buildingRoutes, batchSize); }uniro_backend/src/test/java/com/softeer5/uniro_backend/fixture/NodeFixture.java (1)
31-39: Consider adding parameter validation.The method follows a consistent pattern with other factory methods. Consider adding validation for the height parameter to ensure it's within acceptable bounds.
public static Node createNodeWithHeight(double x, double y, double height){ + if (height < 0) { + throw new IllegalArgumentException("Height cannot be negative"); + } return Node.builder() .univId(1001L) .isCore(false) .coordinates(geometryFactory.createPoint(new Coordinate(x,y))) .height(height) .status(HeightStatus.READY) .build(); }uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/FastestRouteResultDTO.java (2)
22-34: Improve parameter alignment in factory method.The parameter alignment in the factory method could be improved for better readability.
public static FastestRouteResultDTO of(boolean hasCaution, - boolean hasDanger, - double totalDistance, - double heightIncreaseWeight, - double heightDecreaseWeight, - List<RouteInfoResDTO> routeInfoDTOS){ + boolean hasDanger, + double totalDistance, + double heightIncreaseWeight, + double heightDecreaseWeight, + List<RouteInfoResDTO> routeInfoDTOS) {
22-34: Improve parameter alignment in factory method.The parameters in the factory method are not consistently aligned, which affects readability.
- public static FastestRouteResultDTO of(boolean hasCaution, - boolean hasDanger, - double totalDistance, - double heightIncreaseWeight, - double heightDecreaseWeight, - List<RouteInfoResDTO> routeInfoDTOS){ + public static FastestRouteResultDTO of( + boolean hasCaution, + boolean hasDanger, + double totalDistance, + double heightIncreaseWeight, + double heightDecreaseWeight, + List<RouteInfoResDTO> routeInfoDTOS) { return new FastestRouteResultDTO(hasCaution, - hasDanger, - totalDistance, - heightIncreaseWeight, - heightDecreaseWeight, - routeInfoDTOS); + hasDanger, + totalDistance, + heightIncreaseWeight, + heightDecreaseWeight, + routeInfoDTOS);uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/dto/UnivInfo.java (3)
38-42: Consider adding coordinate validation in factory method.The factory method successfully maps all required fields from the Univ entity. Consider adding validation for the coordinate values to ensure they are within valid ranges.
public static UnivInfo of(Univ univ) { + // Validate coordinates are within valid ranges + Map<String, Double> center = univ.getCenterXY(); + if (center != null) { + validateCoordinates(center.get("lat"), center.get("lng")); + } return new UnivInfo(univ.getId(), univ.getName(), univ.getImageUrl(), univ.getCenterXY(), univ.getLeftTopXY(), univ.getRightBottomXY(), univ.getAreaPolygonXY()); } +private static void validateCoordinates(Double lat, Double lng) { + if (lat == null || lng == null || + lat < -90 || lat > 90 || + lng < -180 || lng > 180) { + throw new IllegalArgumentException("Invalid coordinates"); + } +}
26-36: Consider using dedicated coordinate classes for type safety.Using
Map<String, Double>for coordinates is not type-safe and could lead to runtime errors if incorrect keys are used. Consider creating dedicated classes for coordinates.@Getter @RequiredArgsConstructor public class Coordinate { private final double lng; private final double lat; } @Getter @RequiredArgsConstructor public class AreaPolygon { private final List<Coordinate> coordinates; }Then update the fields:
- private final Map<String, Double> centerPoint; - private final Map<String, Double> leftTopPoint; - private final Map<String, Double> rightBottomPoint; - private final List<Map<String, Double>> areaPolygon; + private final Coordinate centerPoint; + private final Coordinate leftTopPoint; + private final Coordinate rightBottomPoint; + private final AreaPolygon areaPolygon;
38-42: Improve factory method readability.The constructor call has a long parameter list which affects readability. Consider breaking it into multiple lines.
public static UnivInfo of(Univ univ) { - return new UnivInfo(univ.getId(), univ.getName(), univ.getImageUrl(), univ.getCenterXY(), - univ.getLeftTopXY(), univ.getRightBottomXY(), univ.getAreaPolygonXY()); + return new UnivInfo( + univ.getId(), + univ.getName(), + univ.getImageUrl(), + univ.getCenterXY(), + univ.getLeftTopXY(), + univ.getRightBottomXY(), + univ.getAreaPolygonXY() + ); }uniro_backend/src/main/java/com/softeer5/uniro_backend/map/entity/Node.java (1)
66-68: Consider adding validation in the setter method.While the setter implementation is correct, consider adding validation to ensure the status is not set to null, maintaining the
@NotNullconstraint at runtime.public void setStatus(HeightStatus status) { + if (status == null) { + throw new IllegalArgumentException("Status cannot be null"); + } this.status = status; }uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/RouteRepository.java (1)
96-96: Consider adding @query annotation for explicit SQL.While the method name convention works, consider adding an explicit @query for consistency with other counting methods in the repository.
- int countByUnivId(Long univId); + @Query("SELECT COUNT(r) FROM Route r WHERE r.univId = :univId") + int countByUnivId(@Param("univId") Long univId);uniro_backend/src/main/java/com/softeer5/uniro_backend/map/entity/Route.java (1)
112-117: Consider adding validation and synchronization to node setters.While the setters are straightforward, consider:
- Adding null validation to maintain the
@NotNullconstraint- Adding synchronization if used in concurrent contexts
- Documenting the intended usage scope
public void setNode1(Node node1) { + if (node1 == null) { + throw new IllegalArgumentException("node1 cannot be null"); + } this.node1 = node1; } public void setNode2(Node node2) { + if (node2 == null) { + throw new IllegalArgumentException("node2 cannot be null"); + } this.node2 = node2; }uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/response/GetAllRoutesByRevisionResDTO.java (1)
25-28: Improve parameter alignment in factory method.The parameters in the factory method are not consistently aligned, which affects readability.
- public static GetAllRoutesByRevisionResDTO of(AllRoutesInfo routesInfo, GetRiskRoutesResDTO getRiskRoutesResDTO, - LostRoutesDTO lostRoutes, List<ChangedRouteDTO> changedList) { + public static GetAllRoutesByRevisionResDTO of( + AllRoutesInfo routesInfo, + GetRiskRoutesResDTO getRiskRoutesResDTO, + LostRoutesDTO lostRoutes, + List<ChangedRouteDTO> changedList) { return new GetAllRoutesByRevisionResDTO(routesInfo, getRiskRoutesResDTO, lostRoutes, changedList);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
uniro_backend/Dockerfile(1 hunks)uniro_backend/build.gradle(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/UniroBackendApplication.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/aspect/RevisionOperationAspect.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/response/ChangedRouteDTO.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/response/GetAllRoutesByRevisionResDTO.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/NodeAuditRepository.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RevInfoRepository.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RouteAuditRepository.java(3 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/service/AdminService.java(8 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/CustomReversionListener.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/RevisionContext.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/building/controller/BuildingApi.java(0 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/building/controller/BuildingController.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/building/dto/response/SearchBuildingResDTO.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/building/entity/Building.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/building/repository/BuildingCustomRepository.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/building/repository/BuildingCustomRepositoryImpl.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/building/service/BuildingService.java(4 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/AsyncConfig.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/CorsConfig.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/RedisConfig.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/SchedulerConfig.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/WebMvcConfig.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/error/ErrorCode.java(3 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/exception/GlobalExceptionHandler.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/logging/ExecutionLoggingAop.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/elevation/MapClient.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/elevation/MapClientImpl.java(3 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/event/RouteCreatedEvent.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/event/RouteEventListener.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/event/RouteEventService.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/redis/RedisService.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapApi.java(5 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java(6 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/FastestRouteDTO.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/FastestRouteResultDTO.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/AllRoutesInfo.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/FastestRouteResDTO.java(3 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/GetAllRoutesResDTO.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/GetChangedRoutesByRevisionResDTO.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/RouteDetailResDTO.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/entity/Node.java(4 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/entity/Route.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/enums/DirectionType.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/enums/HeightStatus.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/NodeRepository.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/RouteRepository.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java(4 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCacheCalculator.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java(14 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightNode.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightRoute.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightRoutes.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/controller/UnivApi.java(0 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/controller/UnivController.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/dto/SearchUnivResDTO.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/dto/UnivInfo.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/entity/Univ.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/repository/UnivCustomRepository.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/repository/UnivCustomRepositoryImpl.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/service/UnivService.java(2 hunks)uniro_backend/src/main/resources/application-dev.yml(2 hunks)uniro_backend/src/main/resources/application-local.yml(2 hunks)uniro_backend/src/main/resources/data.sql(0 hunks)uniro_backend/src/test/java/com/softeer5/uniro_backend/admin/AdminServiceTest.java(8 hunks)uniro_backend/src/test/java/com/softeer5/uniro_backend/external/MapClientImplTest.java(1 hunks)uniro_backend/src/test/java/com/softeer5/uniro_backend/fixture/NodeFixture.java(3 hunks)uniro_backend/src/test/java/com/softeer5/uniro_backend/map/service/MapServiceTest.java(3 hunks)uniro_backend/src/test/java/com/softeer5/uniro_backend/map/service/RouteCalculatorTest.java(1 hunks)uniro_backend/src/test/resources/sql/delete-all-data.sql(1 hunks)uniro_backend/src/test/resources/sql/insert-univ-data.sql(1 hunks)
💤 Files with no reviewable changes (3)
- uniro_backend/src/main/resources/data.sql
- uniro_backend/src/main/java/com/softeer5/uniro_backend/building/controller/BuildingApi.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/controller/UnivApi.java
✅ Files skipped from review due to trivial changes (4)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/map/enums/HeightStatus.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/external/event/RouteCreatedEvent.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/external/elevation/MapClient.java
- uniro_backend/src/test/java/com/softeer5/uniro_backend/external/MapClientImplTest.java
🔇 Additional comments (81)
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/response/ChangedRouteDTO.java (1)
19-21: LGTM! Factory method updated consistently.The static factory method has been updated to use the new parameter name
difference, maintaining consistency with the field rename.uniro_backend/src/test/resources/sql/delete-all-data.sql (1)
19-20: Added auto-increment reset statements for new tablesThe newly introduced statements for
route_audandnode_audappropriately extend the reset logic to all relevant tables. These additions ensure consistency with the existing auto-increment resets in the script, aligning with our database cleanup requirements.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RevInfoRepository.java (1)
10-10: LGTM!The addition of the Optional import is necessary for the new method signatures.
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/GetAllRoutesResDTO.java (4)
3-4: LGTM!The List import is correctly placed and necessary for the class fields.
21-22: LGTM!The versionId field is well-documented and follows immutability best practices with private final modifiers.
24-25: LGTM!The factory method signature is properly updated to include the versionId parameter with logical parameter ordering.
26-26: LGTM!The factory method implementation correctly forwards all parameters to the constructor.
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/error/ErrorCode.java (3)
12-12: LGTM! Standard internal server error code added.The addition of
INTERNAL_SERVER_ERRORwith status 500 is a necessary and standard error code for handling unexpected server-side issues.
22-22: LGTM! Business validation error code added.The
CREATE_ROUTE_LIMIT_EXCEEDEDerror with status 400 is appropriately placed in the route-related section and correctly uses 400 for a client-side validation error.
38-39: LGTM! Standard resource not found error added.The
UNIV_NOT_FOUNDerror with status 404 follows REST conventions for resource not found scenarios.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightRoutes.java (1)
1-10: LGTM! Well-organized package structure and imports.The package structure follows Java conventions, and imports are appropriately selected for the required functionality.
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/WebMvcConfig.java (1)
3-19: Verify the necessity of @EnableWebMvc annotation.Adding
@EnableWebMvcdisables Spring Boot's auto-configuration for Spring MVC, which might remove some useful default configurations. Consider if this is really necessary for your use case.Additionally, several imports are unused since the
configureMessageConvertersmethod is commented out:
StandardCharsetsCollectionsMediaType- FastJson2 related imports
Please confirm if
@EnableWebMvcis required. If not, you can safely remove it along with the unused imports.Also applies to: 24-26
uniro_backend/src/main/resources/application-dev.yml (1)
1-46: Removal of H2 console configuration.The removal of the H2 database console settings (
h2.console.enabledandh2.console.path) is noticeable. This is likely intentional given the shift to a MySQL datasource and the new pooling/Redis configurations; however, ensure that any development or debugging workflows that previously relied on the H2 console have been appropriately updated or are no longer required.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/RouteDetailResDTO.java (3)
4-4: LGTM! Import is properly organized.The new import for DangerFactor enum is correctly placed with other enum imports and follows the established package structure.
29-34: LGTM! Factory method properly updated.The factory method has been correctly updated to include the new dangerFactors parameter while maintaining consistent formatting and following established patterns.
26-27:❓ Verification inconclusive
Verify the example values in the Swagger documentation.
The example values for dangerFactors (
["SLOPE", "STAIRS"]) appear to be copied from cautionFactors. Please verify if these are valid DangerFactor enum values.
🏁 Script executed:
#!/bin/bash # Description: Check the actual values defined in DangerFactor enum ast-grep --pattern 'enum DangerFactor { $$$ }'Length of output: 50
Verify DangerFactor example values manually
The Swagger documentation example for
dangerFactors(["SLOPE", "STAIRS"]) appears to have been copied from cautionFactors. The automated search for values in theDangerFactorenum did not return any conclusive output. Please manually inspect theDangerFactorenum in the codebase to confirm that these example values are valid. If they aren’t valid, update the Swagger example accordingly.uniro_backend/src/main/resources/application-local.yml (1)
19-21: SQL Initialization Settings Commented Out
Commenting out the SQL initialization block (spring.sql.init) signals a potential shift in your database initialization strategy. Make sure that if you're moving to an alternative mechanism (like Flyway, Liquibase, etc.), all necessary initialization steps are covered elsewhere to avoid unexpected schema issues.uniro_backend/src/main/java/com/softeer5/uniro_backend/building/controller/BuildingController.java (2)
42-42: LGTM! Pagination size adjustment looks good.The increase in default page size from 6 to 10 is a reasonable change, and the parameter configuration follows REST API best practices.
44-44:❓ Verification inconclusive
Verify frontend compatibility with pagination changes.
The removal of cursor-based pagination in favor of simple pagination needs to be coordinated with frontend changes to ensure compatibility.
🏁 Script executed:
#!/bin/bash # Search for frontend components that might be affected by the pagination change rg -l "cursorId|cursor-id" --type ts --type tsxLength of output: 77
Below is an updated verification step to help confirm that the frontend no longer depends on the removed cursor-based pagination. The previous search command failed due to the unrecognized file type "tsx." Please re-run the following script to search for any references to
cursorIdin both.tsand.tsxfiles. Once verified, please confirm that no frontend component is expecting the parameter:#!/bin/bash # Re-run the search for cursor-based pagination references in frontend files using file globs for .ts and .tsx rg -l "cursorId|cursor-id" -g "*.ts" -g "*.tsx"Verify frontend compatibility with pagination changes.
The backend change—removing the
cursorIdparameter—aligns with the updated pagination strategy. However, to ensure a smooth transition, please manually verify that no frontend components rely on the removed parameter for pagination and that simple pagination operates correctly.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RouteAuditRepository.java (1)
3-6: Imports look good.
No issues with these newly added imports; they're appropriate for the updated data structures used below.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/CustomReversionListener.java (1)
11-13: LGTM! Clean implementation of timestamp handling.The timestamp check and assignment follow the same pattern as the existing univId and action handling, with proper null checking.
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/RevisionContext.java (2)
35-41: LGTM! Clean implementation of timestamp management.The getter and setter methods follow the established pattern and maintain thread safety.
43-48:✅ Verification successful
Verify proper cleanup in all code paths.
The clear() method correctly removes all ThreadLocal values. However, ensure that clear() is called in all code paths to prevent memory leaks.
🏁 Script executed:
#!/bin/bash # Search for try-finally blocks that might be missing clear() calls rg -U "try\s*\{[^}]*\}\s*finally\s*\{[^}]*\}" --type javaLength of output: 4973
RevisionContext cleanup confirmed in all critical try-finally blocks.
The shell script output shows that all invocations ofRevisionContext.clear()are properly placed within the finally blocks (e.g., inRevisionOperationAspect.javaandDisableEnversAspect.java), ensuring thread-local cleanup even when exceptions occur. No missing clear() calls were detected in these paths.uniro_backend/src/main/java/com/softeer5/uniro_backend/building/service/BuildingService.java (3)
8-8: LGTM! Import changes reflect package restructuring.The imports have been appropriately updated to accommodate the package restructuring and new enum usage.
Also applies to: 12-12
75-75: LGTM! Node status initialization is properly handled.Setting the initial status to
HeightStatus.READYbefore fetching heights provides clear state management for the elevation data processing pipeline.
51-57:❓ Verification inconclusive
Verify the impact of removing cursor-based pagination.
The removal of cursor-based pagination in favor of a simple limit-based approach might impact the handling of large datasets. Consider:
- The performance implications for large result sets
- How the client will handle pagination without cursor information
- Whether this aligns with the frontend's pagination requirements
🏁 Script executed:
#!/bin/bash # Check if there are any remaining cursor-based pagination implementations # or if this is a project-wide change # Search for cursor-related code echo "Searching for cursor-related pagination code..." rg -l "cursor|CursorPage" --type java # Search for other pagination implementations echo "Searching for other pagination implementations..." rg -l "Page|Pageable" --type javaLength of output: 873
Attention: Confirm the New Pagination Strategy
The change in
BuildingServicenow returns a fixed-size list without embedding cursor-based metadata. Given that cursor-related pagination logic still exists in files such asuniro_backend/src/main/java/com/softeer5/uniro_backend/common/CursorPage.javaand the custom repository implementations, please make sure that:
- The client-side handling of large datasets is updated to work without cursor information.
- Significant performance implications for large result sets have been evaluated.
- This simplified approach aligns with the overall design and frontend requirements.
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/SchedulerConfig.java (1)
1-7: LGTM! Clean package structure and imports.The package structure follows Java conventions, and imports are appropriately scoped to only what's needed.
uniro_backend/src/main/java/com/softeer5/uniro_backend/building/dto/response/SearchBuildingResDTO.java (2)
18-20: LGTM! Factory method simplified appropriately.The
offactory method has been correctly simplified to match the removal of pagination-related fields. The implementation maintains clean code principles with a clear single responsibility.
1-21:❓ Verification inconclusive
LGTM! Clean removal of cursor-based pagination.
The simplification of the DTO by removing cursor-based pagination fields (
nextCursorandhasNext) makes the code more maintainable. The class now focuses solely on its core responsibility of carrying building data.Let's verify that all related pagination code has been properly removed:
🏁 Script executed:
#!/bin/bash # Search for any remaining cursor-based pagination references rg -i 'cursor|hasNext' --type javaLength of output: 1294
Review on Pagination Refactoring in SearchBuildingResDTO
The updated
SearchBuildingResDTOis correctly simplified—removing the pagination fields (nextCursorandhasNext) allows the DTO to focus purely on delivering building data through thedatafield, and the factory method is streamlined accordingly.However, a repository-wide search still reveals references to pagination, specifically the
CursorPageclass (with itsnextCursorandhasNextfields) in:
uniro_backend/src/main/java/com/softeer5/uniro_backend/building/repository/BuildingCustomRepository.javauniro_backend/src/main/java/com/softeer5/uniro_backend/building/repository/BuildingCustomRepositoryImpl.javauniro_backend/src/main/java/com/softeer5/uniro_backend/common/CursorPage.javaAction Items:
- Confirm Intent: Verify if these remaining pagination components are expected as part of a phased migration.
- Review Impact: Ensure that retaining these references does not introduce inconsistencies with the simplified DTO.
- Plan Next Steps: If the goal is a complete removal of cursor-based pagination, plan further refactoring in the repository and common modules.
Overall, the DTO changes are good, but please double-check the overall pagination strategy for the codebase to ensure consistency.
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/NodeAuditRepository.java (2)
3-6: Imports look good.These newly added imports (
ArrayList,HashMap,Map) are appropriate for the revised logic in the subsequent code.
34-38: Verify that overwriting revisions to keep only the latest state is intended.Because we iterate in ascending order by revision number, the map will hold the final version for each node ID. This ensures you get the node's latest revision, but if you need to preserve older states, you may need a different approach. Also note that a regular
HashMapdoes not guarantee insertion order, so if a specific order is required in the returned list, consider sorting or using a linked map.uniro_backend/src/main/java/com/softeer5/uniro_backend/building/repository/BuildingCustomRepositoryImpl.java (1)
26-26:❓ Verification inconclusive
Verify the impact of removing cursor-based pagination.
The removal of cursor-based pagination could affect the client's ability to handle large datasets efficiently. Please ensure that:
- The frontend has been updated to handle the new non-paginated response
- The performance impact of fetching all buildings at once has been considered
- The
pageSizeparameter is still sufficient for limiting the result set
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining cursor-based pagination references # and verify frontend adaptations # Check for any remaining cursor references in the codebase echo "Checking for remaining cursor references..." rg -i "cursor" --type java # Check for related frontend changes echo "Checking for frontend pagination handling..." rg -i "building.*pagination|pagination.*building" --type typescript --type javascriptLength of output: 919
Verify the impact of removing cursor-based pagination
The changes to the
searchBuildingsmethod inBuildingCustomRepositoryImpl.java(line 26) remove cursor-based pagination and simplify the API by:
- Renaming the
nameparameter totrimmedName(suggesting pre-processing)- Removing the
cursorIdparameter- Changing the return type from
CursorPage<List<BuildingNode>>toList<BuildingNode>However, the shell script revealed that references to
CursorPagestill exist in other parts of the code (e.g., inuniro_backend/common/CursorPage.javaand the repository interface). Please ensure that these remaining references are either no longer in use or have been properly addressed.Additionally, because the frontend pagination check was inconclusive (the search for TypeScript files yielded an “unrecognized file type” error), manually verify that the frontend has been updated to handle the new, non-paginated response (e.g., confirm that any pagination components or logic relying on
CursorPagehave been adjusted accordingly). Also, review the performance implications of fetching larger, non-paginated datasets using thepageSizeparameter.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightNode.java (2)
1-10: LGTM! Well-organized imports and package structure.The imports are properly organized and include all necessary dependencies for JSON serialization (Jackson) and boilerplate reduction (Lombok).
23-28:✅ Verification successful
Verify coordinate mapping in the constructor.
The constructor maps Node.x to lng and Node.y to lat. Please verify this mapping is intentional as it's different from typical geographic coordinate systems where:
- Longitude (lng) corresponds to x-coordinate
- Latitude (lat) corresponds to y-coordinate
🏁 Script executed:
#!/bin/bash # Search for other coordinate system usages in the codebase rg -l "lat|lng|latitude|longitude" --type javaLength of output: 2386
Confirm coordinate mapping in LightNode constructor
The constructor assigns
node.getY()tolatandnode.getX()tolng. Our search across the codebase (e.g., in GeoUtils and other related classes) shows this pattern is consistently used. Please double-check that mapping Node.x to longitude and Node.y to latitude is intentional per the domain model and expected geographic conventions.
- File:
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/vo/LightNode.java(Lines 23–28)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java (1)
73-79: Great improvement in API response!The change to return created route information instead of void provides better feedback to clients and follows REST best practices.
uniro_backend/src/main/java/com/softeer5/uniro_backend/external/event/RouteEventListener.java (2)
1-12: LGTM! Package structure and imports are well-organized.The package structure appropriately reflects the component's role in handling external events, and all necessary imports are present.
17-17:❓ Verification inconclusive
Verify RouteEventService implementation.
Ensure that
RouteEventService.fetchHeight()is properly implemented with appropriate error handling and logging.Also applies to: 35-35
🏁 Script executed:
#!/bin/bash # Search for RouteEventService implementation ast-grep --pattern 'class RouteEventService { $$$ fetchHeight() { $$$ } $$$ }'Length of output: 92
Below is a revised review comment that now asks for additional manual verification since the initial
ast-grepsearch produced no output. I've also included a new shell script to help locate theRouteEventServicedefinition and itsfetchHeight()method:
Action Required: Verify RouteEventService Implementation
Please verify that the
RouteEventServiceclass exists and that itsfetchHeight()method is implemented with proper error handling and logging. The initial search usingast-grepproduced no output, so use the script below for further investigation.
- Steps to verify:
- Ensure that
class RouteEventServiceis defined in the codebase.- Confirm that the
fetchHeight()method is present within this class.- Check that the implementation includes appropriate error handling and logging.
Suggested Verification Script:
These checks should also extend to the integration in
RouteEventListener.java(notably at lines 17 and 35).
🏁 Script executed:
[run_scripts] #!/bin/bash # Searching for the RouteEventService class definition echo "Searching for 'class RouteEventService' definition:" rg -n 'class\s+RouteEventService' || echo "No RouteEventService definition found." # Searching for the fetchHeight method implementation echo "Searching for 'fetchHeight' method within the codebase:" rg -n 'fetchHeight\s*\(' || echo "No fetchHeight method found."Length of output: 836
Action Required: Verify Error Handling and Logging in RouteEventService.fetchHeight() Implementation
- The
RouteEventServiceclass was found inuniro_backend/src/main/java/com/softeer5/uniro_backend/external/event/RouteEventService.java(starting at line 17), and thefetchHeight()method is defined at line 22.- The
RouteEventListenercorrectly invokesfetchHeight()at line 35.- Next Steps: Please manually review the body of
fetchHeight()to ensure it includes the required error handling (e.g., try/catch blocks) and proper logging practices, as the shell script output confirmed the method signature but did not provide its internal details.uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java (1)
18-18:❓ Verification inconclusive
Verify operational limits and add documentation.
Please verify and document:
- Is 2000 routes an appropriate limit based on system capacity?
- Is 300 the optimal batch size for Google API calls considering their quotas and rate limits?
Also applies to: 21-21
🌐 Web query:
What are the current batch size limits for Google Maps API?💡 Result:
Google Maps APIs have varying batch size limits depending on the specific service used:
Directions API
- 8 waypoints allowed between start/end points[1]
Distance Matrix API
- 25 origins or 25 destinations per request[6]
- 100 elements (origin-destination pairs) maximum per server/client-side request[6]
Maps JavaScript API
- 30K requests/minute overall rate limit[9]
- 300 requests/minute per IP address[9]
General Batching Notes
For services without native batching:
- Developers often split large datasets into chunks meeting individual API limits
- Common strategies include:
- Sequential processing with delays between batches[8]
- Parallel processing within rate constraints[9]
- Error handling for failed requests[8]
These limits ensure system stability while allowing scalable implementations through proper batch management techniques[1][6][8].
Citations:
- 1: https://lemonharpy.wordpress.com/2011/12/15/working-around-8-waypoint-limit-in-google-maps-directions-api/
- 2: http://ads-developers.googleblog.com/2022/04/upcoming-enforcement-of-custom-batch.html
- 3: https://cloud.google.com/batch/quotas
- 4: https://developers.arcgis.com/python/latest/guide/part4-batch-geocoding/
- 5: https://developers.google.com/admin-sdk/directory/v1/guides/batch
- 6: https://developers.google.com/maps/documentation/distance-matrix/usage-and-billing
- 7: https://groups.google.com/g/google-content-api-for-shopping/c/dBxmale8tLk
- 8: https://www.geoapify.com/tutorial/batch-geocoding-js-and-rate-limits/
- 9: https://developers.google.com/maps/documentation/javascript/usage-and-billing
- 10: https://developers.google.com/shopping-content/guides/how-tos/batch
Action Required: Validate and Document Operational Limits
- Route Limit: Confirm that the
CREATE_ROUTE_LIMIT_COUNT = 2000is sustainable in production based on your system’s capacity and load testing results. If adjustments or further justification are needed, please document the reasoning.- Google API Batch Size: Verify that
MAX_GOOGLE_API_BATCH_SIZE = 300aligns with the current API constraints. Recent references indicate that for the Maps JavaScript API, a limit of 300 requests/minute per IP may be applicable. Ensure this value is optimal for your use case and document any dependencies or assumptions regarding this limit.Please update inline documentation in
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.javato clearly explain the derivations and intended usage of these limits.uniro_backend/src/main/java/com/softeer5/uniro_backend/UniroBackendApplication.java (1)
6-6: LGTM! Async processing capability enabled.The addition of
@EnableAsynccorrectly enables asynchronous method execution across the application. This will allow methods annotated with@Asyncto run in separate threads.Also applies to: 10-10
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/AsyncConfig.java (2)
13-18: LGTM! Thread pool configuration is well-structured.The thread pool configuration with core size of 10, max size of 20, and queue capacity of 100 provides a good balance for handling async tasks.
19-30: LGTM! Request context propagation is properly handled.The task decorator correctly preserves and propagates request context for async tasks, ensuring request-scoped data remains available.
uniro_backend/build.gradle (2)
75-77: LGTM! Prometheus monitoring support added.The addition of Micrometer Prometheus registry will enable metrics collection and monitoring capabilities.
91-96: LGTM! Comprehensive caching and serialization support added.The additions include:
- Redis for distributed caching
- Spring Boot Cache abstraction
- JSON serialization support for Java 8 time and Hibernate 6
- FastJSON2 for Spring 6
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/CorsConfig.java (1)
18-18:❓ Verification inconclusive
Good security practice: Restricting CORS headers!
The change from wildcard
*to specific headers is a security improvement that follows the principle of least privilege.However, let's verify if the new Redis and Prometheus features require any additional CORS headers:
Also, please verify the following:
- Does Redis require any specific headers for cross-origin requests?
- Does Prometheus require any headers for metrics collection?
🏁 Script executed:
#!/bin/bash # Description: Check if Redis or Prometheus configurations require additional CORS headers # Search for Redis and Prometheus configurations that might need CORS headers rg -i '(redis|prometheus).*header' --type javaLength of output: 48
🌐 Web query:
What CORS headers are required for Redis and Prometheus in Spring Boot?💡 Result:
CORS headers are primarily relevant for browser-based requests between different domains. For Redis and Prometheus integrations in Spring Boot:
Redis (Spring Data Redis):
- No specific CORS headers required as Redis interactions occur server-side
- Browser-to-server CORS rules don't apply to direct Redis operations[1][7]
Prometheus (Actuator Endpoints):
When exposing metrics via Spring Boot Actuator:# Minimum recommended configuration management.endpoints.web.cors.allowed-origins=https://your-prometheus-domain management.endpoints.web.cors.allowed-methods=GETFull example configuration:
management: endpoints: web: cors: allowed-origins: "https://prometheus.example.com" allowed-methods: "GET" allowed-headers: "*" exposed-headers: "Content-Disposition" max-age: 3600Key considerations:
- Prometheus typically scrapes metrics server-to-server (no browser involved), making CORS unnecessary in most cases[3][7]
- If accessing actuator endpoints directly from browser-based tools:
- Configure allowed-origins to match your monitoring UI domain
- Enable required HTTP methods (usually GET)
- Set appropriate max-age for preflight caching[3][8]
For mixed monitoring setups combining Prometheus and browser-based tools:
@Configuration public class MonitoringCorsConfig implements WebMvcConfigurer { @Override public void addCorsMappings(CorsRegistry registry) { registry.addMapping("/actuator/**") .allowedOrigins("https://prometheus.example.com", "https://grafana.example.com") .allowedMethods("GET", "OPTIONS"); } }This configuration ensures both Prometheus server and Grafana UI can access metrics endpoint while maintaining security constraints[3][7][10].
Citations:
- 1: https://docs.spring.io/spring-cloud-gateway/reference/spring-cloud-gateway/cors-configuration.html
- 2: https://www.restack.io/p/java-spring-boot-answer-cors-configuration
- 3: https://docs.spring.io/spring-boot/docs/2.4.3/reference/html/production-ready-features.html
- 4: https://docs.spring.io/spring-boot/docs/2.1.7.RELEASE/reference/html/production-ready-endpoints.html
- 5: https://docs.spring.io/spring-data/rest/reference/customizing/configuring-cors.html
- 6: https://spring.io/guides/gs/rest-service-cors
- 7: https://github.com/burakahmetyoruk/redis-prometheus-grafana
- 8: https://docs.spring.io/spring-boot/appendix/application-properties/index.html
- 9: https://www.cnblogs.com/davidwang456/articles/13728133.html
- 10: https://reflectoring.io/spring-cors/
Security Update: Validated CORS Allowed Headers
The update restricting allowed headers to
"Authorization","Content-Type","Accept", and"X-Requested-With"is a sound security improvement following the principle of least privilege. Our investigation confirms that:
- Redis Integration: As interactions occur server-side, no additional CORS headers are required.
- Prometheus Integration: CORS is generally not needed for Prometheus actuator endpoints since these are typically scraped server-to-server. However, if you plan to access these endpoints directly from a browser (for example, when integrating with Grafana), ensure a dedicated CORS configuration is set up in your management settings (rather than in the global CORS config).
Please verify that any Prometheus actuator endpoint access via a browser has the appropriate CORS settings configured separately.
uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/service/UnivService.java (2)
19-20: Simplified method signature looks good.Removing cursor-based pagination in favor of a simpler two-parameter approach makes the code more straightforward. Ensure that calling code handles the optional nature of
pageSizeto avoid potential NPE issues.
22-24: Appreciate the streamlined data mapping.The direct
.map(UnivInfo::of)usage and finalSearchUnivResDTO.of(univInfos)call are clear and concise. This eliminates intermediate steps and improves readability.uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/repository/UnivCustomRepositoryImpl.java (3)
20-20: Removed cursor parameter aligns with simplified design.The new signature
public List<Univ> searchUniv(String trimmedName, Integer pageSize)is consistent with the service changes. Consider validating thatpageSizeis non-null or within an acceptable range in upstream code to avoid potential runtime issues.
22-28: Query structure is straightforward and readable.Using
.selectFrom(univ)with a direct.orderBy(univ.name.asc())and.limit(pageSize)is simpler than cursor-based logic. This is an improvement if cursor-based pagination is no longer needed.
31-32: Conditional logic for name filtering is concise.Returning
nullfor theBooleanExpressionwhentrimmedNameis empty effectively omits the condition. This is a known Querydsl pattern and should be fine for optional name-based filtering.uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/entity/Univ.java (3)
34-55: Added fields expand the entity’s spatial data.Fields like
trimmedName,point-based fields, andPolygonsupport advanced geographic needs. The use of@NotNullis appropriate to ensure data consistency. Validate that any migration scripts handle existing rows properly for these new columns.
66-68: Center point mapping is clear.The method returns latitude and longitude in a way that’s consistent with typical (lat, lng) usage, referencing (Y, X) in JTS.
70-72: Left top point accessor is consistent.Similarly to getCenterXY, this returns lat/lng in the correct (Y, X) manner.
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCacheCalculator.java (1)
104-107: Verify non-null adjacency lists before iteration.
IfadjMap.get(now.getId())is ever null or missing during BFS, it may trigger a NullPointerException. Consider adding a null check or gracefully handling disconnected nodes to avoid potential runtime errors.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/service/AdminService.java (1)
249-256: Verify x and y usage for node coordinates.
The code callsnode.getX()andnode.getY(), which might differ from lat/lng if the coordinate system is not consistent. Ensure these fields correctly represent the node’s geographic location and align with any lat/lng usage elsewhere in the codebase.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java (5)
12-13: Imports seem correct and necessary.These imports for
FastestRouteDTO,FastestRouteResultDTO,DangerFactor, andHeightStatusappear to be in use later in the code for the updated routing logic.Also applies to: 18-18, 21-21
48-48: Consider clarifying the naming and usage ofcautionCount.
cautionCountimplies a count of caution factors, but it’s updated in the route exploration logic. Be sure to reset or handle its state correctly when exploring multiple routes.
132-135: ValidatefastestRouteDTOusage incalculateFastestRoute.You're skipping routes if
fastestRouteDTOisnull. Ensure this is an intended fallback and that no partial routes are lost due to, for example, a misconfiguration in policy-based route filtering.
196-240: A logic infindFastestRoute: confirm consistent updates tocautionCount.*
- You increase
currentCautionCountfor each caution route (lines 222–225) but do not reset it for subsequent non-caution routes. This can accumulate caution counts across multiple edges, potentially inflating the path cost.- Ensure the final cost for the end node remains consistent if you re-enter caution segments.
330-353: Direction calculation: confirm indexing logic.
- You handle “idx-1 ≥ 0” and “idx+2 ≤ ...” to adjust first/third nodes. Off-by-one errors are possible here.
- Double-check how each boundary condition influences the final direction type.
uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/repository/UnivCustomRepository.java (1)
3-3: Streamlined repository method signature.Changing to
List<Univ> searchUniv(String name, Integer pageSize)removes pagination logic. Confirm that the calling code can handle the potentially larger result sets.Also applies to: 8-8
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/enums/DirectionType.java (1)
10-11: New enum valuesCAUTIONandDANGER.The approach is straightforward for indicating route hazards. Double-check that existing logic does not break when encountering these new values.
uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/dto/SearchUnivResDTO.java (1)
18-19: LGTM! Simplified DTO structure.The removal of cursor-based pagination and simplification of the
ofmethod improves code maintainability. The changes align well with the broader refactoring effort across related components.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/NodeRepository.java (1)
21-23: LGTM! Well-structured repository methods.The new finder methods follow Spring Data JPA naming conventions and integrate well with the HeightStatus enum. The methods enhance the repository's querying capabilities while maintaining clean code principles.
uniro_backend/src/main/java/com/softeer5/uniro_backend/univ/controller/UnivController.java (1)
19-21:❓ Verification inconclusive
Verify frontend compatibility with pagination changes.
The removal of cursor-based pagination and change in default page size from 6 to 10 could impact the frontend implementation.
Please ensure:
- Frontend pagination logic has been updated to handle offset-based pagination
- UI components can handle the new default page size of 10 items
- No regressions in scroll or load-more functionality
🏁 Script executed:
#!/bin/bash # Search for frontend pagination-related code rg -l "pageSize|cursorId" --type js --type tsLength of output: 196
The initial ripgrep command applied file type filters that prevented any files from being searched. Please run the above script manually to verify if any frontend code (possibly outside JavaScript/TypeScript) references "pageSize" or "cursorId".
Action Required: Verify Frontend Pagination Handling
The changes in
UnivController.javaremove cursor-based pagination and update the default page size from 6 to 10. This may impact the frontend's pagination logic. Please ensure:
- The frontend code now correctly uses offset-based pagination.
- UI components and load-more/scroll functionality correctly handle the new default value of 10 items.
- If applicable, adjust any pagination-related logic that previously relied on the
cursorIdparameter.Manual verification is advised due to the uncertainty around frontend file types and locations.
🏁 Script executed:
[run_scripts] #!/bin/bash # Retry searching for frontend pagination references in the entire repository (no file type filters) rg -n "pageSize|cursorId" .Length of output: 3019
ACTION: Verify Frontend Pagination Handling
The changes in
UnivController.java—removing thecursorIdparameter and updating the defaultpageSizefrom 6 to 10—impact the pagination logic. While our backend search shows that offset-based pagination is now in use (as seen in the declared parameters in multiple controllers), no direct frontend JavaScript/TypeScript references for cursor-based pagination were found.Please ensure the following:
- Frontend Pagination Logic: Confirm that any frontend components consuming these endpoints have been updated to work with offset-based pagination instead of the previous cursor-based approach.
- Default Page Size: Verify that the frontend is expecting and correctly handling the new default page size of 10.
- Integration Check: If the frontend code is maintained in a separate repository or module, coordinate with the respective team to ensure no regressions in scroll or load-more functionality.
Manual verification is advised to ensure the changes align with the frontend behavior.
uniro_backend/src/test/java/com/softeer5/uniro_backend/fixture/NodeFixture.java (2)
18-18: LGTM! Setting default status for new nodes.Setting
HeightStatus.READYas the default status for new nodes is appropriate.
27-27: LGTM! Consistent status initialization.Setting
HeightStatus.READYmaintains consistency with thecreateNodemethod.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/response/GetAllRoutesByRevisionResDTO.java (1)
17-17: LGTM! Type change is well-documented.The change from
GetAllRoutesResDTOtoAllRoutesInfois consistent across the field declaration and factory method, with clear Swagger documentation.Also applies to: 25-26
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/entity/Node.java (2)
46-48: LGTM! Well-structured field declaration.The
statusfield is properly declared with appropriate annotations:
@NotNullensures the field is always populated@Enumerated(EnumType.STRING)stores enum values as strings, making them more maintainable and readable in the database
86-92: LGTM! Constructor properly initialized with new field.The constructor correctly initializes all fields including the new
statusfield.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/FastestRouteResDTO.java (2)
17-20: LGTM! Clear separation of safety concerns.Good separation between caution and danger indicators with clear Swagger documentation for API consumers.
34-52: LGTM! Factory method properly updated.The
ofmethod correctly handles all parameters while maintaining immutability through the private constructor.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/RouteRepository.java (2)
24-28: LGTM! Well-optimized streaming query.Good use of:
@EntityGraphfor efficient loading of related entities@QueryHintfor optimizing fetch sizeStreamreturn type for memory-efficient processing of large datasets
29-38: LGTM! Efficient lightweight route query.Excellent optimization using:
- Constructor expression in JPQL for efficient object creation
- Stream return type for memory efficiency
- Fetch size optimization through query hints
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapApi.java (3)
33-38: LGTM! Good use of Server-Sent Events for real-time map updates.Using SSE for streaming map updates is a good choice as it provides real-time data with less overhead compared to WebSocket for one-way server-to-client communication.
70-71: LGTM! Return type change improves API response.Changing the return type from
voidtoAllRoutesInfoprovides better feedback to clients about the created routes.
91-97: LGTM! Version comparison endpoint facilitates change tracking.The new endpoint for comparing route versions is well-documented with clear API responses.
uniro_backend/src/test/java/com/softeer5/uniro_backend/map/service/RouteCalculatorTest.java (2)
282-321: LGTM! Comprehensive test for height-based routing.The test thoroughly validates route finding with height considerations, including:
- Node height setup
- Distance calculations
- Result size verification
323-369: LGTM! Well-structured test for caution factors.The test effectively validates route finding with caution factors by:
- Setting up routes with caution factors
- Verifying multiple route options
- Checking expected distances
uniro_backend/src/test/java/com/softeer5/uniro_backend/admin/AdminServiceTest.java (3)
65-78: LGTM! Well-configured Redis test container.Good use of testcontainers for Redis integration testing:
- Proper port configuration
- Dynamic property registration
- Clean container lifecycle management
768-814: LGTM! Thorough test for basic version comparison.The test effectively validates route version comparison by:
- Setting up multiple versions
- Verifying lost routes
- Checking node and route counts
1083-1131: LGTM! Comprehensive test for risk factor changes.The test thoroughly validates risk and caution factor changes between versions:
- Sets up routes with different factors
- Verifies changes in factors
- Checks expected route modifications
| // @Override | ||
| // public void configureMessageConverters(List<HttpMessageConverter<?>> converters) { | ||
| // FastJsonHttpMessageConverter converter = new FastJsonHttpMessageConverter(); | ||
| // //custom configuration... | ||
| // FastJsonConfig config = new FastJsonConfig(); | ||
| // config.setDateFormat("yyyy-MM-dd HH:mm:ss"); | ||
| // config.setReaderFeatures(JSONReader.Feature.FieldBased, JSONReader.Feature.SupportArrayToBean); | ||
| // config.setWriterFeatures(JSONWriter.Feature.WriteMapNullValue, JSONWriter.Feature.PrettyFormat); | ||
| // converter.setFastJsonConfig(config); | ||
| // converter.setDefaultCharset(StandardCharsets.UTF_8); | ||
| // | ||
| // converter.setSupportedMediaTypes(List.of( | ||
| // MediaType.APPLICATION_JSON, // application/json 지원 | ||
| // new MediaType("application", "json", StandardCharsets.UTF_8), | ||
| // new MediaType("application", "openmetrics-text", StandardCharsets.UTF_8) | ||
| // )); | ||
| // | ||
| // converters.add(0, converter); | ||
| // converters.forEach(c -> log.info("✔ {}", c.getClass().getName())); | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or implement the commented message converter configuration.
Large blocks of commented code should not be committed to the main branch. If this is a work in progress:
- Document why it's commented out using a TODO comment
- Create a separate issue to track this implementation
- Consider moving this to a feature branch
If this configuration is no longer needed, remove it entirely.
| @Override | ||
| public void addInterceptors(InterceptorRegistry registry) { | ||
| // JWT 인터셉터는 더 먼저 실행되도록 우선순위 낮춤 | ||
| registry.addInterceptor(jwtInterceptor) | ||
| .addPathPatterns("/admin/{univId}/**") // "/admin/{univId}/" 패턴만 적용 | ||
| .excludePathPatterns("/admin/auth/login") | ||
| .excludePathPatterns("/**", HttpMethod.OPTIONS.name()) |
There was a problem hiding this comment.
Security Issue: Overly permissive exclude patterns.
The current configuration has a critical security flaw:
.excludePathPatterns("/**", HttpMethod.OPTIONS.name())The "/**" pattern excludes ALL paths, effectively bypassing both JWT and Admin interceptors. This nullifies your security checks.
Remove the overly broad exclude pattern:
-.excludePathPatterns("/**", HttpMethod.OPTIONS.name())
+.excludePathPatterns(HttpMethod.OPTIONS.name())Also applies to: 70-70
| data: | ||
| redis: | ||
| host: redis | ||
| password: ${REDIS_PASSWORD} | ||
| port: 6379 |
There was a problem hiding this comment.
Introduced Redis configuration.
The new data.redis block configures Redis with the host set to redis, a password drawn from the REDIS_PASSWORD environment variable, and port 6379. Verify that:
- The environment variable
REDIS_PASSWORDis properly set in all deployment environments. - The hostname
redisresolves correctly in your network. - No sensitive information (like the Redis password) is inadvertently exposed through logs or debug outputs.
| @Column(length = 30) | ||
| private String trimmedName; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify database migration impact.
The addition of this new column requires a database migration. Please ensure that:
- A migration script exists to add the column
- The script includes logic to populate
trimmedNamefor existing records - The deployment plan accounts for this schema change
Run this script to check for migration files:
🏁 Script executed:
#!/bin/bash
# Search for migration files that might contain the trimmedName column addition
rg -g '*.sql' -i 'trimmed.*name|alter.*building'Length of output: 334
Database migration script for trimmedName column required
The executed search only returned test SQL files referencing trimmed_name (in src/test/resources/sql/insert-univ-data.sql). There isn’t any dedicated migration script that:
- Adds the
trimmedNamecolumn to the production Building table. - Includes logic to populate
trimmedNamefor existing records. - Addresses the deployment impact of this schema change.
Please verify that a proper production migration script is created and applied. If it exists in another location or as part of a different process, kindly update the code comments and deployment plan to reflect that.
| @@ -80,6 +81,7 @@ public void createBuildingNode(Long univId, CreateBuildingNodeReqDTO createBuild | |||
| .phoneNumber(createBuildingNodeReqDTO.getPhoneNumber()) | |||
| .address(createBuildingNodeReqDTO.getAddress()) | |||
| .name(createBuildingNodeReqDTO.getBuildingName()) | |||
| .trimmedName(createBuildingNodeReqDTO.getBuildingName().strip()) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure consistent string trimming across the application.
While the trimming operation is implemented here, it should be consistent with other building name operations throughout the application.
🏁 Script executed:
#!/bin/bash
# Check for consistent string trimming across building name handling
# Search for building name assignments
echo "Searching for building name handling..."
rg -A 2 "\.setName\(|\.name\s*=" --type java
# Search for string trimming operations
echo "Searching for string trimming operations..."
rg "\.strip\(\)|\.trim\(\)" --type javaLength of output: 733
Action: Standardize the String Trimming Method
The current code in BuildingService.java uses .strip(), whereas similar operations in CautionListConverter.java and DangerListConverter.java use .trim(). To avoid discrepancies in whitespace handling, please standardize on one method across the application. For example:
- Replace
.strip()with.trim()inBuildingService.java, if consistent behavior with existing converters is desired. - Alternatively, update converters to use
.strip()if the Unicode-aware trimming is required throughout.
Snippet In Question:
.trimmedName(createBuildingNodeReqDTO.getBuildingName().strip())| public synchronized AllRoutesInfo createRoute(Long univId, CreateRoutesReqDTO requests){ | ||
|
|
||
| if(requests.getCoordinates().size() >= CREATE_ROUTE_LIMIT_COUNT){ | ||
| throw new RouteException("creat route limit exceeded", CREATE_ROUTE_LIMIT_EXCEEDED); | ||
| } | ||
|
|
||
| List<Route> savedRoutes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | ||
|
|
||
| List<Node> nodesForSave = routeCalculator.createValidRouteNodes(univId, requests.getStartNodeId(), | ||
| requests.getEndNodeId(), | ||
| requests.getCoordinates(), savedRoutes); | ||
|
|
||
| mapClient.fetchHeights(nodesForSave); | ||
|
|
||
| List<Route> routes = routeCalculator.createLinkedRouteAndSave(univId, nodesForSave); | ||
|
|
||
| nodeRepository.saveAll(nodesForSave); | ||
| routeRepository.saveAll(routes); | ||
|
|
||
| int routeCount = routeRepository.countByUnivId(univId); | ||
|
|
||
| redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); | ||
|
|
||
| eventPublisher.publishEvent(new RouteCreatedEvent()); | ||
| return routeCalculator.assembleRoutes(routes); | ||
| } | ||
| } |
There was a problem hiding this comment.
Invalidate or refresh local cache after creating new routes.
Currently, createRoute does not remove or refresh entries in cache, leading to stale data if subsequent calls rely on getAllRoutesByLocalCache. Either invalidate the affected cache entry or update it to reflect newly created routes.
| FastestRouteResultDTO fastestRouteResult = generateResult(shortestRoutes, startNode); | ||
|
|
||
| //처음과 마지막을 제외한 구간에서 빌딩노드를 거쳐왔다면, 이는 유효한 길이 없는 것이므로 예외처리 | ||
| if (totalDistance > BUILDING_ROUTE_DISTANCE - 1) continue; | ||
| if (fastestRouteResult.getTotalDistance() > BUILDING_ROUTE_DISTANCE - 1) continue; | ||
|
|
||
| List<RouteDetailResDTO> details = getRouteDetail(startNode, endNode, shortestRoutes); | ||
|
|
||
| result.add(FastestRouteResDTO.of(policy, hasCaution, totalDistance, | ||
| calculateCost(policy, PEDESTRIAN_SECONDS_PER_MITER, totalDistance), | ||
| calculateCost(policy, MANUAL_WHEELCHAIR_SECONDS_PER_MITER,totalDistance), | ||
| calculateCost(policy, ELECTRIC_WHEELCHAIR_SECONDS_PER_MITER,totalDistance), | ||
| routeInfoDTOS, details)); | ||
| result.add(FastestRouteResDTO.of(policy, | ||
| fastestRouteResult.isHasCaution(), | ||
| fastestRouteResult.isHasDanger(), | ||
| fastestRouteResult.getTotalDistance(), | ||
| calculateCost(policy, PEDESTRIAN_SECONDS_PER_MITER, | ||
| fastestRouteDTO.getTotalWeightDistance() % BUILDING_ROUTE_DISTANCE | ||
| + fastestRouteResult.getHeightIncreaseWeight() - fastestRouteResult.getHeightDecreaseWeight()), | ||
| calculateCost(policy, MANUAL_WHEELCHAIR_SECONDS_PER_MITER, | ||
| fastestRouteDTO.getTotalWeightDistance() % BUILDING_ROUTE_DISTANCE | ||
| + fastestRouteResult.getHeightIncreaseWeight() + fastestRouteResult.getHeightDecreaseWeight()), | ||
| calculateCost(policy, ELECTRIC_WHEELCHAIR_SECONDS_PER_MITER, | ||
| fastestRouteDTO.getTotalWeightDistance() % BUILDING_ROUTE_DISTANCE), | ||
| fastestRouteResult.getRouteInfoDTOS(), | ||
| details)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check correctness of cost calculations with caution/danger weighting.
When building the FastestRouteResDTO (lines around 166–179), ensure you correctly combine route distance, caution/danger heuristics, and the separate cost calculations by policy. Overlapping risk factors or multiple caution increments might lead to unexpected totals.
| public void fetchHeight(){ | ||
| List<Node> readyNodes = nodeRepository.findAllByStatus(HeightStatus.READY); | ||
| mapClient.fetchHeights(readyNodes); | ||
| setStatus(readyNodes,HeightStatus.DONE); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and retry mechanism for external service calls.
The fetchHeight method lacks error handling for the external MapClient call and could benefit from a retry mechanism.
Consider implementing:
- Error handling for MapClient failures
- Retry mechanism for transient failures
- Batch processing for status updates
+ @Retryable(maxAttempts = 3, backoff = @Backoff(delay = 1000))
@DisableAudit
public void fetchHeight(){
+ try {
List<Node> readyNodes = nodeRepository.findAllByStatus(HeightStatus.READY);
+ if (readyNodes.isEmpty()) return;
mapClient.fetchHeights(readyNodes);
setStatus(readyNodes,HeightStatus.DONE);
+ } catch (Exception e) {
+ log.error("Failed to fetch heights: {}", e.getMessage());
+ throw new ExternalServiceException("Height fetch failed", 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 fetchHeight(){ | |
| List<Node> readyNodes = nodeRepository.findAllByStatus(HeightStatus.READY); | |
| mapClient.fetchHeights(readyNodes); | |
| setStatus(readyNodes,HeightStatus.DONE); | |
| } | |
| @Retryable(maxAttempts = 3, backoff = @Backoff(delay = 1000)) | |
| @DisableAudit | |
| public void fetchHeight(){ | |
| try { | |
| List<Node> readyNodes = nodeRepository.findAllByStatus(HeightStatus.READY); | |
| if (readyNodes.isEmpty()) return; | |
| mapClient.fetchHeights(readyNodes); | |
| setStatus(readyNodes, HeightStatus.DONE); | |
| } catch (Exception e) { | |
| log.error("Failed to fetch heights: {}", e.getMessage()); | |
| throw new ExternalServiceException("Height fetch failed", e); | |
| } | |
| } |
| @Getter | ||
| @Setter | ||
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary @Setter annotation.
Since all fields are final and the class is immutable, the @Setter annotation is unnecessary and contradicts the immutable design.
@Getter
-@Setter
@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
public class FastestRouteResultDTO {📝 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.
| @Getter | |
| @Setter | |
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) | |
| @Getter | |
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) | |
| public class FastestRouteResultDTO { | |
| // rest of the class code... | |
| } |
🛠️ Refactor suggestion
Remove unnecessary @Setter annotation.
The class has all final fields which cannot be modified after initialization, making the @Setter annotation redundant and potentially misleading.
@Getter
-@Setter
@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
public class FastestRouteResultDTO {📝 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.
| @Getter | |
| @Setter | |
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) | |
| @Getter | |
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) | |
| public class FastestRouteResultDTO { | |
| // Class implementation goes here | |
| } |
| INSERT INTO univ | ||
| (id, name, trimmed_name, image_url, center_point, left_top_point, right_bottom_point, area_polygon, limit_version) | ||
| VALUES | ||
| ( | ||
| 1001, | ||
| 'HYUNIV', | ||
| 'HYUNIV', | ||
| 'http://example.com/hanyang.jpg', | ||
| ST_GeomFromText('POINT(126.9780 37.5665)', 4326), | ||
| ST_GeomFromText('POINT(126.9720 37.5700)', 4326), | ||
| ST_GeomFromText('POINT(126.9850 37.5620)', 4326), | ||
| ST_GeomFromText('POLYGON((126.9720 37.5700, 126.9850 37.5700, 126.9850 37.5620, 126.9720 37.5620, 126.9720 37.5700))', 4326), | ||
| 0 | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the spatial data coordinates.
The SQL insert uses spatial data types correctly, but please verify:
- The coordinates represent the actual university location
- The polygon bounds are accurate
- The SRID (4326) is appropriate for your use case
Also, verify that the image URL is accessible:
🏁 Script executed:
#!/bin/bash
# Check if the coordinates are within valid ranges for WGS84
echo "Validating WGS84 coordinates..."
python3 << 'EOF'
def validate_wgs84(lon, lat):
return -180 <= lon <= 180 and -90 <= lat <= 90
points = [
(126.9780, 37.5665),
(126.9720, 37.5700),
(126.9850, 37.5620)
]
for lon, lat in points:
if not validate_wgs84(lon, lat):
print(f"Invalid coordinates: {lon}, {lat}")
exit(1)
print("All coordinates are within valid WGS84 ranges")
EOFLength of output: 128
🏁 Script executed:
#!/bin/bash
# Check if the image URL is accessible
curl -I http://example.com/hanyang.jpgLength of output: 827
Spatial Data Verification Update: Coordinates are valid, but the image URL returns a 404
- The spatial coordinates used in the INSERT statement have been validated and are within valid WGS84 ranges.
- However, the image URL (http://example.com/hanyang.jpg) is not accessible (HTTP 404). Please verify if this URL is intended as a placeholder for test data or update it to a valid resource.
* [UNI-329] refactor : fetchSize=1로 변경하여 stream 하게 오도록 변경 * [UNI-329] fix : fetchSize에 정상적인 값 들어가도록 수정
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/RouteRepository.java (1)
30-41: Consider adding Javadoc to document the performance characteristics.The implementation is well-optimized using constructor expressions and proper join statements. The query hints for fetch size and caching are appropriately configured.
Add Javadoc to document:
- The streaming nature of the method
- Memory usage characteristics
- Proper usage guidelines
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (3)
100-100: Enhance cache hit logging.The current log message "🚀🚀🚀🚀🚀🚀🚀HIT🚀🚀🚀🚀🚀🚀🚀" could be more informative. Consider including relevant context such as the university ID and cache key.
- log.info("🚀🚀🚀🚀🚀🚀🚀HIT🚀🚀🚀🚀🚀🚀🚀"); + log.info("Cache hit for university ID: {} (key: {})", univId, univId.toString());
133-136: Enhance error handling in SSE streaming.The current error handling only logs the error and completes the emitter. Consider:
- Adding specific error types and handling
- Implementing retry logic for transient failures
- Sending error details to the client for better debugging
295-319: Add input validation for route creation.While the method checks for route limit, consider adding additional validations:
- Validate coordinate values are within acceptable ranges
- Check for duplicate coordinates
- Validate minimum distance between coordinates
Also, consider documenting the reason for making this method synchronized in the code comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/RouteRepository.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy to Server
🔇 Additional comments (4)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/repository/RouteRepository.java (3)
6-6: LGTM! Import statements are well-organized.The new imports support streaming capabilities and query optimization features.
Also applies to: 8-9, 14-14
100-100: LGTM! Simple and effective count method.The derived query method is appropriate for this simple counting operation.
22-28:❓ Verification inconclusive
Ensure proper Stream handling in the service layer.
The streaming implementation with entity graph and fetch size optimization looks good. However, streams must be properly closed to prevent resource leaks.
Let's verify the Stream handling in the service layer:
🏁 Script executed:
#!/bin/bash # Search for try-with-resources or close() calls on the stream ast-grep --pattern 'try ($_ = routeRepository.findAllRouteByUnivIdWithNodesStream($_)) { $$$_ }'Length of output: 98
Verify proper Stream closure in the service layer
The implementation of the streaming method in
RouteRepositorywith an entity graph and fetch size optimization appears correct. However, our initial search for try-with-resources or explicitclose()calls on the Stream returned byfindAllRouteByUnivIdWithNodesStreamdid not yield any results. Please manually verify that all usages of this method in the service layer correctly handle the Stream—either with a try-with-resources block or by explicitly callingclose()—to prevent potential resource leaks.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (1)
315-315: Invalidate or refresh local cache after creating new routes.Currently,
createRoutedoes not remove or refresh entries incache, leading to stale data if subsequent calls rely ongetAllRoutesByLocalCache. Either invalidate the affected cache entry or update it to reflect newly created routes.
| public GetAllRoutesResDTO getAllRoutesByLocalCache(Long univId) { | ||
|
|
||
| if(!cache.containsKey(univId)){ | ||
| List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | ||
| List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList(); | ||
| cache.put(univId, lightRoutes); | ||
| } | ||
|
|
||
| List<LightRoute> routes = cache.get(univId); | ||
|
|
||
| // 맵이 존재하지 않을 경우 예외 | ||
| if(routes.isEmpty()) { | ||
| throw new RouteException("Route Not Found", ROUTE_NOT_FOUND); | ||
| } | ||
|
|
||
| RevInfo revInfo = revInfoRepository.findFirstByUnivIdOrderByRevDesc(univId) | ||
| .orElseThrow(() -> new RouteException("Revision not found", RECENT_REVISION_NOT_FOUND)); | ||
|
|
||
| AllRoutesInfo allRoutesInfo = routeCacheCalculator.assembleRoutes(routes); | ||
| GetAllRoutesResDTO response = GetAllRoutesResDTO.of(allRoutesInfo.getNodeInfos(), allRoutesInfo.getCoreRoutes(), | ||
| allRoutesInfo.getBuildingRoutes(), revInfo.getRev()); | ||
|
|
||
| return response; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cache invalidation strategy.
The local cache implementation lacks a clear invalidation strategy. This could lead to stale data, especially after route modifications. Consider:
- Adding a method to invalidate cache entries
- Implementing cache entry expiration
- Adding cache size limits to prevent memory issues
| private final MapClient mapClient; | ||
| private final RedisService redisService; | ||
|
|
||
| private final Map<Long, List<LightRoute>> cache = new HashMap<>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a thread-safe cache implementation.
The current HashMap implementation is not thread-safe, which could lead to race conditions in a multi-threaded environment. Consider using ConcurrentHashMap instead.
- private final Map<Long, List<LightRoute>> cache = new HashMap<>();
+ private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>();📝 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 final Map<Long, List<LightRoute>> cache = new HashMap<>(); | |
| private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>(); |
* [UNI-199] fix: 로컬 캐시 삭제 * [UNI-199] feat: 배치 사이즈 저장 로직 구현 * [UNI-199] fix: redis put 로직 이동
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (2)
66-89: 🛠️ Refactor suggestionAdd cache eviction strategy to prevent memory leaks.
The local cache implementation lacks:
- Cache entry expiration
- Cache size limits
- Clear invalidation strategy
This could lead to memory issues and stale data.
Consider implementing a cache eviction strategy:
- Use Caffeine or Guava cache for automatic expiration
- Add cache size limits
- Implement cache invalidation when routes are modified
Example implementation using Caffeine:
private final Cache<Long, List<LightRoute>> cache = Caffeine.newBuilder() .expireAfterWrite(1, TimeUnit.HOURS) .maximumSize(100) .build();
298-322:⚠️ Potential issueImprove route creation with proper cache invalidation.
The synchronized route creation method needs improvements:
- Cache invalidation is only done for Redis, not local cache
- No validation for univId
- No error handling for event publishing
Apply these improvements:
@Transactional @RevisionOperation(RevisionOperationType.CREATE_ROUTE) public synchronized AllRoutesInfo createRoute(Long univId, CreateRoutesReqDTO requests){ + if (univId == null) { + throw new IllegalArgumentException("University ID cannot be null"); + } if(requests.getCoordinates().size() >= CREATE_ROUTE_LIMIT_COUNT){ throw new RouteException("creat route limit exceeded", CREATE_ROUTE_LIMIT_EXCEEDED); } // ... existing code ... redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); + // Invalidate local cache + cache.remove(univId); try { eventPublisher.publishEvent(new RouteCreatedEvent()); } catch (Exception e) { log.error("Failed to publish RouteCreatedEvent", e); } return routeCalculator.assembleRoutes(routes); }uniro_backend/src/main/java/com/softeer5/uniro_backend/external/redis/RedisService.java (2)
28-30: 🛠️ Refactor suggestionAdd validation and error handling.
The method lacks input validation and error handling.
public Object getData(String key) { + if (key == null) { + throw new IllegalArgumentException("Key must not be null"); + } + try { - return redisTemplate.opsForValue().get(key); + Object data = redisTemplate.opsForValue().get(key); + if (data == null) { + log.debug("No data found in Redis for key: {}", key); + } + return data; + } catch (Exception e) { + log.error("Failed to get data from Redis for key: {}", key, e); + throw new RuntimeException("Failed to get data from Redis", e); + } }
39-45: 🛠️ Refactor suggestionOptimize batch deletion.
The current implementation makes multiple Redis calls. Consider using batch deletion.
public void deleteRoutesData(String key, int batchNumber) { + if (key == null) { + throw new IllegalArgumentException("Key must not be null"); + } + if (batchNumber <= 0) { + throw new IllegalArgumentException("Batch number must be positive"); + } String redisKeyPrefix = key + ":"; + Set<String> keysToDelete = new HashSet<>(); for(int i=1; i<=batchNumber; i++){ - redisTemplate.delete(redisKeyPrefix + i); + keysToDelete.add(redisKeyPrefix + i); } + try { + redisTemplate.delete(keysToDelete); + } catch (Exception e) { + log.error("Failed to delete batch data from Redis for key prefix: {}", redisKeyPrefix, e); + throw new RuntimeException("Failed to delete batch data from Redis", e); + } }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/RedisConfig.java (1)
17-29: 🛠️ Refactor suggestionAdd type parameters and initialize template.
The current implementation uses raw types and doesn't properly initialize the template.
- public RedisTemplate redisTemplate(RedisConnectionFactory connectionFactory) { - RedisTemplate template = new RedisTemplate<>(); + public RedisTemplate<String, LightRoutes> redisTemplate(RedisConnectionFactory connectionFactory) { + RedisTemplate<String, LightRoutes> template = new RedisTemplate<>(); template.setConnectionFactory(connectionFactory); // 키는 String, 값은 FastJson으로 변환된 JSON 문자열 저장 StringRedisSerializer serializer = new StringRedisSerializer(); FastJsonRedisSerializer<LightRoutes> fastJsonRedisSerializer = new FastJsonRedisSerializer<>(LightRoutes.class); template.setKeySerializer(serializer); template.setValueSerializer(fastJsonRedisSerializer); + template.setHashKeySerializer(serializer); + template.setHashValueSerializer(fastJsonRedisSerializer); + template.afterPropertiesSet(); return template; }
🧹 Nitpick comments (2)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (2)
91-118: Improve Redis caching implementation.The Redis caching implementation could be enhanced for better reliability and maintainability:
- Missing TTL for Redis entries
- No error handling for Redis operations
- Logging could be more informative
Consider these improvements:
if(!redisService.hasData(univId.toString())){ List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId); List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList(); LightRoutes value = new LightRoutes(lightRoutes); - redisService.saveData(univId.toString(), value); + redisService.saveData(univId.toString(), value, Duration.ofHours(1)); + log.info("Cache miss for univId: {}, saving {} routes to Redis", univId, lightRoutes.size()); } else { - log.info("🚀🚀🚀🚀🚀🚀🚀HIT🚀🚀🚀🚀🚀🚀🚀"); + log.info("Cache hit for univId: {}", univId); }
120-204: Enhance error handling in async streaming.The async streaming implementation has several areas for improvement:
- No timeout handling for SSE connection
- No retry mechanism for Redis operations
- No backpressure handling for large datasets
Consider these enhancements:
@Async public void getAllRoutesByStream(Long univId, SseEmitter emitter) { + // Set timeout for SSE connection + emitter.onTimeout(() -> { + emitter.complete(); + log.info("SSE connection timed out for univId: {}", univId); + }); + + // Set completion callback + emitter.onCompletion(() -> { + log.info("SSE connection completed for univId: {}", univId); + }); String redisKeyPrefix = univId + ":"; int batchNumber = 1; try { if (processRedisData(univId, redisKeyPrefix, batchNumber, emitter)) { return; } processDatabaseData(univId, redisKeyPrefix, batchNumber, emitter); } catch (Exception e) { + log.error("Error in SSE streaming for univId: {}", univId, e); emitter.completeWithError(e); - log.error("SSE error: {}", e.getMessage(), e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/RedisConfig.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/redis/RedisService.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy to Server
🔇 Additional comments (2)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (1)
64-64: Use ConcurrentHashMap for thread safety.The local cache implementation using
HashMapis not thread-safe and could lead to race conditions in a multi-threaded environment.Apply this diff:
-private final Map<Long, List<LightRoute>> cache = new HashMap<>(); +private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>();uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/RedisConfig.java (1)
31-34: LGTM!The StringRedisTemplate configuration is correct and follows best practices.
| public String getDataToString(String key){ | ||
| return stringRedisTemplate.opsForValue().get(key); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling.
Similar improvements needed as in getData.
public String getDataToString(String key){
+ if (key == null) {
+ throw new IllegalArgumentException("Key must not be null");
+ }
+ try {
- return stringRedisTemplate.opsForValue().get(key);
+ String data = stringRedisTemplate.opsForValue().get(key);
+ if (data == null) {
+ log.debug("No string data found in Redis for key: {}", key);
+ }
+ return data;
+ } catch (Exception e) {
+ log.error("Failed to get string data from Redis for key: {}", key, e);
+ throw new RuntimeException("Failed to get string data from Redis", 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 String getDataToString(String key){ | |
| return stringRedisTemplate.opsForValue().get(key); | |
| } | |
| public String getDataToString(String key){ | |
| if (key == null) { | |
| throw new IllegalArgumentException("Key must not be null"); | |
| } | |
| try { | |
| String data = stringRedisTemplate.opsForValue().get(key); | |
| if (data == null) { | |
| log.debug("No string data found in Redis for key: {}", key); | |
| } | |
| return data; | |
| } catch (Exception e) { | |
| log.error("Failed to get string data from Redis for key: {}", key, e); | |
| throw new RuntimeException("Failed to get string data from Redis", e); | |
| } | |
| } |
| private final RedisTemplate redisTemplate; | ||
| private final StringRedisTemplate stringRedisTemplate; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type parameters to RedisTemplate for type safety.
The current implementation uses raw types which can lead to runtime errors. Consider parameterizing the RedisTemplate to match the configuration in RedisConfig.
- private final RedisTemplate redisTemplate;
+ private final RedisTemplate<String, LightRoutes> redisTemplate;📝 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 final RedisTemplate redisTemplate; | |
| private final StringRedisTemplate stringRedisTemplate; | |
| private final RedisTemplate<String, LightRoutes> redisTemplate; | |
| private final StringRedisTemplate stringRedisTemplate; |
| public boolean hasData(String key){ | ||
| return redisTemplate.hasKey(key); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling to hasData.
The method lacks input validation and error handling.
public boolean hasData(String key){
+ if (key == null) {
+ throw new IllegalArgumentException("Key must not be null");
+ }
+ try {
- return redisTemplate.hasKey(key);
+ Boolean exists = redisTemplate.hasKey(key);
+ return exists != null && exists;
+ } catch (Exception e) {
+ log.error("Failed to check key existence in Redis for key: {}", key, e);
+ throw new RuntimeException("Failed to check key existence in Redis", 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 boolean hasData(String key){ | |
| return redisTemplate.hasKey(key); | |
| } | |
| public boolean hasData(String key){ | |
| if (key == null) { | |
| throw new IllegalArgumentException("Key must not be null"); | |
| } | |
| try { | |
| Boolean exists = redisTemplate.hasKey(key); | |
| return exists != null && exists; | |
| } catch (Exception e) { | |
| log.error("Failed to check key existence in Redis for key: {}", key, e); | |
| throw new RuntimeException("Failed to check key existence in Redis", e); | |
| } | |
| } |
| public void saveData(String key, Object value) { | ||
| redisTemplate.opsForValue().set(key, value, Duration.ofMinutes(10000)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling.
The method lacks input validation and error handling. Consider:
- Adding null checks
- Extracting TTL to a configuration
- Adding exception handling
+ private static final long DEFAULT_TTL_MINUTES = 10000;
+
public void saveData(String key, Object value) {
+ if (key == null || value == null) {
+ throw new IllegalArgumentException("Key and value must not be null");
+ }
+ try {
redisTemplate.opsForValue().set(key, value, Duration.ofMinutes(10000));
+ } catch (Exception e) {
+ log.error("Failed to save data to Redis for key: {}", key, e);
+ throw new RuntimeException("Failed to save data to Redis", 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 saveData(String key, Object value) { | |
| redisTemplate.opsForValue().set(key, value, Duration.ofMinutes(10000)); | |
| } | |
| private static final long DEFAULT_TTL_MINUTES = 10000; | |
| public void saveData(String key, Object value) { | |
| if (key == null || value == null) { | |
| throw new IllegalArgumentException("Key and value must not be null"); | |
| } | |
| try { | |
| redisTemplate.opsForValue().set(key, value, Duration.ofMinutes(10000)); | |
| } catch (Exception e) { | |
| log.error("Failed to save data to Redis for key: {}", key, e); | |
| throw new RuntimeException("Failed to save data to Redis", e); | |
| } | |
| } |
| public void saveDataToString(String key, Object value) { | ||
| stringRedisTemplate.opsForValue().set(key, value.toString(), Duration.ofMinutes(10000)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and safer string conversion.
The method has similar issues to saveData and uses unsafe string conversion.
public void saveDataToString(String key, Object value) {
+ if (key == null || value == null) {
+ throw new IllegalArgumentException("Key and value must not be null");
+ }
+ String stringValue = String.valueOf(value); // Safer than toString()
+ try {
- stringRedisTemplate.opsForValue().set(key, value.toString(), Duration.ofMinutes(10000));
+ stringRedisTemplate.opsForValue().set(key, stringValue, Duration.ofMinutes(DEFAULT_TTL_MINUTES));
+ } catch (Exception e) {
+ log.error("Failed to save string data to Redis for key: {}", key, e);
+ throw new RuntimeException("Failed to save string data to Redis", 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 saveDataToString(String key, Object value) { | |
| stringRedisTemplate.opsForValue().set(key, value.toString(), Duration.ofMinutes(10000)); | |
| } | |
| public void saveDataToString(String key, Object value) { | |
| if (key == null || value == null) { | |
| throw new IllegalArgumentException("Key and value must not be null"); | |
| } | |
| String stringValue = String.valueOf(value); // Safer than toString() | |
| try { | |
| stringRedisTemplate.opsForValue().set(key, stringValue, Duration.ofMinutes(DEFAULT_TTL_MINUTES)); | |
| } catch (Exception e) { | |
| log.error("Failed to save string data to Redis for key: {}", key, e); | |
| throw new RuntimeException("Failed to save string data to Redis", e); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (2)
62-64: Coordinate cache implementations.The service uses both Redis and local caching without clear coordination between them. This could lead to data inconsistency.
Consider:
- Using Redis as the primary cache
- Using local cache as a secondary/temporary cache with TTL
- Implementing a cache synchronization strategy
100-100: Use professional logging format.Avoid using emojis in log messages as they can cause issues with log parsing and monitoring tools.
- log.info("🚀🚀🚀🚀🚀🚀🚀HIT🚀🚀🚀🚀🚀🚀🚀"); + log.info("Cache hit for univId: {}", univId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy to Server
🔇 Additional comments (3)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (3)
64-64: Use ConcurrentHashMap for thread safety.The current HashMap implementation is not thread-safe, which could lead to race conditions in a multi-threaded environment.
- private final Map<Long, List<LightRoute>> cache = new HashMap<>(); + private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>();
66-89: Enhance local cache management.The local cache implementation lacks essential features for production use:
- No cache entry expiration/TTL
- No cache size limits
- No invalidation strategy
Consider implementing a proper caching solution using libraries like Caffeine or EhCache that provide these features out of the box.
298-322: Invalidate all caches after route creation.The method only invalidates Redis cache but not the local cache after creating new routes.
Add local cache invalidation:
redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); + cache.remove(univId); // Invalidate local cache eventPublisher.publishEvent(new RouteCreatedEvent());
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in stream processing.
The catch block wraps all exceptions in RuntimeException, losing valuable error context.
- throw new RuntimeException(e);
+ log.error("Error processing database stream for univId: {}", univId, e);
+ throw new RouteException("Error processing route stream", ROUTE_STREAM_PROCESSING_ERROR, 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.
| } catch (Exception e) { | |
| throw new RuntimeException(e); | |
| } | |
| } catch (Exception e) { | |
| log.error("Error processing database stream for univId: {}", univId, e); | |
| throw new RouteException("Error processing route stream", ROUTE_STREAM_PROCESSING_ERROR, e); | |
| } |
* [UNI-205] feat : Stream + 캐시 로직 작성 * [UNI-205] GetAllRoutesResInfo -> AllRoutesInfo로 변경 * 테스트용 CD * 테스트용 CD 삭제 * [UNI-205] feat : 캐시가 지워지지 않던 문제 해결 * cd 삭제
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (1)
64-64:⚠️ Potential issueConsider using a thread-safe cache implementation.
Access tocachemay occur concurrently. AHashMapis not inherently thread-safe, leading to potential race conditions.Apply this diff to switch to
ConcurrentHashMap:- private final Map<Long, List<LightRoute>> cache = new HashMap<>(); + private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>();
🧹 Nitpick comments (7)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapApi.java (1)
33-39: Assess duplication between methods.
getAllRoutesAndNodesStreamreturnsAllRoutesInfosimilarly togetAllRoutesAndNodes. If both require similar logic, consider consolidating common functionality to minimize duplication.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (4)
53-53: Check layering responsibility for caching logic.
The service now manages both local and Redis caches. Ensure that responsibilities (e.g., expiration, invalidation, concurrency control) are clear to avoid confusion.Also applies to: 60-60, 62-62
84-105: Redis fallback logic.
getAllRoutesretrieves from or writes to Redis. Consider logging or metrics for cache hits/misses to monitor performance.
192-209: SSE event flow.
The SSE paths appear well-structured but rely on repeated patterns of batch processing. Consider extracting shared SSE logic into reusable methods or a dedicated helper class.Also applies to: 211-230, 232-258, 260-276
313-316: Robust error messages.
The thrown exceptions provide clear messages. Ensure logging is consistent (e.g.,log.errorbefore throwing) to aid debugging.Also applies to: 339-342, 355-363
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java (2)
3-3: New admin service dependency.
Upon injection ofAdminService, confirm that the two services (MapServiceandAdminService) do not overlap responsibilities.
26-26: Potential cyclical dependency.
AdminServiceis injected here whileAdminServicemay also referenceMapService. Check for circular references or move shared logic to a dedicated service if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/external/redis/RedisService.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapApi.java(5 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java(5 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/external/redis/RedisService.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java
🔇 Additional comments (12)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapApi.java (6)
5-5: Import usage looks fine.The new imports for
dto.response.*andSseEmitterappear correct and consistent with the code changes that returnAllRoutesInfoand support SSE operations.Also applies to: 19-19
31-31: Confirm new return type usage.
Ensure all callers ofgetAllRoutesAndNodesproperly handle the newAllRoutesInforeturn type.
40-46: Verify SSE approach.
The SSE-based methodgetAllRoutesis declared here. Ensure that any SSE timeouts, reconnection strategies, and client disconnection handling are addressed in the controller or service layer to avoid partial data transmissions.
77-78: Change in return type.
createRoutenow returnsAllRoutesInfoinstead ofVoid. Verify that any existing clients or tests are updated accordingly to handle the new response structure.
95-96: Adding validation for building-route creation.
The method correctly accepts aCreateBuildingRouteReqDTO. Confirm that separate validations (like checking for building node existence) are handled to reduce chances of incomplete data.
98-104: New endpoint for version-to-version diff.
Ensure thatgetChangedRoutesByRevisionis documented and tested. Consider edge cases, such as invalidversionIdor differences when no changes exist.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (2)
3-3: Imports and annotations seem consistent.
These imports, logging, async features, and event publishing annotations align with the updated caching, SSE streaming, and route creation logic.Also applies to: 8-8, 10-11, 13-13, 20-21, 29-30, 33-34, 41-42, 45-45, 50-50
107-120: Streaming approach
The streaming logic processes data from Redis or DB in batches. Confirm that chunked data is merged correctly and that partial data scenarios are handled if exceptions occur mid-stream.Also applies to: 122-138, 140-166, 168-176, 178-190
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java (4)
6-6: Additional imports.
Imports fordto.response.*andSseEmitterare aligned with the interface definitions. No immediate issues.Also applies to: 17-17
28-32: Local cache endpoint.
getAllRoutesAndNodesByLocalCacheis introduced. Ensure this is truly needed ifgetAllRoutesAndNodesalready fetches from Redis. Duplicating endpoints might cause confusion for clients.
36-39: Consistent responses withAllRoutesInfo.
These methods consistently returnAllRoutesInfoorSseEmitter. Confirm that the new response structure is documented for any frontend consumers.Also applies to: 41-46, 49-54, 82-85
105-111: Revision changes endpoint.
getChangedRoutesByRevisiondelegates toadminService. Ensure the method handles large diffs gracefully. Add tests for edge cases (e.g., older or nonexistent version).
| public AllRoutesInfo getAllRoutesByLocalCache(Long univId) { | ||
|
|
||
| if(!cache.containsKey(univId)){ | ||
| List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | ||
| List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList(); | ||
| cache.put(univId, lightRoutes); | ||
| } | ||
|
|
||
| List<LightRoute> routes = cache.get(univId); | ||
|
|
||
| // 맵이 존재하지 않을 경우 예외 | ||
| if(routes.isEmpty()) { | ||
| throw new RouteException("Route Not Found", ROUTE_NOT_FOUND); | ||
| } | ||
|
|
||
| return routeCalculator.assembleRoutes(routes); | ||
| return routeCacheCalculator.assembleRoutes(routes); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for cache invalidation strategy.
getAllRoutesByLocalCache populates cache if absent, but there's no explicit invalidation or refresh after route changes. This can lead to stale data.
| public synchronized AllRoutesInfo createRoute(Long univId, CreateRoutesReqDTO requests){ | ||
|
|
||
| if(requests.getCoordinates().size() >= CREATE_ROUTE_LIMIT_COUNT){ | ||
| throw new RouteException("creat route limit exceeded", CREATE_ROUTE_LIMIT_EXCEEDED); | ||
| } | ||
|
|
||
| List<Route> savedRoutes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | ||
|
|
||
| List<Node> nodesForSave = routeCalculator.createValidRouteNodes(univId, requests.getStartNodeId(), | ||
| requests.getEndNodeId(), | ||
| requests.getCoordinates(), savedRoutes); | ||
|
|
||
| mapClient.fetchHeights(nodesForSave); | ||
|
|
||
| List<Route> routes = routeCalculator.createLinkedRouteAndSave(univId, nodesForSave); | ||
|
|
||
| nodeRepository.saveAll(nodesForSave); | ||
| routeRepository.saveAll(routes); | ||
|
|
||
| int routeCount = routeRepository.countByUnivId(univId); | ||
|
|
||
| redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); | ||
|
|
||
| eventPublisher.publishEvent(new RouteCreatedEvent()); | ||
| return routeCalculator.assembleRoutes(routes); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Local cache not invalidated on route creation.
createRoute only deletes Redis entries. To avoid stale data in cache, consider clearing or updating cache for univId.
Optional improvement for event details.
Publishing new RouteCreatedEvent() lacks context about the created routes or univId. Consider passing relevant data in the event to aid event listeners.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/service/AdminService.java (3)
3-50: Good architectural improvements with Redis integration and enhanced error handling!The addition of Redis caching and specific exception types improves the system's robustness and performance.
Consider documenting the caching strategy and cache invalidation policies in the class-level JavaDoc to help maintainers understand the caching behavior.
220-236: Reduce code duplication in handleRevisionRoute methods.The two handleRevisionRoute methods share similar logic, with one being a subset of the other.
Consider refactoring to use a single method with an optional parameter:
- private void handleRevisionRoute(Route revRoute, Route route, List<ChangedRouteDTO> changedRoutes, List<Route> riskRoutes) { - if(!route.isEqualRoute(revRoute)){ - changedRoutes.add(ChangedRouteDTO.of(route.getId(), RouteDifferInfo.of(route), RouteDifferInfo.of(revRoute))); - } - else if(!route.getCautionFactors().isEmpty() || !route.getDangerFactors().isEmpty()){ - riskRoutes.add(route); - } - } - - private void handleRevisionRoute(Route revRoute, Route route, List<ChangedRouteDTO> changedRoutes) { - if(!route.isEqualRoute(revRoute)){ - changedRoutes.add(ChangedRouteDTO.of(route.getId(), RouteDifferInfo.of(route), RouteDifferInfo.of(revRoute))); - } - } + private void handleRevisionRoute(Route revRoute, Route route, List<ChangedRouteDTO> changedRoutes, List<Route> riskRoutes) { + if(!route.isEqualRoute(revRoute)){ + changedRoutes.add(ChangedRouteDTO.of(route.getId(), RouteDifferInfo.of(route), RouteDifferInfo.of(revRoute))); + } + else if(riskRoutes != null && (!route.getCautionFactors().isEmpty() || !route.getDangerFactors().isEmpty())){ + riskRoutes.add(route); + } + }
195-204: Add null checks in fetchNodes method.The method assumes that all node IDs exist in the nodeMap, which could lead to NullPointerException.
Add null checks:
private void fetchNodes(List<Route> routes, List<Node> nodes) { Map<Long, Node> nodeMap = new HashMap<>(); for(Node revNode : nodes){ nodeMap.put(revNode.getId(), revNode); } for(Route revRoute : routes){ - revRoute.setNode1(nodeMap.get(revRoute.getNode1().getId())); - revRoute.setNode2(nodeMap.get(revRoute.getNode2().getId())); + Node node1 = nodeMap.get(revRoute.getNode1().getId()); + Node node2 = nodeMap.get(revRoute.getNode2().getId()); + if (node1 == null || node2 == null) { + throw new RouteException("Node not found for route: " + revRoute.getId(), ROUTE_NODE_NOT_FOUND); + } + revRoute.setNode1(node1); + revRoute.setNode2(node2); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RevInfoRepository.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/service/AdminService.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/repository/RevInfoRepository.java
🔇 Additional comments (1)
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/service/AdminService.java (1)
117-119: Verify Redis cache invalidation logic.The cache invalidation uses a division operation that might round down, potentially missing the last chunk of routes.
- redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); + redisService.deleteRoutesData(univId.toString(), (int) Math.ceil((double) routeCount / STREAM_FETCH_SIZE));
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java (2)
85-97: Consider moving node info collection after building routes.The code adds building nodes to nodeInfos after creating the result, which could lead to duplicate entries if a building node was already present in nodeMap.
- for(Node node : buildingNodes) { - nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getX(), node.getY())); - } + // Add building nodes that aren't already in nodeInfos + for(Node node : buildingNodes) { + if (!nodeMap.containsKey(node.getId())) { + nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getX(), node.getY())); + } + }
603-604: Review height initialization for new nodes.New nodes inherit height from the previous node without considering terrain or actual elevation data.
Consider:
- Integrating with an elevation API service
- Adding a validation step for significant height differences
- Marking suspicious height values for manual review
Also applies to: 625-628
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java
🔇 Additional comments (4)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java (4)
12-14: LGTM! Well-structured class enhancements.The additions of FastestRouteDTO, FastestRouteResultDTO, DangerFactor, and HeightStatus imports, along with the cautionCount field in CostToNextNode, provide a solid foundation for handling route risks and height-based calculations.
Also applies to: 18-18, 21-21, 48-48
295-300: Review height weight calculations for extreme values.The exponential weight calculation for height differences could lead to overflow or extremely large values for significant height changes.
Consider adding bounds checking:
- if(heightDiff > 0){ - heightIncreaseWeight += Math.max(LIMIT_RANGE ,Math.exp(heightDiff) - 1); - } - else{ - heightDecreaseWeight += Math.max(LIMIT_RANGE, Math.exp(-heightDiff) - 1); - } + double maxHeightDiff = 50.0; // Define a reasonable maximum height difference + if(heightDiff > 0){ + heightDiff = Math.min(heightDiff, maxHeightDiff); + heightIncreaseWeight += Math.max(LIMIT_RANGE, Math.exp(heightDiff) - 1); + } + else{ + heightDiff = Math.max(heightDiff, -maxHeightDiff); + heightDecreaseWeight += Math.max(LIMIT_RANGE, Math.exp(-heightDiff) - 1); + }
318-326: Handle edge cases in direction calculation.The code assumes the availability of previous and next nodes but doesn't fully validate the array bounds.
Consider adding additional validation:
- if(idx-1 >= 0){ + if(idx-1 >= 0 && shortestRoutes.size() > 0){ firstNode = shortestRoutes.get(idx-1).getNode1().equals(secondNode) ? shortestRoutes.get(idx-1).getNode2() : shortestRoutes.get(idx-1).getNode1(); } - if(idx+2 <= shortestRoutes.size()-1){ + if(idx+2 < shortestRoutes.size()){ thirdNode = shortestRoutes.get(idx+2).getNode2().equals(secondNode) ? shortestRoutes.get(idx+2).getNode1() : shortestRoutes.get(idx+2).getNode2(); }
222-225:❓ Verification inconclusive
Verify risk heuristic weight calculation for multiple caution factors.
The exponential growth of risk weight (currentCautionCount^2) might lead to disproportionate route costs when multiple caution factors are present.
Consider validating the weight calculation with real-world scenarios:
🏁 Script executed:
#!/bin/bash # Find routes with multiple caution factors to verify weight calculation ast-grep --pattern 'route.getCautionFactors().size() > 1'Length of output: 59
Attention: Verify Risk Heuristic Weight Calculation
The current changes add a risk heuristic that increases route cost based on the number of caution factors. However, our initial script to verify multiple caution factors returned no results, so we haven't yet confirmed if the exponential growth of risk (implied by the use of the
currentCautionCountin the weighting function) is intentional or if it might lead to disproportionate route costs in real-world scenarios.
Action Required:
• Manually verify the behavior ofgetRiskHeuristicWeight(currentCautionCount)to ensure that its exponential scaling (e.g., using currentCautionCount^2 or similar) aligns with your routing cost expectations under varying caution factor conditions.Next Steps:
• Confirm through additional tests or manual inspection that the risk calculation appropriately balances the route optimization without over-penalizing routes with multiple caution factors.
* [UNI-353] refactor : 사용하지 않는 코드 삭제 * [UNI-353] refactor : 사용하지 않는 코드, 주석 삭제 * [UNI-353] refactor : 사용하지 않는 클래스 삭제 * [UNI-353] refactor : 패키지 컨밴션에 맞도록 이동 * [UNI-353] refactor : 패키지 컨밴션에 맞도록 이동 * [UNI-353] refactor : 사용하지 않는 상수 삭제 * [UNI-353] refactor : 건물 이름을 strip() 에서 replaceAll로 변경 * [UNI-353] refactor : 로컬캐시 관련 코드 삭제 * [UNI-353] refactor : 컨버터 관련 코드 삭제
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCacheCalculator.java (1)
62-69:⚠️ Potential issueBuilding nodes are not included in the returned AllRoutesInfo object.
Building nodes are appended to
nodeInfosonly afterAllRoutesInfois constructed. Theresultobject doesn't reflect these newly added nodes, potentially causing incomplete or inconsistent data.Apply this diff to fix the issue:
- AllRoutesInfo result = AllRoutesInfo.of(nodeInfos, getCoreRoutes(adjMap, startNode), buildingRoutes); - for(LightNode node : buildingNodes) { - nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getLng(), node.getLat())); - } - return result; + buildingNodes.forEach(node -> nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getLng(), node.getLat()))); + return AllRoutesInfo.of(nodeInfos, getCoreRoutes(adjMap, startNode), buildingRoutes);uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (6)
63-63:⚠️ Potential issueReplace HashMap with ConcurrentHashMap for thread safety.
The current
HashMapimplementation is not thread-safe, which could lead to race conditions in a multi-threaded environment.- private final Map<Long, List<LightRoute>> cache = new HashMap<>(); + private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>();
237-239:⚠️ Potential issueImprove error handling in stream processing.
The catch block wraps all exceptions in RuntimeException, losing valuable error context.
- throw new RuntimeException(e); + log.error("Error processing database stream for univId: {}", univId, e); + throw new RouteException("Error processing route stream", ROUTE_STREAM_PROCESSING_ERROR, e);
352-375:⚠️ Potential issueInvalidate local cache after creating new routes.
The
createRoutemethod deletes Redis entries but doesn't update the localcache, which could lead to stale data.redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); + cache.remove(univId); eventPublisher.publishEvent(new RouteCreatedEvent());
237-239:⚠️ Potential issueImprove error handling in stream processing.
The catch block wraps all exceptions in RuntimeException, losing valuable error context.
- throw new RuntimeException(e); + log.error("Error processing database stream for univId: {}", univId, e); + throw new RouteException("Error processing route stream", ROUTE_STREAM_PROCESSING_ERROR, e);
352-375:⚠️ Potential issueEnhance route creation with proper cache invalidation and event data.
- Local cache is not invalidated after route creation.
- RouteCreatedEvent lacks context about created routes.
@Transactional @RevisionOperation(RevisionOperationType.CREATE_ROUTE) public synchronized AllRoutesInfo createRoute(Long univId, CreateRoutesReqDTO requests){ // ... existing code ... redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); + cache.remove(univId); // Invalidate local cache - eventPublisher.publishEvent(new RouteCreatedEvent()); + eventPublisher.publishEvent(new RouteCreatedEvent(univId, routes)); return routeCalculator.assembleRoutes(routes); }
63-63:⚠️ Potential issueReplace HashMap with ConcurrentHashMap for thread safety.
The current
HashMapimplementation is not thread-safe and could lead to race conditions in a multi-threaded environment.- private final Map<Long, List<LightRoute>> cache = new HashMap<>(); + private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>();uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java (2)
44-48: 🛠️ Refactor suggestionEnhance SSE implementation with proper error handling.
The current SSE implementation lacks proper error handling and uses hardcoded timeout.
public SseEmitter getAllRoutes(@PathVariable("univId") Long univId){ - SseEmitter emitter = new SseEmitter(60 * 1000L); // timeout 1분 + SseEmitter emitter = new SseEmitter(sseConfig.getTimeout()); + emitter.onCompletion(() -> log.info("SSE completed for univId: {}", univId)); + emitter.onTimeout(() -> log.warn("SSE timeout for univId: {}", univId)); + emitter.onError((ex) -> log.error("SSE error for univId: {}", univId, ex)); mapService.getAllRoutesBySSE(univId, emitter); return emitter; }
44-48: 🛠️ Refactor suggestionImprove SSE endpoint implementation.
The current SSE implementation has hardcoded timeout and lacks proper error handling.
+ @Value("${sse.timeout:60000}") + private long sseTimeout; + @Override @GetMapping("/{univId}/routes/sse") public SseEmitter getAllRoutes(@PathVariable("univId") Long univId){ - SseEmitter emitter = new SseEmitter(60 * 1000L); // timeout 1분 + SseEmitter emitter = new SseEmitter(sseTimeout); + emitter.onCompletion(() -> log.info("SSE completed for univId: {}", univId)); + emitter.onTimeout(() -> log.warn("SSE timeout for univId: {}", univId)); + emitter.onError(ex -> log.error("SSE error for univId: {}", univId, ex)); mapService.getAllRoutesBySSE(univId, emitter); return emitter; }
🧹 Nitpick comments (10)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCacheCalculator.java (3)
71-73: Clarify building route distance threshold logic.The
-1offset in the comparison withBUILDING_ROUTE_DISTANCEmakes the logic less clear. Consider defining a separate constant for the threshold or documenting why the offset is needed.- return route.getDistance() > BUILDING_ROUTE_DISTANCE - 1; + // Define a new constant if this is a business rule + private static final double BUILDING_ROUTE_THRESHOLD = BUILDING_ROUTE_DISTANCE - 1; + return route.getDistance() > BUILDING_ROUTE_THRESHOLD;
75-81: Improve readability by extracting predicates.The node filtering logic could be more readable by extracting the predicates into well-named methods.
- return nodeMap.values().stream() - .filter(node -> node.isCore() - || (adjMap.containsKey(node.getId()) && adjMap.get(node.getId()).size() == 1)) - .toList(); + return nodeMap.values().stream() + .filter(node -> isStartNode(node, adjMap)) + .toList(); + + private boolean isStartNode(LightNode node, Map<Long, List<LightRoute>> adjMap) { + return node.isCore() || hasExactlyOneConnection(node, adjMap); + } + + private boolean hasExactlyOneConnection(LightNode node, Map<Long, List<LightRoute>> adjMap) { + return adjMap.containsKey(node.getId()) && adjMap.get(node.getId()).size() == 1; + }
83-139: Consider breaking down the complex BFS logic into smaller methods.While the method is well-commented, its high cyclomatic complexity makes it harder to maintain. Consider extracting some logic into helper methods.
Here's a suggested refactoring:
private List<CoreRouteResDTO> getCoreRoutes(Map<Long, List<LightRoute>> adjMap, List<LightNode> startNode) { List<CoreRouteResDTO> result = new ArrayList<>(); Set<Long> visitedCoreNodes = new HashSet<>(); Set<Long> routeSet = new HashSet<>(); Queue<LightNode> nodeQueue = initializeQueue(startNode, visitedCoreNodes); while(!nodeQueue.isEmpty()) { LightNode now = nodeQueue.poll(); processAdjacentRoutes(now, adjMap, routeSet, visitedCoreNodes, nodeQueue, result); } return result; } + private Queue<LightNode> initializeQueue(List<LightNode> startNodes, Set<Long> visitedCoreNodes) { + Queue<LightNode> queue = new LinkedList<>(); + startNodes.forEach(n -> { + queue.add(n); + visitedCoreNodes.add(n.getId()); + }); + return queue; + } + private void processAdjacentRoutes(LightNode now, Map<Long, List<LightRoute>> adjMap, + Set<Long> routeSet, Set<Long> visitedCoreNodes, Queue<LightNode> nodeQueue, + List<CoreRouteResDTO> result) { + for(LightRoute r : adjMap.get(now.getId())) { + if(routeSet.contains(r.getId())) continue; + processRoute(now, r, routeSet, visitedCoreNodes, nodeQueue, adjMap, result); + } + } + private void processRoute(LightNode now, LightRoute r, Set<Long> routeSet, + Set<Long> visitedCoreNodes, Queue<LightNode> nodeQueue, + Map<Long, List<LightRoute>> adjMap, List<CoreRouteResDTO> result) { + LightNode currentNode = getNextNode(now, r); + List<RouteCoordinatesInfoResDTO> coreRoute = new ArrayList<>(); + coreRoute.add(RouteCoordinatesInfoResDTO.of(r.getId(), now.getId(), currentNode.getId())); + routeSet.add(r.getId()); + + while (true) { + if (isTerminalNode(currentNode, now)) { + handleTerminalNode(currentNode, visitedCoreNodes, nodeQueue); + break; + } + if (isEndPoint(currentNode, adjMap)) break; + currentNode = processSubNode(currentNode, adjMap, routeSet, coreRoute); + } + result.add(CoreRouteResDTO.of(now.getId(), currentNode.getId(), coreRoute)); + }uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java (1)
233-235: Consider limiting exponential growth of risk heuristic weight.The current implementation uses squared caution count, which could lead to disproportionately high weights for routes with many caution factors.
Consider adding an upper limit or using a more gradual scaling:
- return Math.pow(currentCautionCount,2); + return Math.min(10.0, Math.pow(currentCautionCount, 1.5));uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (4)
75-75: Replace emoji logging with professional log format.The current log message uses emojis which is not a professional logging practice. Consider using a more standard format.
- log.info("🚀🚀🚀🚀🚀🚀🚀HIT🚀🚀🚀🚀🚀🚀🚀"); + log.info("Cache hit for univId: {}", univId);
75-75: Replace emoji-based logging with descriptive message.The current log message uses emojis and is not suitable for production logging.
- log.info("🚀🚀🚀🚀🚀🚀🚀HIT🚀🚀🚀🚀🚀🚀🚀"); + log.info("Cache hit for univId: {}", univId);
374-374: Enhance RouteCreatedEvent with context.The event lacks information about the created routes or univId, which could be useful for event listeners.
- eventPublisher.publishEvent(new RouteCreatedEvent()); + eventPublisher.publishEvent(new RouteCreatedEvent(univId, routes));
75-75: Replace emoji logging with standard log message.Using emojis in log messages is not a standard practice and may cause issues with log parsing tools.
- log.info("🚀🚀🚀🚀🚀🚀🚀HIT🚀🚀🚀🚀🚀🚀🚀"); + log.info("Cache hit for univId: {}", univId);uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java (2)
76-79: Consider adding custom validation for routes.While @Valid is used, consider adding custom validation for the routes parameter to ensure business rules are met.
Consider creating a custom validator:
@Validated public class CreateRoutesValidator { @AssertTrue(message = "Routes must form a connected path") public boolean isValidRoutePath() { // Implement path connectivity validation } }
99-105: Add API documentation for version comparison endpoint.The new endpoint for comparing route versions would benefit from detailed API documentation.
Add Swagger annotations:
+ @Operation(summary = "Get changed routes by version", + description = "Retrieves the differences in routes between the current version and a specified version") + @ApiResponse(responseCode = "200", description = "Successfully retrieved route changes") + @ApiResponse(responseCode = "404", description = "Version not found") @Override @GetMapping("/{univId}/routes/{versionId}") public ResponseEntity<GetChangedRoutesByRevisionResDTO> getChangedRoutesByRevision(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
uniro_backend/src/main/java/com/softeer5/uniro_backend/building/service/BuildingService.java(4 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/WebMvcConfig.java(2 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/common/utils/RouteUtils.java(0 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java(5 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/FastestRouteResDTO.java(4 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/enums/RoadExclusionPolicy.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java(4 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCacheCalculator.java(1 hunks)uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java(15 hunks)
💤 Files with no reviewable changes (1)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/common/utils/RouteUtils.java
✅ Files skipped from review due to trivial changes (1)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/map/enums/RoadExclusionPolicy.java
🚧 Files skipped from review as they are similar to previous changes (4)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/building/service/BuildingService.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/map/dto/response/FastestRouteResDTO.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/WebMvcConfig.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/common/constant/UniroConst.java
👮 Files not reviewed due to content moderation or server errors (2)
- uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java
- uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java
🔇 Additional comments (9)
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCacheCalculator.java (1)
1-27: LGTM! Well-organized imports and proper Spring component annotation.The class structure follows best practices with organized imports and proper Spring framework integration.
uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/RouteCalculator.java (3)
5-6: LGTM! Policy-based cost calculation improves route evaluation.The addition of policy-based route evaluation and cost calculation improves the modularity of the code.
41-51: LGTM! Enhanced cost tracking with caution count.The addition of
cautionCounttoCostToNextNodeenables better tracking of risk factors during route traversal.
152-172: Check correctness of cost calculations with caution/danger weighting.When building the
FastestRouteResDTO, ensure you correctly combine route distance, caution/danger heuristics, and the separate cost calculations by policy. Overlapping risk factors or multiple caution increments might lead to unexpected totals.uniro_backend/src/main/java/com/softeer5/uniro_backend/map/service/MapService.java (3)
63-63: Replace HashMap with ConcurrentHashMap for thread safety.The current
HashMapimplementation is not thread-safe and could lead to race conditions in a multi-threaded environment.- private final Map<Long, List<LightRoute>> cache = new HashMap<>(); + private final Map<Long, List<LightRoute>> cache = new ConcurrentHashMap<>();
237-239: Improve error handling in stream processing.The catch block wraps all exceptions in RuntimeException, losing valuable error context.
- throw new RuntimeException(e); + log.error("Error processing database stream for univId: {}", univId, e); + throw new RouteException("Error processing route stream", ROUTE_STREAM_PROCESSING_ERROR, e);
352-375: Invalidate local cache after creating new routes.The method only invalidates Redis cache but not the local cache, which could lead to stale data.
redisService.deleteRoutesData(univId.toString(), routeCount / STREAM_FETCH_SIZE + 1); + cache.remove(univId); // Invalidate local cache eventPublisher.publishEvent(new RouteCreatedEvent());uniro_backend/src/main/java/com/softeer5/uniro_backend/map/controller/MapController.java (2)
44-48: Enhance SSE implementation with proper error handling.The current SSE implementation lacks proper error handling and uses a hardcoded timeout.
public SseEmitter getAllRoutes(@PathVariable("univId") Long univId){ - SseEmitter emitter = new SseEmitter(60 * 1000L); // timeout 1분 + SseEmitter emitter = new SseEmitter(sseConfig.getTimeout()); + emitter.onCompletion(() -> log.info("SSE completed for univId: {}", univId)); + emitter.onTimeout(() -> log.warn("SSE timeout for univId: {}", univId)); + emitter.onError((ex) -> log.error("SSE error for univId: {}", univId, ex)); mapService.getAllRoutesBySSE(univId, emitter); return emitter; }
99-105: LGTM!The implementation of version comparison endpoint is clean and follows the established patterns.
| List<Node>startNode = determineStartNodes(adjMap, nodeMap); | ||
|
|
||
| List<Long> endNodes = adjMap.entrySet().stream() | ||
| .filter(entry -> entry.getValue().size() == 1) | ||
| .map(Map.Entry::getKey) | ||
| .toList(); | ||
| AllRoutesInfo result = AllRoutesInfo.of(nodeInfos, getCoreRoutes(adjMap, startNode), buildingRoutes); | ||
|
|
||
| if (endNodes.size() == IS_SINGLE_ROUTE) { | ||
| return nodeMap.get(endNodes.get(0)); | ||
| } else if (endNodes.isEmpty()) { | ||
| return routes.get(0).getNode1(); | ||
| } else { | ||
| throw new RouteException("Invalid Map", ErrorCode.INVALID_MAP); | ||
| for(Node node : buildingNodes) { | ||
| nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getX(), node.getY())); | ||
| } |
There was a problem hiding this comment.
Fix the order of node info addition.
Building node information is added to nodeInfos after the AllRoutesInfo result is created, making these nodes unavailable in the result.
Move the building node addition before creating the result:
- AllRoutesInfo result = AllRoutesInfo.of(nodeInfos, getCoreRoutes(adjMap, startNode), buildingRoutes);
-
- for(Node node : buildingNodes) {
- nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getX(), node.getY()));
- }
-
- return result;
+ for(Node node : buildingNodes) {
+ nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getX(), node.getY()));
+ }
+
+ return AllRoutesInfo.of(nodeInfos, getCoreRoutes(adjMap, startNode), buildingRoutes);📝 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.
| List<Node>startNode = determineStartNodes(adjMap, nodeMap); | |
| List<Long> endNodes = adjMap.entrySet().stream() | |
| .filter(entry -> entry.getValue().size() == 1) | |
| .map(Map.Entry::getKey) | |
| .toList(); | |
| AllRoutesInfo result = AllRoutesInfo.of(nodeInfos, getCoreRoutes(adjMap, startNode), buildingRoutes); | |
| if (endNodes.size() == IS_SINGLE_ROUTE) { | |
| return nodeMap.get(endNodes.get(0)); | |
| } else if (endNodes.isEmpty()) { | |
| return routes.get(0).getNode1(); | |
| } else { | |
| throw new RouteException("Invalid Map", ErrorCode.INVALID_MAP); | |
| for(Node node : buildingNodes) { | |
| nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getX(), node.getY())); | |
| } | |
| List<Node>startNode = determineStartNodes(adjMap, nodeMap); | |
| for(Node node : buildingNodes) { | |
| nodeInfos.add(NodeInfoResDTO.of(node.getId(), node.getX(), node.getY())); | |
| } | |
| return AllRoutesInfo.of(nodeInfos, getCoreRoutes(adjMap, startNode), buildingRoutes); |
| if(!redisService.hasData(univId.toString())){ | ||
| List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | ||
| List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList(); | ||
| LightRoutes value = new LightRoutes(lightRoutes); | ||
| redisService.saveData(univId.toString(), value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Redis operations.
Redis operations should be wrapped in try-catch blocks to handle potential connection issues or serialization errors.
- if(!redisService.hasData(univId.toString())){
- List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId);
- List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList();
- LightRoutes value = new LightRoutes(lightRoutes);
- redisService.saveData(univId.toString(), value);
- }
+ try {
+ if(!redisService.hasData(univId.toString())){
+ List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId);
+ List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList();
+ LightRoutes value = new LightRoutes(lightRoutes);
+ redisService.saveData(univId.toString(), value);
+ }
+ } catch (Exception e) {
+ log.error("Redis operation failed for univId: {}", univId, e);
+ // Fallback to database
+ List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId);
+ return routeCacheCalculator.assembleRoutes(routes.stream().map(LightRoute::new).toList());
+ }📝 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.
| if(!redisService.hasData(univId.toString())){ | |
| List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | |
| List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList(); | |
| LightRoutes value = new LightRoutes(lightRoutes); | |
| redisService.saveData(univId.toString(), value); | |
| } | |
| try { | |
| if(!redisService.hasData(univId.toString())){ | |
| List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | |
| List<LightRoute> lightRoutes = routes.stream().map(LightRoute::new).toList(); | |
| LightRoutes value = new LightRoutes(lightRoutes); | |
| redisService.saveData(univId.toString(), value); | |
| } | |
| } catch (Exception e) { | |
| log.error("Redis operation failed for univId: {}", univId, e); | |
| // Fallback to database | |
| List<Route> routes = routeRepository.findAllRouteByUnivIdWithNodes(univId); | |
| return routeCacheCalculator.assembleRoutes(routes.stream().map(LightRoute::new).toList()); | |
| } |
| @Async | ||
| public void getAllRoutesBySSE(Long univId, SseEmitter emitter) { | ||
| String redisKeyPrefix = univId + ":"; | ||
| int batchNumber = 1; | ||
|
|
||
| try { | ||
| // 1️⃣ Redis 데이터가 있다면 우선 처리 | ||
| if (processRedisData(univId, redisKeyPrefix, batchNumber, emitter)) { | ||
| return; | ||
| } | ||
|
|
||
| // 2️⃣ Redis에 데이터가 없으면 기존 DB 조회 방식 사용 | ||
| processDatabaseData(univId, redisKeyPrefix, batchNumber, emitter); | ||
| } catch (Exception e) { | ||
| emitter.completeWithError(e); | ||
| log.error("SSE error: {}", e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance SSE implementation with proper error handling.
The current SSE implementation lacks proper error handling and cleanup mechanisms. Consider:
- Moving timeout to configuration
- Adding completion callback
- Adding timeout callback
- Proper cleanup of resources
+ @Value("${sse.timeout:60000}")
+ private long sseTimeout;
+
+ private final Set<SseEmitter> emitters = ConcurrentHashMap.newKeySet();
+
@Async
public void getAllRoutesBySSE(Long univId, SseEmitter emitter) {
+ emitters.add(emitter);
+ emitter.onCompletion(() -> emitters.remove(emitter));
+ emitter.onTimeout(() -> {
+ emitters.remove(emitter);
+ emitter.complete();
+ });
+
String redisKeyPrefix = univId + ":";
int batchNumber = 1;
try {
// 1️⃣ Redis 데이터가 있다면 우선 처리
if (processRedisData(univId, redisKeyPrefix, batchNumber, emitter)) {
+ emitters.remove(emitter);
return;
}
// 2️⃣ Redis에 데이터가 없으면 기존 DB 조회 방식 사용
processDatabaseData(univId, redisKeyPrefix, batchNumber, emitter);
} catch (Exception e) {
+ emitters.remove(emitter);
emitter.completeWithError(e);
log.error("SSE error: {}", e.getMessage(), 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.
| @Async | |
| public void getAllRoutesBySSE(Long univId, SseEmitter emitter) { | |
| String redisKeyPrefix = univId + ":"; | |
| int batchNumber = 1; | |
| try { | |
| // 1️⃣ Redis 데이터가 있다면 우선 처리 | |
| if (processRedisData(univId, redisKeyPrefix, batchNumber, emitter)) { | |
| return; | |
| } | |
| // 2️⃣ Redis에 데이터가 없으면 기존 DB 조회 방식 사용 | |
| processDatabaseData(univId, redisKeyPrefix, batchNumber, emitter); | |
| } catch (Exception e) { | |
| emitter.completeWithError(e); | |
| log.error("SSE error: {}", e.getMessage(), e); | |
| } | |
| } | |
| @Value("${sse.timeout:60000}") | |
| private long sseTimeout; | |
| private final Set<SseEmitter> emitters = ConcurrentHashMap.newKeySet(); | |
| @Async | |
| public void getAllRoutesBySSE(Long univId, SseEmitter emitter) { | |
| emitters.add(emitter); | |
| emitter.onCompletion(() -> emitters.remove(emitter)); | |
| emitter.onTimeout(() -> { | |
| emitters.remove(emitter); | |
| emitter.complete(); | |
| }); | |
| String redisKeyPrefix = univId + ":"; | |
| int batchNumber = 1; | |
| try { | |
| // 1️⃣ Redis 데이터가 있다면 우선 처리 | |
| if (processRedisData(univId, redisKeyPrefix, batchNumber, emitter)) { | |
| emitters.remove(emitter); | |
| return; | |
| } | |
| // 2️⃣ Redis에 데이터가 없으면 기존 DB 조회 방식 사용 | |
| processDatabaseData(univId, redisKeyPrefix, batchNumber, emitter); | |
| } catch (Exception e) { | |
| emitters.remove(emitter); | |
| emitter.completeWithError(e); | |
| log.error("SSE error: {}", e.getMessage(), e); | |
| } | |
| } |
📝 PR 타입
🚀 변경 사항
💡 To Reviewer
🧪 테스트 결과
✅ 반영 브랜치
Summary by CodeRabbit
AUTO_INCREMENTvalues for specific tables to maintain consistency in database state.