Development: Add check for public methods in controllers that are not endpoints
#9729
Conversation
WalkthroughThe changes in this pull request enhance the testing capabilities for REST controller conventions within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleResourceArchitectureTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/ModuleArchitectureTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/ModuleArchitectureTest.java (2)
55-58: LGTM! Consider adding Javadoc.The implementation correctly identifies endpoints and aligns with the PR objective. Consider adding Javadoc to document:
- The purpose of this method
- What constitutes an endpoint
- The return value and its usage
Example Javadoc:
/** * Returns a rule conjunction that matches all public endpoint methods in REST controllers within this module. * An endpoint is a public method in a class annotated with @RestController. * * @return ArchUnit rule conjunction for matching endpoint methods */
55-58: Consider a more flexible implementation for future extensibility.The current implementation only checks for
@RestController. Consider making it more flexible to handle different types of controllers or endpoint annotations.Example enhancement:
- default GivenMethodsConjunction endpointsOfThisModule() { + default GivenMethodsConjunction endpointsOfThisModule(Class<?>... controllerAnnotations) { + var conjunction = methodsOfThisModuleThat(); + for (Class<?> annotation : controllerAnnotations.length > 0 ? controllerAnnotations + : new Class<?>[]{ RestController.class }) { + conjunction = conjunction.areDeclaredInClassesThat().areAnnotatedWith(annotation).or(); + } + return conjunction.and().arePublic(); }src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleResourceArchitectureTest.java (1)
90-103: Enhance error message for better debuggingThe implementation correctly checks for request mapping annotations, but the error message could be more descriptive to help developers quickly identify and fix issues.
Consider enhancing the error message:
- events.add(violated(item, createMessage(item, "Method should be annotated with @RequestMapping"))); + events.add(violated(item, createMessage(item, + String.format("Public method '%s' in REST controller should be annotated with one of: %s", + item.getName(), + annotationClasses.stream() + .map(Class::getSimpleName) + .collect(joining(", "))))));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleResourceArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/ModuleArchitectureTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleResourceArchitectureTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/ModuleArchitectureTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🪛 GitHub Check: Java Architecture Tests
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleResourceArchitectureTest.java
[failure] 87-87: de.tum.cit.aet.artemis.buildagent.architecture.BuildAgentResourceArchitectureTest ► shouldHaveNoPublicMethodsExceptForEndpoints()
Failed test found in:
build/test-results/test/TEST-de.tum.cit.aet.artemis.buildagent.architecture.BuildAgentResourceArchitectureTest.xml
Error:
java.lang.AssertionError: Rule 'methods that are declared in classes that reside in a package 'de.tum.cit.aet.artemis.buildagent..' and are declared in classes that are annotated with @RestController and are public should be annotated with request mapping annotation, because all public methods should be endpoints' failed to check any classes. This means either that no classes have been passed to the rule at all, or that no classes passed to the rule matched the that() clause. To allow rules being evaluated without checking any classes you can either use ArchRule.allowEmptyShould(true) on a single rule or set the configuration property archRule.failOnEmptyShould = false to change the behavior globally.
[failure] 87-87: de.tum.cit.aet.artemis.core.architecture.CoreResourceArchitectureTest ► shouldHaveNoPublicMethodsExceptForEndpoints()
Failed test found in:
build/test-results/test/TEST-de.tum.cit.aet.artemis.core.architecture.CoreResourceArchitectureTest.xml
Error:
java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'methods that are declared in classes that reside in a package 'de.tum.cit.aet.artemis.core..' and are declared in classes that are annotated with @RestController and are public should be annotated with request mapping annotation, because all public methods should be endpoints' was violated (2 times):
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/ModuleArchitectureTest.java (1)
8-8: LGTM!
The import is correctly placed and necessary for the new functionality.
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleResourceArchitectureTest.java (2)
112-112: LGTM: Improved encapsulation
The visibility change from protected to private is appropriate as the method is only used within this class.
79-79: LGTM: Improved test scoping
The change from methods() to methodsOfThisModuleThat() correctly scopes the test to the current module, improving test isolation.
Let's verify that this change doesn't miss any important checks:
| @Test | ||
| void shouldHaveNoPublicMethodsExceptForEndpoints() { | ||
| ArchRule rule = methodsOfThisModuleThat().areDeclaredInClassesThat().areAnnotatedWith(RestController.class).and().arePublic().should(beAnnotatedWithRequestMapping()) | ||
| .because("all public methods should be endpoints"); | ||
| rule.check(productionClasses); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add allowEmptyShould for modules without REST controllers
The test is correctly enforcing that all public methods in REST controllers must be endpoints. However, the static analysis shows failures in BuildAgent and Core modules, likely because they don't have REST controllers or have public methods that aren't endpoints.
Apply this change to handle modules without REST controllers:
@Test
void shouldHaveNoPublicMethodsExceptForEndpoints() {
ArchRule rule = methodsOfThisModuleThat().areDeclaredInClassesThat().areAnnotatedWith(RestController.class).and().arePublic().should(beAnnotatedWithRequestMapping())
.because("all public methods should be endpoints");
- rule.check(productionClasses);
+ rule.allowEmptyShould(true).check(productionClasses);
}📝 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.
| @Test | |
| void shouldHaveNoPublicMethodsExceptForEndpoints() { | |
| ArchRule rule = methodsOfThisModuleThat().areDeclaredInClassesThat().areAnnotatedWith(RestController.class).and().arePublic().should(beAnnotatedWithRequestMapping()) | |
| .because("all public methods should be endpoints"); | |
| rule.check(productionClasses); | |
| } | |
| @Test | |
| void shouldHaveNoPublicMethodsExceptForEndpoints() { | |
| ArchRule rule = methodsOfThisModuleThat().areDeclaredInClassesThat().areAnnotatedWith(RestController.class).and().arePublic().should(beAnnotatedWithRequestMapping()) | |
| .because("all public methods should be endpoints"); | |
| rule.allowEmptyShould(true).check(productionClasses); | |
| } |
🧰 Tools
🪛 GitHub Check: Java Architecture Tests
[failure] 87-87: de.tum.cit.aet.artemis.buildagent.architecture.BuildAgentResourceArchitectureTest ► shouldHaveNoPublicMethodsExceptForEndpoints()
Failed test found in:
build/test-results/test/TEST-de.tum.cit.aet.artemis.buildagent.architecture.BuildAgentResourceArchitectureTest.xml
Error:
java.lang.AssertionError: Rule 'methods that are declared in classes that reside in a package 'de.tum.cit.aet.artemis.buildagent..' and are declared in classes that are annotated with @RestController and are public should be annotated with request mapping annotation, because all public methods should be endpoints' failed to check any classes. This means either that no classes have been passed to the rule at all, or that no classes passed to the rule matched the that() clause. To allow rules being evaluated without checking any classes you can either use ArchRule.allowEmptyShould(true) on a single rule or set the configuration property archRule.failOnEmptyShould = false to change the behavior globally.
[failure] 87-87: de.tum.cit.aet.artemis.core.architecture.CoreResourceArchitectureTest ► shouldHaveNoPublicMethodsExceptForEndpoints()
Failed test found in:
build/test-results/test/TEST-de.tum.cit.aet.artemis.core.architecture.CoreResourceArchitectureTest.xml
Error:
java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'methods that are declared in classes that reside in a package 'de.tum.cit.aet.artemis.core..' and are declared in classes that are annotated with @RestController and are public should be annotated with request mapping annotation, because all public methods should be endpoints' was violated (2 times):
Checklist
General
Motivation and Context
To maintain good code quality REST controllers should not provide any functionality for the service layer or other controllers. Thus all public methods should be endpoints. Thus developers are not tempted to call methods of the web layer from any other layer.
Description
Adds architecture test to ensure REST controllers only contain public methods that are endpoints
Review Progress
Performance Review
Code Review
Summary by CodeRabbit
New Features
Bug Fixes
Refactor