Feat/logging implementation#140
Merged
Merged
Conversation
- Introduced `SLF4J` logging for key actions in `VetController` (e.g., endpoints: `create`, `getAll`, `getById`) and `VetService` (e.g., methods: `createVet`, `getAllVets`, `getVetById`). - Enhanced traceability and debugging capabilities by logging request details and actions.
- Introduced `SLF4J` logging in `PetController` and `PetService` for key actions (create, read, update, delete). - Improved traceability and debugging by logging request details and method activities.
- Introduced `SLF4J` logging in `UserController` and `UserService` for key actions (create, read, update, delete). - Enhanced traceability and debugging by logging request details and method activities.
- Introduced `SLF4J` logging in `ClinicController` and `ClinicService` for key actions (create, read, update, delete). - Enhanced traceability and debugging by logging request details and method activities.
- Introduced `SLF4J` logging in `MedicalRecordController` and `MedicalRecordService` for key actions (create, read, update, delete). - Enhanced traceability and debugging by logging request details and method activities.
- Introduced `SLF4J` logging in `CommentController` and `CommentService` for key actions (create, read, update, delete, count). - Enhanced traceability and debugging by logging request details and method activities.
- Introduced `SLF4J` logging in `ActivityLogController` and `ActivityLogService` for key actions (create, read). - Enhanced traceability and debugging by logging request details and method activities.
- Introduced `SLF4J` logging in `AttachmentController` for key actions (create, read, delete). - Enhanced traceability and debugging by logging request details for API endpoints. Closes #94
Contributor
Author
|
Closes #94 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded SLF4J loggers and log statements (info/debug/warn) across multiple controllers and services; no endpoint signatures, control flow, or return behaviors were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/org/example/vet1177/services/VetService.java (1)
40-46: Consider adding WARN logs before business-rule exceptions.Per the PR's stated logging policy ("WARN precis före business-rule throws"), these
BusinessRuleExceptionthrows should be preceded by warn-level logs for consistency with the rest of the codebase.📝 Suggested addition
if (vetRepository.existsByLicenseId(request.licenseId())) { + log.warn("License ID already exists: {}", request.licenseId()); throw new BusinessRuleException("Licens-ID " + request.licenseId() + " används redan"); } if(vetRepository.existsById(request.userId())){ + log.warn("User already registered as vet userId={}", request.userId()); throw new BusinessRuleException("Användaren är redan registrerad som veterinär"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/VetService.java` around lines 40 - 46, Add WARN-level logging immediately before each BusinessRuleException throw in the VetService registration flow: log a warning using the same logger used by this class (e.g., logger.warn(...)) right before the check that throws for vetRepository.existsByLicenseId(request.licenseId()) and before the check that throws for vetRepository.existsById(request.userId()); include the conflicting identifier (request.licenseId() or request.userId()) and a short message matching the exception text so logs and exceptions correlate.src/main/java/org/example/vet1177/services/ActivityLogService.java (2)
42-42: Variable naming creates potential confusion.The local variable
log(anActivityLogentity) shadows what would be the typical logger variable name. While this works because the class usesLOGinstead oflog, it could cause confusion for future maintainers. Consider renaming the local variable toactivityLogfor clarity.Suggested change
- ActivityLog log = new ActivityLog(action, description, user, record); - repository.save(log); + ActivityLog activityLog = new ActivityLog(action, description, user, record); + repository.save(activityLog);And on line 62:
- .filter(log -> canView(currentUser, log)) + .filter(entry -> canView(currentUser, entry))Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/ActivityLogService.java` at line 42, Rename the local variable named "log" in ActivityLogService to avoid confusion with logger; change occurrences where an ActivityLog is constructed/used (e.g., the constructor call ActivityLog log = new ActivityLog(action, description, user, record) and the other usage at the second occurrence) to a clearer name like "activityLog" and update all subsequent references in the same method (such as any save or return calls) to that new identifier; keep the class-level logger (LOG) unchanged.
21-22: Logger naming inconsistency with other files.This file uses
LOG(uppercase) while all other files in this PR uselog(lowercase). Consider using consistent naming across the codebase.Suggested change for consistency
- private static final Logger LOG = LoggerFactory.getLogger(ActivityLogService.class); + private static final Logger log = LoggerFactory.getLogger(ActivityLogService.class);Note: If you rename to
log, you'll need to also rename theActivityLog logvariable on line 42 to avoid shadowing (e.g.,ActivityLog activityLog).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/ActivityLogService.java` around lines 21 - 22, Rename the logger constant in ActivityLogService from LOG to log to match project convention (change Logger LOG = LoggerFactory.getLogger(ActivityLogService.class) to Logger log = ...), and also rename the method-local or field variable of type ActivityLog currently named log to avoid shadowing (e.g., rename the ActivityLog variable to activityLog) so there is no name conflict between the logger and the ActivityLog instance; update all references to these symbols (ActivityLogService.log and ActivityLog activityLog) accordingly.src/main/java/org/example/vet1177/services/MedicalRecordService.java (1)
143-145: Consider adding WARN logs before throwingBusinessRuleException.Per the PR's stated logging policy ("WARN precis före business-rule throws"), the
BusinessRuleExceptionthrows at lines 144, 169-171, 174-176, 204, and 235 lack preceding WARN logs. This is a minor inconsistency with the stated policy.Example for line 143-145
if (record.getStatus().isFinal()) { + log.warn("Attempt to update closed record id={}", id); throw new BusinessRuleException("Stängda ärenden kan inte uppdateras"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 143 - 145, In MedicalRecordService, add a WARN log immediately before each BusinessRuleException throw (including the record.getStatus().isFinal() check and the other throw sites referenced) so the code follows the "WARN before business-rule throws" policy; use the class logger (e.g. logger.warn or log.warn) to emit a concise contextual message that includes the record identifier and status/action being prevented, then throw the BusinessRuleException as before (ensure the WARN is placed directly above the throw in each throw site).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/services/UserService.java`:
- Line 58: Don't log raw email addresses in UserService; in createUser,
duplicate-email warnings, and getByEmail replace direct logging of
request.getEmail()/email with a privacy-safe value (either a masked form or a
deterministic hash) or, after successful creation, log the created user's
identifier (user.getId()). Introduce/use a small utility (e.g., maskEmail or
hashEmail) and call it where log.info("Creating user..."), the duplicate-email
log (log.warn...), and in getByEmail so logs contain no raw PII.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/services/ActivityLogService.java`:
- Line 42: Rename the local variable named "log" in ActivityLogService to avoid
confusion with logger; change occurrences where an ActivityLog is
constructed/used (e.g., the constructor call ActivityLog log = new
ActivityLog(action, description, user, record) and the other usage at the second
occurrence) to a clearer name like "activityLog" and update all subsequent
references in the same method (such as any save or return calls) to that new
identifier; keep the class-level logger (LOG) unchanged.
- Around line 21-22: Rename the logger constant in ActivityLogService from LOG
to log to match project convention (change Logger LOG =
LoggerFactory.getLogger(ActivityLogService.class) to Logger log = ...), and also
rename the method-local or field variable of type ActivityLog currently named
log to avoid shadowing (e.g., rename the ActivityLog variable to activityLog) so
there is no name conflict between the logger and the ActivityLog instance;
update all references to these symbols (ActivityLogService.log and ActivityLog
activityLog) accordingly.
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java`:
- Around line 143-145: In MedicalRecordService, add a WARN log immediately
before each BusinessRuleException throw (including the
record.getStatus().isFinal() check and the other throw sites referenced) so the
code follows the "WARN before business-rule throws" policy; use the class logger
(e.g. logger.warn or log.warn) to emit a concise contextual message that
includes the record identifier and status/action being prevented, then throw the
BusinessRuleException as before (ensure the WARN is placed directly above the
throw in each throw site).
In `@src/main/java/org/example/vet1177/services/VetService.java`:
- Around line 40-46: Add WARN-level logging immediately before each
BusinessRuleException throw in the VetService registration flow: log a warning
using the same logger used by this class (e.g., logger.warn(...)) right before
the check that throws for vetRepository.existsByLicenseId(request.licenseId())
and before the check that throws for vetRepository.existsById(request.userId());
include the conflicting identifier (request.licenseId() or request.userId()) and
a short message matching the exception text so logs and exceptions correlate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c3fac57-16be-4d60-b348-eb56f45e41f5
📒 Files selected for processing (15)
src/main/java/org/example/vet1177/controller/ActivityLogController.javasrc/main/java/org/example/vet1177/controller/AttachmentController.javasrc/main/java/org/example/vet1177/controller/ClinicController.javasrc/main/java/org/example/vet1177/controller/CommentController.javasrc/main/java/org/example/vet1177/controller/MedicalRecordController.javasrc/main/java/org/example/vet1177/controller/PetController.javasrc/main/java/org/example/vet1177/controller/UserController.javasrc/main/java/org/example/vet1177/controller/VetController.javasrc/main/java/org/example/vet1177/services/ActivityLogService.javasrc/main/java/org/example/vet1177/services/ClinicService.javasrc/main/java/org/example/vet1177/services/CommentService.javasrc/main/java/org/example/vet1177/services/MedicalRecordService.javasrc/main/java/org/example/vet1177/services/PetService.javasrc/main/java/org/example/vet1177/services/UserService.javasrc/main/java/org/example/vet1177/services/VetService.java
| passwordHash, | ||
| request.getRole()); | ||
| applyClinicRules(user, request.getClinicId()); | ||
| log.info("Creating user with email={}", request.getEmail()); |
There was a problem hiding this comment.
Consider not logging email addresses for privacy compliance.
Email addresses are PII (Personally Identifiable Information). Logging them may create GDPR/CCPA compliance concerns, as logs often have different retention policies and access controls than primary data stores.
Consider logging user IDs instead after the user is created, or using a masked/hashed representation.
Suggested changes
For line 58 (createUser):
- log.info("Creating user with email={}", request.getEmail());
+ log.info("Creating user");For line 62 (duplicate email warning):
- log.warn("Duplicate email on create: {}", request.getEmail());
+ log.warn("Duplicate email on create");For line 69 (getByEmail):
- log.debug("Fetching user by email={}", email);
+ log.debug("Fetching user by email");Also applies to: 62-62, 69-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/services/UserService.java` at line 58,
Don't log raw email addresses in UserService; in createUser, duplicate-email
warnings, and getByEmail replace direct logging of request.getEmail()/email with
a privacy-safe value (either a masked form or a deterministic hash) or, after
successful creation, log the created user's identifier (user.getId()).
Introduce/use a small utility (e.g., maskEmail or hashEmail) and call it where
log.info("Creating user..."), the duplicate-email log (log.warn...), and in
getByEmail so logs contain no raw PII.
Contributor
|
Ser bra ut! Tror du vi ska vänta med att logga userId tills security är på plats, så vi slipper röra samma kod två gånger? |
TatjanaTrajkovic
approved these changes
Apr 7, 2026
TatjanaTrajkovic
left a comment
TatjanaTrajkovic
left a comment
Contributor
There was a problem hiding this comment.
Looks good! Main suggestion is to standardize authentication approach and avoid logging sensitive data like emails.
Contributor
Author
|
TÄnker att detta är oberoende från spring security? och att vi senare kan lägga till → WARN — misslyckad inloggning |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/services/PetService.java`:
- Line 46: The info logs ("Creating pet currentUserId={} ownerId={}", and the
similar lines at the other two locations) are emitted before
authorization/validation and DB operations complete; change them to DEBUG-level
"attempt" logs and move the INFO-level success logs to immediately after the
persistence/update/delete call returns successfully. Specifically, in
PetService, replace the early log.info(...) calls (the one logging
currentUserId/ownerId and the two other pre-op info logs) with log.debug(...) to
indicate an attempt, and add new log.info(...) lines after the respective
successful calls in createPet, updatePet, and deletePet (or the methods where
the currentUserId/ownerId logs exist) so success is only logged after the
operation completes without exceptions. Ensure the original log message
parameters (currentUserId, ownerId, etc.) are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c5608ea-821a-4e52-b1a4-684930a40da1
📒 Files selected for processing (2)
src/main/java/org/example/vet1177/controller/PetController.javasrc/main/java/org/example/vet1177/services/PetService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/vet1177/controller/PetController.java
- Updated logging levels to `debug` for non-critical actions. - Added detailed `info` logs for successful create, update, and delete operations to improve traceability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Modifierade filer: 8 controllers + 7 services (UserService, PetService, ClinicService, VetService, MedicalRecordService, CommentService, ActivityLogService).
Lämnade som de var (hade redan SLF4J): AttachmentService, FileStorageService, GlobalExceptionHandler. Skippad per policy: SecurityConfig (bara @Bean-wiring).
Policy som applicerades:
Summary by CodeRabbit
Note: This release contains no user-visible changes.