Redis2.0-User-Cat#49
Conversation
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
# Conflicts: # src/main/java/org/example/springboot25/controller/CatRestController.java # src/main/java/org/example/springboot25/controller/CatViewController.java # src/main/java/org/example/springboot25/dto/CatInputDTO.java # src/main/java/org/example/springboot25/entities/Cat.java # src/main/java/org/example/springboot25/mapper/CatMapper.java # src/main/java/org/example/springboot25/service/CatService.java
|
""" WalkthroughThis update introduces Redis caching to the application by modifying configuration files, adding Redis and cache dependencies, and updating the Docker Compose setup to include a Redis service. Major refactoring was performed on controllers and services to use DTOs instead of entities, centralizing mapping logic and validation. Caching annotations were added to user and cat-related service and controller methods to optimize data retrieval and consistency. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Mapper
participant Repository
participant Redis
Client->>Controller: API Request (e.g., GET /cats)
Controller->>Service: getAllCats()
Service->>Redis: Check cache for cats
alt Cache hit
Redis-->>Service: Return cached cats
else Cache miss
Service->>Repository: Fetch cats from DB
Repository-->>Service: Return cat entities
Service->>Mapper: Map entities to DTOs
Mapper-->>Service: Return DTOs
Service->>Redis: Store DTOs in cache
end
Service-->>Controller: Return DTOs
Controller-->>Client: Respond with DTOs
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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: 12
🔭 Outside diff range comments (4)
src/main/java/org/example/springboot25/service/UserService.java (3)
47-64:⚠️ Potential issueCache key strategy is incomplete – username & email caches may become stale
@Cacheablecorrectly stores look‑ups by ID, username and email in three independent caches.
However, only theuserscache is kept in sync when the record is later mutated (see@CachePut/@CacheEvictfurther below). As a consequence:
- After a user changes her e‑mail or username,
usersByEmail/usersByUsernamewill still return the old data.- Deleting a user will not evict the old entries, because the key passed to
@CacheEvictis the userId while those caches are keyed by the email / username.Unless a cache‑level TTL is relied upon (which is brittle), these entries will live forever and return wrong data.
Two minimal fixes:
-@CachePut(value = "users", key = "#userId") +@Caching(put = { + @CachePut(value = "users", key = "#userId"), + @CachePut(value = "usersByUsername", key = "#result.userName"), + @CachePut(value = "usersByEmail", key = "#result.userEmail") +})and
-@CacheEvict(value = { "users", "usersByUsername", "usersByEmail" }, key = "#userId") +@Caching(evict = { + @CacheEvict(value = "users", key = "#userId"), + @CacheEvict(value = "usersByUsername", key = "#result.userName", beforeInvocation = true), + @CacheEvict(value = "usersByEmail", key = "#result.userEmail", beforeInvocation = true) +})(You may also opt for
allEntries = trueto keep the code shorter.)Failing to address this will surface as hard‑to‑spot inconsistencies in production.
122-136: 🛠️ Refactor suggestion
@CachePutmust update username / email caches as wellSame reasoning as above – when a full update is performed this method can modify both
userNameanduserEmail, but only theuserscache entry is refreshed. Use the multi‑cache@Cachingnotation so that the auxiliary caches stay coherent.
186-193:⚠️ Potential issue
@CacheEvictuses the wrong key for"usersByUsername"/"usersByEmail"Because those caches are keyed by the value (username / email) rather than the ID, the eviction here will never hit. Either:
- compute the relevant keys and evict them explicitly, or
- simply use
allEntries = trueon those caches.Not changing this leads to “ghost” users that appear to exist after deletion.
src/main/java/org/example/springboot25/controller/CatRestController.java (1)
98-109: 🛠️ Refactor suggestionPartial update lacks bean validation
Same as for the PUT endpoint – add
@Validto ensure incoming data respects min/max/blank constraints. Without it,catAge = -5sails through.
🧹 Nitpick comments (10)
src/main/resources/application-dev.properties (2)
1-1: Application name defined in multiple locations.The application name is defined here and also in the main application.properties file. While this works, it's generally better to define such properties in a single location to avoid potential inconsistencies.
Consider removing this duplicate definition since it's already defined in the main properties file.
13-15: Consider using consistent property formats across environments.In the main application.properties, you're using environment variables with fallbacks, but here you're using hardcoded values. While this works for local development, it creates an inconsistency in configuration styles.
Consider using the same environment variable approach with fallbacks for consistency:
-spring.cache.type=redis -spring.data.redis.host=localhost -spring.data.redis.port=6379 +spring.cache.type=redis +spring.data.redis.host=${SPRING_REDIS_HOST:localhost} +spring.data.redis.port=${SPRING_REDIS_PORT:6379}src/main/java/org/example/springboot25/mapper/UserMapper.java (1)
13-13: Consider caching consistency across mapping methodsThe
@CachePutannotation is only applied to thetoUser(UserUpdateDTO)method but not to other mapping methods liketoUserUpdateDTOorupdateUserFromDto. This could potentially lead to cache inconsistencies when these other methods are used.Consider adding appropriate cache annotations to these methods to ensure consistent cache state, particularly for
updateUserFromDtowhich handles partial updates:public UserUpdateDTO toUserUpdateDTO(User user) { if (user == null) throw new IllegalArgumentException("User cannot be null"); UserUpdateDTO dto = new UserUpdateDTO(); dto.setUserId(user.getUserId()); dto.setUserFullName(user.getUserFullName()); dto.setUserName(user.getUserName()); dto.setUserEmail(user.getUserEmail()); dto.setUserLocation(user.getUserLocation()); dto.setUserRole(user.getUserRole()); dto.setUserAuthProvider(user.getUserAuthProvider()); return dto; } + @CachePut(value = "userDtoCache", key = "#dto.userId") public void updateUserFromDto(UserUpdateDTO dto, User user) { if (dto == null || user == null) throw new IllegalArgumentException("DTO or User cannot be null");Also applies to: 69-81, 97-110
compose.yaml (1)
16-22: Good Redis integration, but consider adding authentication.The Redis service is properly configured with ports and persistent storage. However, Redis is running without authentication, which could be a security concern in production environments.
Consider adding Redis authentication for better security:
redis: image: 'redis:latest' container_name: 'redis-cache' + command: redis-server --requirepass ${REDIS_PASSWORD} ports: - '6379:6379' volumes: - redis-data:/dataThen update the app environment variables:
- SPRING_REDIS_HOST=redis - SPRING_REDIS_PORT=6379 + - SPRING_REDIS_PASSWORD=${REDIS_PASSWORD}src/main/java/org/example/springboot25/entities/User.java (1)
23-24: Dual ID annotation approach could use documentation.The approach of using both JPA and Redis ID annotations is correct for dual persistence, but could benefit from an explanatory comment.
@jakarta.persistence.Id - @Id // Redis ID + @Id // Redis ID - dual annotation required for both JPA and Redis persistence @GeneratedValue(strategy = GenerationType.IDENTITY) private Long userId;src/main/java/org/example/springboot25/mapper/CatMapper.java (1)
73-80: Consider refactoring to reduce code duplication.The new updateCatFromInputDTO method contains logic that's very similar to the toCat method, which could lead to maintenance issues if the mapping logic changes.
Consider refactoring to reduce duplication by extracting the common mapping logic:
-public void updateCatFromInputDTO(CatInputDTO dto, Cat cat) { - cat.setCatName(dto.getCatName().trim()); - cat.setCatProfilePicture(dto.getCatProfilePicture() != null ? dto.getCatProfilePicture().trim() : null); - cat.setCatBreed(dto.getCatBreed().trim()); - cat.setCatGender(dto.getCatGender().trim()); - cat.setCatAge(dto.getCatAge()); - cat.setCatPersonality(dto.getCatPersonality() != null ? dto.getCatPersonality().trim() : null); +// Extract common mapping logic to a private method +private void mapCatFieldsFromInputDTO(CatInputDTO dto, Cat cat) { + cat.setCatName(dto.getCatName().trim()); + cat.setCatProfilePicture(dto.getCatProfilePicture() != null ? dto.getCatProfilePicture().trim() : null); + cat.setCatBreed(dto.getCatBreed().trim()); + cat.setCatGender(dto.getCatGender().trim()); + cat.setCatAge(dto.getCatAge()); + cat.setCatPersonality(dto.getCatPersonality() != null ? dto.getCatPersonality().trim() : null); +} + +public Cat toCat(CatInputDTO dto) { + Cat cat = new Cat(); + mapCatFieldsFromInputDTO(dto, cat); + + User user = userRepository.findById(dto.getUserId()) + .orElseThrow(() -> new NotFoundException("User with ID " + dto.getUserId() + " not found")); + + cat.setUserCatOwner(user); + return cat; +} + +public void updateCatFromInputDTO(CatInputDTO dto, Cat cat) { + mapCatFieldsFromInputDTO(dto, cat); }src/main/java/org/example/springboot25/controller/CatGraphQLController.java (1)
37-40: Inconsistent layering – controller performs mapping only in this methodAll other queries/mutations delegate completely to
CatService, which already returnsCatOutputDTO.
Here, however, the controller pulls anOptional<Cat>and performs the mapping itself. This breaks the single‑responsibility rule and surprises future maintainers.Consider adding
Optional<CatOutputDTO> getCatByIdDTO(...)(or changing the existing service signature) so the controller never touches entities.src/main/java/org/example/springboot25/controller/CatRestController.java (1)
3-8: Unused imports & missing validation
HttpStatusandMapare no longer used after the refactor. More importantly, the new DTO parameters are not annotated with@Valid, so the JSR‑303 constraints defined inCatInputDTO/CatUpdateDTOare silently ignored.-import org.springframework.http.HttpStatus; -import java.util.Map; +import jakarta.validation.Valid;and mark the method parameters accordingly (
@Valid @RequestBody CatInputDTO ...).src/main/java/org/example/springboot25/controller/UserViewController.java (1)
17-20: Remove unused caching imports
@CacheEvict,@CachePut, and@Cacheableare no longer referenced in this controller. Keeping unused imports adds noise, triggers IDE warnings, and may mislead future readers into thinking the controller does its own caching.-import org.springframework.cache.annotation.CacheEvict; -import org.springframework.cache.annotation.CachePut; -import org.springframework.cache.annotation.Cacheable;src/main/java/org/example/springboot25/controller/CatViewController.java (1)
62-77: Duplicate manual mapping – delegate toCatMapperManually copying every field from
CatOutputDTOtoCatInputDTOis error‑prone and violates DRY. Use your mapper:-var inputDto = new CatInputDTO(); -// field‑by‑field copying… -catService.createCat(inputDto); +catService.createCat(catMapper.toInputDTO(catDTO, user.getUserId()));Add a mapper helper:
CatInputDTO toInputDTO(CatOutputDTO dto, Long userId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
compose.yaml(1 hunks)pom.xml(1 hunks)src/main/java/org/example/springboot25/Springboot25Application.java(1 hunks)src/main/java/org/example/springboot25/controller/CatGraphQLController.java(1 hunks)src/main/java/org/example/springboot25/controller/CatRestController.java(5 hunks)src/main/java/org/example/springboot25/controller/CatViewController.java(3 hunks)src/main/java/org/example/springboot25/controller/ReminderViewController.java(4 hunks)src/main/java/org/example/springboot25/controller/UserGraphQLController.java(3 hunks)src/main/java/org/example/springboot25/controller/UserRestController.java(6 hunks)src/main/java/org/example/springboot25/controller/UserViewController.java(4 hunks)src/main/java/org/example/springboot25/dto/CatInputDTO.java(2 hunks)src/main/java/org/example/springboot25/dto/UserInputDTO.java(1 hunks)src/main/java/org/example/springboot25/dto/UserOutputDTO.java(1 hunks)src/main/java/org/example/springboot25/dto/UserUpdateDTO.java(1 hunks)src/main/java/org/example/springboot25/entities/Cat.java(4 hunks)src/main/java/org/example/springboot25/entities/User.java(3 hunks)src/main/java/org/example/springboot25/mapper/CatMapper.java(1 hunks)src/main/java/org/example/springboot25/mapper/UserMapper.java(2 hunks)src/main/java/org/example/springboot25/security/CustomOAuth2UserService.java(1 hunks)src/main/java/org/example/springboot25/service/CatService.java(1 hunks)src/main/java/org/example/springboot25/service/UserService.java(6 hunks)src/main/resources/application-dev.properties(2 hunks)src/main/resources/application.properties(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/main/java/org/example/springboot25/mapper/UserMapper.java (1)
Learnt from: ehottinen
PR: storkforge/catinder#32
File: src/main/java/org/example/springboot25/dto/UserUpdateDTO.java:7-12
Timestamp: 2025-04-14T15:50:31.116Z
Learning: In the DTO-based architecture, validation for UserUpdateDTO fields (e.g., email format) is handled inside UserMapper.updateUserFromDto() method rather than using validation annotations directly on the DTO. The service layer only checks for user existence and delegates field validation to the mapper.
🧬 Code Graph Analysis (4)
src/main/java/org/example/springboot25/controller/UserRestController.java (1)
src/main/java/org/example/springboot25/controller/AdminController.java (1)
PreAuthorize(19-158)
src/main/java/org/example/springboot25/security/CustomOAuth2UserService.java (1)
src/main/java/org/example/springboot25/utility/RandomUsernameGenerator.java (1)
RandomUsernameGenerator(5-9)
src/main/java/org/example/springboot25/controller/CatRestController.java (4)
src/main/java/org/example/springboot25/dto/CatInputDTO.java (1)
CatInputDTO(7-84)src/main/java/org/example/springboot25/dto/CatOutputDTO.java (1)
CatOutputDTO(3-76)src/main/java/org/example/springboot25/dto/CatUpdateDTO.java (1)
CatUpdateDTO(3-59)src/main/java/org/example/springboot25/exceptions/NotFoundException.java (1)
NotFoundException(3-7)
src/main/java/org/example/springboot25/controller/CatViewController.java (2)
src/main/java/org/example/springboot25/dto/CatOutputDTO.java (1)
CatOutputDTO(3-76)src/main/java/org/example/springboot25/exceptions/NotFoundException.java (1)
NotFoundException(3-7)
🔇 Additional comments (40)
src/main/java/org/example/springboot25/dto/UserOutputDTO.java (1)
5-7: Good documentation addition!The added Javadoc comment clearly describes the purpose of this DTO - for exposing user details without sensitive information. This improves code clarity and follows best practices for API documentation.
pom.xml (1)
71-78: Redis and cache dependencies correctly added.The addition of Spring Boot Redis and Cache starter dependencies is appropriate for implementing Redis-based caching. These dependencies will provide the necessary infrastructure for the Redis caching configuration referenced in the application properties.
src/main/resources/application.properties (1)
13-18: Well-configured Redis settings with environment variables.The Redis configuration is properly set up with:
- Explicit cache type setting
- Environment variables with sensible fallbacks
- Clear comments explaining the purpose
This approach enables flexible deployment across different environments while maintaining local development simplicity.
src/main/java/org/example/springboot25/controller/ReminderViewController.java (1)
77-77:Details
✅ Verification successful
Method updates aligned with DTO refactoring.
The changes from
getAllCatsByUsertogetAllCatsByUserAsEntityreflect a broader refactoring to separate DTO and entity returns in the service layer. This controller needs entity objects, so the change is appropriate.Also applies to: 97-97, 115-115, 128-128
🏁 Script executed:
#!/bin/bash # Verify the implementation of the new method in CatService rg -A 5 "getAllCatsByUserAsEntity" --type javaLength of output: 3469
Approved: Controller correctly uses the new entity-returning service method
Verified that
getAllCatsByUserAsEntity(User)is implemented inCatService.java(returnscatRepository.findAllByUserCatOwner(user)) and returns the requiredCatentities. The controller’s calls to this method are appropriate.Locations updated in
ReminderViewController.java:
- Line 77
- Line 97
- Line 115
- Line 128
No further action required.
src/main/java/org/example/springboot25/dto/UserUpdateDTO.java (2)
5-7: Great addition of class-level JavadocThe added documentation clearly describes the purpose of this DTO, which improves code maintainability and helps other developers understand the intended use case.
10-10: LGTM: Improved field organizationMoving the userId field and its accessors to the top of the class makes sense since this is the primary key field used for cache operations. This reorganization aligns well with the caching strategy implemented throughout the application.
Also applies to: 20-26
src/main/java/org/example/springboot25/Springboot25Application.java (1)
5-6: Well-implemented caching and repository configurationThe addition of
@EnableCachingproperly activates Spring's caching infrastructure, which is essential for the Redis caching implementation. The@EnableJpaRepositoriesannotation with explicit package configuration is a good practice that improves startup performance by limiting the scanning scope.These annotations ensure that:
- Caching annotations like
@Cacheable,@CachePut, and@CacheEvictwill work properly throughout the application- JPA repositories are explicitly configured with a targeted package scan
Also applies to: 11-12
src/main/java/org/example/springboot25/mapper/UserMapper.java (3)
7-7: Appropriate cache update implementationAdding the
@CachePutannotation ensures that when a user is updated via this mapper, the cache is also updated with the latest data. This is a key component of maintaining cache consistency.Also applies to: 13-13
15-15: Error message improvementThe updated error message explicitly mentions
UserUpdateDTO, which improves clarity for debugging.
18-18: Consistent ID handling for cachingExplicitly setting the
userIdin both entity and DTO mapping methods ensures proper identity preservation, which is crucial for caching operations that rely on the userId as a key.Also applies to: 73-73
src/main/java/org/example/springboot25/dto/CatInputDTO.java (2)
3-3: Appropriate import for validationThe added import for
jakarta.validation.constraints.Minis correctly included to support the new minimum value validation on thecatAgefield.
14-14: Good validation constraints for cat dataThe added validation constraints improve data integrity:
@NotBlankoncatBreedandcatGenderensures these required string fields aren't empty@NotNulland@Min(0)oncatAgeprevent null values and negative agesThese constraints align well with the broader DTO refactoring for cat-related operations and the change from enum to string for
catGender.Also applies to: 17-17, 20-21
src/main/java/org/example/springboot25/dto/UserInputDTO.java (2)
7-9: Good addition of class-level documentation.The Javadoc clearly describes the purpose of this DTO class, which improves code readability and helps developers understand its intended use.
12-29: Well-implemented custom validation messages.Adding descriptive validation messages to all constraints is a great improvement. These custom messages will provide clearer feedback to clients when validation fails, making the API more user-friendly.
src/main/java/org/example/springboot25/security/CustomOAuth2UserService.java (2)
47-54: Good improvement to conditional user update logic.The selective update approach that only modifies the user when the name has changed is an optimization that will reduce unnecessary database operations and cache updates.
73-74: Good practice: Re-fetching user after changes.Re-fetching the user after update/creation ensures the OAuth2User is created with the most current data, which is especially important with caching in place.
compose.yaml (2)
26-27: Proper service dependencies and environment configuration.The app service correctly depends on Redis and has the necessary environment variables to connect to it. This ensures services start in the right order and the app can locate the Redis service.
Also applies to: 34-35
39-41: Good implementation of persistent volumes.Defining named volumes for both PostgreSQL and Redis ensures data persistence across container restarts and updates, which is essential for production environments.
src/main/java/org/example/springboot25/entities/User.java (1)
68-70: Nice code organization improvement.Adding section dividers improves code readability, especially in large entity classes.
src/main/java/org/example/springboot25/controller/UserGraphQLController.java (5)
8-10: Adding caching dependencies to support Redis caching strategy.The addition of these Spring caching annotations imports is appropriate for implementing the Redis caching strategy in the GraphQL controller.
28-28: Properly caching the list of all users.The
@Cacheable(value = "users")annotation will improve performance by caching the results ofgetAllUsers()in Redis.
34-34: Implementing user-specific caching with appropriate key.The
@Cacheable(value = "users", key = "#userId")annotation correctly uses the user ID as the cache key.
40-40: Implementing username-based caching with appropriate key.The
@Cacheable(value = "usersByUsername", key = "#userName")annotation appropriately caches user data by username.
51-51: Ensuring cache consistency on user updates.The
@CachePutannotation ensures the cache is updated when a user is modified through the API.src/main/java/org/example/springboot25/entities/Cat.java (6)
8-8: Adding Serializable support for Redis caching.Implementing Serializable is necessary for Redis caching to properly serialize and deserialize entity objects.
13-13: Implementing Serializable for Redis cache compatibility.The Cat entity now implements Serializable, which is required for objects stored in Redis cache.
24-24: Adding validation and simplifying the gender field type.The @notblank annotations add proper validation for required fields, and changing the gender field from an enum to String simplifies serialization for caching purposes.
Also applies to: 27-28
36-36: Using lazy loading for improved performance.Changing to FetchType.LAZY for the owner relationship improves performance by avoiding unnecessary loading of related entities when the Cat entity is fetched from the database.
53-55: Adding setter for the catId field.Adding a setter for the primary key field helps with entity mapping operations, particularly during cache operations.
81-83: Updated getter/setter to use String instead of enum type.The method signatures for getCatGender() and setCatGender() now correctly use String instead of the previous enum type, consistent with the field type change.
Also applies to: 85-87
src/main/java/org/example/springboot25/mapper/CatMapper.java (3)
21-35: Improved toCat mapping with consistent string handling.The updated mapper method properly trims string values and handles null values appropriately. The handling of user lookup is also improved.
37-56: Enhanced update method with proper null checks and trimming.The updated method properly checks for null values before updating each field and consistently trims string values.
58-71: Simplified DTO mapping with direct assignments.The toDTO method is now cleaner with direct field assignments and proper null handling for the user relationship.
src/main/java/org/example/springboot25/controller/UserRestController.java (2)
10-12: Adding caching dependencies to support Redis caching strategy.The addition of Spring caching annotations imports supports the Redis caching implementation in the REST controller.
48-48: Caching all users list to improve API performance.The @Cacheable annotation on getAllUsers() will significantly improve performance for this potentially expensive operation.
src/main/java/org/example/springboot25/service/UserService.java (1)
138-184: Partial update can silently leave stale cache entriesThe map‑based
updateUservariant also writes only to the"users"cache, even though the supplied map may contain"userName"or"userEmail". Please mirror the fix suggested for the DTO‑based update.src/main/java/org/example/springboot25/controller/CatGraphQLController.java (1)
48-55: Method naming mismatch between GraphQL update & service
updateCat(full) receives aCatInputDTO, whilepartialUpdateCatusesCatUpdateDTO. That is good.
But in REST (seeCatRestController) the full update also forwards an Input DTO, after overriding theuserId. Make sure the service layer validates ownership for GraphQL as well, otherwise a malicious caller could change the owner field inside the input.src/main/java/org/example/springboot25/controller/CatRestController.java (2)
38-45:isNotOwnerOrAdminreturnstruewhenuserCatOwnerisnull– is this intended?A
nullowner currently blocks access for everyone (including ADMIN), because the helper instantly returnstrue. If unowned cats are a valid use‑case, consider:if (cat.getUserCatOwner() == null) { return !isAdmin; // allow admins, block others }Clarify the rule or document the current behaviour.
82-93:⚠️ Potential issueUpdating a cat unintentionally transfers ownership
catInputDTO.setUserId(currentUser.getUserId());assigns the authenticated user as the new owner on every full update. If an ADMIN edits somebody else’s cat, the animal will suddenly belong to the admin account.Move this line to the creation endpoint only, or augment the service to ignore the field on updates unless explicitly allowed.
- catInputDTO.setUserId(currentUser.getUserId()); + // Keep the original owner; do not overwrite automatically⛔ Skipped due to learnings
Learnt from: ehottinen PR: storkforge/catinder#16 File: src/main/java/org/example/springboot25/service/CatService.java:45-56 Timestamp: 2025-04-03T07:22:36.756Z Learning: In the CatService class, the userCatOwner field is intentionally not updated in the updateCat method to keep it immutable after creation and prevent accidental ownership switches.src/main/java/org/example/springboot25/service/CatService.java (1)
85-92:@CachePuton partial update can cache stale data
partialUpdateCatupdates the cache before the transaction commits.
If the transaction rolls back, Redis will hold a value that was never persisted.Add
@TransactionalEventListener–based cache population (after commit) or move the@CachePutcall to a separate,@Transactional(propagation = REQUIRES_NEW, afterCommit)method.
| User user = new User(); | ||
| user.setUserEmail(email); | ||
|
|
||
| String fullName = oAuth2User.getAttribute("name"); | ||
| user.setUserFullName(fullName != null ? fullName : ""); | ||
| user.setUserName(RandomUsernameGenerator.getRandomUsername()); | ||
|
|
||
| String providerName = userRequest.getClientRegistration().getRegistrationId(); | ||
| user.setUserAuthProvider(providerName); | ||
| user.setUserRole(UserRole.BASIC); | ||
| userService.addUser(user); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider setting userLocation for new users.
When creating new users, all fields with @notblank constraints should be initialized to avoid validation issues. While most required fields are set, userLocation is missing an initial value.
user.setUserEmail(email);
String fullName = oAuth2User.getAttribute("name");
user.setUserFullName(fullName != null ? fullName : "");
user.setUserName(RandomUsernameGenerator.getRandomUsername());
+user.setUserLocation("Unknown"); // Set default location to satisfy validation
String providerName = userRequest.getClientRegistration().getRegistrationId();
user.setUserAuthProvider(providerName);
user.setUserRole(UserRole.BASIC);📝 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.
| User user = new User(); | |
| user.setUserEmail(email); | |
| String fullName = oAuth2User.getAttribute("name"); | |
| user.setUserFullName(fullName != null ? fullName : ""); | |
| user.setUserName(RandomUsernameGenerator.getRandomUsername()); | |
| String providerName = userRequest.getClientRegistration().getRegistrationId(); | |
| user.setUserAuthProvider(providerName); | |
| user.setUserRole(UserRole.BASIC); | |
| userService.addUser(user); | |
| User user = new User(); | |
| user.setUserEmail(email); | |
| String fullName = oAuth2User.getAttribute("name"); | |
| user.setUserFullName(fullName != null ? fullName : ""); | |
| user.setUserName(RandomUsernameGenerator.getRandomUsername()); | |
| user.setUserLocation("Unknown"); // Set default location to satisfy validation | |
| String providerName = userRequest.getClientRegistration().getRegistrationId(); | |
| user.setUserAuthProvider(providerName); | |
| user.setUserRole(UserRole.BASIC); |
|
|
||
| public Cat toCat(CatInputDTO catInputDTO) { | ||
| public Cat toCat(CatInputDTO dto) { | ||
| Cat cat = new Cat(); | ||
| cat.setCatName(catInputDTO.getCatName().trim()); | ||
| cat.setCatProfilePicture(catInputDTO.getCatProfilePicture() != null ? | ||
| catInputDTO.getCatProfilePicture().trim() : null); | ||
| cat.setCatBreed(catInputDTO.getCatBreed().trim()); | ||
| if (catInputDTO.getCatGender() != null) { | ||
| cat.setCatGender(CatGender.valueOf(catInputDTO.getCatGender().trim().toUpperCase())); | ||
| } else { | ||
| cat.setCatGender(null); // or handle as needed | ||
| } | ||
| cat.setCatAge(catInputDTO.getCatAge()); | ||
| cat.setCatPersonality(catInputDTO.getCatPersonality() != null ? | ||
| catInputDTO.getCatPersonality().trim() : null); | ||
| cat.setCatName(dto.getCatName().trim()); | ||
| cat.setCatProfilePicture(dto.getCatProfilePicture() != null ? dto.getCatProfilePicture().trim() : null); | ||
| cat.setCatBreed(dto.getCatBreed().trim()); | ||
| cat.setCatGender(dto.getCatGender().trim()); | ||
| cat.setCatAge(dto.getCatAge()); | ||
| cat.setCatPersonality(dto.getCatPersonality() != null ? dto.getCatPersonality().trim() : null); | ||
|
|
||
| User user = userRepository.findById(catInputDTO.getUserId()) | ||
| .orElseThrow(() -> new NotFoundException("User with ID " + catInputDTO.getUserId() + " not found")); | ||
| User user = userRepository.findById(dto.getUserId()) | ||
| .orElseThrow(() -> new NotFoundException("User with ID " + dto.getUserId() + " not found")); | ||
|
|
||
| cat.setUserCatOwner(user); | ||
| return cat; | ||
| } |
There was a problem hiding this comment.
Du har två versioner av samma metod ihopklistrade:
cat.setCatGender(CatGender.valueOf(catInputDTO.getCatGender().trim().toUpperCase())); och
cat.setCatGender(dto.getCatGender().trim());
Att använda både catInputDTO och dto i samma metod gör det rörigt.
Du försöker dessutom sätta en String till ett fält som förväntar sig en CatGender enum – vilket leder till ett kompileringsfel.
Det ser ut som att du råkade merga metoden utan att rensa dubbletter.
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
# Conflicts: # src/main/java/org/example/springboot25/controller/CatGraphQLController.java # src/main/java/org/example/springboot25/controller/CatRestController.java # src/main/java/org/example/springboot25/controller/CatViewController.java # src/main/java/org/example/springboot25/controller/ReminderViewController.java # src/main/java/org/example/springboot25/controller/UserViewController.java # src/main/java/org/example/springboot25/dto/UserInputDTO.java # src/main/java/org/example/springboot25/dto/UserUpdateDTO.java # src/main/java/org/example/springboot25/entities/User.java # src/main/java/org/example/springboot25/mapper/CatMapper.java # src/main/java/org/example/springboot25/service/CatService.java
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/main/java/org/example/springboot25/service/CatService.java (2)
42-48: Consider marking this read‑only query as@Transactional(readOnly = true)
getAllCats()performs a pure read yet participates in the default read‑write transaction inherited from the class‑level@Transactional.
Declaring the method (or annotating the class with@Transactional(readOnly = true)and overriding only mutators) avoids accidental flushes and gives the persistence provider a hint for optimisations.
69-73: Minor: exposeisAdminlogic through Spring Security’shasRoleexpressionInstead of manual authority scanning, consider a method‑level security annotation:
@PreAuthorize("hasRole('ADMIN') or #currentUser.userId == principal.userId") public List<Cat> getCatsVisibleTo(Authentication auth, User currentUser) { … }It removes boiler‑plate and pushes the concern to Spring Security.
src/main/java/org/example/springboot25/controller/CatViewController.java (3)
38-46: Remove the unusedAuthenticationparameter
authenticationis injected but never used.
Cleaning it up simplifies the method signature:-public String listCats(Model model, - @AuthenticationPrincipal OAuth2User principal, - Authentication authentication) { +public String listCats(Model model, + @AuthenticationPrincipal OAuth2User principal) {
46-48: Handle the “user record not found” edge case
userService.findUserByEmail(...)may throwNotFoundException, which will bubble up as a 500 error.
Return an error view or redirect the user gracefully.try { User me = userService.findUserByEmail(email); } catch (NotFoundException ex) { redirectAttributes.addFlashAttribute("error", ex.getMessage()); return "redirect:/"; }
52-57: Service should return a DTO, not the controller doing the mapping
CatService#getCatByIdreturns an entity which the controller immediately converts.
Move the conversion into the service to keep presentation logic out of controllers and to ensure the cached value is already a DTO.-Optional<Cat> getCatById(Long id) +Optional<CatOutputDTO> getCatById(Long id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/org/example/springboot25/controller/CatRestController.java(1 hunks)src/main/java/org/example/springboot25/controller/CatViewController.java(2 hunks)src/main/java/org/example/springboot25/controller/UserGraphQLController.java(3 hunks)src/main/java/org/example/springboot25/controller/UserRestController.java(6 hunks)src/main/java/org/example/springboot25/controller/UserViewController.java(9 hunks)src/main/java/org/example/springboot25/dto/UserOutputDTO.java(1 hunks)src/main/java/org/example/springboot25/service/CatService.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/example/springboot25/dto/UserOutputDTO.java
- src/main/java/org/example/springboot25/controller/UserGraphQLController.java
- src/main/java/org/example/springboot25/controller/UserRestController.java
- src/main/java/org/example/springboot25/controller/CatRestController.java
🔇 Additional comments (2)
src/main/java/org/example/springboot25/controller/UserViewController.java (1)
56-60: Good move switching to the primitive cache keyUsing
catService.getAllCatsByUserId(userId)avoids the serialisation pitfalls of passing the fullUserentity.
No further remarks here.src/main/java/org/example/springboot25/controller/CatViewController.java (1)
122-130: HTML forms cannot issueDELETE– confirm front‑end supportBrowsers only support
GETandPOST.
Unless you rely on JavaScript or Spring’s hidden_methodfield, this endpoint will never be hit.
If you intend a plain<form>post, keep it as@PostMapping("/delete").
| @Caching(evict = { | ||
| @CacheEvict(value = "cats", allEntries = true), | ||
| @CacheEvict(value = "cat", allEntries = true) | ||
| }) | ||
| public CatOutputDTO createCat(CatInputDTO dto) { | ||
| if (!userRepository.existsById(dto.getUserId())) { | ||
| throw new NotFoundException("User not found with id " + dto.getUserId()); | ||
| } | ||
| log.info("Deleting cat with id: {}", catId); | ||
| catRepository.deleteById(catId); | ||
| } | ||
|
|
||
| public void deleteCat(Long catId) { | ||
| deleteCatById(catId); | ||
| } | ||
|
|
||
| // ======================== | ||
| // Externa metoder (DTO) | ||
| // ======================== | ||
|
|
||
| public List<CatOutputDTO> getAllCats() { | ||
| return catRepository.findAll().stream().map(catMapper::toDto).toList(); | ||
| } | ||
|
|
||
| public List<CatOutputDTO> getAllCatsByUser(User user) { | ||
| return catRepository.findAllByUserCatOwner(user).stream().map(catMapper::toDto).toList(); | ||
| } | ||
|
|
||
| public List<CatOutputDTO> getCatsByName(String name) { | ||
| return catRepository.findByCatNameContainingIgnoreCase(name).stream().map(catMapper::toDto).toList(); | ||
| } | ||
|
|
||
| public CatOutputDTO getCatDtoById(Long id) { | ||
| return catMapper.toDto(findCatById(id)); | ||
| } | ||
|
|
||
| public CatOutputDTO addCat(CatInputDTO dto) { | ||
| log.info("Creating new cat: {}", dto.getCatName()); | ||
| Cat cat = catMapper.toCat(dto); | ||
| return catMapper.toDto(saveCat(cat)); | ||
| return catMapper.toDTO(catRepository.save(cat)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
allEntries = true nukes the entire cache – evict only what you modified
Every new cat invalidates every cached list and individual cat.
You can keep unrelated cache lines warm and reduce Redis traffic by targeting specific keys:
-@Caching(evict = {
- @CacheEvict(value = "cats", allEntries = true),
- @CacheEvict(value = "cat", allEntries = true)
-})
+@Caching(evict = {
+ @CacheEvict(value = "cats", key = "#dto.userId"), // user‑specific list
+ @CacheEvict(value = "cat", key = "#result.catId") // just the created cat
+})(The same applies to updateCat, partialUpdateCat, and deleteCat.)
📝 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.
| @Caching(evict = { | |
| @CacheEvict(value = "cats", allEntries = true), | |
| @CacheEvict(value = "cat", allEntries = true) | |
| }) | |
| public CatOutputDTO createCat(CatInputDTO dto) { | |
| if (!userRepository.existsById(dto.getUserId())) { | |
| throw new NotFoundException("User not found with id " + dto.getUserId()); | |
| } | |
| log.info("Deleting cat with id: {}", catId); | |
| catRepository.deleteById(catId); | |
| } | |
| public void deleteCat(Long catId) { | |
| deleteCatById(catId); | |
| } | |
| // ======================== | |
| // Externa metoder (DTO) | |
| // ======================== | |
| public List<CatOutputDTO> getAllCats() { | |
| return catRepository.findAll().stream().map(catMapper::toDto).toList(); | |
| } | |
| public List<CatOutputDTO> getAllCatsByUser(User user) { | |
| return catRepository.findAllByUserCatOwner(user).stream().map(catMapper::toDto).toList(); | |
| } | |
| public List<CatOutputDTO> getCatsByName(String name) { | |
| return catRepository.findByCatNameContainingIgnoreCase(name).stream().map(catMapper::toDto).toList(); | |
| } | |
| public CatOutputDTO getCatDtoById(Long id) { | |
| return catMapper.toDto(findCatById(id)); | |
| } | |
| public CatOutputDTO addCat(CatInputDTO dto) { | |
| log.info("Creating new cat: {}", dto.getCatName()); | |
| Cat cat = catMapper.toCat(dto); | |
| return catMapper.toDto(saveCat(cat)); | |
| return catMapper.toDTO(catRepository.save(cat)); | |
| @Caching(evict = { | |
| @CacheEvict(value = "cats", key = "#dto.userId"), // user‑specific list | |
| @CacheEvict(value = "cat", key = "#result.catId") // just the created cat | |
| }) | |
| public CatOutputDTO createCat(CatInputDTO dto) { | |
| if (!userRepository.existsById(dto.getUserId())) { | |
| throw new NotFoundException("User not found with id " + dto.getUserId()); | |
| } | |
| Cat cat = catMapper.toCat(dto); | |
| return catMapper.toDTO(catRepository.save(cat)); | |
| } |
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
# Conflicts: # pom.xml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/main/java/org/example/springboot25/service/CatService.java (3)
75-78:⚠️ Potential issueAvoid caching
Optionalwrappers – cache the entity itself
@Cacheablewill store the wrapper (Optional<Cat>) in Redis. The default Jackson‑based serializer tends to mis-handleOptional, leading to deserialisation errors or silent cache misses.-@Cacheable(value = "cat", key = "#catId") -public Optional<Cat> getCatById(Long catId) { - return catRepository.findById(catId); -} +@Cacheable(value = "cat", key = "#catId") +public Cat getCatById(Long catId) { + return catRepository.findById(catId) + .orElseThrow(() -> new NotFoundException("Cat not found with id " + catId)); +}Callers that need an
Optionalcan wrap the result on their side.
50-58: 🛠️ Refactor suggestionStop constructing a dummy
Userjust to query by FKCreating a detached
Userinstance only to hit the repository is brittle and can trigger extra selects when the proxy is initialised.
Prefer a repository method that filters directly on the foreign‑key column:-User user = new User(); -user.setUserId(userId); -return catRepository.findAllByUserCatOwner(user) +return catRepository.findAllByUserCatOwnerUserId(userId) .stream() .map(catMapper::toDTO) .collect(Collectors.toList());Add
List<Cat> findAllByUserCatOwnerUserId(Long userId);toCatRepository.
97-105: 🛠️ Refactor suggestion
allEntries = truenukes the entire cacheEvery new cat invalidates every cached list, even from other users, defeating the purpose of caching.
Evict only the affected user’s list and consider putting the freshly‑created cat into its individual cache entry:-@CacheEvict(value = "cats", allEntries = true) +@Caching(evict = { + @CacheEvict(value = "cats", key = "#dto.userId") +}, put = { + @CachePut(value = "cat", key = "#result.catId") +})(The same adjustment applies to
updateCat,partialUpdateCat, anddeleteCat.)src/main/java/org/example/springboot25/controller/CatViewController.java (1)
103-116: 🛠️ Refactor suggestionStill binding the form to an output DTO – switch to
CatInputDTOUsing
CatOutputDTOas the form‑backing object forces this manual, error‑prone copy block and skips Bean‑Validation rules declared onCatInputDTO.-@PostMapping("/{catId}") -public String updateCat(@PathVariable Long catId, @ModelAttribute("cat") CatOutputDTO catDTO) { +@PostMapping("/{catId}") +public String updateCat(@PathVariable Long catId, + @Valid @ModelAttribute("cat") CatInputDTO catDTO, + BindingResult errors) { + if (errors.hasErrors()) { + return "cat/existing-edit-cat-form"; + } try { - Long currentOwnerId = catService.getOwnerIdOfCat(catId); - CatInputDTO inputDto = new CatInputDTO(); - inputDto.setCatName(catDTO.getCatName()); - … - inputDto.setUserId(currentOwnerId); // protect ownership - - catService.updateCat(catId, inputDto); + Long currentOwnerId = catService.getOwnerIdOfCat(catId); + catDTO.setUserId(currentOwnerId); // protect ownership + catService.updateCat(catId, catDTO); } catch (Exception e) { throw new RuntimeException("Update failed for cat with id " + catId, e); } return "redirect:/cats"; }This removes the redundant object copy and restores validation.
🧹 Nitpick comments (1)
src/main/java/org/example/springboot25/controller/CatViewController.java (1)
70-88: HandleuserService.findUserByEmailfailures gracefullyIf the OAuth2 user exists in the Identity Provider but not yet in your local DB,
findUserByEmailwill throwNotFoundException, causing a 500 error.
Consider on‑the‑fly user provisioning or redirecting the user to a signup page instead of blowing up:try { User user = userService.findUserByEmail(email); catDTO.setUserId(user.getUserId()); catService.createCat(catDTO); } catch (NotFoundException e) { return "redirect:/signup"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xml(2 hunks)src/main/java/org/example/springboot25/controller/CatViewController.java(1 hunks)src/main/java/org/example/springboot25/dto/CachedUserDTO.java(1 hunks)src/main/java/org/example/springboot25/service/CatService.java(1 hunks)src/main/java/org/example/springboot25/service/UserService.java(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/springboot25/dto/CachedUserDTO.java
🚧 Files skipped from review as they are similar to previous changes (2)
- pom.xml
- src/main/java/org/example/springboot25/service/UserService.java
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/main/java/org/example/springboot25/controller/CatGraphQLController.java (1)
55-63:⚠️ Potential issueSwallowing
NotFoundExceptionhides useful error information from GraphQL clientsReturning
falseon anyNotFoundExceptionmeans the client receives no error details and cannot distinguish “not found” from other failures (e.g., database outage).
GraphQL already conveys exceptions nicely; letting the exception propagate results in a propererrorsarray in the response.Suggested fix:
- @MutationMapping - public boolean deleteCat(@Argument Long catId) { - try { - catService.deleteCat(catId); - return true; - } catch (NotFoundException e) { - return false; - } - } + @MutationMapping + public Boolean deleteCat(@Argument Long catId) { + catService.deleteCat(catId); // will throw NotFoundException automatically + return true; + }If you still prefer a boolean indicator, consider returning a custom payload containing an
okfield and optionalerrordetails, or log the exception so it’s not silently discarded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/org/example/springboot25/controller/CatGraphQLController.java(2 hunks)src/main/java/org/example/springboot25/controller/CatRestController.java(1 hunks)src/main/java/org/example/springboot25/controller/CatViewController.java(1 hunks)src/main/java/org/example/springboot25/controller/ReminderViewController.java(3 hunks)src/main/java/org/example/springboot25/entities/User.java(3 hunks)src/main/java/org/example/springboot25/repository/CatRepository.java(1 hunks)src/main/java/org/example/springboot25/service/CatService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/org/example/springboot25/controller/ReminderViewController.java
- src/main/java/org/example/springboot25/entities/User.java
- src/main/java/org/example/springboot25/controller/CatRestController.java
- src/main/java/org/example/springboot25/service/CatService.java
- src/main/java/org/example/springboot25/controller/CatViewController.java
🔇 Additional comments (3)
src/main/java/org/example/springboot25/repository/CatRepository.java (1)
10-10: Good addition that aligns with architectural changes.The new method follows Spring Data JPA naming conventions correctly and enables direct filtering of cats by user ID. This change supports the broader refactoring effort to use DTOs instead of entities and optimizes data retrieval by allowing cats to be queried by ID rather than requiring the complete User entity. This is more efficient when only the ID is needed for lookups.
src/main/java/org/example/springboot25/controller/CatGraphQLController.java (2)
45-48: Check DTO choice for full updates
updateCatnow expectsCatInputDTO, notCatUpdateDTO.
•CatInputDTOusually represents object creation and may require all fields.
• For a PUT‑style update you normally need an “all fields required” DTO, so this change might be intentional – just ensure:
CatInputDTOvalidation rules match update semantics (non‑null for mandatory fields).CatService#updateCatsignature was updated accordingly; otherwise compilation will fail.Please confirm the service layer has been adapted.
50-53: Good addition ofpartialUpdateCatmutationIntroducing a distinct mutation for PATCH semantics avoids over‑loading the full update endpoint and keeps validation rules clear.
| private final CatService catService; | ||
| private final UserService userService; | ||
| private final CatMapper catMapper; | ||
|
|
||
| public CatGraphQLController(CatService catService, UserService userService) { | ||
| public CatGraphQLController(CatService catService, CatMapper catMapper) { | ||
| this.catService = catService; | ||
| this.userService = userService; | ||
| this.catMapper = catMapper; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constructor injects CatMapper but its use is limited to a single call – consider pushing mapping to the service layer
CatMapper is injected and only referenced once in getCatById(). In contrast, getAllCats() relies on the service to return CatOutputDTO objects directly.
This split responsibility makes it unclear which layer owns the entity‑to‑DTO conversion and can easily lead to inconsistencies or duplicated work.
Recommendation
- Move the
catMapper.toDTO(...)call intoCatService#getCatByIdso all controller methods consume the same DTO contract. - Remove the
CatMapperdependency from this controller if no other mapping remains.
This keeps the controller thin, centralises conversion logic, and avoids unnecessary constructor parameters.
| @QueryMapping | ||
| @PreAuthorize("isAuthenticated()") | ||
| public List<CatOutputDTO> getAllCats(Authentication auth) { | ||
| User currentUser = userService.findUserByUserName(auth.getName()); | ||
| return catService.getAllCatsByUser(currentUser); | ||
| public List<CatOutputDTO> getAllCats() { | ||
| return catService.getAllCats(); | ||
| } | ||
|
|
||
| @QueryMapping | ||
| @PreAuthorize("isAuthenticated()") | ||
| public CatOutputDTO getCatById(@Argument Long catId, Authentication auth) { | ||
| CatOutputDTO cat = catService.getCatDtoById(catId); | ||
| User currentUser = userService.findUserByUserName(auth.getName()); | ||
|
|
||
| if (!cat.getUserId().equals(currentUser.getUserId()) && | ||
| !currentUser.getUserRole().name().equals("ADMIN")) { | ||
| throw new AccessDeniedException("You can only view your own cats."); | ||
| } | ||
|
|
||
| return cat; | ||
| public CatOutputDTO getCatById(@Argument Long catId) { | ||
| return catMapper.toDTO(catService.getCatById(catId)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistent DTO handling between getAllCats and getCatById
getAllCats() trusts the service to return List<CatOutputDTO> while getCatById() manually maps the service result.
If CatService#getCatById ever changes its return type (e.g., to CatOutputDTO) the controller mapping will silently double‑map or fail.
- public CatOutputDTO getCatById(@Argument Long catId) {
- return catMapper.toDTO(catService.getCatById(catId));
+ public CatOutputDTO getCatById(@Argument Long catId) {
+ return catService.getCatById(catId); // let service return DTO
}Ensure CatService consistently exposes DTOs (or entities) for all queries and update tests accordingly.
🏁 Script executed:
#!/bin/bash
# Locate CatService definition and method signatures
rg -n "interface CatService"
rg -n "class CatService"
# Find service method implementations or declarations
rg -n "getAllCats"
rg -n "getCatById"Length of output: 4379
🏁 Script executed:
#!/bin/bash
file=src/main/java/org/example/springboot25/service/CatService.java
echo "=== getAllCats implementation ==="
rg -n -C5 'public List<CatOutputDTO> getAllCats' "$file"
echo ""
echo "=== getCatById implementation ==="
rg -n -C5 'public Cat getCatById' "$file"Length of output: 1903
Standardize CatService return types for consistency
The service currently returns DTOs for getAllCats() but returns an entity for getCatById(), forcing the GraphQL controller to map manually. This inconsistency can lead to double mapping or silent failures if signatures change.
Please update the service and controller as follows:
• In src/main/java/org/example/springboot25/service/CatService.java
– Change the signature of getCatById to return CatOutputDTO
– Map the entity inside the service using catMapper.toDTO(...)
• Example diff:
diff - @Cacheable(value = "cat", key = "#catId") - public Cat getCatById(Long catId) { - Cat cat = catRepository.findById(catId) - .orElseThrow(() -> new NotFoundException("Cat not found with id " + catId)); - return cat; - } + @Cacheable(value = "cat", key = "#catId") + public CatOutputDTO getCatById(Long catId) { + Cat cat = catRepository.findById(catId) + .orElseThrow(() -> new NotFoundException("Cat not found with id " + catId)); + return catMapper.toDTO(cat); + }
• In src/main/java/org/example/springboot25/controller/CatGraphQLController.java
– Remove the manual mapping and simply return the DTO from the service
• Example diff:
diff - public CatOutputDTO getCatById(@Argument Long catId) { - return catMapper.toDTO(catService.getCatById(catId)); - } + public CatOutputDTO getCatById(@Argument Long catId) { + return catService.getCatById(catId); + }
• Update any affected tests (e.g., controller and service tests) and other callers of getCatById to expect CatOutputDTO.
• Verify that all controllers and GraphQL schema definitions remain aligned with the new DTO return type.
There was a problem hiding this comment.
Since we removed lombok, we need to make getters and setters manually
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
|
I think the build is failing because of a lack of constructors for cached user dto and possibly user? I'm looking into it |
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
| import java.util.List; | ||
|
|
||
| @Entity | ||
| @Table(name = "app_user") |
There was a problem hiding this comment.
Why the renaming, have you fixed this elsewhere?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/org/example/springboot25/dto/CachedUserDTO.java (1)
1-16: Add validation annotations for field constraintsConsider adding validation annotations (such as from Jakarta Validation) to ensure data integrity, especially for fields like email.
Example implementation:
import jakarta.validation.constraints.Email; import jakarta.validation.constraints.NotBlank; import jakarta.validation.constraints.NotNull; // Then on your fields: @NotNull private Long userId; @NotBlank private String userName; @NotBlank @Email private String userEmail; // Add appropriate constraints for other fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/springboot25/dto/CachedUserDTO.java(1 hunks)src/main/java/org/example/springboot25/entities/User.java(3 hunks)src/main/resources/application-dev.properties(2 hunks)src/main/resources/application.properties(0 hunks)
💤 Files with no reviewable changes (1)
- src/main/resources/application.properties
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/application-dev.properties
- src/main/java/org/example/springboot25/entities/User.java
| public class CachedUserDTO implements Serializable { | ||
|
|
||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| private Long userId; | ||
| private String userName; | ||
| private String userEmail; | ||
| private String userLocation; | ||
| private String userRole; | ||
| } |
There was a problem hiding this comment.
Missing constructors and accessor methods
The class lacks both a default constructor and a parameterized constructor, which are needed for proper serialization/deserialization with Redis. Additionally, since Lombok has been removed, you need to manually implement getters and setters for all fields.
Add the following constructors and methods:
// Default constructor
public CachedUserDTO() {
}
// Parameterized constructor
public CachedUserDTO(Long userId, String userName, String userEmail, String userLocation, String userRole) {
this.userId = userId;
this.userName = userName;
this.userEmail = userEmail;
this.userLocation = userLocation;
this.userRole = userRole;
}
// Getters and Setters
public Long getUserId() {
return userId;
}
public void setUserId(Long userId) {
this.userId = userId;
}
public String getUserName() {
return userName;
}
public void setUserName(String userName) {
this.userName = userName;
}
public String getUserEmail() {
return userEmail;
}
public void setUserEmail(String userEmail) {
this.userEmail = userEmail;
}
public String getUserLocation() {
return userLocation;
}
public void setUserLocation(String userLocation) {
this.userLocation = userLocation;
}
public String getUserRole() {
return userRole;
}
public void setUserRole(String userRole) {
this.userRole = userRole;
}
// equals, hashCode and toString methods would also be needed| public class CachedUserDTO implements Serializable { | ||
|
|
||
| private static final long serialVersionUID = 1L; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding equals, hashCode, and toString methods
For proper object comparison and debugging, implement these methods manually since Lombok is no longer available.
Here's an implementation example for equals and hashCode:
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
CachedUserDTO that = (CachedUserDTO) o;
return Objects.equals(userId, that.userId) &&
Objects.equals(userName, that.userName) &&
Objects.equals(userEmail, that.userEmail) &&
Objects.equals(userLocation, that.userLocation) &&
Objects.equals(userRole, that.userRole);
}
@Override
public int hashCode() {
return Objects.hash(userId, userName, userEmail, userLocation, userRole);
}
@Override
public String toString() {
return "CachedUserDTO{" +
"userId=" + userId +
", userName='" + userName + '\'' +
", userEmail='" + userEmail + '\'' +
", userLocation='" + userLocation + '\'' +
", userRole='" + userRole + '\'' +
'}';
}Don't forget to add the import: import java.util.Objects;
| import lombok.Data; | ||
| import java.io.Serializable; | ||
|
|
||
| @Data |
There was a problem hiding this comment.
Lombok usage conflicts with project configuration
Based on the previous review comment, Lombok has been removed from the project, but this class still uses the @Data annotation from Lombok. This is likely contributing to the build failure mentioned in the PR comments.
Remove the Lombok dependency and implement the required methods manually:
-import lombok.Data;
import java.io.Serializable;
-@Data
public class CachedUserDTO implements Serializable {📝 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.Data; | |
| import java.io.Serializable; | |
| @Data | |
| // src/main/java/org/example/springboot25/dto/CachedUserDTO.java | |
| import java.io.Serializable; | |
| public class CachedUserDTO implements Serializable { | |
| // TODO: implement getters, setters, equals, hashCode, toString | |
| // … | |
| } |
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/org/example/springboot25/dto/CachedUserDTO.java (2)
6-54: Missing equals, hashCode, and toString methodsFor proper object comparison and debugging, implement equals, hashCode, and toString methods. This is especially important for cached objects where identity and equality checks are common.
@Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; CachedUserDTO that = (CachedUserDTO) o; return Objects.equals(userId, that.userId) && Objects.equals(userName, that.userName) && Objects.equals(userEmail, that.userEmail) && Objects.equals(userLocation, that.userLocation) && Objects.equals(userRole, that.userRole); } @Override public int hashCode() { return Objects.hash(userId, userName, userEmail, userLocation, userRole); } @Override public String toString() { return "CachedUserDTO{" + "userId=" + userId + ", userName='" + userName + '\'' + ", userEmail='" + userEmail + '\'' + ", userLocation='" + userLocation + '\'' + ", userRole='" + userRole + '\'' + '}'; }Don't forget to add the import:
import java.util.Objects;
7-14:⚠️ Potential issueMissing constructors needed for proper Redis serialization
The class lacks both a default constructor and a parameterized constructor, which are needed for proper serialization/deserialization with Redis. This is likely contributing to the build failure mentioned in the PR comments.
Add the following constructors:
// Default constructor public CachedUserDTO() { } // Parameterized constructor public CachedUserDTO(Long userId, String userName, String userEmail, String userLocation, String userRole) { this.userId = userId; this.userName = userName; this.userEmail = userEmail; this.userLocation = userLocation; this.userRole = userRole; }
🧹 Nitpick comments (1)
src/main/java/org/example/springboot25/dto/CachedUserDTO.java (1)
7-7: Consider adding serialVersionUID for versioningWhen implementing Serializable, it's recommended to declare a serialVersionUID field for better version control of serialized objects. This helps maintain compatibility between different versions of the class.
public class CachedUserDTO implements Serializable { private static final long serialVersionUID = 1L; // rest of the class }
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
Signed-off-by: LinusAltemark <Linalt0905@gmail.com>
3 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores