feature : Architecture testing - #43
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMoves direct repository access out of controllers into services, adds service methods for attachment/audit retrieval, updates controller constructors and usages accordingly, adds ArchUnit test suite and dependency, and documents the architecture in README. No production-facing API signatures were removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller
participant Service
participant Repository
participant Minio as MinioStorageService
Client->>Controller: request /tickets/{id} or /token/{token}
Controller->>Service: getAttachmentsByTicketId(ticketId)
Service->>Repository: findByTicketId(ticketId)
Repository-->>Service: List<Attachment>
Service-->>Controller: List<Attachment>
Controller->>Service: getAuditLogsForTicket(ticketId)
Service->>Repository: findByTicketIdOrderByCreatedAtDesc(ticketId)
Repository-->>Service: List<AuditLog>
Service-->>Controller: List<AuditLog>
alt Download attachment endpoint
Client->>Controller: GET /attachments/{id}/download
Controller->>Service: getAttachmentById(id)
Service->>Repository: findById(id)
Repository-->>Service: Attachment
Service-->>Controller: Attachment
Controller->>Minio: downloadObject(attachment.path)
Minio-->>Controller: InputStream / error
Controller-->>Client: streaming response / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (1)
src/test/java/org/example/alfs/ArchitectureTest.java (1)
84-89: Narrow the documentedJwtServiceexception.The comment says only
JwtServiceis exempt, but Line 88 allows any*Serviceclass under..security... Split this into the default*Service → ..services..rule plus a targetedJwtService → ..security..exception so future services cannot bypass the package rule accidentally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/ArchitectureTest.java` around lines 84 - 89, Update the test so the broad „*Service -> ..security..“ allowance is removed and replaced by two targeted rules: keep the default rule in services_should_be_in_services_package that asserts classes().that().haveSimpleNameEndingWith("Service").should().resideInAnyPackage("..services..") and add a second, separate rule that explicitly asserts classes().that().haveSimpleName("JwtService").should().resideInAnyPackage("..security.."); call check(classes) on both rules so JwtService is the only service allowed in the security package and other *Service classes cannot bypass the packages rule.
🤖 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/test/java/org/example/alfs/ArchitectureTest.java`:
- Around line 31-62: Remove the unused repository imports from the three
controller classes (TicketController, AttachmentController,
AttachmentDownloadController) so ArchUnit tests
controllers_should_only_access_services_not_repositories and
controllers_dto_and_mapper_should_not_access_repositories no longer fail due to
mere imports; locate any import statements referencing repository types in those
controllers and either (a) move repository usage behind the appropriate service
and inject the service instead, or (b) delete the unused import and any dead
code that referenced it, and then re-run the tests to ensure controllers only
depend on services (methods:
controllers_should_only_access_services_not_repositories,
controllers_dto_and_mapper_should_not_access_repositories).
---
Nitpick comments:
In `@src/test/java/org/example/alfs/ArchitectureTest.java`:
- Around line 84-89: Update the test so the broad „*Service -> ..security..“
allowance is removed and replaced by two targeted rules: keep the default rule
in services_should_be_in_services_package that asserts
classes().that().haveSimpleNameEndingWith("Service").should().resideInAnyPackage("..services..")
and add a second, separate rule that explicitly asserts
classes().that().haveSimpleName("JwtService").should().resideInAnyPackage("..security..");
call check(classes) on both rules so JwtService is the only service allowed in
the security package and other *Service classes cannot bypass the packages rule.
🪄 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: 231a4256-74f7-4c22-81f7-797cd83d8288
📒 Files selected for processing (3)
README.mdpom.xmlsrc/test/java/org/example/alfs/ArchitectureTest.java
| @Test | ||
| void controllers_should_only_access_services_not_repositories() { | ||
| noClasses() | ||
| .that().resideInAPackage("..controllers..") | ||
| .should().dependOnClassesThat() | ||
| .resideInAPackage("..repositories..") | ||
| .check(classes); | ||
| } | ||
|
|
||
| @Test | ||
| void services_should_not_depend_on_controllers() { | ||
| noClasses() | ||
| .that().resideInAPackage("..services..") | ||
| .should().dependOnClassesThat() | ||
| .resideInAPackage("..controllers..") | ||
| .check(classes); | ||
| } | ||
|
|
||
| /** | ||
| * Repository Access Rule | ||
| * | ||
| * Controllers, DTOs and mappers must not access repositories directly. | ||
| * Repository access should go through the service layer. | ||
| */ | ||
| @Test | ||
| void controllers_dto_and_mapper_should_not_access_repositories() { | ||
| noClasses() | ||
| .that().resideInAnyPackage("..controllers..", "..dto..", "..mapper..") | ||
| .should().dependOnClassesThat() | ||
| .resideInAPackage("..repositories..") | ||
| .check(classes); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify controller classes that directly import repositories and will fail the new architecture rules.
fd -e java 'Controller\.java$' \
| xargs rg -nP -C2 '^\s*import\s+org\.example\.alfs\.repositories\.'Repository: ithsjava25/project-backend-alfs
Length of output: 1911
🏁 Script executed:
cat -n src/test/java/org/example/alfs/ArchitectureTest.javaRepository: ithsjava25/project-backend-alfs
Length of output: 4390
🏁 Script executed:
# Check if repositories are actually called/used in each controller, not just imported
for controller in TicketController AttachmentController AttachmentDownloadController; do
echo "=== $controller ==="
rg -n "new\s+\w*Repository|repository\." "src/main/java/org/example/alfs/controllers/${controller}.java" || echo "No repository usage found"
doneRepository: ithsjava25/project-backend-alfs
Length of output: 243
🏁 Script executed:
# Check the exact lines mentioned in the review for the supposed duplicate rule
sed -n '31,62p' src/test/java/org/example/alfs/ArchitectureTest.java | cat -nRepository: ithsjava25/project-backend-alfs
Length of output: 1403
Remove unused repository imports before enabling these architecture rules.
The three controllers (TicketController, AttachmentController, AttachmentDownloadController) import repository classes that are not actually used in the code. ArchUnit will fail the architecture tests on these imports alone. Either move the logic behind services or remove the unused imports before merging.
The controllers_dto_and_mapper_should_not_access_repositories() rule (lines 56–61) is not a duplicate of the earlier controller rule—it additionally covers DTOs and mappers, so both tests serve distinct purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/example/alfs/ArchitectureTest.java` around lines 31 - 62,
Remove the unused repository imports from the three controller classes
(TicketController, AttachmentController, AttachmentDownloadController) so
ArchUnit tests controllers_should_only_access_services_not_repositories and
controllers_dto_and_mapper_should_not_access_repositories no longer fail due to
mere imports; locate any import statements referencing repository types in those
controllers and either (a) move repository usage behind the appropriate service
and inject the service instead, or (b) delete the unused import and any dead
code that referenced it, and then re-run the tests to ensure controllers only
depend on services (methods:
controllers_should_only_access_services_not_repositories,
controllers_dto_and_mapper_should_not_access_repositories).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/alfs/controllers/AttachmentController.java (1)
7-7:⚠️ Potential issue | 🟡 MinorRemove the stale
AttachmentRepositoryimport.
AttachmentRepositoryis no longer referenced in this controller after the refactor, and leaving the import contradicts the architectural intent of this PR (controllers must not depend on repository packages — the exact rule the new ArchUnit suite enforces). Whether ArchUnit catches it depends on whether its rule runs onimportedClassesor only resolved references, so drop it explicitly.🔧 Proposed fix
import org.example.alfs.services.AttachmentService; -import org.example.alfs.repositories.AttachmentRepository; import org.springframework.http.HttpStatus;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/AttachmentController.java` at line 7, The AttachmentController currently contains a stale import of org.example.alfs.repositories.AttachmentRepository which is no longer referenced and violates the new ArchUnit rule (controllers must not depend on repository packages); remove the unused import statement for AttachmentRepository from AttachmentController (class name: AttachmentController) so the controller only imports needed service/DTO classes and no repository package references, then run/build to ensure no remaining compilation errors or unused-import warnings.
🧹 Nitpick comments (1)
src/main/java/org/example/alfs/controllers/TicketController.java (1)
31-38: LGTM.Constructor now correctly injects
AttachmentService/AuditServicein place of the prior repositories, and the two view endpoints (/token/{token}and/{id}) consistently delegate attachment and audit reads through the service layer. Aligns with the ArchUnit rules added in this PR.Minor nit (optional): the constructor signature has a couple of inconsistent spaces (missing space after
,beforeAttachmentService, double space beforeSecurityUtils). Purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/TicketController.java` around lines 31 - 38, The constructor signature in TicketController has inconsistent spacing around commas (e.g., no space before AttachmentService and double space before SecurityUtils); update the TicketController(TicketService ticketService, TicketCommentService ticketCommentService, AttachmentService attachmentService, AuditService auditService, SecurityUtils securityUtils, UserService userService) constructor declaration to use a single space after each comma so the parameter list is consistently formatted.
🤖 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/alfs/controllers/AttachmentDownloadController.java`:
- Around line 5-6: Remove the unused import of AttachmentRepository from
AttachmentDownloadController to avoid a direct controller→repository dependency;
locate the import statement referencing
org.example.alfs.repositories.AttachmentRepository in
AttachmentDownloadController and delete it, ensuring the controller only uses
AttachmentService and calls attachmentService.getAttachmentById(id) (and that
AttachmentService is imported and used) so the ArchitectureTest no longer
detects a repositories dependency.
---
Outside diff comments:
In `@src/main/java/org/example/alfs/controllers/AttachmentController.java`:
- Line 7: The AttachmentController currently contains a stale import of
org.example.alfs.repositories.AttachmentRepository which is no longer referenced
and violates the new ArchUnit rule (controllers must not depend on repository
packages); remove the unused import statement for AttachmentRepository from
AttachmentController (class name: AttachmentController) so the controller only
imports needed service/DTO classes and no repository package references, then
run/build to ensure no remaining compilation errors or unused-import warnings.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/controllers/TicketController.java`:
- Around line 31-38: The constructor signature in TicketController has
inconsistent spacing around commas (e.g., no space before AttachmentService and
double space before SecurityUtils); update the TicketController(TicketService
ticketService, TicketCommentService ticketCommentService, AttachmentService
attachmentService, AuditService auditService, SecurityUtils securityUtils,
UserService userService) constructor declaration to use a single space after
each comma so the parameter list is consistently formatted.
🪄 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: 1b986720-760b-45fc-ad1a-e17961e432d8
📒 Files selected for processing (5)
src/main/java/org/example/alfs/controllers/AttachmentController.javasrc/main/java/org/example/alfs/controllers/AttachmentDownloadController.javasrc/main/java/org/example/alfs/controllers/TicketController.javasrc/main/java/org/example/alfs/services/AttachmentService.javasrc/main/java/org/example/alfs/services/AuditService.java
|
@FionaSprinkles @simonforsberg All tests green! Nitpicks fixed. Should be ready to merge if @FionaSprinkles was 100% done with her tests :) |
2 participants
Introduces ArchUnit tests to enforce a clean layered architecture in the project.
Summary by CodeRabbit
Documentation
Tests
Refactor