Conversation
WalkthroughThis update introduces multiple new features and refinements throughout the project. New GitHub Actions workflows automate continuous integration and deployment of the Uniro-server. Docker configurations, Gradle dependency adjustments, and environment-specific settings have been added or updated. A comprehensive admin revision module—with new annotations, aspects, DTOs, entities, repositories, and services—has been implemented for auditing. The routing system has been modified for enhanced route calculation and error handling, and new map client functionality has been introduced for elevation data. Additionally, test fixtures and several unit tests have been added. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AnnotatedMethod
participant RevisionAspect
participant RevisionContext
Caller->>AnnotatedMethod: Invoke method (@RevisionOperation)
AnnotatedMethod->>RevisionAspect: Interception
RevisionAspect->>RevisionContext: Set univId & action
RevisionAspect->>AnnotatedMethod: Proceed with method
AnnotatedMethod-->>RevisionAspect: Return result
RevisionAspect->>RevisionContext: Clear context
sequenceDiagram
participant Client
participant RouteController
participant RouteService
participant Repository
Client->>RouteController: GET /routes/{univId}
RouteController->>RouteService: getAllRoutes(univId)
RouteService->>Repository: Query routes with nodes
Repository-->>RouteService: List<Route>
RouteService->>RouteService: Build BFS, check intersections
RouteService-->>RouteController: Construct GetAllRoutesResDTO
RouteController-->>Client: HTTP 200 with DTO
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 17
🧹 Nitpick comments (51)
uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/RouteDetailDTO.java (1)
18-19: Fix typo in Schema example and consider type safety improvements.
- The Schema example has a typo: "lag" should be "lng" for longitude.
- Consider using a more specific type than
Map<String, Double>to ensure coordinate consistency. Options:
- Create a dedicated
Coordinatesclass- Use an enum for map keys
- Use a record class
Example implementation using a record:
public record Coordinates(double lat, double lng) {}Then update the field:
-@Schema(description = "상세 경로의 좌표", example = "{\"lag\": 127.123456, \"lat\": 37.123456}") -private final Map<String, Double> coordinates; +@Schema(description = "상세 경로의 좌표", example = "{\"lng\": 127.123456, \"lat\": 37.123456}") +private final Coordinates coordinates;uniro_backend/src/main/java/com/softeer5/uniro_backend/route/controller/RouteApi.java (1)
23-23: Improve error documentation.The error response is currently documented as "EXCEPTION(임시)" which appears to be a temporary placeholder. Consider documenting specific error scenarios and their corresponding response structures.
Example improvement:
-@ApiResponse(responseCode = "400", description = "EXCEPTION(임시)", content = @Content), +@ApiResponse(responseCode = "400", description = "Invalid university ID or data not found", content = @Content), +@ApiResponse(responseCode = "500", description = "Internal server error", content = @Content)uniro_backend/src/main/resources/application-test.yml (1)
33-33: Newline at End of FileYAMLlint has flagged that the file does not end with a newline. Adding a newline at the end of the file improves compatibility with many tools and adheres to best practices.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 33-33: no new line character at the end of file
(new-line-at-end-of-file)
uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/RouteCoordinatesInfo.java (2)
5-5: Remove unused import.The
java.util.Mapimport is not used in this class.-import java.util.Map;
10-12: Consider adding null validation.The fields are properly marked as final, but consider adding null validation to prevent NullPointerExceptions.
public static RouteCoordinatesInfo of(Long routeId, Long startNodeId, Long endNodeId) { + Objects.requireNonNull(routeId, "routeId must not be null"); + Objects.requireNonNull(startNodeId, "startNodeId must not be null"); + Objects.requireNonNull(endNodeId, "endNodeId must not be null"); return new RouteCoordinatesInfo(routeId, startNodeId, endNodeId); }Don't forget to add the import:
+import java.util.Objects;uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/CoreRouteDTO.java (2)
11-11: Consider making the routes list immutable.The
routeslist is marked as final but its contents can still be modified. Consider making it immutable to ensure complete immutability of the DTO.- private final List<RouteCoordinatesInfo> routes; + private final List<RouteCoordinatesInfo> routes = Collections.unmodifiableList(routes);Don't forget to add the import:
import java.util.Collections;
7-8: Add class-level documentation.As this is a DTO class that's part of the public API, it should have class-level documentation explaining its purpose and usage.
+/** + * Data Transfer Object representing core routing information between two nodes. + * This class is immutable and should be created using the static factory method {@link #of}. + */ @Getter public class CoreRouteDTO {uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/CreateRouteServiceReqDTO.java (1)
6-8: Add class-level documentation.While the annotations are appropriate, please add Javadoc to describe:
- The purpose of this DTO
- What the x and y coordinates represent
- Any validation rules or constraints for the coordinates
Example:
+/** + * DTO for creating route service requests, containing coordinate information. + * + * @param x The x-coordinate (longitude/latitude) + * @param y The y-coordinate (longitude/latitude) + */ @Getter @RequiredArgsConstructor public class CreateRouteServiceReqDTO {uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/NodeInfo.java (1)
15-17: Add input validation in the factory method.The factory method should validate input parameters to prevent invalid objects from being created.
public static NodeInfo of(Long nodeId, Map<String, Double> coordinates) { + if (nodeId == null || coordinates == null) { + throw new IllegalArgumentException("NodeId and coordinates must not be null"); + } + if (!coordinates.containsKey("latitude") || !coordinates.containsKey("longitude")) { + throw new IllegalArgumentException("Coordinates must contain latitude and longitude"); + } return new NodeInfo(nodeId, Map.copyOf(coordinates)); }uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/GetAllRoutesResDTO.java (1)
14-17: Add example values in Swagger annotations.The Swagger annotations have empty example values, which reduces API documentation quality.
- @Schema(description = "노드 정보 (id, 좌표)", example = "") + @Schema(description = "노드 정보 (id, 좌표)", example = "[{\"nodeId\": 1, \"coordinates\": {\"latitude\": 37.5665, \"longitude\": 126.9780}}]") private final List<NodeInfo> nodeInfos; - @Schema(description = "루트 정보 (id, startNodeId, endNodeId)", example = "") + @Schema(description = "루트 정보 (id, startNodeId, endNodeId)", example = "[{\"routeId\": 1, \"startNodeId\": 1, \"endNodeId\": 2}]") private final List<CoreRouteDTO> coreRoutes;uniro_backend/src/main/java/com/softeer5/uniro_backend/node/client/MapClientImpl.java (2)
28-30: WebClient setup
The default WebClient builder approach is straightforward. Consider externalizing the base URL to ensure maintainability if multiple endpoints are used.
46-65: Blocking with Mono.when(...).block()
This method fetches and processes elevation data. Blocking the thread in a reactive stack may affect performance. Ideally, consider returning a reactive type or using a non-blocking approach if performance is critical.uniro_backend/src/main/java/com/softeer5/uniro_backend/route/service/RouteService.java (3)
30-82:getAllRoutesmethod BFS logic
- Constructing an adjacency map and performing BFS is a solid approach.
- Properly checks for an empty route set to throw
RouteNotFoundException.- Nicely resolves the scenario where no core node exists, analyzing end nodes for single-route retrieval.
- Consider ensuring cyclical routes (and large graphs) don’t lead to performance bottlenecks. Possibly keep track of visited route IDs in BFS to avoid re-traversing the same edges.
84-139:getCoreRoutesBFS
- Very clear BFS approach for core nodes.
- Using
visitedCoreNodesandrouteSethelps avoid duplication.- The code might become complex if new route types or special cases are introduced. Consider extracting the BFS logic into a dedicated utility class to reduce service complexity.
141-160:getSingleRoutesBFS
- Traverses from a subnode, capturing route segments into
coreRoute.- The loop uses a
flagto continue iteration if a connecting route is found. This approach is fine, though more explicit BFS/DFS structures might be clearer to future maintainers.uniro_backend/src/main/java/com/softeer5/uniro_backend/route/service/RouteCalculationService.java (6)
3-4: Consider removing wildcard imports
Using wildcard imports can lead to naming conflicts and reduced clarity.
95-103: Node ordering logic
SwappingfirstNodeandsecondNodeensures consistent route direction. However, consider documenting this logic to reduce confusion.
216-217: Initial checkpoint setup
Defining checkpoint coordinates and the default direction type is sensible. Confirm if a typed object might be more explicit than a Map.
233-234: Append route finish detail
Setting distance to zero for the FINISH step is a design choice. Consider clarifying rationale if needed.
366-366: Check for self-route and potential overlap
Ensuring the first and last coordinates differ is crucial. This throws an exception if they're the same, consistent with earlier logic. The rest of the method reuses the STRtree approach for intersection checks effectively. Consider clarifying edge cases (e.g., repeated midpoints).Also applies to: 368-368, 370-372, 374-375, 377-421
492-494: Coordinate hashing
Using a string with a delimiter is straightforward. Consider potential floating-point precision issues leading to near-duplicates.uniro_backend/src/main/java/com/softeer5/uniro_backend/node/client/MapClient.java (1)
1-9: Interface for fetching heights
This contract is straightforward. Ensure there is a robust implementation and error handling for external API calls.uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/annotation/RevisionOperation.java (1)
1-12: LGTM! Consider adding Javadoc.The annotation is well-structured with proper metadata annotations. Consider adding Javadoc to document the purpose and usage of this annotation.
Add documentation like this:
+/** + * Annotation to mark methods that perform revision operations. + * The type of revision operation can be specified using {@link RevisionOperationType}. + */ @Target(ElementType.METHOD) @Retention(RetentionPolicy.RUNTIME) @Documented public @interface RevisionOperation { RevisionOperationType value() default RevisionOperationType.DEFAULT; }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/exception/custom/RouteCalculationException.java (1)
1-10: LGTM! Consider adding Javadoc.The exception class is well-structured and follows Java exception handling best practices.
Add documentation like this:
+/** + * Exception thrown when route calculation fails. + * This exception carries both a descriptive message and a specific error code. + */ public class RouteCalculationException extends CustomException { public RouteCalculationException(String message, ErrorCode errorCode) { super(message, errorCode); } }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/exception/custom/InvalidMapException.java (1)
1-10: LGTM! Consider adding Javadoc.The exception class is well-structured and follows Java exception handling best practices.
Add documentation like this:
+/** + * Exception thrown when map data is invalid. + * This exception carries both a descriptive message and a specific error code. + */ public class InvalidMapException extends CustomException { public InvalidMapException(String message, ErrorCode errorCode) { super(message, errorCode); } }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/exception/custom/ElevationApiException.java (1)
1-10: LGTM! Consider adding Javadoc.The exception class is well-structured and follows Java exception handling best practices.
Add documentation like this:
+/** + * Exception thrown when the elevation API call fails. + * This exception carries both a descriptive message and a specific error code. + */ public class ElevationApiException extends CustomException { public ElevationApiException(String message, ErrorCode errorCode) { super(message, errorCode); } }uniro_backend/src/test/java/com/softeer5/uniro_backend/fixture/NodeFixture.java (1)
12-18: Consider parameterizing univId and refactoring duplicate code.The current implementation has two issues:
- Hardcoded
univIdreduces test flexibility and isolation- Duplicate code between
createNodeandcreateCoreNodeConsider this refactoring:
- public static Node createNode(double x, double y){ - return Node.builder() - .univId(1001L) - .isCore(false) - .coordinates(geometryFactory.createPoint(new Coordinate(x,y))) - .build(); - } - - public static Node createCoreNode(double x, double y){ - return Node.builder() - .univId(1001L) - .isCore(true) - .coordinates(geometryFactory.createPoint(new Coordinate(x,y))) - .build(); - } + public static Node createNode(double x, double y, Long univId, boolean isCore) { + return Node.builder() + .univId(univId) + .isCore(isCore) + .coordinates(geometryFactory.createPoint(new Coordinate(x,y))) + .build(); + } + + public static Node createNode(double x, double y) { + return createNode(x, y, 1001L, false); + } + + public static Node createCoreNode(double x, double y) { + return createNode(x, y, 1001L, true); + }Also applies to: 20-26
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/entity/RevInfo.java (2)
15-15: Standardize table name casing.The table name "Revinfo" uses mixed case. For consistency with JPA naming conventions and to avoid issues with case-sensitive databases, consider using either all uppercase with underscores or all lowercase.
-@Table(name = "Revinfo") +@Table(name = "REVINFO")
25-27: Add validation and documentation for fields.Consider adding:
@Minvalidation forunivIdsince it's non-nullable- Documentation for expected
actionvalues+ @Min(1) @Column(name = "univ_id", nullable = false) private Long univId; + + /** + * Represents the type of revision action performed. + * Expected values: ["INSERT", "UPDATE", "DELETE"] + */ private String action;uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/RevInfoDTO.java (1)
14-20: Maintain consistent comment language.The inline comments mix English and Korean. Consider using a single language for consistency.
- private final Long rev; // Revision 번호 - private final LocalDateTime revTime; // Revision 시간 - private final Long univId; //UnivId - private final String action; // 행위 + private final Long rev; // 리비전 번호 + private final LocalDateTime revTime; // 리비전 시간 + private final Long univId; // 대학교 ID + private final String action; // 변경 행위uniro_backend/src/main/java/com/softeer5/uniro_backend/common/utils/GeoUtils.java (1)
11-11: LGTM! Good singleton implementation.The renaming to
GeoUtilsbetter reflects the class's purpose, and the singleton pattern is appropriately implemented forGeometryFactory. Consider adding a@ThreadSafeannotation to explicitly document the thread safety of this utility class.+@ThreadSafe public final class GeoUtils {Also applies to: 14-20
uniro_backend/src/main/java/com/softeer5/uniro_backend/node/entity/Node.java (1)
49-59: Consider making Node immutable.The addition of setter methods breaks immutability. Consider removing these setters and handling state changes through the builder pattern or creating new instances.
-public void setHeight(double height) { - this.height = height; -} - -public void setCoordinates(Point coordinates) { - this.coordinates = coordinates; -} - -public void setCore(boolean isCore){ - this.isCore = isCore; -}uniro_backend/src/test/java/com/softeer5/uniro_backend/node/client/MapClientImplTest.java (2)
29-32: Document test data coordinatesThe test uses hardcoded coordinates without explaining their significance. Consider adding comments to explain what these coordinates represent (e.g., San Francisco, New York, Seoul).
- Point p1 = geometryFactory.createPoint(new Coordinate(-122.4194, 37.7749)); - Point p2 = geometryFactory.createPoint(new Coordinate(-74.0060, 40.7128)); - Point p3 = geometryFactory.createPoint(new Coordinate(126.9780, 37.5665)); + // San Francisco coordinates + Point p1 = geometryFactory.createPoint(new Coordinate(-122.4194, 37.7749)); + // New York coordinates + Point p2 = geometryFactory.createPoint(new Coordinate(-74.0060, 40.7128)); + // Seoul coordinates + Point p3 = geometryFactory.createPoint(new Coordinate(126.9780, 37.5665));Also applies to: 56-58
48-48: Replace System.out with proper test loggingUsing System.out.println for logging in tests is not recommended. Consider using a proper logging framework or test reporter.
- System.out.println("Node coordinates: " + node.getCoordinates() + ", Elevation: " + node.getHeight()); + log.debug("Node coordinates: " + node.getCoordinates() + ", Elevation: " + node.getHeight());Also applies to: 79-79, 84-84
uniro_backend/src/test/java/com/softeer5/uniro_backend/route/RouteCalculationServiceTest.java (2)
43-48: Extract test data setup to helper methodsThe test data setup is repetitive across test methods. Consider extracting the creation of request DTOs to helper methods for better maintainability.
+ private List<CreateRouteServiceReqDTO> createTestRequests(double... coordinates) { + List<CreateRouteServiceReqDTO> requests = new ArrayList<>(); + for (int i = 0; i < coordinates.length; i += 2) { + requests.add(new CreateRouteServiceReqDTO(coordinates[i], coordinates[i + 1])); + } + return requests; + }Also applies to: 72-77, 96-100, 128-132, 152-158
50-53: Document coordinate system and unitsThe test uses various coordinate values without documenting the coordinate system or units. Consider adding documentation to explain the coordinate space being used.
Also applies to: 79-81, 102-107, 134-136, 160-164
uniro_backend/Dockerfile (1)
1-10: Add memory limits and health checkConsider adding:
- JVM memory settings to prevent container OOM issues
- HEALTHCHECK instruction for container health monitoring
FROM openjdk:17 ARG JAR_FILE=./build/libs/*.jar ARG SPRING_PROFILE COPY ${JAR_FILE} uniro-server.jar ENV SPRING_PROFILE=${SPRING_PROFILE} +ENV JAVA_OPTS="-Xms512m -Xmx1024m" -ENTRYPOINT ["java", "-Duser.timezone=Asia/Seoul","-Dspring.profiles.active=${SPRING_PROFILE}" ,"-jar" ,"uniro-server.jar"] +ENTRYPOINT ["java", "-Duser.timezone=Asia/Seoul","-Dspring.profiles.active=${SPRING_PROFILE}", ${JAVA_OPTS} ,"-jar" ,"uniro-server.jar"] + +HEALTHCHECK --interval=30s --timeout=3s \ + CMD curl -f http://localhost:8080/actuator/health || exit 1uniro_backend/src/main/resources/application-local.yml (2)
24-26: Consider using a secrets management solutionThe Google API key is being injected via environment variables. Consider using a secrets management solution for better security.
41-42: Add newline at end of fileAdd a newline character at the end of the file to comply with POSIX standards.
cors: allowed-origins: ${allowed-origins} +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: no new line character at the end of file
(new-line-at-end-of-file)
uniro_backend/src/main/resources/application-dev.yml (1)
43-44: CORS Configuration and File FormattingThe CORS allowed origins are configured via an environment variable. Additionally, static analysis (YAMLlint) flagged that there is no newline character at the end of the file. Please add a newline at the end to comply with formatting best practices.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 44-44: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/be-ci.yml (4)
13-13: Update Checkout Action VersionStatic analysis indicates that the runner for
actions/checkout@v3is outdated. Consider updating to a more recent version. For example:- - uses: actions/checkout@v3 + - uses: actions/checkout@v4This update may help leverage the latest features and performance improvements.
🧰 Tools
🪛 actionlint (1.7.4)
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
18-18: Update Setup-Java Action VersionThe workflow uses
actions/setup-java@v3, which static analysis flags as outdated. Consider updating to a later version if available (e.g., v4):- uses: actions/setup-java@v3 + uses: actions/setup-java@v4This helps ensure better compatibility and improved caching options.
🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
23-30: Update Cache Action VersionThe cache action is currently
actions/cache@v3and has been flagged as outdated. Consider updating to the latest stable version:- - uses: actions/cache@v3 + - uses: actions/cache@v4This update can improve caching efficiency during builds.
🧰 Tools
🪛 actionlint (1.7.4)
24-24: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
41-42: Ensure Trailing NewlineYAMLlint reports that there is no newline at the end of the file. Please add a newline at the end to conform with standard file formatting guidelines.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/be-cd.yml (5)
13-13: Update Checkout Action Version in CD WorkflowSimilar to the CI workflow, update the checkout action to a newer version:
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4This ensures consistency and benefits from any improvements in the action.
🧰 Tools
🪛 actionlint (1.7.4)
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-22: Update Setup-Java Action Version in CD WorkflowThe
actions/setup-java@v3step should be updated as well. For instance:- uses: actions/setup-java@v3 + uses: actions/setup-java@v4Updating this action can help mitigate any potential compatibility issues with JDK setup in the workflow.
🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
23-30: Update Cache Action Version in CD WorkflowPlease update the caching action to the latest version to improve performance:
- - uses: actions/cache@v3 + - uses: actions/cache@v4This change aligns with our upgrade recommendations in the CI workflow.
🧰 Tools
🪛 actionlint (1.7.4)
24-24: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-42: Update Docker Buildx Action VersionThe docker buildx action used here is
docker/setup-buildx-action@v2, which is flagged as outdated. Consider updating to a later version if available:- - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v2 + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3This ensures better compatibility with newer Docker features.
🧰 Tools
🪛 actionlint (1.7.4)
42-42: the runner of "docker/setup-buildx-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-45: Update Docker Login Action VersionThe docker login action is currently
docker/login-action@v1and should be updated. For example:- - name: Docker Hub Login - uses: docker/login-action@v1 + - name: Docker Hub Login + uses: docker/login-action@v2This update helps to address potential security and performance improvements.
🧰 Tools
🪛 actionlint (1.7.4)
45-45: the runner of "docker/login-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
uniro_backend/src/test/java/com/softeer5/uniro_backend/fixture/RouteFixture.java (2)
9-10: Add class documentation and consider null check.Consider adding Javadoc to explain the purpose of this fixture class and its usage in tests. Also, consider adding a null check for the GeometryFactory instance.
+/** + * Test fixture for creating Route entities with predefined values. + * Used to simplify test setup and maintain consistency across tests. + */ public class RouteFixture { - static GeometryFactory geometryFactory = GeoUtils.getInstance(); + static final GeometryFactory geometryFactory = Objects.requireNonNull( + GeoUtils.getInstance(), + "GeometryFactory instance cannot be null" + );
12-18: Enhance method documentation and flexibility.The method could benefit from the following improvements:
- Add method documentation
- Add parameter validation
- Consider making univId configurable for different test scenarios
+ /** + * Creates a Route entity connecting two nodes. + * + * @param node1 the starting node + * @param node2 the ending node + * @return a new Route instance + * @throws IllegalArgumentException if either node is null + */ public static Route createRoute(Node node1, Node node2) { + Objects.requireNonNull(node1, "node1 must not be null"); + Objects.requireNonNull(node2, "node2 must not be null"); + return Route.builder() .node1(node1) .node2(node2) .univId(1001L) .build(); } + /** + * Creates a Route entity with custom university ID. + * + * @param node1 the starting node + * @param node2 the ending node + * @param univId the university ID + * @return a new Route instance + * @throws IllegalArgumentException if either node is null + */ + public static Route createRoute(Node node1, Node node2, Long univId) { + Objects.requireNonNull(node1, "node1 must not be null"); + Objects.requireNonNull(node2, "node2 must not be null"); + Objects.requireNonNull(univId, "univId must not be null"); + + return Route.builder() + .node1(node1) + .node2(node2) + .univId(univId) + .build(); + }
🛑 Comments failed to post (17)
uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/CoreRouteDTO.java (2)
9-11:
⚠️ Potential issueFix typo in field name.
The field name
cordNode2Idappears to have a typo and should becoreNode2Idto maintain consistency withcoreNode1Id.private final Long coreNode1Id; - private final Long cordNode2Id; + private final Long coreNode2Id; private final List<RouteCoordinatesInfo> routes;📝 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 Long coreNode1Id; private final Long coreNode2Id; private final List<RouteCoordinatesInfo> routes;
19-21: 🛠️ Refactor suggestion
Add documentation and align parameter names with field names.
The factory method is part of the public API and should be documented. Also, the parameter names (
startNode,endNode) don't match the field names (coreNode1Id,cordNode2Id), which could cause confusion.+ /** + * Creates a new CoreRouteDTO instance. + * + * @param coreNode1Id the ID of the first core node + * @param coreNode2Id the ID of the second core node + * @param routes the list of route coordinate information + * @return a new CoreRouteDTO instance + */ - public static CoreRouteDTO of(Long startNode, Long endNode, List<RouteCoordinatesInfo> routes){ - return new CoreRouteDTO(startNode, endNode, routes); + public static CoreRouteDTO of(Long coreNode1Id, Long coreNode2Id, List<RouteCoordinatesInfo> routes){ + return new CoreRouteDTO(coreNode1Id, coreNode2Id, routes); }📝 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./** * Creates a new CoreRouteDTO instance. * * @param coreNode1Id the ID of the first core node * @param coreNode2Id the ID of the second core node * @param routes the list of route coordinate information * @return a new CoreRouteDTO instance */ public static CoreRouteDTO of(Long coreNode1Id, Long coreNode2Id, List<RouteCoordinatesInfo> routes){ return new CoreRouteDTO(coreNode1Id, coreNode2Id, routes); }uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/CreateRouteServiceReqDTO.java (1)
9-11: 🛠️ Refactor suggestion
Add validation constraints for coordinates.
Consider adding validation annotations to ensure coordinate values are valid:
+import javax.validation.constraints.NotNull; +import javax.validation.constraints.DecimalMin; +import javax.validation.constraints.DecimalMax; @Getter @RequiredArgsConstructor public class CreateRouteServiceReqDTO { + @NotNull(message = "x coordinate cannot be null") + @DecimalMin(value = "-180.0", message = "x coordinate must be greater than or equal to -180") + @DecimalMax(value = "180.0", message = "x coordinate must be less than or equal to 180") private final double x; + @NotNull(message = "y coordinate cannot be null") + @DecimalMin(value = "-90.0", message = "y coordinate must be greater than or equal to -90") + @DecimalMax(value = "90.0", message = "y coordinate must be less than or equal to 90") private final double y; }📝 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.import javax.validation.constraints.NotNull; import javax.validation.constraints.DecimalMin; import javax.validation.constraints.DecimalMax; import lombok.Getter; import lombok.RequiredArgsConstructor; @Getter @RequiredArgsConstructor public class CreateRouteServiceReqDTO { @NotNull(message = "x coordinate cannot be null") @DecimalMin(value = "-180.0", message = "x coordinate must be greater than or equal to -180") @DecimalMax(value = "180.0", message = "x coordinate must be less than or equal to 180") private final double x; @NotNull(message = "y coordinate cannot be null") @DecimalMin(value = "-90.0", message = "y coordinate must be greater than or equal to -90") @DecimalMax(value = "90.0", message = "y coordinate must be less than or equal to 90") private final double y; }uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/NodeInfo.java (2)
8-11: 🛠️ Refactor suggestion
Remove unnecessary @Setter annotation.
The class has final fields and follows immutability principles, but the
@Setterannotation contradicts this design. Remove it to maintain consistency.@RequiredArgsConstructor(access = AccessLevel.PRIVATE) @Getter -@Setter public class NodeInfo{📝 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.@RequiredArgsConstructor(access = AccessLevel.PRIVATE) @Getter public class NodeInfo{
12-13: 🛠️ Refactor suggestion
Protect against Map mutability.
The
coordinatesMap can be modified after object creation, breaking immutability. Consider making a defensive copy or using an immutable Map.private final Long nodeId; -private final Map<String, Double> coordinates; +private final Map<String, Double> coordinates = new HashMap<>();And update the factory method to make a defensive copy:
public static NodeInfo of(Long nodeId, Map<String, Double> coordinates) { - return new NodeInfo(nodeId, coordinates); + return new NodeInfo(nodeId, Map.copyOf(coordinates)); }Committable suggestion skipped: line range outside the PR's diff.
uniro_backend/src/main/java/com/softeer5/uniro_backend/route/dto/GetAllRoutesResDTO.java (2)
20-23: 🛠️ Refactor suggestion
Protect against List mutability and add input validation.
The Lists can be modified after object creation, and there's no input validation in the factory method.
private GetAllRoutesResDTO(List<NodeInfo> nodeInfos, List<CoreRouteDTO> coreRoutes){ - this.nodeInfos = nodeInfos; - this.coreRoutes = coreRoutes; + this.nodeInfos = List.copyOf(nodeInfos); + this.coreRoutes = List.copyOf(coreRoutes); } public static GetAllRoutesResDTO of(List<NodeInfo> nodeInfos, List<CoreRouteDTO> coreRoutes){ + if (nodeInfos == null || coreRoutes == null) { + throw new IllegalArgumentException("NodeInfos and coreRoutes must not be null"); + } return new GetAllRoutesResDTO(nodeInfos, coreRoutes); }Also applies to: 25-27
4-6: 🛠️ Refactor suggestion
Remove unnecessary Lombok annotations.
The class has a private constructor and final fields, making
@AllArgsConstructorand@Setterunnecessary.-import lombok.AllArgsConstructor; import lombok.Getter; -import lombok.Setter;📝 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.import lombok.Getter;uniro_backend/src/main/java/com/softeer5/uniro_backend/route/service/RouteCalculationService.java (1)
260-343: 🛠️ Refactor suggestion
Large method and TODO placeholders
The method is quite extensive with multiple responsibilities. Consider breaking it down into smaller private methods. The TODO comments about limiting MBR queries and using caching are valid enhancements.Would you like assistance implementing partial MBR loading and caching strategies?
uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/CustomReversionListener.java (1)
8-12: 🛠️ Refactor suggestion
Add error handling for type casting and ThreadLocal access.
The current implementation has potential issues:
- Unchecked cast to RevInfo could throw ClassCastException
- No null checks for ThreadLocal values from RevisionContext
Consider applying this safer implementation:
@Override public void newRevision(Object revisionEntity) { + if (!(revisionEntity instanceof RevInfo)) { + throw new IllegalArgumentException("Unexpected revision entity type: " + revisionEntity.getClass()); + } RevInfo revinfo = (RevInfo) revisionEntity; - revinfo.setUnivId(RevisionContext.getUnivId()); - revinfo.setAction(RevisionContext.getAction()); + Long univId = RevisionContext.getUnivId(); + String action = RevisionContext.getAction(); + if (univId == null || action == null) { + throw new IllegalStateException("Missing required revision context"); + } + revinfo.setUnivId(univId); + revinfo.setAction(action); }📝 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.@Override public void newRevision(Object revisionEntity) { if (!(revisionEntity instanceof RevInfo)) { throw new IllegalArgumentException("Unexpected revision entity type: " + revisionEntity.getClass()); } RevInfo revinfo = (RevInfo) revisionEntity; Long univId = RevisionContext.getUnivId(); String action = RevisionContext.getAction(); if (univId == null || action == null) { throw new IllegalStateException("Missing required revision context"); } revinfo.setUnivId(univId); revinfo.setAction(action); }uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/setting/RevisionContext.java (2)
3-26: 🛠️ Refactor suggestion
Add context validation and initialization check.
The class would benefit from methods to validate context state and ensure proper cleanup.
Add these utility methods:
public static boolean isInitialized() { return univIdHolder.get() != null && actionHolder.get() != null; } public static void validateContext() { if (!isInitialized()) { throw new IllegalStateException("RevisionContext not properly initialized"); } }Also, consider using try-with-resources pattern in the calling code:
try (var ignored = new AutoCloseable() { @Override public void close() { RevisionContext.clear(); } }) { RevisionContext.setUnivId(univId); RevisionContext.setAction(action); // ... use the context }
23-25:
⚠️ Potential issueFix incomplete context clearing.
The clear() method only removes univId but not action, which could lead to memory leaks.
public static void clear() { univIdHolder.remove(); + actionHolder.remove(); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public static void clear() { univIdHolder.remove(); actionHolder.remove(); }uniro_backend/src/main/java/com/softeer5/uniro_backend/common/config/CorsConfig.java (1)
15-19:
⚠️ Potential issueReview CORS security configuration.
Current CORS configuration might be too permissive:
allowedHeaders("*")withallowCredentials(true)is a security risk- Missing
maxAge()setting could lead to excessive preflight requests- No
exposedHeaders()configuration for client accessConsider this more secure configuration:
registry.addMapping("/**") .allowedOrigins(allowedOrigins) .allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS", "PATCH") - .allowedHeaders("*") + .allowedHeaders("Authorization", "Content-Type", "Accept") + .exposedHeaders("X-Total-Count") + .maxAge(3600L) .allowCredentials(true);📝 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.registry.addMapping("/**") .allowedOrigins(allowedOrigins) .allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS", "PATCH") .allowedHeaders("Authorization", "Content-Type", "Accept") .exposedHeaders("X-Total-Count") .maxAge(3600L) .allowCredentials(true);uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/controller/AdminController.java (1)
19-22:
⚠️ Potential issueAdd security, pagination, and error handling.
The current implementation has several concerns:
- Missing authorization checks for admin endpoint
- No pagination for potentially large revision lists
- No error handling for invalid univId
Consider these improvements:
+ @PreAuthorize("hasRole('ADMIN')") @GetMapping("/admin/revision/{univId}") - public ResponseEntity<List<RevInfoDTO>> getAllRev(@PathVariable("univId") Long univId) { - return ResponseEntity.ok().body(adminService.getAllRevInfo(univId)); + public ResponseEntity<Page<RevInfoDTO>> getAllRev( + @PathVariable("univId") Long univId, + @PageableDefault(size = 20) Pageable pageable) { + return ResponseEntity.ok().body(adminService.getAllRevInfo(univId, pageable)); }Add these imports:
import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.web.PageableDefault;uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/controller/AdminApi.java (1)
18-21: 🛠️ Refactor suggestion
Add security-related API responses for admin endpoint.
Consider adding common error responses for admin endpoints:
- 401 (Unauthorized): When authentication is missing/invalid
- 403 (Forbidden): When user lacks admin privileges
@ApiResponses(value = { @ApiResponse(responseCode = "200", description = "모든 버전정보 조회 성공"), @ApiResponse(responseCode = "400", description = "EXCEPTION(임시)", content = @Content), + @ApiResponse(responseCode = "401", description = "인증되지 않은 요청", content = @Content), + @ApiResponse(responseCode = "403", description = "권한 없음", content = @Content) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.@ApiResponses(value = { @ApiResponse(responseCode = "200", description = "모든 버전정보 조회 성공"), @ApiResponse(responseCode = "400", description = "EXCEPTION(임시)", content = @Content), @ApiResponse(responseCode = "401", description = "인증되지 않은 요청", content = @Content), @ApiResponse(responseCode = "403", description = "권한 없음", content = @Content) })uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/service/AdminService.java (2)
22-22: 🛠️ Refactor suggestion
Use explicit timezone instead of system default.
Using system default timezone can lead to inconsistencies across different environments.
- LocalDateTime.ofInstant(Instant.ofEpochMilli(r.getRevTimeStamp()), ZoneId.systemDefault()), + LocalDateTime.ofInstant(Instant.ofEpochMilli(r.getRevTimeStamp()), ZoneId.of("Asia/Seoul")),📝 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.LocalDateTime.ofInstant(Instant.ofEpochMilli(r.getRevTimeStamp()), ZoneId.of("Asia/Seoul")),
20-20: 🛠️ Refactor suggestion
Add validation for univId parameter.
The method should validate the univId parameter to prevent potential issues with null or invalid values.
public List<RevInfoDTO> getAllRevInfo(Long univId){ + if (univId == null) { + throw new IllegalArgumentException("University ID cannot be null"); + } return revInfoRepository.findAllByUnivId(univId).stream()📝 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 List<RevInfoDTO> getAllRevInfo(Long univId){ if (univId == null) { throw new IllegalArgumentException("University ID cannot be null"); } return revInfoRepository.findAllByUnivId(univId).stream() }uniro_backend/src/main/java/com/softeer5/uniro_backend/admin/dto/RevInfoDTO.java (1)
10-10: 🛠️ Refactor suggestion
Update schema name to match class purpose.
The schema name "GetBuildingResDTO" and description "건물 노드 조회 DTO" don't match the actual purpose of this DTO (revision information).
-@Schema(name = "GetBuildingResDTO", description = "건물 노드 조회 DTO") +@Schema(name = "RevInfoDTO", description = "리비전 정보 조회 DTO")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.@Schema(name = "RevInfoDTO", description = "리비전 정보 조회 DTO")
2 participants
📝 PR 타입
🚀 변경 사항
💡 To Reviewer
🧪 테스트 결과
✅ 반영 브랜치
Summary by CodeRabbit