Cacheable 주석 처리 #33
Cacheable 주석 처리 #33
Conversation
dependency submission code removed
merging main retry
WalkthroughThe changes introduce dependency injection of Jackson's Changes
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 4
🧹 Nitpick comments (3)
src/main/java/com/example/Jinus/controller/v2/NoticeControllerV2.java (1)
38-38: Remove unnecessary blank lineThis blank line appears to be an unintentional formatting change.
public String responseDepartmentNotice(@RequestBody RequestDto requestDto) { String userId = requestDto.getUserRequest().getUser().getId(); - int departmentId = userServiceV2.getUserDepartmentId(userId);src/main/java/com/example/Jinus/service/v2/cafeteria/CacheServiceV2.java (2)
21-26: Add context for disabled caching
The@Cacheableannotation block is commented out to run performance comparisons without caching. To avoid confusion later, please add aTODO(or similar) explaining the purpose of this change, linking to the performance test plan and indicating when caching should be re-enabled.
37-37: Add context for disabled caching
The single-line@CacheableongetCafeteriaListis also commented out. Likewise, annotate this with a brief note (e.g., aTODO) so maintainers know it’s intentionally disabled for benchmarking and when to revert.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/example/Jinus/controller/v2/CafeteriaControllerV2.java(2 hunks)src/main/java/com/example/Jinus/controller/v2/DietControllerV2.java(2 hunks)src/main/java/com/example/Jinus/controller/v2/NoticeControllerV2.java(3 hunks)src/main/java/com/example/Jinus/service/v2/cafeteria/CacheServiceV2.java(2 hunks)src/main/java/com/example/Jinus/service/v2/cafeteria/DietServiceV2.java(3 hunks)src/main/java/com/example/Jinus/utility/JsonUtils.java(1 hunks)
🔇 Additional comments (4)
src/main/java/com/example/Jinus/service/v2/cafeteria/DietServiceV2.java (4)
23-24: Field renaming improves clarity
RenamingqueryServiceV2→cafeteriaQueryServiceV2andparameterServiceV2→dietParameterServiceV2better reflects each service’s responsibility, enhancing readability and consistency.
34-34: Updated service reference
The call todietParameterServiceV2.setParameters(...)correctly uses the renamed field. No further changes needed here.
42-42: Updated service reference
The invocation ofcafeteriaQueryServiceV2.getCafeteriaId(...)aligns with the renamed field. This update is consistent and looks good.
57-57: Updated service reference
Updating tocafeteriaQueryServiceV2.getImgUrl(cafeteriaId)matches the field rename and preserves existing logic. Approved.
|
|
||
| public class JsonUtils { | ||
| private static final ObjectMapper objectMapper = new ObjectMapper(); | ||
| public static final ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the implications of exposing ObjectMapper as public
Changing the visibility of the ObjectMapper from private to public exposes it for external modification and direct access. While this enables reuse across controllers, it introduces risks:
- External code could modify its configuration, leading to inconsistent behavior
- It creates two ways to handle JSON: via the utility method and direct access
- The static instance may have thread-safety concerns in certain scenarios
Consider whether the direct access is necessary, or if all JSON processing could be encapsulated through methods in JsonUtils.
If direct access is required, consider:
- public static final ObjectMapper objectMapper = new ObjectMapper();
+ // Create an immutable public instance with defense against modification
+ public static final ObjectMapper OBJECT_MAPPER = createObjectMapper();
+
+ private static ObjectMapper createObjectMapper() {
+ ObjectMapper mapper = new ObjectMapper();
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
+ return mapper;
+ }And remove the static block since initialization would happen in the createObjectMapper method.
| import com.fasterxml.jackson.core.JsonProcessingException; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.beans.factory.annotation.Autowired; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports
The imports for JsonProcessingException, ObjectMapper, and Autowired are added but not used in this controller. Unlike other controllers, no ObjectMapper field is injected here.
This creates inconsistency with other controllers and clutters the code.
- import com.fasterxml.jackson.core.JsonProcessingException;
- import com.fasterxml.jackson.databind.ObjectMapper;
import lombok.RequiredArgsConstructor;
- import org.springframework.beans.factory.annotation.Autowired;📝 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 com.fasterxml.jackson.core.JsonProcessingException; | |
| import com.fasterxml.jackson.databind.ObjectMapper; | |
| import lombok.RequiredArgsConstructor; | |
| import org.springframework.beans.factory.annotation.Autowired; | |
| import lombok.RequiredArgsConstructor; |
| @Autowired | ||
| private ObjectMapper objectMapper; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused ObjectMapper and consider consistent DI style
The injected ObjectMapper is never used in this class, making it unnecessary. Additionally, the class mixes constructor injection (via @requiredargsconstructor) with field injection (@Autowired), which is inconsistent.
Spring best practices generally recommend constructor injection for required dependencies.
If the ObjectMapper is intended for future use, consider:
- @Autowired
- private ObjectMapper objectMapper;If it will be needed later, add it through constructor injection instead:
private final ObjectMapper objectMapper;
// Update the class to use @AllArgsConstructor instead of @RequiredArgsConstructor
// or create an explicit constructor| @Autowired | ||
| private ObjectMapper objectMapper; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused ObjectMapper and consider consistent DI style
The injected ObjectMapper is never used in this class, making it unnecessary. Additionally, the class mixes constructor injection (via @requiredargsconstructor) with field injection (@Autowired), which is inconsistent.
Spring best practices generally recommend constructor injection for required dependencies.
If the ObjectMapper is intended for future use, consider:
- @Autowired
- private ObjectMapper objectMapper;If it will be needed later, add it through constructor injection instead:
private final ObjectMapper objectMapper;
// Update the class to use @AllArgsConstructor instead of @RequiredArgsConstructor
// or create an explicit constructor
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
1 participant
캐싱 전 후 성능 테스트 위해서 잠깐 보류합니다.
Summary by CodeRabbit
Chores
Refactor