✨ Add comprehensive integration test suite with CI/CD pipeline #168
Conversation
* Phase 1 Adds unit tests, for parsing, validation of the inpput to the handler. * Phase 2 adds a golang jsonrpc client to run a set of integration tests against test projects to verify the accuracy of results. * The testing infrastructure was also added to use the phases to validate any changes on PR or merge. * README.md in java-analyzer-bundle.test/integration/ to document the testing. Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
WalkthroughThis PR introduces Phase 2 integration testing infrastructure with Go-based LSP client tests, updates Docker configuration for Java 21 support, refines symbol resolution with stricter pattern matching and erasure handling, and adds two comprehensive test projects with extensive documentation for the analyzer bundle. Changes
Sequence Diagram(s)sequenceDiagram
actor Runner
participant Go_Client as Go Test Client
participant JDT.LS as JDT.LS Server
participant Analyzer as Analyzer Bundle
Runner->>Go_Client: Start test
Go_Client->>JDT.LS: spawn process
Go_Client->>JDT.LS: initialize (LSP handshake)
JDT.LS->>Analyzer: load bundle
JDT.LS->>Go_Client: initialization result
rect rgba(100, 150, 200, 0.2)
Note over Go_Client,Analyzer: Search for symbol
Go_Client->>JDT.LS: workspace/executeCommand<br/>java.execute.workspaceSymbol
JDT.LS->>Analyzer: query symbol (location, type, etc.)
Analyzer->>Analyzer: Custom AST visitor<br/>Pattern matching (with erasure)
Analyzer-->>JDT.LS: symbol matches
JDT.LS-->>Go_Client: symbol results
end
rect rgba(150, 200, 100, 0.2)
Note over Go_Client: Verify results
Go_Client->>Go_Client: assert symbol name<br/>assert location<br/>assert kind
end
Go_Client->>JDT.LS: shutdown request
JDT.LS->>Go_Client: shutdown response
Go_Client->>JDT.LS: exit notification
Go_Client->>Runner: test result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring focused attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
142-160: Kill JDT.LS if dial fails.If
jsonrpc2.Dialerrors, the child process keeps running and the pipes stay open because we return without stoppingc.cmd. Please terminatec.cmd(and close the pipes) on that error path to avoid orphaned servers during retries.
🧹 Nitpick comments (23)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql (1)
1-5: Add a trailing newline to the file.SQL files should end with a newline character for consistency with common practices and many linters.
Apply this change:
INSERT INTO customers(id, username, name, surname, address, zipcode, city, country) VALUES (5, 'i_rule_scabb', 'Largo', 'LaGrande', 'Swamp Rot Inn', 'SC-002', 'Woodtick', 'Scabb Island'); +java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java (1)
1-21: LGTM! Clean exception implementation for testing.The implementation is correct and follows standard Java exception patterns. The class appropriately extends
Exception, includesserialVersionUIDfor serialization compatibility, and provides the most common constructors.For completeness, consider adding the fourth common exception constructor:
public CustomException(String message, Throwable cause) { super(message, cause); } + + public CustomException(Throwable cause) { + super(cause); + } }This would provide full coverage of standard exception construction patterns, which could be beneficial for comprehensive inheritance testing scenarios.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java (1)
16-21: Consider adding ServletException and character encoding.While throwing only IOException is legal, servlet methods conventionally declare both ServletException and IOException. Additionally, setting the character encoding helps avoid encoding issues.
Apply this diff to add ServletException and character encoding:
@Override protected void doGet(HttpServletRequest request, HttpServletResponse response) - throws IOException { + throws IOException, jakarta.servlet.ServletException { + response.setCharacterEncoding("UTF-8"); response.setContentType("text/html"); response.getWriter().println("<h1>Hello World</h1>"); }java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java (1)
3-5: Consider Jakarta EE namespace for modern JPA versions.The imports use the legacy
javax.persistencenamespace (JPA 2.x). Modern applications (Jakarta EE 9+) usejakarta.persistence(JPA 3.0+).If this test project is meant to validate analyzer behavior with legacy JPA applications, the current imports are appropriate. However, consider adding a corresponding test class using
jakarta.persistenceto ensure the analyzer handles both the legacy and modern namespaces correctly.java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java (1)
10-10: Add missing space for consistency.Missing space between
Page<Customer>andfindAll. For consistency with line 8, add a space after the closing angle bracket.Apply this diff:
- public Page<Customer>findAll(Pageable pageable); + public Page<Customer> findAll(Pageable pageable);java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (3)
20-21: Prefer constructor injection over field injection.Field injection makes testing harder and prevents making dependencies final. Constructor injection is the recommended approach for Spring beans.
Apply this diff to use constructor injection:
- @Autowired - private CustomerService customerService; + private final CustomerService customerService; + + @Autowired + public CustomerController(CustomerService customerService) { + this.customerService = customerService; + }
23-23: Make loggerstatic final.Logger fields should be declared as
static finalfor consistency with Java conventions.Apply this diff:
- private static Logger logger = Logger.getLogger( CustomerController.class.getName() ); + private static final Logger logger = Logger.getLogger(CustomerController.class.getName());
35-38: Use@GetMappingfor explicit HTTP method declaration.While
@RequestMappingworks, using@GetMappingmakes the HTTP method explicit and improves code readability.Apply this diff:
- @RequestMapping + @GetMapping public Page<Customer> findAll(Pageable pageable){ return customerService.findAll(pageable); }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java (1)
156-173: Consider documenting location value meanings.The test validates location values 0-14, but there's no documentation explaining what these numbers represent (e.g., location types or constants). Adding a brief comment would improve maintainability.
Example:
@Test public void testAllLocations() { - // Test all valid location types + // Test all valid location types (0-14 correspond to specific location constants) + // e.g., PACKAGE=0, TYPE=1, METHOD=2, etc. int[] locations = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14};java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java (1)
18-27: Inconsistent indentation.The indentation starting from line 18 is inconsistent with the rest of the method. Lines 18, 20, 21, 23-24, 26-27 appear to use different spacing than line 17.
Apply consistent indentation (assuming tabs or consistent spaces):
- AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext(); - context.setConfigLocation("io.konveyor.demo.ordermanagement.config"); - - context.scan("io.konveyor.demo.ordermanagement"); - container.addListener(new ContextLoaderListener(context)); - - ServletRegistration.Dynamic dispatcher = container - .addServlet("dispatcher", new DispatcherServlet(context)); - - dispatcher.setLoadOnStartup(1); - dispatcher.addMapping("/"); + AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext(); + context.setConfigLocation("io.konveyor.demo.ordermanagement.config"); + + context.scan("io.konveyor.demo.ordermanagement"); + container.addListener(new ContextLoaderListener(context)); + + ServletRegistration.Dynamic dispatcher = container + .addServlet("dispatcher", new DispatcherServlet(context)); + + dispatcher.setLoadOnStartup(1); + dispatcher.addMapping("/");java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java (1)
72-74: Consider removing printStackTrace() call.Line 74 calls
printStackTrace()which can produce verbose output in logs. Since the error is already logged vialogInfo()on line 73, the stack trace printing is redundant and can clutter logs.Consider removing the
printStackTrace()call or making it conditional based on a debug flag:} catch (Exception e) { logInfo("Error processing package declaration: " + e.toString()); - e.printStackTrace(); }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (1)
34-249: LGTM! Good location type coverage, consider adding result validation.The expanded test suite provides excellent coverage of various location types (method calls, constructors, annotations, inheritance, imports, packages, fields, declarations) and analysis modes. The tests successfully verify that the command handler executes without errors for each scenario.
For enhanced validation, consider adding assertions on result content in future iterations, such as:
- Verifying expected symbol counts for known test projects
- Checking that returned symbols have expected names or kinds
- Validating that location information is properly populated
This would strengthen the test suite by verifying correctness beyond non-crashing behavior.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java (2)
79-177: Consider using constants for location values.The location values (4, 10, etc.) are used throughout the tests as magic numbers. While comments clarify their meaning, defining them as constants would improve readability and maintainability.
Consider adding constants at the class level:
+ private static final int ANNOTATION_LOCATION = 4; + private static final int TYPE_LOCATION = 10; + @Test public void testFromMapOnAnnotationLocation() { Map<String, Object> annotationQueryMap = new HashMap<>(); annotationQueryMap.put("pattern", "javax.ejb.Stateless"); annotationQueryMap.put("elements", new ArrayList<>()); - // Location 4 is annotation location - AnnotationQuery query = AnnotationQuery.fromMap("fallbackQuery", annotationQueryMap, 4); + AnnotationQuery query = AnnotationQuery.fromMap("fallbackQuery", annotationQueryMap, ANNOTATION_LOCATION);
212-231: Consider adding null-safety and edge-case test coverage.The location iteration test is comprehensive, but consider adding tests for additional edge cases that aren't currently covered:
- Null pattern (not empty, but null) in
fromMap- Null input to
matchesAnnotation- Malformed elements in the elements list (e.g., maps missing "name" or "value" keys)
Example null-safety tests:
@Test public void testMatchesAnnotationNullInput() { AnnotationQuery query = new AnnotationQuery("test.Type", new HashMap<>(), false); // Document expected behavior: should it throw NPE or return false? assertFalse(query.matchesAnnotation(null)); } @Test public void testFromMapNullPattern() { Map<String, Object> annotationQueryMap = new HashMap<>(); annotationQueryMap.put("pattern", null); annotationQueryMap.put("elements", new ArrayList<>()); AnnotationQuery query = AnnotationQuery.fromMap("fallbackQuery", annotationQueryMap, 10); // Verify behavior with null pattern assertNotNull(query); assertEquals("fallbackQuery", query.getType()); }java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile (1)
4-5: Consider removing commented-out base image references.The commented lines 4 and 20 show the old Java 8 base images. Since this is a test project and the upgrade to Java 11 is intentional, these comments could be removed for cleaner code.
Apply this diff to remove the commented lines:
-# FROM maven:3.6-jdk-8-slim as build FROM maven:3.8-openjdk-11 as buildAnd similarly for line 20:
-# FROM tomcat:9-jdk8-openjdk-slim FROM tomcat:9-jdk11-openjdk-slimDockerfile.test (3)
3-6: Consider adding checksum verification for downloaded artifacts.Downloading external artifacts without checksum verification can introduce security risks if the source is compromised or serves unexpected content. While this is a test image, adding SHA256 verification would improve security posture.
Example approach:
-RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.50.0/jdt-language-server-1.50.0-202509041425.tar.gz &&\ +RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.50.0/jdt-language-server-1.50.0-202509041425.tar.gz &&\ + echo "EXPECTED_SHA256 jdtls.tar.gz" | sha256sum -c - &&\ tar -xvf jdtls.tar.gz --no-same-owner &&\
24-25: Track the TODO for periodic Maven index updates.The TODO indicates a need for a process to update the Maven index periodically. Consider creating a tracking issue for this maintenance task.
Do you want me to open a new issue to track the periodic update of the Maven search index?
49-49: Consider pinning the Go version for reproducible builds.Installing
golangwithout a version specifier may lead to different Go versions across builds as the UBI9 repository is updated. For more reproducible test environments, consider specifying a version or using a Go-specific base image stage.Example approach:
-RUN microdnf install -y golang +RUN microdnf install -y golang-1.23Or verify the available version matches your requirements in go.mod (go 1.23.9).
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
35-38: Consider null-safety in compareTo for robustness.The
compareToimplementation assumes boththis.serviceNameandother.serviceNameare non-null. While the constructor ensures initial non-null values, subclasses have protected access and could setserviceNameto null. For defensive programming, consider adding null checks.Example:
@Override public int compareTo(BaseService other) { + if (this.serviceName == null && other.serviceName == null) return 0; + if (this.serviceName == null) return -1; + if (other.serviceName == null) return 1; return this.serviceName.compareTo(other.serviceName); }java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile (2)
16-20: Use-fflag instead ofecho Yfor non-interactive pruning.The current approach using
echo Y |is fragile and non-standard. Docker'simage prunecommand supports the-f(force) flag for non-interactive execution.Apply this diff:
.PHONY: clean clean: remove $(info Removing all dangling build cache) - echo Y | docker image prune + docker image prune -f
32-36: Consider adding error handling for missing images.The
docker rmicommands will fail if the specified images don't exist, which may be unexpected during cleanup. Consider ignoring errors for more robust cleanup.Example approach:
.PHONY: remove remove: ## Removes all built images $(info Removing all built images...) - docker rmi $(REMOTE_IMAGE_NAME) - docker rmi $(LOCAL_IMAGE_NAME) + docker rmi $(REMOTE_IMAGE_NAME) || true + docker rmi $(LOCAL_IMAGE_NAME) || true.github/workflows/integration-tests.yml (1)
63-65: Consider quoting the command substitution.While generally safe in GitHub Actions, the unquoted
$(pwd)could theoretically cause word splitting. Quoting is a shell best practice.Apply this diff:
run: | podman run --rm \ - -v $(pwd)/java-analyzer-bundle.test:/tests:Z \ + -v "$(pwd)/java-analyzer-bundle.test:/tests:Z" \ -e WORKSPACE_DIR=/tests/projects \java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (1)
66-71: Consider try-with-resources for FileWriter.Switching to a try-with-resources block ensures
FileWritercloses even ifwritethrows, keeping the sample leak-free.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
java-analyzer-bundle.test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (70)
.github/workflows/integration-tests.yml(1 hunks).gitignore(1 hunks)Dockerfile(4 hunks)Dockerfile.test(1 hunks)Makefile(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(6 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java(3 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(7 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.test/README.md(1 hunks)java-analyzer-bundle.test/docs/README.md(1 hunks)java-analyzer-bundle.test/docs/integration-tests.md(1 hunks)java-analyzer-bundle.test/docs/query-reference.md(1 hunks)java-analyzer-bundle.test/docs/quick-reference.md(1 hunks)java-analyzer-bundle.test/docs/test-projects.md(1 hunks)java-analyzer-bundle.test/integration/client/jdtls_client.go(1 hunks)java-analyzer-bundle.test/integration/go.mod(1 hunks)java-analyzer-bundle.test/integration/integration_test.go(1 hunks)java-analyzer-bundle.test/integration/run_local.sh(1 hunks)java-analyzer-bundle.test/integration/test_helpers.go(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties(1 hunks)java-analyzer-bundle.test/projects/test-project/.classpath(1 hunks)java-analyzer-bundle.test/projects/test-project/.project(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/pom.xml(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/entity/Product.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/JdbcOnlyService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java(2 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java(1 hunks)pom.xml(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java (1)
MessageDriven(12-23)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java (1)
DataSourceDefinition(9-21)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java (1)
EnumConstantSymbolProvider(19-53)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java (1)
PackageDeclarationSymbolProvider(16-84)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java (1)
DataSourceDefinition(9-21)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (1)
MessageDriven(12-22)
java-analyzer-bundle.test/integration/integration_test.go (1)
java-analyzer-bundle.test/integration/client/jdtls_client.go (4)
JDTLSClient(19-28)NewJDTLSClient(87-96)AnnotationQuery(252-255)AnnotationElement(246-249)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
BaseService(9-39)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
SampleDelegateCommandHandler(43-427)
🪛 actionlint (1.7.8)
.github/workflows/integration-tests.yml
63-63: shellcheck reported issue in this script: SC2046:warning:2:6: Quote this to prevent word splitting
(shellcheck)
🪛 checkmake (0.2.2)
Makefile
[warning] 25-25: Target body for "help" exceeds allowed length of 5 (24).
(maxbodylength)
[warning] 115-115: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile
[warning] 38-38: Missing required phony target "all"
(minphony)
[warning] 38-38: Missing required phony target "test"
(minphony)
[warning] 9-9: Target "all" should be declared PHONY.
(phonydeclared)
🪛 LanguageTool
java-analyzer-bundle.test/docs/README.md
[style] ~73-~73: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...uick-reference.md#running-tests) ### I want to understand what's tested → [Quick Refer...
(REP_WANT_TO_VB)
[style] ~77-~77: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...erence.md#tested-search-queries) ### I want to add a new test → [Integration Tests Gui...
(REP_WANT_TO_VB)
[style] ~81-~81: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ence.md#untested-query-patterns) ### I want to understand the architecture → [Integrat...
(REP_WANT_TO_VB)
[style] ~85-~85: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...integration-tests.md#components) ### I want to understand test projects → [Test Projec...
(REP_WANT_TO_VB)
[style] ~89-~89: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...est-projects.md#coverage-matrix) ### I want to add Java test code → [Test Projects - A...
(REP_WANT_TO_VB)
[style] ~95-~95: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ration-tests.md#troubleshooting) ### I want to understand migration testing → [Query R...
(REP_WANT_TO_VB)
java-analyzer-bundle.test/docs/integration-tests.md
[grammar] ~384-~384: Use a hyphen to join words.
Context: ...: JDT.LS needs time to index projects (5 second wait in Initialize()) 2. **Build ...
(QB_NEW_EN_HYPHEN)
java-analyzer-bundle.test/docs/query-reference.md
[grammar] ~608-~608: Ensure spelling is correct
Context: ...**: ~5 seconds Per-Query Time: ~100-500ms depending on result count **Result Coun...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
java-analyzer-bundle.test/README.md
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/integration-tests.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
311-311: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/test-projects.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
206-206: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.2.3)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: QOS.ch Logback vulnerable to Deserialization of Untrusted Data
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: logback serialization vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: org.postgresql:postgresql vulnerable to SQL Injection via line comment generation
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: TemporaryFolder on unix-like systems does not limit access to created files
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Arbitrary File Write Vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: Path traversal in org.postgresql:postgresql
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: PostgreSQL JDBC Driver SQL Injection in ResultSet.refreshRow() with malicious column names
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Does Not Check Class Instantiation when providing Plugin Classes
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DoS via conditional HTTP request
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DataBinder Case Sensitive Match Exception
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Pivotal Spring Framework contains unsafe Java deserialization methods
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Web vulnerable to Open Redirect or Server Side Request Forgery
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Remote Code Execution in Spring Framework
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework is vulnerable to security bypass via mvcRequestMatcher pattern mismatch
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Path traversal vulnerability in functional web frameworks
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework Path Traversal vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework MVC Applications Path Traversal Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring MVC controller vulnerable to a DoS attack
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests (Phase 1)
| - name: Build JDT.LS container image with Podman | ||
| run: | | ||
| podman build -t jdtls-analyzer:test . | ||
There was a problem hiding this comment.
Critical: Dockerfile inconsistency between CI and local environments.
Line 60 builds from the default Dockerfile, but the local test script (run_local.sh line 32) and Makefile (line 92) explicitly use Dockerfile.test. This environment drift will cause CI and local test runs to use different container configurations, potentially masking issues.
Apply this diff to align with local scripts:
- name: Build JDT.LS container image with Podman
run: |
- podman build -t jdtls-analyzer:test .
+ podman build -t jdtls-analyzer:test -f Dockerfile.test .📝 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.
| - name: Build JDT.LS container image with Podman | |
| run: | | |
| podman build -t jdtls-analyzer:test . | |
| - name: Build JDT.LS container image with Podman | |
| run: | | |
| podman build -t jdtls-analyzer:test -f Dockerfile.test . | |
🤖 Prompt for AI Agents
.github/workflows/integration-tests.yml around lines 58 to 61: the workflow
builds the container using the default Dockerfile which differs from local
scripts; update the podman build command to explicitly use the test Dockerfile
(e.g., add -f Dockerfile.test or --file Dockerfile.test) so CI uses the same
Dockerfile.test as run_local.sh and the Makefile, ensuring consistent container
configuration across environments.
| } | ||
| symbol.setContainerName(mod.getParent().getElementName()); | ||
| symbol.setLocation(location); | ||
| logInfo("Location; " + location); |
There was a problem hiding this comment.
Fix typo in log message.
The log message has a semicolon instead of a colon: "Location;" should be "Location:".
- logInfo("Location; " + location);
+ logInfo("Location: " + location);📝 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.
| logInfo("Location; " + location); | |
| logInfo("Location: " + location); |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java
around line 53, fix the typo in the log message by replacing the semicolon after
"Location" with a colon so the string reads "Location: " + location; leaving
surrounding code unchanged.
| IJavaElement element = JDTUtils.findElementAtSelection(mod.getTypeRoot(), location.getRange().getStart().getLine(), location.getRange().getStart().getCharacter(), JavaLanguageServerPlugin.getPreferencesManager(), new NullProgressMonitor()); | ||
|
|
||
| logInfo("je" + element); |
There was a problem hiding this comment.
Remove unused element retrieval or clarify its purpose.
The element variable is retrieved but only logged and never used in any subsequent logic. This appears to be leftover debugging code.
If this code is not needed, remove it along with the now-unused imports at lines 9, 20-21:
-import org.eclipse.core.runtime.NullProgressMonitor;-import org.eclipse.jdt.ls.core.internal.JDTUtils;
-import org.eclipse.jdt.ls.core.internal.JavaLanguageServerPlugin;- IJavaElement element = JDTUtils.findElementAtSelection(mod.getTypeRoot(), location.getRange().getStart().getLine(), location.getRange().getStart().getCharacter(), JavaLanguageServerPlugin.getPreferencesManager(), new NullProgressMonitor());
-
- logInfo("je" + element);
-
-If this code serves a diagnostic purpose, please add a comment explaining its intent.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java
around lines 55-57, the local variable `element` is only retrieved and logged
but never used; remove the call to JDTUtils.findElementAtSelection and the
`logInfo("je"+element)` line, and delete the now-unused imports referenced at
lines 9 and 20-21; if the retrieval was intended for diagnostics, instead keep
it but replace the log with a clear explanatory comment above the retrieval
describing its diagnostic purpose and why it must remain (or persist only under
a debug log flag) so the intent is explicit.
| ASTParser astParser = ASTParser.newParser(AST.getJLSLatest()); | ||
| astParser.setSource(unit); | ||
| astParser.setResolveBindings(true); | ||
| CompilationUnit cu = (CompilationUnit) astParser.createAST(null); | ||
| CustomASTVisitor visitor = new CustomASTVisitor(query, match, QueryLocation.CONSTRUCTOR_CALL); | ||
| // Under tests, resolveConstructorBinding will return null if there are problems | ||
| cu.accept(visitor); | ||
| if (visitor.symbolMatches()) { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the removal of the qualification-match guard.
According to the summary, a guard condition (queryQualificationMatches(...)) was removed before this AST parsing block. Since AST parsing with binding resolution (lines 69-72) is computationally expensive, please verify:
- Whether the removed guard was filtering out unnecessary AST parsing operations
- Whether removing it causes performance regressions, especially for large codebases
- What conditions now trigger this expensive operation
Additionally, the comment at line 74 references "Under tests" but doesn't explain the broader context. Consider clarifying when and why resolveConstructorBinding might return null, and whether that affects the correctness of the analysis.
Generate a script to search for similar patterns in the codebase where qualification checks guard AST operations:
🏁 Script executed:
#!/bin/bash
# Description: Search for queryQualificationMatches usage and AST parsing patterns
# Search for queryQualificationMatches usage across symbol providers
echo "=== Searching for queryQualificationMatches usage ==="
rg -n "queryQualificationMatches" --type java -C3
# Search for similar AST parsing patterns
echo -e "\n=== Searching for AST parsing with binding resolution ==="
rg -n "ASTParser\.newParser.*setResolveBindings.*createAST" --type java -A5 -B5
# Search for other SymbolProvider implementations to compare patterns
echo -e "\n=== Finding other SymbolProvider implementations ==="
fd -e java -x rg -l "implements SymbolProvider" {}Length of output: 5565
🏁 Script executed:
#!/bin/bash
# Description: Examine ConstructorCallSymbolProvider and compare with MethodCallSymbolProvider
echo "=== ConstructorCallSymbolProvider context around lines 60-80 ==="
rg -n . -A20 -B5 "class ConstructorCallSymbolProvider" java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java | head -60
echo -e "\n=== Searching for AST parsing block in ConstructorCallSymbolProvider ==="
rg -n "ASTParser\|setResolveBindings\|createAST" java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java -B3 -A3
echo -e "\n=== MethodCallSymbolProvider context for comparison ==="
rg -n "queryQualificationMatches" java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java -B5 -A10Length of output: 11076
🏁 Script executed:
#!/bin/bash
# Description: View exact lines from ConstructorCallSymbolProvider.java
echo "=== Lines 60-85 of ConstructorCallSymbolProvider.java ==="
sed -n '60,85p' java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java
echo -e "\n=== Full file length ==="
wc -l java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java
echo -e "\n=== Search for all occurrences of 'ASTParser' in the file ==="
grep -n "ASTParser\|setResolveBindings\|createAST\|CustomASTVisitor" java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.javaLength of output: 2015
🏁 Script executed:
#!/bin/bash
# Description: Check if queryQualificationMatches is available and look for any inline documentation
echo "=== ConstructorCallSymbolProvider implementation declaration ==="
grep -n "implements\|extends" java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java
echo -e "\n=== Check if ConstructorCallSymbolProvider could use queryQualificationMatches ==="
cat java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java | head -30
echo -e "\n=== Full context around line 61 guard (lines 55-75) ==="
sed -n '55,75p' java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.javaLength of output: 2673
Replace weak guard with proper qualification matching to align with other SymbolProviders.
The guard at line 61 (if (this.query.contains("."))) is insufficient and inconsistent with other symbol providers. Both MethodCallSymbolProvider (line 56) and AnnotationSymbolProvider (line 99) use queryQualificationMatches() to validate qualification before triggering expensive AST parsing with binding resolution.
The current string-based check (this.query.contains(".")) will trigger AST parsing far more frequently than the proper qualification validation, causing unnecessary performance degradation on large codebases.
Replace line 61 with:
if (this.queryQualificationMatches(this.query, mod, unit, location)) {This restores the established pattern and ensures AST parsing is only performed when truly necessary.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java
around lines 69 to 76, the current string-based guard (this.query.contains("."))
is too weak and causes unnecessary AST parsing; replace that guard with the
established qualification check by calling queryQualificationMatches(this.query,
mod, unit, location) so AST parsing and binding resolution only run when the
query is truly qualified, mirroring MethodCallSymbolProvider and
AnnotationSymbolProvider behavior.
| if (binding == null) { | ||
| return false; | ||
| } | ||
| logInfo("getTypeParameters: " + binding.getTypeParameters()); | ||
| // get fqn of the method being called | ||
| ITypeBinding declaringClass = binding.getDeclaringClass(); | ||
| if (declaringClass == null) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Don't drop matches when binding metadata is missing.
Returning false here without setting symbolMatches flips previously-accepted matches into false negatives whenever JDT cannot resolve the binding (common with broken projects or partial classpaths). We still “can’t tell for sure,” per the comment, so we should keep the fallback behaviour. Set symbolMatches = true before these early returns so unresolved bindings remain accepted.
Apply this diff:
- if (binding == null) {
- return false;
- }
+ if (binding == null) {
+ this.symbolMatches = true;
+ return false;
+ }
...
- if (declaringClass == null) {
- return false;
- }
+ if (declaringClass == null) {
+ this.symbolMatches = true;
+ return false;
+ }Also applies to: 148-150
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
around lines 142-150 (and likewise for lines 148-150), the code returns false
when the IMethodBinding or its declaring class is null which causes
previously-accepted symbolMatches to flip to false; before these early returns
set symbolMatches = true to preserve the fallback "accept" behavior when
bindings are unresolved, then return false as before so unresolved bindings
remain treated as matches.
| public SampleApplication(String name) { | ||
| this.applicationName = name; | ||
| this.items = new ArrayList<String>(); // Explicit type parameter for testing | ||
| } | ||
|
|
||
| // Method declaration | ||
| public void processData() { | ||
| // Variable declarations | ||
| String tempData = "temporary"; | ||
| int count = 0; | ||
|
|
||
| // Method calls | ||
| System.out.println("Processing: " + tempData); | ||
| items.add(tempData); | ||
|
|
||
| // More constructor calls | ||
| File tempFile = new File("/tmp/data.txt"); | ||
| List<String> results = new ArrayList<String>(); // Explicit type parameter for testing | ||
| } | ||
|
|
||
| // Method with return type | ||
| public String getName() { | ||
| return applicationName; | ||
| } | ||
|
|
||
| // Method with parameters | ||
| public void writeToFile(String data) throws IOException { | ||
| // Constructor call with chained method call | ||
| FileWriter writer = new FileWriter(configFile); | ||
| writer.write(data); | ||
| writer.close(); | ||
| } | ||
|
|
||
| // Static method |
There was a problem hiding this comment.
Initialize configFile in the second constructor.
Objects created via new SampleApplication("name") leave configFile null, so writeToFile will throw a NullPointerException when it tries to open the writer. Please initialise configFile (and any other shared fields) in this constructor as well.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java
around lines 40 to 73, the second constructor sets applicationName and items but
leaves configFile null causing writeToFile to NPE; update this constructor to
initialize configFile to the same default used by the other constructor (or a
sensible default like a path derived from applicationName or a temp file) so
that FileWriter has a valid File, and ensure any required imports or exception
handling remain correct.
| // Method calling methods from java.io package | ||
| public void fileOperations() throws IOException { | ||
| // Multiple constructor calls | ||
| File dir = new File("/tmp"); | ||
| File file1 = new File(dir, "test.txt"); | ||
|
|
||
| // Method calls | ||
| if (dir.exists()) { | ||
| dir.mkdirs(); | ||
| } | ||
|
|
||
| String path = file1.getAbsolutePath(); | ||
| System.out.println(path); | ||
| } |
There was a problem hiding this comment.
Fix the directory existence check.
dir.mkdirs() only runs when dir.exists() is true, which is the opposite of what we need. Flip the condition so we create the directory when it’s missing.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java
around lines 78 to 91, the directory creation logic is inverted: currently
dir.mkdirs() is called inside an if (dir.exists()) block. Change the condition
to if (!dir.exists()) { dir.mkdirs(); } so the directory is created only when it
does not already exist (optionally handle the return value or exceptions from
mkdirs()).
| @Override | ||
| protected void doPost(HttpServletRequest request, HttpServletResponse response) | ||
| throws IOException { | ||
| String data = request.getParameter("data"); | ||
| response.getWriter().println("Received: " + data); | ||
| } |
There was a problem hiding this comment.
XSS vulnerability: parameter value written to response without sanitization.
Line 27 writes the request parameter directly to the response without HTML encoding or sanitization. This creates a cross-site scripting (XSS) vulnerability. Even in test code, this sets a poor security example that could be copied by developers.
Apply this diff to fix the XSS vulnerability and improve consistency:
@Override
protected void doPost(HttpServletRequest request, HttpServletResponse response)
- throws IOException {
+ throws IOException, jakarta.servlet.ServletException {
+ response.setCharacterEncoding("UTF-8");
+ response.setContentType("text/html");
String data = request.getParameter("data");
- response.getWriter().println("Received: " + data);
+ // HTML encode to prevent XSS
+ String encoded = data != null ?
+ data.replace("&", "&")
+ .replace("<", "<")
+ .replace(">", ">")
+ .replace("\"", """)
+ .replace("'", "'") : "";
+ response.getWriter().println("Received: " + encoded);
}Alternatively, if a library is available, use a proper HTML encoding utility like Apache Commons Text's StringEscapeUtils.escapeHtml4().
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java
around lines 23-28, the request parameter is written to the response directly
causing an XSS risk; update the handler to HTML-escape the parameter before
printing: obtain the parameter, handle null safely, and pass it through an
HTML-encoding routine (preferably Apache Commons Text
StringEscapeUtils.escapeHtml4(data) and add the import) or, if that library is
not available, replace/encode characters like <, >, &, ", ' with their HTML
entities before calling response.getWriter().println.
| @Test | ||
| public void testElementsAreIndependent() { | ||
| Map<String, String> elements = new HashMap<>(); | ||
| elements.put("value", "original"); | ||
|
|
||
| AnnotationQuery query = new AnnotationQuery("test.Type", elements, false); | ||
|
|
||
| // Modify original map | ||
| elements.put("value", "modified"); | ||
|
|
||
| // Query should still have the same reference (we're not doing defensive copy) | ||
| assertEquals("modified", query.getElements().get("value")); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Misleading test name and verify if shared mutable state is intentional.
The test name testElementsAreIndependent contradicts what the test actually verifies. The assertion confirms that modifying the original map DOES affect the query's internal state (i.e., they are NOT independent). The comment acknowledges this behavior but doesn't explain if it's intentional.
Sharing mutable references violates encapsulation and allows external code to modify the query's internal state after construction, which is typically considered a design flaw in Java APIs.
Two concerns to address:
- Rename the test to accurately reflect what it verifies:
- public void testElementsAreIndependent() {
+ public void testElementsMapIsShared() {- Verify if this behavior is intentional. If not, the
AnnotationQueryconstructor should perform a defensive copy:
// In AnnotationQuery constructor:
this.elements = new HashMap<>(elements);If the shared reference is intentional for performance reasons, document this clearly in the AnnotationQuery class javadoc to warn API users.
🏁 Script executed:
#!/bin/bash
# Find AnnotationQuery.java and inspect the elements field and constructor
find . -type f -name "AnnotationQuery.java" ! -path "*/test/*" ! -path "*/.git/*" | head -20Length of output: 101
🏁 Script executed:
#!/bin/bash
# Search for AnnotationQuery constructor and elements field handling
rg -n -A 10 "class AnnotationQuery" --type=java -g '!*test*' | head -50Length of output: 1428
🏁 Script executed:
#!/bin/bash
# Check for defensive copy patterns in constructors across the codebase
rg -n "new HashMap<>" --type=java -B 2 -A 2 | head -40Length of output: 5106
🏁 Script executed:
#!/bin/bash
# Search for other tests of AnnotationQuery to understand intended behavior
rg -n "testElements\|AnnotationQuery" java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about AnnotationQuery's design
rg -n "AnnotationQuery\|elements" --type=java -B 1 -A 1 | grep -i "defensive\|immutable\|shared\|reference" | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get the full AnnotationQuery.java implementation
cat java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQuery.javaLength of output: 2441
🏁 Script executed:
#!/bin/bash
# Get the full test file to see all tests and intent
cat java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.javaLength of output: 7635
🏁 Script executed:
#!/bin/bash
# Search for other Query classes to check if they follow a pattern
find . -type f -name "*Query.java" ! -path "*/test/*" ! -path "*/.git/*" -exec basename {} \; | sort | uniqLength of output: 60
🏁 Script executed:
#!/bin/bash
# Check WithAnnotationQuery and WithQuery for defensive copy patterns
cat java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/WithAnnotationQuery.javaLength of output: 135
🏁 Script executed:
#!/bin/bash
# Check WithQuery
cat java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/WithQuery.javaLength of output: 125
🏁 Script executed:
#!/bin/bash
# Search broadly for these Query classes
find . -type f -name "WithQuery.java" -o -name "WithAnnotationQuery.java" | head -10Length of output: 203
🏁 Script executed:
#!/bin/bash
# Check if there are similar mutable field patterns in constructors
rg -n "this\." java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/ -A 1 | grep -E "this\.\w+\s*=\s*(?!new)" | head -20Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check WithQuery implementation
cat ./java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/WithQuery.javaLength of output: 113
🏁 Script executed:
#!/bin/bash
# Check WithAnnotationQuery implementation
cat ./java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/WithAnnotationQuery.javaLength of output: 9676
🏁 Script executed:
#!/bin/bash
# Look for all constructors with direct field assignment in query/symbol classes
rg -n "this\.\w+\s*=\s*\w+;" java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/ --type=java -B 3 | head -60Length of output: 7436
Misleading test name contradicts actual behavior; verify defensive copy design.
The test name testElementsAreIndependent is factually incorrect. The test and its own comment confirm that elements ARE NOT independent—modifying the original map DOES affect the query because the constructor does not perform a defensive copy (line 28 of AnnotationQuery.java: this.elements = elements;).
This design inconsistency is concerning: the constructor stores a direct reference to the mutable map, but the fromMap() factory method creates a new HashMap with defensive copying. This inconsistency suggests the lack of copying in the constructor may be unintentional.
Address both issues:
- Rename the test to reflect what it actually verifies:
- public void testElementsAreIndependent() {
+ public void testElementsMapIsShared() {- Add defensive copy to the constructor to match the pattern in
fromMap()and protect against external mutation:
// In AnnotationQuery constructor:
this.elements = new HashMap<>(elements);If the shared reference is intentional for performance reasons, document this clearly in AnnotationQuery's class-level javadoc to warn API users about the mutable state.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java
around lines 179 to 191, the test name and comment are misleading because the
test actually verifies that the AnnotationQuery retains a live reference to the
original map; rename the test to reflect that behavior (e.g.,
testElementsReflectExternalMutation) and update the comment accordingly, and
then modify the AnnotationQuery constructor to perform a defensive copy of the
provided map (matching fromMap()) so external mutations do not affect internal
state; alternatively, if retaining the shared reference is intentional, add a
clear class-level Javadoc in AnnotationQuery documenting the mutable/aliasing
behavior instead of changing tests.
| @Test | ||
| public void testAnnotationQueryWithElements() { | ||
| List<Object> arguments = new ArrayList<>(); | ||
| Map<String, Object> params = new HashMap<>(); | ||
| params.put("project", "test-project"); | ||
| params.put("query", "javax.ejb.Stateless"); | ||
| params.put("location", "4"); | ||
| params.put("analysisMode", "full"); | ||
|
|
||
| Map<String, Object> annotationQuery = new HashMap<>(); | ||
| annotationQuery.put("pattern", "javax.ejb.Stateless"); | ||
| List<Map<String, String>> elements = new ArrayList<>(); | ||
| Map<String, String> element = new HashMap<>(); | ||
| element.put("name", "value"); | ||
| element.put("value", "TestBean"); | ||
| elements.add(element); | ||
| annotationQuery.put("elements", elements); | ||
|
|
||
| params.put("annotationQuery", annotationQuery); | ||
| arguments.add(params); | ||
|
|
||
| RuleEntryParams ruleParams = new RuleEntryParams("test-command", arguments); | ||
|
|
||
| assertNotNull(ruleParams.getAnnotationQuery()); | ||
| assertEquals("javax.ejb.Stateless", ruleParams.getAnnotationQuery().getType()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Incomplete validation of annotation query elements.
The test creates elements for the annotationQuery (lines 186-191) but only validates the pattern/type on line 199. The elements structure is never asserted, leaving a gap in test coverage.
Add assertions to verify the elements were parsed correctly:
assertNotNull(ruleParams.getAnnotationQuery());
assertEquals("javax.ejb.Stateless", ruleParams.getAnnotationQuery().getType());
+ assertNotNull(ruleParams.getAnnotationQuery().getElements());
+ assertEquals(1, ruleParams.getAnnotationQuery().getElements().size());
+ assertEquals("value", ruleParams.getAnnotationQuery().getElements().get(0).getName());
+ assertEquals("TestBean", ruleParams.getAnnotationQuery().getElements().get(0).getValue());
}🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java
around lines 175 to 200, the test builds annotationQuery elements but never
asserts they were parsed; add assertions after the existing type check to
validate the elements: assert the annotationQuery.getElements() is not null, has
size 1, and that the first element's name equals "value" and its value equals
"TestBean" (use the appropriate getter or map accessors consistent with
RuleEntryParams' element representation). Ensure these assertions follow the
existing pattern and fail the test if elements are missing or incorrect.
1 participant
handler.
against test projects to verify the accuracy of results.
validate any changes on PR or merge.
testing.
Signed-off-by: Shawn Hurley [email protected]
Summary by CodeRabbit
New Features
Tests
Documentation
Chores