Introduce JDK 21 compilation support #2433
Conversation
WalkthroughThis PR updates build/runtime baselines (parent POM bumps, Java 21), migrates JAXB from javax → jakarta, removes JaCoCo executions, replaces PowerMock with Mockito (including MockedStatic/MockedConstruction), removes Nashorn in favor of GraalVM JS, adjusts shell scripts for Java 9+ endorsed JAR handling, and makes targeted code/test fixes (JNDI dereferencing, visibility change, test removals). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant DSFinder as DataSourceFinder
participant JNDI as Initial JNDI Context
participant NM as NamingManager
participant Obj as LookupObject
participant DS as DataSource
Caller->>DSFinder: find(dsName, ctx)
DSFinder->>JNDI: context.lookup(dsName)
JNDI-->>DSFinder: obj (may be Reference or DataSource)
alt obj == null
DSFinder-->>Caller: return null
else obj != null
DSFinder->>NM: NamingManager.getObjectInstance(obj, null, null, env)
NM-->>DSFinder: dereferencedObj (or original obj)
DSFinder->>Obj: check instanceof DataSource
alt dereferencedObj is DataSource
DSFinder-->>Caller: return DataSource
else
DSFinder-->>Caller: throw SynapseCommonsException
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.java (1)
38-87: PowerMock migration verified; update deprecated Assert import.PowerMock removal is correct—the file contains no PowerMock imports or annotations. All mocking uses standard Mockito for instance methods (
isInitialized(),getRunningTaskCount(),schedule()), which Mockito handles natively.However, line 20 imports deprecated
junit.framework.Assert. Update toorg.junit.Assertfor consistency with JUnit 4 test framework.modules/distribution/src/main/bin/ciphertool.sh (1)
105-105: Remove dead code: unusedJAVA_ENDORSED_DIRSCygwin conversion.The conversion of
JAVA_ENDORSED_DIRSon line 105 is no longer needed since endorsed directories are now handled via classpath augmentation (lines 92–98) rather than the JVM option.CLASSPATH=`cygpath --path --windows "$CLASSPATH"` - JAVA_ENDORSED_DIRS=`cygpath --path --windows "$JAVA_ENDORSED_DIRS"` fimodules/transports/core/nhttp/pom.xml (1)
244-257: Remove duplicate mockito-core dependency.The
mockito-coredependency is declared twice (lines 244-247 and 254-257), both with test scope. Remove one of the duplicate entries.Apply this diff to remove the duplicate:
<dependency> <groupId>org.mockito</groupId> <artifactId>mockito-core</artifactId> <scope>test</scope> </dependency> <dependency> <groupId>org.mockito</groupId> <artifactId>mockito-inline</artifactId> <scope>test</scope> </dependency> - <dependency> - <groupId>org.mockito</groupId> - <artifactId>mockito-core</artifactId> - <scope>test</scope> - </dependency>
🧹 Nitpick comments (25)
modules/integration/src/test/java/org/apache/synapse/samples/framework/TestSamplesHandlerSuite.java (2)
28-29: Remove or comment out unused imports.Since Sample353 and Sample354 are no longer registered in the suite (lines 277-278 are commented out), these imports are now unused and should be removed or commented out for consistency.
Apply this diff to comment out the unused imports:
-import org.apache.synapse.samples.framework.tests.advanced.Sample353; -import org.apache.synapse.samples.framework.tests.advanced.Sample354; +//import org.apache.synapse.samples.framework.tests.advanced.Sample353; +//import org.apache.synapse.samples.framework.tests.advanced.Sample354;Alternatively, remove the imports entirely if these samples won't be re-enabled.
277-278: Add explanatory comment for disabled samples.The commented lines lack context about why Sample353 and Sample354 are disabled. Adding a brief explanation would help future maintainers understand this is related to JDK 21 compatibility and Ruby language support removal.
Apply this diff to add context:
+ // Samples 353 and 354 are disabled due to Ruby language support removal in JDK 21 // sampleClassRepo.put("353", Sample353.class); // sampleClassRepo.put("354", Sample354.class);modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java (1)
89-89: Remove redundant mock initialization.
MockitoAnnotations.initMocks(this)is unnecessary when using@RunWith(MockitoJUnitRunner.class), which automatically initializes the@Mockand@InjectMocksannotations.Apply this diff:
- MockitoAnnotations.initMocks(this);modules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.java (1)
21-21: Consider updating to JUnit 4 Assert.The
junit.framework.Assertclass is from JUnit 3 and deprecated. Since the tests use JUnit 4 (@Testannotation), consider updating toorg.junit.Assert.Apply this diff to modernize the import:
-import junit.framework.Assert; +import org.junit.Assert;Alternatively, use static imports for cleaner assertions:
-import junit.framework.Assert; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue;Then update the assertion calls to remove the
Assert.prefix (e.g.,assertTrue(...)instead ofAssert.assertTrue(...)).modules/distribution/src/main/bin/ciphertool.sh (1)
92-92: Add deprecation context to comment for consistency with other distribution scripts.While the comment explains what is being done, it omits context about why this pattern replaced the older approach. Other distribution scripts clarify that
-Djava.endorsed.dirswas removed in Java 9+.-# Add endorsed JARs to classpath (for Java 9+ compatibility) +# endorsed dir - Note: -Djava.endorsed.dirs is removed in Java 9+ +# Endorsed JARs are now added to classpath insteadmodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (1)
65-65: Remove redundant mock initialization.The
initMocks(this)call is unnecessary becauseMockitoJUnitRunner(line 45) automatically initializes all@Mockannotated fields before each test method executes.Apply this diff:
@Test public void testToAdmin() throws Exception { - initMocks(this); SessionID id = new SessionID(new BeginString("FIX.4.1"), new SenderCompID("SYNAPSE"),modules/transports/optional/fix/src/main/java/org/apache/synapse/transport/fix/FIXTransportSender.java (1)
277-278: Prefer package-private over protected for test-only visibility.Since the test class resides in the same package (
org.apache.synapse.transport.fix), package-private (default) visibility would suffice and provide better encapsulation thanprotected. Theprotectedmodifier unnecessarily exposes this method to all subclasses, which may not align with the intended API surface.Apply this diff to use package-private visibility:
- protected boolean sendUsingEPR(String targetEPR, String serviceName, Message fixMessage, + boolean sendUsingEPR(String targetEPR, String serviceName, Message fixMessage, String srcSession, int counter, MessageContext msgCtx) throws AxisFault {modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (4)
42-45: Mockito runner and static mocking setup look correct; runner is optional hereUsing
MockedStaticwith a try‑with‑resources block is the right way to scope theHttpClientBuilderstatic mock, and the imports are consistent. Since this test only uses locally created mocks (no@Mock/@InjectMocksfields),@RunWith(MockitoJUnitRunner.class)is not strictly necessary, though it doesn’t hurt. You could drop the runner later to simplify the class or, conversely, migrate fully away fromjunit.framework.TestCaseto a pure JUnit 4/5 style when convenient.Also applies to: 55-55
66-79: Static mocking ofHttpClientBuilderand fluent chain matches production codeThe mocking setup for
HttpClientBuilder.create()and the fluent calls (setDefaultRequestConfig,setConnectionManager,setSSLSocketFactory,build) correctly mirrors the productiongetSecureClientWithoutProxyimplementation, and returningmockHttpClientfrombuild()plus stubbingexecute(HttpPost)is sufficient for the test to exerciseOAuthClient.generateTokenwithout hitting the network. Consider optionally adding aMockito.verify(mockHttpClient).execute(any(HttpPost.class));to ensure the test would fail if the HTTP call path is refactored away, but the current behavior is functionally sound.
80-89: Response/status stubbing is consistent and narrowly scopedStubbing the
StatusLineto return 200, attaching it tomockResponse, and providing a small JSON payload viaentity.getContent()is a clean way to drive the happy‑path token parsing. The use ofByteArrayInputStreamwith simple ASCII JSON is safe here. If you later extend coverage, you might add separate tests for non‑200 responses or malformed JSON, but that’s outside the scope of this migration.
90-101: Context setup is adequate; minor SynapseConfiguration reuse improvement possibleThe Axis2/Synapse context wiring is sufficient for
OAuthClient.generateTokento run, and the parameters (timeouts set to-1, newProxyConfigsandTrustStoreConfigs) match the expected usage. One small clean‑up: you create aSynapseConfigurationforsynapseEnvironment, then pass a newSynapseConfiguration()instance intoAxis2MessageContext. Reusing the samesynapseConfigurationinstance in both places would be slightly clearer and avoid allocating an extra config object, though behavior is currently correct.modules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.java (1)
89-89: Simplify the matcher forevaluateValueThe explicit cast in
((MessageContext) any())is unnecessary; generics will infer the type. You can make this more idiomatic and type-safe as:Mockito.when(value.evaluateValue(any(MessageContext.class))).thenReturn(helloKey);This keeps the matcher type explicit and avoids the extra cast.
Please recompile the test after this tweak to confirm there are no import or overload resolution issues.
modules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.java (1)
90-91: Consider preserving exception context in RuntimeException.The broad
Exceptioncatch wraps any non-NamingException into a bareRuntimeException, which loses the original exception message and context.Apply this diff to preserve exception details:
} catch (Exception e) { - throw new RuntimeException(e); + throw new RuntimeException("Error dereferencing DataSource: " + dsName, e); }modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java (1)
36-40: Remove unused@Afterimport on line 40.The import is confirmed to be unused—no method in the test class is annotated with
@After. Removing this import will clean up the imports section.modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (1)
36-38: Mockito-based static mocking migration is consistent and scoped correctlyThe switch to
MockitoJUnitRunnerplusMockedStatic<BeanUtils>in a try‑with‑resources block is clean and matches the pattern used elsewhere in the suite. StubbingbeanstalkManager/beanstalkbefore callingejbMediator.init(...)and then mediating with a realAxis2MessageContextkeeps the test behaviorally equivalent to the prior PowerMock version.One minor, non‑blocking thought:
ejbMediatorandbeanstalkare static fields; if more tests are ever added to this class, you may want to make them instance fields or reinitialize in a@Beforemethod to avoid cross‑test state bleed, but this is not introduced by this PR.Please confirm the test passes under the Mockito version configured for this PR, as static mocking support is version‑sensitive.
Also applies to: 49-49, 87-105
modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLMapper.java (1)
28-30: Jakarta JAXB switch changes public API surface – confirm this is an intentional break
JsonXMLMappernow depends onjakarta.xml.bind.JAXBContext/JAXBExceptioninstead ofjavax.xml.bind.*. Functionally this is fine and aligns with the Jakarta migration, but it does change the public API types exposed by this class, which can break external consumers compiled against the oldjavaxtypes.If this class is part of your supported public API, consider:
- Explicitly documenting this as a breaking change in release notes, and/or
- Aligning versioning (e.g., major bump) to reflect the Jakarta migration.
Please verify that all modules (and external integrations, if any) using
JsonXMLMapperare updated tojakarta.xml.bindbefore releasing this change.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (1)
59-148: PowerMock → Mockito static mocking migration preserves behaviourThe three tests now use
try (MockedStatic<TargetContext> = mockStatic(...))with appropriate stubbing ofTargetContextstatic methods and rely on the same interaction patterns as before, so test intent and coverage are preserved while dropping PowerMock.If you ever touch these again, you might consider adding explicit
verifycalls onDeliveryAgent/TargetContextto assert behaviour rather than just “no exception”, but that’s outside this PR’s scope.modules/commons/pom.xml (2)
56-59: Jacoco plugin now declared without executionsThe
jacoco-maven-pluginis present but has no module-local<executions>; if coverage for this module is meant to be driven from the parent POM, this is fine, otherwise you may want to confirm that you’re still instrumenting and reporting coverage forsynapse-commons.If coverage drops unexpectedly for this module after the change, consider either re-adding executions here or centralising all Jacoco configuration in the parent POM.
237-247: Ensure JAXB runtime matches Jakarta APIYou’ve switched the API dependency to
jakarta.xml.bind:jakarta.xml.bind-apiwhile still depending oncom.sun.xml.bind:jaxb-coreandjaxb-impl. Please verify that the effective versions of those runtime artifacts are compatible with the Jakarta JAXB API (and not only with the legacyjavax.xml.bindAPI), otherwise you may see runtime failures when creatingJAXBContext.You can check the resolved versions and their package names (
javax.xml.bindvsjakarta.xml.bind) via your dependency tree (mvn dependency:tree) and by consulting the library documentation or release notes.modules/integration/pom.xml (1)
457-474: Mix of javax JAXB test deps and Jakarta migration—verify classpath behaviourThese new test‑scoped dependencies bring in the classic
javax.xml.bind/javax.activationstack for integration tests, while other modules (e.g. commons) now use the Jakarta JAXB API. That’s reasonable if tests still rely on javax‑based libraries, but please double‑check that:
- You don’t accidentally satisfy Jakarta
jakarta.xml.bindexpectations with only javax providers, and- There are no conflicting JAXB providers on the test classpath.
Running
mvn -pl modules/integration dependency:treeand cross‑checking provider packages (javax vs jakarta) against your usage is a good way to confirm this.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (1)
66-89: Unused mock variable in testFalse().The
targetContextMockis created but never used for stubbing or verification. If the static mock is not needed for this test, consider removing it to simplify the test.Apply this diff if the mock is not needed:
@Test public void testFalse() throws Exception { - try (MockedStatic<TargetContext> targetContextMock = mockStatic(TargetContext.class)) { - ConfigurationContext configurationContext = new ConfigurationContext(new AxisConfiguration()); - WorkerPool workerPool = new NativeWorkerPool(3, 4, 5, 5, "name", "id"); - PassThroughTransportMetricsCollector metrics = new PassThroughTransportMetricsCollector(true, "testScheme"); + ConfigurationContext configurationContext = new ConfigurationContext(new AxisConfiguration()); + WorkerPool workerPool = new NativeWorkerPool(3, 4, 5, 5, "name", "id"); + PassThroughTransportMetricsCollector metrics = new PassThroughTransportMetricsCollector(true, "testScheme"); - TargetConfiguration targetConfiguration = new TargetConfiguration(configurationContext, null, workerPool, - metrics, null); - HttpResponse response = mock(HttpResponse.class, Mockito.RETURNS_DEEP_STUBS); - NHttpClientConnection conn = mock(NHttpClientConnection.class, Mockito.RETURNS_DEEP_STUBS); - TargetConnections connections = mock(TargetConnections.class); - targetConfiguration.setConnections(connections); + TargetConfiguration targetConfiguration = new TargetConfiguration(configurationContext, null, workerPool, + metrics, null); + HttpResponse response = mock(HttpResponse.class, Mockito.RETURNS_DEEP_STUBS); + NHttpClientConnection conn = mock(NHttpClientConnection.class, Mockito.RETURNS_DEEP_STUBS); + TargetConnections connections = mock(TargetConnections.class); + targetConfiguration.setConnections(connections); - HttpRequest httpRequest = new BasicHttpRequest("GET", "test.com"); - when(conn.getContext().getAttribute("http.request")).thenReturn(httpRequest); + HttpRequest httpRequest = new BasicHttpRequest("GET", "test.com"); + when(conn.getContext().getAttribute("http.request")).thenReturn(httpRequest); - TargetResponse targetResponse = new TargetResponse(targetConfiguration, response, conn, false, false); + TargetResponse targetResponse = new TargetResponse(targetConfiguration, response, conn, false, false); - try { - targetResponse.start(conn); - } catch (Exception e) { - logger.error(e); - Assert.fail("Unable to start the target response!"); - } + try { + targetResponse.start(conn); + } catch (Exception e) { + logger.error(e); + Assert.fail("Unable to start the target response!"); } }pom.xml (1)
1152-1162: Consider aligning mockito-inline version with mockito.version property.The
mockito-inlinedependency uses a hardcoded version5.2.0(line 1160), whilemockito-coreuses the${mockito.version}property set to5.3.1(line 1598). Using different versions of the same library might lead to compatibility issues.Apply this diff to use the property for consistency:
<dependency> <groupId>org.mockito</groupId> <artifactId>mockito-inline</artifactId> - <version>5.2.0</version> + <version>${mockito.version}</version> <scope>test</scope> </dependency>Before applying this change, verify that mockito-inline 5.3.1 is compatible with the codebase by checking the Mockito release notes.
modules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.java (1)
57-63: Ruby-engine guard avoids false negatives on JDK 21The runtime check cleanly avoids failing this test when no Ruby
ScriptEngineis present, which is useful on newer JDKs. If you want clearer test reporting and less console noise, consider using a test-framework skip/assumption mechanism instead ofSystem.out.println+return.modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java (2)
432-445: JS vs non-JS message context split looks consistentRouting JAVA_SCRIPT/GRAAL_JAVA_SCRIPT through
GraalVMJavaScriptMessageContextand everything else throughCommonScriptMessageContextlines up with how JSON payloads and input arguments are now handled for JS. Only minor nit: the Javadoc above still references “Nashorn” even though the JS path now uses the Graal-based context; consider updating it for clarity.
667-672: Nashorn → Graal mapping works but leaves dead Nashorn-specific branchesRewriting
NASHORN_JAVA_SCRIPTtoGRAAL_JAVA_SCRIPThere is a sensible compatibility move for JDK 21, and ensures all “nashornJs” usages are executed on the Graal engine. However, after this reassignment:
- Later checks for
NASHORN_JAVA_SCRIPT(e.g., in helper selection,getScriptEngineSupplier,getNewScriptEngine) become effectively unreachable.oracleNashornFactoryand the helper methods around it no longer influence behavior.Consider either removing the now-dead Nashorn-specific branches and unused helpers, or adding a brief comment explaining why they remain, to avoid confusion for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
modules/commons/pom.xml(1 hunks)modules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.java(1 hunks)modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.java(1 hunks)modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLMapper.java(1 hunks)modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProvider.java(1 hunks)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java(1 hunks)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.java(1 hunks)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/ObjectFactory.java(1 hunks)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java(1 hunks)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java(1 hunks)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java(1 hunks)modules/core/pom.xml(0 hunks)modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java(3 hunks)modules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.java(2 hunks)modules/core/src/test/java/org/apache/synapse/core/axis2/ProxyServiceMessageReceiverTest.java(1 hunks)modules/core/src/test/java/org/apache/synapse/endpoints/DynamicLoadBalanceEndpointTest.java(2 hunks)modules/core/src/test/java/org/apache/synapse/endpoints/HttpEndpointTest.java(1 hunks)modules/core/src/test/java/org/apache/synapse/endpoints/RecipientListEndpointTest.java(2 hunks)modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java(2 hunks)modules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.java(4 hunks)modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java(3 hunks)modules/core/src/test/java/org/apache/synapse/message/store/RabbitMQStoreTest.java(1 hunks)modules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.java(4 hunks)modules/coverage-report/pom.xml(1 hunks)modules/distribution/src/main/bin/ciphertool.sh(1 hunks)modules/distribution/src/main/bin/synapse-config-migrator.sh(1 hunks)modules/distribution/src/main/bin/synapse.sh(1 hunks)modules/extensions/pom.xml(0 hunks)modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.java(0 hunks)modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java(3 hunks)modules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.java(2 hunks)modules/integration/pom.xml(2 hunks)modules/integration/src/test/java/org/apache/synapse/samples/framework/TestSamplesHandlerSuite.java(1 hunks)modules/samples/src/main/scripts/axis2Server/axis2server.sh(2 hunks)modules/tasks/pom.xml(0 hunks)modules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.java(1 hunks)modules/transports/core/nhttp/pom.xml(1 hunks)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/nhttp/NHttpTransportListenerTest.java(0 hunks)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/DeliveryAgentTest.java(2 hunks)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java(5 hunks)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/RequestResponseTest.java(0 hunks)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java(4 hunks)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java(4 hunks)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/connections/HostConnectionsTest.java(2 hunks)modules/transports/core/nhttp/src/test/resources/log4j.properties(1 hunks)modules/transports/optional/fix/pom.xml(0 hunks)modules/transports/optional/fix/src/main/java/org/apache/synapse/transport/fix/FIXTransportSender.java(1 hunks)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java(3 hunks)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java(3 hunks)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java(2 hunks)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java(3 hunks)pom.xml(6 hunks)
💤 Files with no reviewable changes (7)
- modules/transports/optional/fix/pom.xml
- modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.java
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/RequestResponseTest.java
- modules/tasks/pom.xml
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/nhttp/NHttpTransportListenerTest.java
- modules/extensions/pom.xml
- modules/core/pom.xml
🧰 Additional context used
🧬 Code graph analysis (18)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (2)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (3)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java (1)
RuntimeStatisticCollector(36-429)modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.java (1)
StatisticsConstants(26-202)modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelper.java (1)
StatisticDataCollectionHelper(32-162)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (2)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (1)
RunWith(41-70)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java (1)
RunWith(43-98)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.java (4)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java (1)
XmlRootElement(26-34)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java (2)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (1)
RunWith(45-125)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (1)
RunWith(41-70)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (2)
modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (1)
RunWith(46-167)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (1)
RunWith(45-125)
modules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.java (1)
modules/commons/src/main/java/org/apache/synapse/commons/resolvers/ResolverFactory.java (1)
ResolverFactory(29-139)
modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (3)
modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (1)
RunWith(46-167)modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (1)
RunWith(55-106)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (1)
RunWith(41-70)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java (2)
modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (1)
RunWith(46-167)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (1)
RunWith(41-70)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetContext.java (1)
TargetContext(34-258)
modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.java (4)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java (1)
XmlRootElement(26-34)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
modules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.java (1)
modules/core/src/main/java/org/apache/synapse/config/Entry.java (1)
Entry(35-251)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/config/PassThroughConfiguration.java (1)
PassThroughConfiguration(38-411)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (2)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetContext.java (1)
TargetContext(34-258)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (2)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)
modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (1)
modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java (1)
OAuthClient(98-464)
modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProvider.java (5)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/ObjectFactory.java (1)
XmlRegistry(23-38)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java (1)
XmlRootElement(26-34)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
🔇 Additional comments (60)
modules/transports/core/nhttp/src/test/resources/log4j.properties (1)
1-8: Verify Log4j v1 compatibility with JDK 21 modernization effort.This test resource uses Log4j v1 (org.apache.log4j.*) configuration, which reached end-of-life in 2015. Given that this PR is a broad modernization effort to support JDK 21, please verify:
- Whether the project intends to continue using Log4j v1 or has plans to migrate to Log4j v2
- That Log4j v1 is explicitly tested and compatible with JDK 21
- How this aligns with other test logging configurations across the codebase
The configuration itself is structurally sound—it properly sets the root logger to INFO with a ConsoleAppender and includes a reasonable timestamp-based pattern layout suitable for tests.
modules/core/src/test/java/org/apache/synapse/core/axis2/ProxyServiceMessageReceiverTest.java (1)
88-88: LGTM! Clean migration from PowerMockito to Mockito.The replacement of
PowerMockito.mockwithMockito.mockis correct for this use case. SinceAxis2SynapseEnvironmentis instantiated directly elsewhere in the test (line 102) and only instance methods are being stubbed, standard Mockito is sufficient and no PowerMockito features are needed.modules/core/src/test/java/org/apache/synapse/endpoints/DynamicLoadBalanceEndpointTest.java (2)
76-77: LGTM! Clean migration from PowerMockito to Mockito.The migration from
PowerMockito.mock()toMockito.mock()is correct and aligns with the PR objective of supporting JDK 21 compilation by removing PowerMock dependencies.
103-103: LGTM! Consistent migration approach.The migration to
Mockito.mock()is correctly applied, maintaining consistency with the other mock creation in this test class.modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java (3)
37-39: LGTM! Standard Mockito imports added.The imports are correctly updated to support the migration from PowerMockito to Mockito.
43-43: LGTM! Correct test runner for Mockito.The migration from
PowerMockRunnertoMockitoJUnitRunneris appropriate for this test class.
92-92: LGTM! Correct migration to Mockito.The change from
PowerMockito.whentoMockito.whenis correct for mocking the instance method.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/connections/HostConnectionsTest.java (2)
33-33: LGTM!The addition of the
Fieldimport is necessary for the reflection-based approach that replaces PowerMockito's field injection.
62-64: Field exists and reflection access is correct.Verification confirms that the
freeConnectionsfield exists as a declared private field in theHostConnectionsclass (line 69). The migration from PowerMockito to standard Java reflection is implemented correctly, and the field name in the reflection code matches the actual declaration.modules/distribution/src/main/bin/synapse.sh (1)
120-128: LGTM: Endorsed JAR classpath handling is correctly implemented.The transition from
-Djava.endorsed.dirsto runtime classpath augmentation is well-structured, with proper directory existence checks and consistent variable naming. This aligns with Java 9+ requirements.modules/distribution/src/main/bin/synapse-config-migrator.sh (1)
107-115: LGTM: Endorsed JAR classpath handling mirrors best practices fromsynapse.sh.Consistent implementation with proper checks and clear deprecation comments.
modules/samples/src/main/scripts/axis2Server/axis2server.sh (2)
116-124: LGTM: Endorsed JAR classpath handling is correct and consistent.Proper implementation with clear deprecation context.
169-169: No verification needed—arguments are already fully supported.The
SampleAxis2Serverclass correctly accepts-repoand-confarguments. TheSampleAxis2ServerManager.start()method implements robust argument parsing usingCommandLineOptionParserwith explicit validation that only "repo" and "conf" options are accepted. Existing unit tests already invokeSampleAxis2Server.main()with these exact arguments and pass successfully, confirming the implementation works. No breaking changes will occur.modules/core/src/test/java/org/apache/synapse/message/store/RabbitMQStoreTest.java (1)
21-47: PowerMock removal is appropriate; tests run fine under the default JUnit runnerThis class no longer uses any PowerMock APIs or features, so removing
PowerMockRunner/@PrepareForTestand relying on the standard JUnit runner is safe and reduces complexity. The tests only use reflection and regular JUnit assertions, so behavior should remain unchanged. Repository-wide verification confirms no remaining PowerMock usages exist.modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (2)
32-33: LGTM! Migration setup is correct.The migration from PowerMockRunner to MockitoJUnitRunner is properly configured with the necessary imports. This aligns with the PR objective to modernize the test framework.
Also applies to: 45-45
76-79: LGTM! Mockito stubbing correctly replaces PowerMockito.The migration from
PowerMockito.whentoMockito.whenis correct and follows standard Mockito patterns for method stubbing.modules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.java (2)
40-40: LGTM! Import migration is correct.The migration from PowerMockito to Mockito is appropriate and correctly implemented.
177-180: Confirm the test passes with current mock setup and verify ifgetType()stubbing is needed.Based on verification of the
EntrySerializercode and mock behavior, the review comment's analysis is accurate. When callingsetIntegerValue(50)on a Mockito mock,getIntegerValue()still returns 0, not 50—Mockito just doesn't match the getter and setter.Since
EntrySerializer.serializeEntry()explicitly callsentry.getType()and checks if it equalsEntry.URL_SRC(value 2), the mock will return 0 (the default for int methods) instead. The test currently passes only because it verifies only the key attribute, which is always serialized regardless of type.The suggestion to add
Mockito.doReturn(Entry.URL_SRC).when(entry).getType();is valid and improves test correctness. However, verify whether this is truly necessary by checking if the test intentionally verifies only partial serialization or if fuller verification is expected.modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java (3)
32-33: LGTM!Clean migration to standard Mockito imports.
60-60: LGTM!Appropriate test runner for Mockito-based testing.
109-114: Spy test is fragile and relies on stubbing to avoid NPEs; verify and improve with proper initialization.The spy is created without calling
init(), leavingcfgCtx(inherited fromAbstractTransportSender) uninitialized. While the test currently passes becausesendUsingEPRis stubbed (preventing its body at line 277 from executing), this approach is fragile:
- Line 286 in
sendUsingEPRaccessescfgCtx.getAxisConfiguration(), which would throw NPE if the method body executed- The test depends entirely on stubbing to work; any refactor removing or delaying the stub would cause failures
- Similar tests in the codebase (e.g.,
FIXOutgoingMessageHandlerTest.java) use@Mockand@InjectMocksfor proper dependency injectionConsider initializing the spy with required dependencies or using the
@Mock/@InjectMockspattern to avoid fragility.modules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.java (2)
28-32: Mockito import migration looks correctUsing
org.mockito.Mockitotogether with staticArgumentMatchers.any/anyStringmatches the usages below and removes the PowerMockito dependency without changing behavior.Please ensure the project’s Mockito version matches this API (e.g., run a quick compile on the test sources).
65-65:Mockito.when(...)stubs correctly replace PowerMockitoThe instance-method stubs for
synapseConfiguration.getEntry(anyString())andsynapsePath.getExpression()are straightforward Mockito equivalents of the old PowerMockito calls, and they keep the tests focused onResourceMapbehavior rather than exact key values.If you want slightly stricter tests later, you could replace
anyString()witheq(helloKey)to assert thatResourceMapuses the expected registry key, but this is not required for correctness.Also applies to: 76-78, 88-88
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (2)
27-29: Mockito migration wiring looks correctUsing
@RunWith(MockitoJUnitRunner.class)together with@Mockand@InjectMocks, plusMockedStaticfor static calls, is a good replacement for the previous PowerMockito-based setup and is consistent with other updated tests in this codebase. No functional issues spotted in this part.Also applies to: 41-50
59-67: Add explicit verification to theSession.sendToTargetmock invocationThe static stub signature correctly matches production usage—
FIXOutgoingMessageHandler.sendMessagedelegates to its privatesendToTargetmethod, which callsSession.sendToTarget(message, sessionID)(line 138). This matches your stubbed overload.However, the test currently verifies only the absence of exceptions. Add an explicit mock verification to ensure the handler properly invokes the QuickFIX session:
try (MockedStatic<Session> mockedSession = Mockito.mockStatic(Session.class)) { mockedSession.when(() -> Session.sendToTarget(any(Message.class), any(SessionID.class))) .thenReturn(true); SessionID id = new SessionID(new BeginString("FIX.4.1"), new SenderCompID("SYNAPSE"), new TargetCompID("BANZAI"), "FIX.4.1:SYNAPSE->BANZAI"); spy.sendMessage(message, id, SESSION_ID, SEQ_NUM, msgCtx, "fix://sample"); + mockedSession.verify(() -> Session.sendToTarget(message, id)); }modules/core/src/test/java/org/apache/synapse/endpoints/HttpEndpointTest.java (1)
162-162: LGTM! PowerMock to Mockito migration applied correctly.The change from
PowerMockito.mocktoMockito.mockis correct and aligns with the PR's objective to migrate away from PowerMock.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/DeliveryAgentTest.java (2)
31-31: LGTM! Import updated for Mockito migration.Correctly replaced PowerMockito import with standard Mockito static import.
49-51: LGTM! Mock creation updated to use Mockito.The migration from
PowerMockito.mock(...)tomock(...)is correct and consistent with the broader Mockito migration.modules/core/src/test/java/org/apache/synapse/endpoints/RecipientListEndpointTest.java (1)
185-185: LGTM! Mockito migration applied correctly.The migration from
PowerMockito.mocktoMockito.mockis correct.modules/coverage-report/pom.xml (1)
95-115: LGTM! JaCoCo reporting consolidated effectively.The refactoring simplifies coverage collection by:
- Pointing directly to module
targetdirectories instead ofcoverage-reportssubdirectories- Using a single
jacoco.execfile per module instead of multiplejacoco-unit*.execfilesThis aligns with the PR's JaCoCo upgrade to 0.8.12 and improves maintainability.
modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (4)
31-33: LGTM! Correct imports for Mockito static mocking.Added the necessary imports for
MockedStaticwhich replaces PowerMock's static mocking capabilities.
46-46: LGTM! Test runner updated for Mockito.The switch to
MockitoJUnitRunneris correct and consistent with the PowerMock to Mockito migration.
136-148: LGTM! Static method mocking implemented correctly.The use of try-with-resources with
MockedStaticis the proper pattern for mocking static methods in Mockito. This ensures the static mock is automatically cleaned up after the test.
156-165: LGTM! Static mocking pattern applied consistently.The static mocking of
RuntimeStatisticCollectormethods follows the correct Mockito pattern with proper scoping via try-with-resources.modules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.java (1)
73-84: LGTM! JNDI dereferencing implemented correctly.The automatic dereferencing of
javax.naming.Referenceobjects usingNamingManager.getObjectInstanceis the correct approach for JNDI lookups that may return references instead of direct objects.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java (3)
45-49: LGTM! Test infrastructure updated for Mockito.The migration includes:
- Correct static imports for Mockito methods
- Addition of
PassThroughTestUtilsfor configuration setup- Proper initialization of
passThroughConfigurationAlso applies to: 78-78
111-113: LGTM! Spy creation migrated to Mockito.The change from
PowerMockito.spytoMockito.spyis correct, and the mock configuration properly useswhen(...).thenReturn(...).
135-148: LGTM! Mock and spy usage updated consistently.All PowerMock calls have been correctly replaced with their Mockito equivalents while preserving test behavior.
modules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.java (4)
26-33: LGTM! JUnit 3 to JUnit 4 migration completed correctly.The changes properly modernize the test:
- Removed
extends TestCasedependency- Switched to JUnit 4 static assertion imports
- Test structure now follows JUnit 4 conventions
35-50: LGTM! Test assertions enhanced.The migration adds explicit
@Testannotations and improves assertions by verifying both the resolver type and its resolution behavior.
52-68: LGTM! Consistent test improvements applied.Additional tests properly migrated with enhanced assertions that verify resolver type and default resolution behavior.
76-83: LGTM! SystemResolver test updated.The test correctly migrates to JUnit 4 and maintains type verification for
SystemResolver.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java (1)
20-22: JAXB annotation namespace migration looks goodImports cleanly switch to
jakarta.xml.bind.annotation.*with no other changes; the DTO and its annotations remain semantically identical.Please ensure the test module’s POM is wired to
jakarta.xml.bind-apiso these annotations resolve under JDK 21.modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.java (1)
24-29: Mechanical JAXB → Jakarta import migration; binder behavior unchangedThe move from
javax.xml.bind.*tojakarta.xml.bind.*in this binder is straightforward, and the rest of the logic (bindability checks, marshal/unmarshal, array handling) remains untouched.Please ensure the runtime includes a Jakarta JAXB implementation compatible with
jakarta.xml.bind-apifor these types.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/ObjectFactory.java (1)
18-20: ObjectFactory JAXB imports correctly migrated to JakartaUpdating
JAXBElement,XmlElementDecl, andXmlRegistryto thejakarta.xml.bindnamespace is consistent with the rest of the Jakarta migration; the factory methods and QNames remain the same.Please confirm the corresponding tests still pass with the Jakarta JAXB runtime under JDK 21.
modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProvider.java (1)
21-28: JAXB javax→jakarta namespace switch looks correctImports now consistently use
jakarta.xml.bind.*andjakarta.xml.bind.annotation.*while the rest of the class logic is unchanged, so behaviour is preserved assuming a Jakarta JAXB runtime is present on the classpath.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.java (1)
21-25: Test imports aligned with Jakarta JAXBUsing
jakarta.xml.bind.*and JAXB annotations here matches the main-class migration and keeps the tests semantically identical.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
26-30: Jakarta JAXB usage in tests is consistentThe switch to
jakarta.xml.bindAPIs and@XmlTypehere matches the main JAXB migration; all calls toJAXBContext,MarshallerandUnmarshallerremain valid.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
18-19: SampleType annotations migrated cleanly to Jakarta
@XmlElementand@XmlTypenow come fromjakarta.xml.bind.annotation, which is consistent with the rest of the JAXB migration.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (1)
38-45: Mockito static-mocking imports are correct
MockedStaticand the static imports fromMockitoare exactly what you need for the new static-mocking pattern used below.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
18-19: Namespace-aware sample type updated to Jakarta JAXBThe move to
jakarta.xml.bind.annotationfor@XmlElementand@XmlTypekeeps the existing namespace configuration (urn:staxon:jaxb:test) unaffected.modules/integration/pom.xml (1)
295-295: Updated Surefire JVM args are JDK‑21 friendlySwitching from
MaxPermSizeto Metaspace options in<argLine>aligns with post‑PermGen JVMs and is compatible with Java 21; no issues from the POM side.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (3)
99-122: LGTM! Proper Mockito migration.The static mocking is correctly implemented using try-with-resources and the mock is properly stubbed to return a TargetContext instance.
130-163: LGTM! Static mocking properly implemented.The migration to Mockito is correct with proper use of try-with-resources and static method stubbing.
172-203: LGTM! Excellent Mockito migration with constructor mocking.The test demonstrates proper use of both static and constructor mocking. The verification of
TargetContext.updateState()ensures the expected state transition occurs.pom.xml (3)
221-222: LGTM! Java 21 compilation target set correctly.The compiler plugin is properly configured for JDK 21, aligning with the PR objectives.
1475-1475: LGTM! Build tooling versions updated appropriately.The updates to JaCoCo (0.8.12), Maven WAR Plugin (3.5.0), and Glassfish Jersey (2.41) align with JDK 21 requirements and are consistent across the POM.
Also applies to: 1482-1482, 1606-1607, 1638-1638
1355-1357: Version compatibility confirmed—no action required.jaxb-impl 4.0.6 is compatible with jakarta.xml.bind-api 4.0.4, as minor/patch bumps within the 4.0.x releases are intended to work together. The version difference noted is not a concern.
modules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.java (1)
24-24: Import addition is appropriate
ScriptEngineManageris used in the Ruby engine availability guard below; the import is correct and scoped to its only usage.modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java (1)
470-498: JSON payload handling correctly prefers Graal path for JS languagesThe updated condition in
processJSONPayloadensures that JAVA_SCRIPT and GRAAL_JAVA_SCRIPT use theGraalVMJavaScriptMessageContext’s JSON serializer, while non-JS languages continue to use the genericjsEngine-based parsing path. This keeps JSON handling consistent with the new JS message context and thepreprocessJSONInputArgumentlogic.
| /** | ||
| * Create a mock SynapseEnvironment object | ||
| * | ||
| * @return Axis2SynapseEnvironment instance | ||
| * @throws AxisFault on creating/mocking object | ||
| */ | ||
| /** | ||
| * Create a mock SynapseEnvironment object | ||
| * | ||
| * @return Axis2SynapseEnvironment instance | ||
| * @throws AxisFault on creating/mocking object | ||
| */ |
There was a problem hiding this comment.
Remove duplicate JavaDoc comment.
The same JavaDoc block appears twice (lines 172-177 and 178-183). Remove the first occurrence to eliminate duplication.
Apply this diff:
- /**
- * Create a mock SynapseEnvironment object
- *
- * @return Axis2SynapseEnvironment instance
- * @throws AxisFault on creating/mocking object
- */
/**
* Create a mock SynapseEnvironment object
*
* @return Axis2SynapseEnvironment instance
* @throws AxisFault on creating/mocking object
*/📝 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.
| /** | |
| * Create a mock SynapseEnvironment object | |
| * | |
| * @return Axis2SynapseEnvironment instance | |
| * @throws AxisFault on creating/mocking object | |
| */ | |
| /** | |
| * Create a mock SynapseEnvironment object | |
| * | |
| * @return Axis2SynapseEnvironment instance | |
| * @throws AxisFault on creating/mocking object | |
| */ | |
| /** | |
| * Create a mock SynapseEnvironment object | |
| * | |
| * @return Axis2SynapseEnvironment instance | |
| * @throws AxisFault on creating/mocking object | |
| */ |
🤖 Prompt for AI Agents
modules/core/src/test/java/org/apache/synapse/endpoints/RecipientListEndpointTest.java
around lines 172-183: remove the first duplicate JavaDoc block (lines 172-177)
so only a single JavaDoc remains immediately above the method; delete the
redundant comment lines and leave the second/remaining JavaDoc intact.
As part of the effort, the following changes have been done. - Migrated Unit tests from PowerMockito to Mockito - Due Mockito limitations, some two testcase files (RequestResponseTest.java & NHttpTransportListenerTest.java) have been removed. - NashornJS related implementation are removed. - ScriptMediator tests related Ruby language have been commented. - Since jaxb-api dependency is outdated, jakarta.xml.bind-api dependency is used instead. - Jacoco agent dependency was updated from 0.8.6 to 0.8.12. - Maven war plugin version was increased from 2.6 to 3.5.0 - Glassfish jersey dependency used in unit test was updated from 2.35 to 2.41.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
modules/distribution/src/main/bin/synapse-config-migrator.sh (1)
105-105: Remove dead code: unusedJAVA_ENDORSED_DIRSCygwin conversion.The conversion of
JAVA_ENDORSED_DIRSon line 105 is no longer needed since endorsed directories are now handled via classpath augmentation (lines 107–115) rather than the JVM option.
🧹 Nitpick comments (14)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (1)
65-65: Remove redundant initMocks call.The
initMocks(this)call is unnecessary becauseMockitoJUnitRunner(line 45) automatically initializes all@Mockfields before each test method. You can safely remove this line.🔎 Proposed cleanup
- initMocks(this); SessionID id = new SessionID(new BeginString("FIX.4.1"), new SenderCompID("SYNAPSE"),modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (1)
61-67: Static mocking correctly implemented with proper resource management.The
MockedStatic<Session>is correctly scoped within a try-with-resources block, ensuring proper cleanup after the test. The stubbing ofSession.sendToTargetfollows the same pattern used in other migrated tests (e.g.,OAuthClientTestandEJBMediatorTest).Optional: Consider adding verification
If you want to strengthen the test, you could verify that
Session.sendToTargetwas actually invoked:try (MockedStatic<Session> mockedSession = Mockito.mockStatic(Session.class)) { mockedSession.when(() -> Session.sendToTarget(any(Message.class), any(SessionID.class))).thenReturn(true); SessionID id = new SessionID(new BeginString("FIX.4.1"), new SenderCompID("SYNAPSE"), new TargetCompID("BANZAI"), "FIX.4.1:SYNAPSE->BANZAI"); spy.sendMessage(message, id, SESSION_ID, SEQ_NUM, msgCtx, "fix://sample"); + mockedSession.verify(() -> Session.sendToTarget(any(Message.class), any(SessionID.class)), times(1)); }This is not essential if the test's purpose is only to verify that
sendMessagecompletes without exceptions.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (2)
85-86: CallingsetAttributeon a mock has no effect.
contextis a mock object, socontext.setAttribute("CONNECTION_POOL", pool)does nothing—the mock doesn't store the value. If the code under test callscontext.getAttribute("CONNECTION_POOL"), it will returnnull.If this attribute is actually used, you need to stub the getter:
🔎 Proposed fix if the attribute is needed
HostConnections pool = new HostConnections(new RouteRequestMapping(new HttpRoute(new HttpHost("localhost")), ""), 1024); HttpContext context = mock(HttpContext.class); - context.setAttribute("CONNECTION_POOL", pool); + when(context.getAttribute("CONNECTION_POOL")).thenReturn(pool);If the attribute is not actually used by the code path being tested, consider removing the dead code.
125-126: Same issue:setAttributeon a mock has no effect.Same concern as in
testOutputReady—either stub the getter or remove if unused.🔎 Proposed fix if the attribute is needed
HostConnections pool = new HostConnections(new RouteRequestMapping(new HttpRoute(new HttpHost("localhost")), ""), 1024); HttpContext context = mock(HttpContext.class); - context.setAttribute("CONNECTION_POOL", pool); + when(context.getAttribute("CONNECTION_POOL")).thenReturn(pool);modules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.java (2)
89-89: Refactor the argument matcher for better clarity.The explicit cast
((MessageContext) any())is awkward and not idiomatic Mockito. Consider usingany(MessageContext.class)for type-safe matching or simplyany()if the method signature is unambiguous.🔎 Proposed refactor
- Mockito.when(value.evaluateValue(((MessageContext) any()))).thenReturn(helloKey); + Mockito.when(value.evaluateValue(any(MessageContext.class))).thenReturn(helloKey);
60-61: Remove redundant setUp() call.Since this test extends
TestCase(JUnit 3), thesetUp()method is automatically invoked before each test. The manual call on line 61 is redundant and inconsistent with the other test methods (lines 73 and 85) that don't call it.🔎 Proposed cleanup
public void testResolveFromMap() throws Exception { - setUp(); - ResourceMap resourceMap = new ResourceMap();modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java (3)
338-338: Use constant instead of string literal.The string literal
"js"should use theJAVA_SCRIPTconstant for consistency and maintainability.🔎 Proposed fix
- if (language.equals("js") || language.equals(GRAAL_JAVA_SCRIPT)) { + if (language.equals(JAVA_SCRIPT) || language.equals(GRAAL_JAVA_SCRIPT)) {
364-365: Consider removing dead code related to NASHORN_JAVA_SCRIPT.Since
NASHORN_JAVA_SCRIPTis converted toGRAAL_JAVA_SCRIPTat initialization (line 668), the following checks forNASHORN_JAVA_SCRIPTare now dead code and can be removed:
- Lines 364-365: Condition in
mediateWithExternalScript- Lines 694-695: Condition in
initScriptEngine- Lines 784-785: Case in
getScriptEngineSupplier- Lines 835-836: Case in
getNewScriptEngineRemoving these would improve code maintainability and reduce cognitive complexity.
Also applies to: 694-695, 784-785, 835-836
720-759: Consider removing unused Nashorn engine creation method.The
createNashornEnginePortable()method appears to be dead code after the Nashorn-to-GraalVM migration. SinceNASHORN_JAVA_SCRIPTis redirected toGRAAL_JAVA_SCRIPTat initialization (line 668), this method is no longer invoked in the normal execution flow.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java (2)
40-40: Remove unused import.The
@Afterimport is not used anywhere in the test class.🔎 Proposed fix
-import org.junit.After;
101-119: Consider modernizing the test setup.The test uses deprecated
MockitoAnnotations.initMocks(this)and mixes JUnit 3 style (extendsTestCase) with JUnit 4 annotations (@Test,@Mock).🔎 Suggested improvements
Option 1: Use JUnit 4 with MockitoJUnitRunner (recommended)
Remove
extends TestCasefrom the class declaration and add the runner:+import org.junit.runner.RunWith; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) -public class PassThroughHttpSenderTest extends TestCase { +public class PassThroughHttpSenderTest {Then remove the
MockitoAnnotations.initMocks(this)calls from both test methods.Option 2: Use openMocks with proper cleanup
Replace deprecated
initMockswithopenMocksand add cleanup:+ private AutoCloseable mocks; + @Test public void testInvoke() throws Exception { - MockitoAnnotations.initMocks(this); + mocks = MockitoAnnotations.openMocks(this); // ... rest of test } + + @After + public void tearDown() throws Exception { + if (mocks != null) { + mocks.close(); + } + }modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java (1)
118-138: Consider migrating reflection-based private method testing.This test still uses reflection to access and test the private
isTargetValidmethod (line 134:isTargetvalid.setAccessible(true)), whiletestFIXTransportSenderSendMessagewas migrated to use Mockito spy without reflection.Recommendation: For consistency with the PowerMockito migration:
- If
isTargetValidshould remain private, this reflection-based approach is acceptable but inconsistent with the spy approach used above- If following the same pattern as
sendUsingEPR, consider changingisTargetValidto package-private visibility and testing it directlyThe current mixed approach (spy for one method, reflection for another) may indicate incomplete migration.
modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (1)
87-93: Mockito migration looks correct; consider instance fields for better test isolation.The transition from PowerMockito to standard Mockito mocking is properly implemented. However,
ejbMediator(line 56) andbeanstalk(line 57) are static fields, which can lead to test isolation issues if additional test methods are added in the future.💡 Optional refactor: Use instance fields
Consider converting static fields to instance fields and initializing them in a
@Beforemethod:- private static EJBMediator ejbMediator = new EJBMediator(); - private static EnterpriseBeanstalk beanstalk; + private EJBMediator ejbMediator; + private EnterpriseBeanstalk beanstalk; + + @Before + public void setUp() { + ejbMediator = new EJBMediator(); + }This ensures each test method gets fresh instances, improving test isolation.
modules/distribution/src/main/bin/ciphertool.sh (1)
105-105: Remove dead code: unusedJAVA_ENDORSED_DIRSCygwin conversion.The conversion of
JAVA_ENDORSED_DIRSon line 105 is no longer needed since the-Djava.endorsed.dirsJVM option has been removed for Java 9+ compatibility, and endorsed directories are now handled via classpath augmentation (lines 92–98).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
modules/commons/pom.xmlmodules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.javamodules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.javamodules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLMapper.javamodules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProvider.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/ObjectFactory.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.javamodules/core/jacoco.execmodules/core/pom.xmlmodules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.javamodules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.javamodules/core/src/test/java/org/apache/synapse/core/axis2/ProxyServiceMessageReceiverTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/DynamicLoadBalanceEndpointTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/HttpEndpointTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/RecipientListEndpointTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.javamodules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.javamodules/core/src/test/java/org/apache/synapse/message/store/RabbitMQStoreTest.javamodules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.javamodules/coverage-report/pom.xmlmodules/distribution/pom.xmlmodules/distribution/src/main/bin/ciphertool.shmodules/distribution/src/main/bin/synapse-config-migrator.shmodules/distribution/src/main/bin/synapse.shmodules/experimental/pom.xmlmodules/extensions/pom.xmlmodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.javamodules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.javamodules/features/org.apache.synapse.samples.feature/pom.xmlmodules/features/org.apache.synapse.transport.fix.feature/pom.xmlmodules/features/org.apache.synapse.transport.nhttp.feature/pom.xmlmodules/features/org.apache.synapse.transport.vfs.feature/pom.xmlmodules/features/org.apache.synapse.transport.vfs.smb.feature/pom.xmlmodules/features/org.apache.synapse.transport.vfs.smb2.feature/pom.xmlmodules/features/org.apache.synapse.wso2.feature/pom.xmlmodules/features/pom.xmlmodules/handler/pom.xmlmodules/integration/pom.xmlmodules/integration/src/test/java/org/apache/synapse/samples/framework/TestSamplesHandlerSuite.javamodules/migrator/pom.xmlmodules/packaging/package-archetype/pom.xmlmodules/packaging/package-skeleton/pom.xmlmodules/patches/pom.xmlmodules/samples/pom.xmlmodules/samples/src/main/scripts/axis2Server/axis2server.shmodules/securevault/pom.xmlmodules/tasks/pom.xmlmodules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.javamodules/transports/core/nhttp/pom.xmlmodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/nhttp/NHttpTransportListenerTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/DeliveryAgentTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/RequestResponseTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/connections/HostConnectionsTest.javamodules/transports/core/nhttp/src/test/resources/log4j.propertiesmodules/transports/core/pipe/pom.xmlmodules/transports/core/vfs/pom.xmlmodules/transports/optional/fix/pom.xmlmodules/transports/optional/fix/src/main/java/org/apache/synapse/transport/fix/FIXTransportSender.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.javamodules/transports/pom.xmlmodules/war/pom.xmlmodules/xar-maven-plugin/pom.xmlpom.xml
💤 Files with no reviewable changes (3)
- modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.java
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/nhttp/NHttpTransportListenerTest.java
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/RequestResponseTest.java
✅ Files skipped from review due to trivial changes (1)
- modules/patches/pom.xml
🚧 Files skipped from review as they are similar to previous changes (23)
- modules/coverage-report/pom.xml
- modules/transports/core/nhttp/src/test/resources/log4j.properties
- modules/transports/optional/fix/src/main/java/org/apache/synapse/transport/fix/FIXTransportSender.java
- modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java
- modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProvider.java
- modules/distribution/src/main/bin/synapse.sh
- modules/core/src/test/java/org/apache/synapse/message/store/RabbitMQStoreTest.java
- modules/integration/src/test/java/org/apache/synapse/samples/framework/TestSamplesHandlerSuite.java
- modules/samples/src/main/scripts/axis2Server/axis2server.sh
- modules/core/src/test/java/org/apache/synapse/endpoints/RecipientListEndpointTest.java
- modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.java
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/connections/HostConnectionsTest.java
- modules/core/src/test/java/org/apache/synapse/endpoints/DynamicLoadBalanceEndpointTest.java
- modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/ObjectFactory.java
- modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java
- modules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.java
- modules/core/src/test/java/org/apache/synapse/core/axis2/ProxyServiceMessageReceiverTest.java
- modules/transports/core/nhttp/pom.xml
- modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java
- modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java
- modules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.java
- modules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.java
- modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLMapper.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T11:12:52.620Z
Learnt from: shilmyhasan
Repo: wso2/wso2-synapse PR: 2427
File: modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java:268-290
Timestamp: 2025-12-12T11:12:52.620Z
Learning: In modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java, when proxy is not enabled, NoopHostnameVerifier.INSTANCE is intentionally used for backward compatibility with the previous implementation, even though it disables hostname verification.
Applied to files:
modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java
🧬 Code graph analysis (9)
modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java (1)
RuntimeStatisticCollector(36-429)modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.java (1)
StatisticsConstants(26-202)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetContext.java (1)
TargetContext(34-258)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (2)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)
modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (4)
modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (1)
RunWith(49-107)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (1)
RunWith(41-70)modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthUtilsTest.java (7)
RunWith(66-517)RunWith(79-170)RunWith(175-280)RunWith(285-336)RunWith(341-381)RunWith(386-430)RunWith(435-478)modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java (1)
OAuthClient(98-464)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (7)
modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (1)
RunWith(46-167)modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (1)
RunWith(55-106)modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (1)
RunWith(49-107)modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/connections/HostConnectionsTest.java (1)
RunWith(39-82)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (1)
RunWith(41-70)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java (1)
RunWith(43-98)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java (1)
RunWith(60-139)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (2)
modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (1)
RunWith(55-106)modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (1)
RunWith(49-107)
modules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.java (1)
modules/commons/src/main/java/org/apache/synapse/commons/resolvers/ResolverFactory.java (1)
ResolverFactory(29-139)
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java (2)
modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (1)
RunWith(55-106)modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java (1)
RunWith(43-98)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.java (4)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java (1)
XmlRootElement(26-34)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
🔇 Additional comments (76)
modules/core/src/test/java/org/apache/synapse/endpoints/HttpEndpointTest.java (1)
162-162: LGTM! Clean PowerMock → Mockito migration.The replacement of
PowerMockito.mock()withMockito.mock()is correct. Both APIs have identical signatures for basic class mocking, and the stubbing on line 165 confirms standard Mockito compatibility.modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (3)
32-33: LGTM! Clean migration to Mockito imports.The new imports correctly replace PowerMock dependencies and align with the PR's migration objective.
45-45: LGTM! Proper migration to MockitoJUnitRunner.This change correctly replaces PowerMockRunner and ensures all
@Mockannotations are automatically initialized.
76-79: LGTM! Correct migration from PowerMockito to Mockito.The stubbing logic has been properly migrated from
PowerMockito.whentoMockito.when, preserving the test behavior while using the standard Mockito API.modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java (2)
27-29: Migration to Mockito looks good!The imports and test runner have been correctly updated to use Mockito's native static mocking capabilities (MockedStatic) instead of PowerMock. This aligns with modern Mockito best practices and JDK 21 compatibility.
Also applies to: 41-41
59-59: Instance mock setup correctly migrated.The change from
PowerMockito.whentoMockito.whenis appropriate for setting up the instance mock.modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (3)
38-44: LGTM!Import changes correctly reflect the migration to Mockito's
MockedStaticpattern for static mocking.
57-73: LGTM!The
MockedStaticis properly scoped within try-with-resources, ensuring automatic cleanup. The static stubbing syntax correctly migrates from PowerMockito to Mockito.
49-150: PowerMock to Mockito migration looks correct overall.The migration properly:
- Uses
MockedStaticwith try-with-resources for automatic cleanup- Updates static stubbing syntax to
targetContextMock.when(() -> ...).thenReturn(...)- Maintains test logic and coverage intent
The tests appear to verify exception-free execution rather than specific behaviors (no assertions). This may be intentional for smoke-testing the handler methods.
modules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.java (2)
28-31: LGTM! Clean migration to standard Mockito.The import changes correctly replace PowerMockito with standard Mockito and update the argument matcher from the deprecated
anyObjecttoany.
65-65: LGTM! Mockito migration executed correctly.The replacement of
PowerMockito.when()withMockito.when()across all test methods is correct and maintains the same test behavior.Also applies to: 76-77, 88-88
modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java (3)
435-435: LGTM! Nashorn removal handled correctly.The condition correctly checks for
JAVA_SCRIPTorGRAAL_JAVA_SCRIPTand instantiatesGraalVMJavaScriptMessageContext. This aligns with the Nashorn-to-GraalVM migration, whereNASHORN_JAVA_SCRIPTis redirected toGRAAL_JAVA_SCRIPTduring initialization (lines 665-669).
481-481: LGTM! JSON payload processing correctly updated.The condition correctly uses GraalVM-based JSON parsing for
JAVA_SCRIPTandGRAAL_JAVA_SCRIPTlanguages, consistent with the overall migration strategy.
671-673: LGTM! GraalVM engine initialization is secure.The script engines are correctly initialized with
GraalJSScriptEngineand include access control configuration, maintaining security posture during the migration.modules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.java (1)
31-31: The PowerMock removal is correct; no changes needed.The removal of
@PrepareForTest(TaskManager.class)is safe for the following reasons:
TaskManager is an interface, not a final class. Interfaces can be mocked with standard Mockito without special handling.
TaskManager contains only instance methods (schedule, isInitialized, getRunningTaskCount, etc.)—no static methods that would require MockedStatic patterns.
The test file uses Mockito.mock(TaskManager.class) directly to mock instance methods, which is the standard approach for interface mocking and requires no additional annotations or dependencies beyond mockito-core.
The migration from PowerMock to Mockito is complete and functional for this test.
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java (1)
135-135: LGTM! Migration from PowerMockito to Mockito completed correctly.The migration from
PowerMockito.mockandPowerMockito.spyto their Mockito equivalents is correctly implemented.Also applies to: 146-148
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/DeliveryAgentTest.java (2)
31-31: LGTM! Clean migration to standard Mockito.The static import change from PowerMockito to Mockito is correct and aligns with the PR's migration objectives.
49-51: Migration to standard Mockito mocks is correct.The replacement of
PowerMockito.mock()withmock()is appropriate for these non-final object mocks. The project uses Mockito 5.3.1, which provides full support for standard mocking without special configuration. Since none of the mocked classes (TargetConfiguration, TargetConnections, ProxyConfig) are final, this migration is safe.modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java (3)
32-33: LGTM: Clean migration to Mockito imports.The import changes correctly replace PowerMockito with standard Mockito equivalents, consistent with the PR's objective to migrate to Mockito for JDK 21 compatibility.
60-60: LGTM: Proper test runner for Mockito.The runner annotation correctly switches to
MockitoJUnitRunner, matching patterns used in other test files in the codebase.
109-114: Verify test annotations and determine actual mocking framework.The test code attempts to directly stub and verify
sendUsingEPRusing standard Mockito syntax, butsendUsingEPRis declared asprivatein FIXTransportSender. Standard Mockito cannot mock private methods. Check whether the test class uses@RunWith(PowerMockRunner.class)and@PrepareForTestannotations—if not, the test will fail. If PowerMockito is being used, its dependency and configuration must be explicit in the test class.modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (3)
36-38: LGTM: Import changes align with Mockito migration.The addition of
MockedStaticandMockitoJUnitRunnercorrectly replaces PowerMock dependencies for JDK 21 compatibility.
49-49: LGTM: Runner annotation correctly updated.The switch to
MockitoJUnitRunneris appropriate for standard Mockito-based testing.
95-105: MockedStatic with try-with-resources is correctly implemented; verify EJBMediator doesn't cast the return value to a specific type.The try-with-resources statement properly manages MockedStatic scoped mocks, ensuring automatic closure when the block exits. Placing both
init()andmediate()calls inside the scoped block is appropriate.The mock returns a generic
new Object()fromBeanUtils.invokeInstanceMethod. If EJBMediator casts this return value to a specific type, aClassCastExceptionwill occur at runtime. Verify that the mediator implementation handles the Object return type correctly or adjust the mock to return a type-appropriate value.Consider adding a verification step:
mockedBeanUtils.verify(() -> BeanUtils.invokeInstanceMethod(any(Object.class), any(Method.class), any(Object[].class)));This ensures the mocked method is actually invoked during the test.
modules/securevault/pom.xml (1)
27-27: Parent version update looks good.The parent POM version bump from 4.0.0-wso2v272-SNAPSHOT to 4.1.0-wso2v1-SNAPSHOT aligns with the JDK 21 support objectives. This change is consistent across all modules in this PR.
modules/features/org.apache.synapse.transport.vfs.feature/pom.xml (1)
22-22: Parent version update is consistent.modules/features/org.apache.synapse.wso2.feature/pom.xml (1)
23-23: Parent version update is consistent.modules/packaging/package-archetype/pom.xml (1)
25-25: Parent version update is consistent.modules/features/org.apache.synapse.samples.feature/pom.xml (1)
22-22: Parent version update is consistent.modules/features/org.apache.synapse.transport.vfs.smb2.feature/pom.xml (1)
23-23: Parent version update is consistent.modules/experimental/pom.xml (1)
26-26: Parent version update is consistent.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.java (1)
21-25: JAXB namespace migration to Jakarta is correct.The migration from
javax.xml.bind.*tojakarta.xml.bind.*is the proper approach for JDK 21 compatibility and aligns with the Jakarta EE specifications. The non-JAXB imports (XMLConstants, QName) correctly remain in thejavax.xmlnamespace.modules/migrator/pom.xml (1)
27-27: LGTM! Parent version bump aligned with JDK 21 support.The parent version update to 4.1.0-wso2v1-SNAPSHOT is consistent with the project-wide migration to support JDK 21 compilation.
modules/xar-maven-plugin/pom.xml (1)
25-25: LGTM! Parent version bump aligned with JDK 21 support.The parent version update to 4.1.0-wso2v1-SNAPSHOT is consistent with the project-wide migration.
modules/transports/core/vfs/pom.xml (1)
26-26: LGTM! Parent version bump aligned with JDK 21 support.The parent version update to 4.1.0-wso2v1-SNAPSHOT is consistent with the project-wide migration.
modules/distribution/pom.xml (1)
25-25: LGTM! Parent version bump aligned with JDK 21 support.The parent version update to 4.1.0-wso2v1-SNAPSHOT is consistent with the project-wide migration.
modules/distribution/src/main/bin/synapse-config-migrator.sh (1)
107-115: LGTM! Correct Java 9+ endorsed JAR handling.The implementation correctly addresses Java 9+ compatibility by prepending endorsed JARs to the classpath instead of using the removed
-Djava.endorsed.dirsoption. The conditional check ensures the directory exists before processing.modules/distribution/src/main/bin/ciphertool.sh (1)
92-98: LGTM! Correct Java 9+ endorsed JAR handling.The implementation correctly addresses Java 9+ compatibility by prepending endorsed JARs to the classpath. The conditional check ensures the directory exists before processing.
modules/transports/optional/fix/pom.xml (1)
26-26: LGTM! Parent version bump and PowerMock removal aligned with JDK 21 support.The parent version update to 4.1.0-wso2v1-SNAPSHOT and the removal of PowerMock test dependencies are consistent with the project-wide migration to JDK 21 and Mockito-based testing. The retained
mockito-coredependency (lines 172-174) provides the necessary mocking capabilities.modules/features/org.apache.synapse.transport.vfs.smb.feature/pom.xml (1)
22-22: LGTM! Parent version bump aligned with JDK 21 support.The parent version update to 4.1.0-wso2v1-SNAPSHOT is consistent with the project-wide migration.
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
18-19: LGTM! Jakarta JAXB namespace migration is correct.The import migration from
javax.xml.bind.annotationtojakarta.xml.bind.annotationaligns with the Jakarta EE 9+ specification and is necessary for JDK 21 compatibility.modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java (3)
31-33: LGTM! Proper migration from PowerMock to Mockito.The test infrastructure has been correctly updated to use
MockitoJUnitRunnerand Mockito's native mocking capabilities, which are compatible with JDK 21.Also applies to: 46-46
136-148: LGTM! Correct usage of MockedStatic for static method interception.The migration from PowerMockito to Mockito's
MockedStaticcorrectly uses try-with-resources to ensure proper cleanup of the static mock scope. The test logic remains unchanged and continues to validate the aggregated parent collection behavior.
156-165: LGTM! Proper static mocking with automatic resource management.The
MockedStaticusage with try-with-resources ensures the static mocks are properly scoped and cleaned up after the test. The test correctly verifies data collection behavior for payloads and properties.modules/transports/core/pipe/pom.xml (1)
26-26: LGTM! Parent version bump is consistent.The parent version update to
4.1.0-wso2v1-SNAPSHOTaligns with the project-wide upgrade to support JDK 21 compilation.modules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.java (2)
26-28: LGTM! Proper JUnit 3 to JUnit 4 migration.The removal of
extends TestCaseand addition of static assertion imports correctly modernizes the test structure to JUnit 4, which is better supported in newer JDK versions.Also applies to: 33-33
35-83: LGTM! Test methods properly annotated for JUnit 4.All test methods now have
@Testannotations, completing the migration from JUnit 3 to JUnit 4. The test logic and assertions remain unchanged, ensuring consistent validation of theResolverFactorybehavior.modules/tasks/pom.xml (1)
26-26: LGTM! Parent version update is consistent.The parent version bump to
4.1.0-wso2v1-SNAPSHOTaligns with the repository-wide upgrade for JDK 21 support.modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (2)
42-44: LGTM! Proper test infrastructure migration to Mockito.The replacement of PowerMock with
MockitoJUnitRunnerand Mockito's native capabilities ensures compatibility with JDK 21 while maintaining the same test coverage.Also applies to: 55-55
66-104: LGTM! Correct migration to MockedStatic for HttpClientBuilder.The static mocking of
HttpClientBuilder.create()usingMockedStaticwith try-with-resources is the recommended approach in Mockito 3.4+. The test correctly validates OAuth token generation with properly scoped mock lifecycle management.modules/handler/pom.xml (1)
26-26: LGTM! Consistent parent version update.The parent version bump to
4.1.0-wso2v1-SNAPSHOTis consistent with the project-wide upgrade to support JDK 21 compilation.modules/war/pom.xml (1)
26-26: LGTM! Parent version update aligns with project-wide changes.The parent version bump to
4.1.0-wso2v1-SNAPSHOTis consistent with the broader effort to enable JDK 21 compilation support across all modules.modules/packaging/package-skeleton/pom.xml (1)
25-25: LGTM! Parent version alignment for JDK 21 support.The parent POM version update to 4.1.0-wso2v1-SNAPSHOT aligns with the repository-wide upgrade for JDK 21 compilation support.
modules/transports/pom.xml (1)
26-26: LGTM! Consistent parent version update.modules/samples/pom.xml (1)
26-26: LGTM! Parent version aligned with project-wide upgrade.modules/extensions/pom.xml (2)
26-26: LGTM! Parent version update aligns with JDK 21 migration.
56-58: JaCoCo plugin executions removed as intended.The JaCoCo plugin declaration is retained without executions, consistent with the PR objective to update JaCoCo from 0.8.6 to 0.8.12 and remove automatic instrumentation. The Surefire plugin at lines 43-53 still references
jacoco-agent.destfile, which may be managed by parent POM configuration.modules/features/org.apache.synapse.transport.fix.feature/pom.xml (1)
22-22: LGTM! Feature module aligned with parent version upgrade.modules/features/pom.xml (1)
25-25: LGTM! Features parent module version updated consistently.modules/features/org.apache.synapse.transport.nhttp.feature/pom.xml (1)
22-22: LGTM! NHttp feature module parent version aligned.modules/commons/pom.xml (3)
26-26: LGTM! Parent version updated for JDK 21 support.
57-59: JaCoCo plugin configuration updated as intended.The executions block removal aligns with the PR objective to refactor JaCoCo configuration while retaining the plugin declaration for potential parent-managed or manual execution.
237-238: JAXB namespace migration to Jakarta EE verified and complete.JAXB was removed from Java SE as of Java SE 11, and Jakarta EE 9 introduced the package namespace change from javax.xml.bind to jakarta.xml.bind. The dependency change from
javax.xml.bind:jaxb-apitojakarta.xml.bind:jakarta.xml.bind-apiin modules/commons/pom.xml is correct and necessary for JDK 21 compatibility.All source files in the commons module (both main and test sources) have been successfully migrated to use
jakarta.xml.bind.*imports. No remainingjavax.xml.bindreferences found in the commons module or dependent modules. Complete migration of JAXB from javax to jakarta is required since both versions cannot co-exist on the classpath.modules/integration/pom.xml (2)
26-26: LGTM! Parent version aligns with Java 21 migration.The parent version bump to 4.1.0-wso2v1-SNAPSHOT is consistent with the project-wide migration to JDK 21.
295-295: LGTM! Correct JVM memory flags for Java 21.The migration from
-XX:MaxPermSizeto-XX:MetaspaceSizeand-XX:MaxMetaspaceSizeis required for Java 8+ and essential for Java 21 compatibility.modules/core/pom.xml (1)
19-19: LGTM! Parent version consistent with Java 21 migration.The parent version bump to 4.1.0-wso2v1-SNAPSHOT aligns with the project-wide JDK 21 migration and removal of PowerMock/JaCoCo as described in the PR objectives.
pom.xml (6)
29-29: LGTM! Project version bump for JDK 21 release.The version increment to 4.1.0-wso2v1-SNAPSHOT appropriately reflects the major upgrade to Java 21.
221-222: Java 21 compilation enabled - verify module compatibility.The Maven compiler configuration correctly targets Java 21. Ensure all modules and their dependencies are compatible with Java 21, especially regarding removed APIs and runtime changes.
1355-1357: LGTM! Correct JAXB migration to jakarta namespace.The migration from
javax.xml.bindtojakarta.xml.bindis required for Jakarta EE 9+ and Java 21 compatibility. Version 4.0.4 is appropriate for this namespace.
1475-1482: Plugin version updates for Java 21 compatibility.The plugin version updates look appropriate:
- JaCoCo 0.8.12 provides Java 21 support
- Maven Bundle Plugin 5.1.9 adds compatibility improvements
- Maven War Plugin 3.5.0 is a significant upgrade from 2.6
Verify that the major Maven War Plugin upgrade (2.6 → 3.5.0) doesn't introduce breaking changes in WAR packaging configuration.
1598-1598: LGTM! Mockito 5.3.1 supports Java 21.The upgrade from Mockito 3.0.0 to 5.3.1 aligns with the PowerMock removal and provides Java 21 compatibility. Mockito 5.x includes MockedStatic and MockedConstruction as mentioned in the PR summary.
1598-1643: Dependency versions align with Java 21 migration.All property updates are consistent with Java 21 compatibility:
- JaCoCo 0.8.12 officially supports Java 21 (lines 1606-1607)
- Jersey 2.30+ introduces no breaking changes, making the 2.41 upgrade from 2.35 compatible (line 1638)
- JAXB 4.0 for Jakarta XML Binding with Java SE 11+ requirement (lines 1642-1643)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (4)
41-51: LGTM: Clean migration to Mockito's static mocking APIs.The imports correctly use Mockito's
MockedStaticandMockedConstructionfor static and constructor mocking, replacing the previous PowerMock dependencies.
99-122: LGTM: Correct static method stubbing.The
MockedStatic<TargetContext>is properly configured withTargetContext.get()stubbing and scoped with try-with-resources for automatic cleanup.
172-203: LGTM: Excellent use of constructor mocking with verification.The nested try-with-resources correctly manages both
MockedStaticandMockedConstructionlifecycles. The lambda-based constructor stubbing forDefaultConnectionReuseStrategyand the verification ofTargetContext.updateState()demonstrate proper migration to Mockito's advanced mocking capabilities.
66-89: Remove the unused MockedStatic mock.The
MockedStatic<TargetContext>is created but never configured with any stubs. SinceexpectResponseBody=falseis passed to the constructor, the code path that would callTargetContext.get()is never executed. The mock can be safely removed.
| if (language.equals(NASHORN_JAVA_SCRIPT)) { | ||
| this.scriptEngine = createNashornEnginePortable(); | ||
| this.jsEngine = createNashornEnginePortable(); | ||
| } else if (language.equals(GRAAL_JAVA_SCRIPT) || language.equals(JAVA_SCRIPT)) { | ||
| log.warn("NashornJS support been removed. Hence defaulting to GraalVMJs"); | ||
| System.setProperty("polyglot.js.nashorn-compat", "true"); | ||
| language = GRAAL_JAVA_SCRIPT; | ||
| } |
There was a problem hiding this comment.
Fix grammatical error in warning message.
The deprecation warning message contains a grammatical error.
🔎 Proposed fix
- log.warn("NashornJS support been removed. Hence defaulting to GraalVMJs");
+ log.warn("NashornJS support has been removed. Hence defaulting to GraalVMJs");📝 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.
| if (language.equals(NASHORN_JAVA_SCRIPT)) { | |
| this.scriptEngine = createNashornEnginePortable(); | |
| this.jsEngine = createNashornEnginePortable(); | |
| } else if (language.equals(GRAAL_JAVA_SCRIPT) || language.equals(JAVA_SCRIPT)) { | |
| log.warn("NashornJS support been removed. Hence defaulting to GraalVMJs"); | |
| System.setProperty("polyglot.js.nashorn-compat", "true"); | |
| language = GRAAL_JAVA_SCRIPT; | |
| } | |
| if (language.equals(NASHORN_JAVA_SCRIPT)) { | |
| log.warn("NashornJS support has been removed. Hence defaulting to GraalVMJs"); | |
| System.setProperty("polyglot.js.nashorn-compat", "true"); | |
| language = GRAAL_JAVA_SCRIPT; | |
| } |
🤖 Prompt for AI Agents
In
modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java
around lines 665 to 669, the warning message logged for Nashorn deprecation is
grammatically incorrect; update the log.warn call to use a correct sentence such
as "Nashorn JS support has been removed; defaulting to GraalVM JS" (or similar),
ensuring consistent spacing and punctuation, then keep the existing
System.setProperty and language reassignment unchanged.
| @InjectMocks | ||
| PassThroughHttpSender sender; | ||
|
|
||
| private PassThroughConfiguration passThroughConfiguration = PassThroughTestUtils.getPassThroughConfiguration(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all references to passThroughConfiguration in the repository
rg -n 'passThroughConfiguration' --type javaRepository: wso2/wso2-synapse
Length of output: 1418
🏁 Script executed:
#!/bin/bash
# Check how many times passThroughConfiguration appears in PassThroughHttpSenderTest.java specifically
rg -n 'passThroughConfiguration' modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.javaRepository: wso2/wso2-synapse
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Show the content of PassThroughHttpSenderTest.java to verify field usage
cat -n modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.javaRepository: wso2/wso2-synapse
Length of output: 7165
Remove the unused passThroughConfiguration field.
The passThroughConfiguration field at line 78 is initialized but never referenced in any test method. This is dead code and should be removed.
🤖 Prompt for AI Agents
In
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java
around line 78, the private field `passThroughConfiguration` is declared and
initialized but never used; remove that field declaration and its
initialization, and also remove or clean up any now-unused imports or setup code
that only supported this field to keep the test class tidy.
| HttpContext context = mock(HttpContext.class); | ||
| context.setAttribute("CONNECTION_POOL", pool); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the test file and examine the testRead() method
find . -name "TargetResponseTest.java" -type fRepository: wso2/wso2-synapse
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for CONNECTION_POOL usage in the codebase
rg -n "CONNECTION_POOL" --type java -B 2 -A 2Repository: wso2/wso2-synapse
Length of output: 22862
🏁 Script executed:
#!/bin/bash
# Search for getAttribute calls related to context in TargetResponse
rg -n "getAttribute" --type java | grep -i "context\|CONNECTION_POOL"Repository: wso2/wso2-synapse
Length of output: 50373
🏁 Script executed:
#!/bin/bash
# Read the TargetResponseTest.java file to see the complete testRead() method
cat -n ./modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java | sed -n '129,163p'Repository: wso2/wso2-synapse
Length of output: 2427
🏁 Script executed:
#!/bin/bash
# Also check the TargetResponse.read() method to understand what it does
cat -n ./modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetResponse.java | sed -n '180,220p'Repository: wso2/wso2-synapse
Length of output: 2536
Use proper stubbing for getAttribute() instead of calling setAttribute() on the mock.
Calling setAttribute on a Mockito mock has no effect—subsequent calls to getAttribute("CONNECTION_POOL") will return null instead of the pool object. While the code handles this gracefully with a null check, the test should properly stub the getAttribute behavior to test the intended code path.
Replace line 137 with:
when(context.getAttribute("CONNECTION_POOL")).thenReturn(pool);🤖 Prompt for AI Agents
In
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java
around lines 136-137, the test mistakenly calls setAttribute on a Mockito mock
which has no effect; replace that line by stubbing
context.getAttribute("CONNECTION_POOL") to return the pool (use Mockito's
when(...).thenReturn(pool)) so subsequent getAttribute calls return the pool and
exercise the intended code path.
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-core</artifactId> | ||
| <version>${mockito.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-core</artifactId> | ||
| <version>${mockito.version}</version> | ||
| <artifactId>mockito-inline</artifactId> | ||
| <version>5.2.0</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Minor version mismatch between mockito-core and mockito-inline.
The mockito-core dependency uses ${mockito.version} (5.3.1 from line 1598), while mockito-inline is hardcoded to version 5.2.0. These should use the same version to avoid potential compatibility issues.
🔎 Recommended fix: Use property for mockito-inline version
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
- <version>5.2.0</version>
+ <version>${mockito.version}</version>
<scope>test</scope>
</dependency>📝 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.
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-core</artifactId> | |
| <version>${mockito.version}</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-core</artifactId> | |
| <version>${mockito.version}</version> | |
| <artifactId>mockito-inline</artifactId> | |
| <version>5.2.0</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-core</artifactId> | |
| <version>${mockito.version}</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-inline</artifactId> | |
| <version>${mockito.version}</version> | |
| <scope>test</scope> | |
| </dependency> |
🤖 Prompt for AI Agents
In pom.xml around lines 1152 to 1162, the mockito-core and mockito-inline
versions are inconsistent (mockito-core uses ${mockito.version} = 5.3.1 at line
1598 while mockito-inline is hardcoded to 5.2.0); update the mockito-inline
dependency to use the ${mockito.version} property (or otherwise align it to the
same version property) so both mockito artifacts use the same version and avoid
compatibility issues.
There was a problem hiding this comment.
Pull request overview
This PR introduces JDK 21 compilation support for Apache Synapse, migrating from JDK 8. The changes include updating the Java compiler target to version 21, migrating the testing framework from PowerMockito to Mockito, replacing the NashornJS engine with GraalVM JavaScript, and updating various dependencies to Java 21-compatible versions.
Key Changes:
- Migrated test mocking framework from PowerMockito to Mockito 5.3.1 with inline mocking support
- Removed NashornJS implementation and defaulted to GraalVM JavaScript engine
- Updated JAXB API from javax.xml.bind to jakarta.xml.bind for Jakarta EE compatibility
- Removed
-Djava.endorsed.dirsJVM argument from shell scripts (deprecated in Java 9+) and added endorsed JARs directly to classpath - Updated plugin versions (Jacoco 0.8.6→0.8.12, Maven War 2.6→3.5.0, Maven Bundle 3.3.0→5.1.9, Glassfish Jersey 2.35→2.41)
- Removed two test files (RequestResponseTest.java and NHttpTransportListenerTest.java) due to Mockito migration limitations
Reviewed changes
Copilot reviewed 71 out of 75 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updated version to 4.1.0-wso2v1-SNAPSHOT, Java compiler target to 21, replaced PowerMock with Mockito, updated jakarta.xml.bind-api, upgraded plugin versions |
| modules/transports/optional/fix/src/main/java/org/apache/synapse/transport/fix/FIXTransportSender.java | Changed sendUsingEPR method visibility from private to protected to enable Mockito spying in tests |
| modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java | Removed NashornJS support, added fallback to GraalVM with nashorn-compat mode |
| modules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.java | Added JNDI Reference dereferencing for Java 9+ compatibility |
| modules/distribution/src/main/bin/*.sh | Removed java.endorsed.dirs usage and added endorsed JARs to classpath for Java 9+ compatibility |
| Test files (multiple) | Migrated from PowerMockRunner to MockitoJUnitRunner, replaced PowerMockito.mockStatic with Mockito.mockStatic |
| modules/commons/src//java/org/apache/synapse/commons/staxon/core/json/jaxb/.java | Updated JAXB imports from javax.xml.bind to jakarta.xml.bind |
| modules/integration/pom.xml | Updated JVM args from MaxPermSize to MetaspaceSize for Java 8+ |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Create a mock SynapseEnvironment object | ||
| * | ||
| * @return Axis2SynapseEnvironment instance | ||
| * @throws AxisFault on creating/mocking object | ||
| */ | ||
| /** | ||
| * Create a mock SynapseEnvironment object | ||
| * | ||
| * @return Axis2SynapseEnvironment instance | ||
| * @throws AxisFault on creating/mocking object | ||
| */ |
There was a problem hiding this comment.
The JavaDoc comment for the getMockedSynapseEnvironment method is duplicated. Lines 172-177 and 178-183 contain identical JavaDoc blocks. Remove one of the duplicate comment blocks.
| @Test | ||
| public void testSystemResolver() { | ||
| String synapseVariable = "$SYSTEM:VAR"; | ||
| String envValue = "https://localhost:8080/service"; | ||
| Resolver resolver = ResolverFactory.getInstance().getResolver(synapseVariable); | ||
| assertNotNull(resolver); | ||
| assertTrue(resolver instanceof SystemResolver); | ||
| EnvironmentVariables environmentVariables = new EnvironmentVariables(); | ||
| environmentVariables.set("VAR", envValue); | ||
| assertEquals(envValue, resolver.resolve()); | ||
| } |
There was a problem hiding this comment.
The test method testSystemResolver has been modified to remove the actual test assertions that verify the resolution behavior. The original test set an environment variable "VAR" to "https://localhost:8080/service" and verified that resolver.resolve() returned this value. Now the test only verifies that the resolver is created and is an instance of SystemResolver, but doesn't test the actual resolution functionality. This significantly reduces test coverage for the SystemResolver class.
| throw new SynapseCommonsException("Error looking up DataSource : " + dsName | ||
| + " using JNDI" + " properties : " + context, e); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
The JNDI datasource lookup logic now includes automatic dereferencing for javax.naming.Reference objects, but the catch block for the generic Exception is problematic. It wraps the exception in a RuntimeException without providing any context or logging. This could hide important debugging information. Consider either logging the exception before rethrowing it, or wrapping it in a SynapseCommonsException with an appropriate message, similar to the NamingException handler above it.
| throw new RuntimeException(e); | |
| throw new SynapseCommonsException("Unexpected error looking up DataSource : " + dsName | |
| + " using JNDI properties : " + context, e, log); |
| this.scriptEngine = createNashornEnginePortable(); | ||
| this.jsEngine = createNashornEnginePortable(); | ||
| } else if (language.equals(GRAAL_JAVA_SCRIPT) || language.equals(JAVA_SCRIPT)) { | ||
| log.warn("NashornJS support been removed. Hence defaulting to GraalVMJs"); |
There was a problem hiding this comment.
The warning message has a grammatical error: "NashornJS support been removed" should be "NashornJS support has been removed".
| if (language.equals(NASHORN_JAVA_SCRIPT)) { | ||
| this.scriptEngine = createNashornEnginePortable(); | ||
| this.jsEngine = createNashornEnginePortable(); | ||
| } else if (language.equals(GRAAL_JAVA_SCRIPT) || language.equals(JAVA_SCRIPT)) { | ||
| log.warn("NashornJS support been removed. Hence defaulting to GraalVMJs"); | ||
| System.setProperty("polyglot.js.nashorn-compat", "true"); | ||
| language = GRAAL_JAVA_SCRIPT; | ||
| } |
There was a problem hiding this comment.
The polyglot.js.nashorn-compat system property is now set only when the language is NashornJS and it's being defaulted to GraalVM. However, this property was previously set unconditionally at line 659 (now removed). This means if users explicitly configure the language as "js" or "graaljs", the nashorn-compat mode won't be enabled, which could potentially break existing scripts that rely on Nashorn compatibility. Consider setting this property for all JavaScript engines (js and graaljs) to maintain backward compatibility, or document this change clearly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (1)
137-137: Use proper stubbing for getAttribute() instead of calling setAttribute() on the mock.Calling
setAttribute()on a Mockito mock has no effect—subsequent calls togetAttribute("CONNECTION_POOL")will returnnullinstead of the pool object. Replace line 137 with proper stubbing:when(context.getAttribute("CONNECTION_POOL")).thenReturn(pool);Context from past review
This issue was previously identified but remains unresolved. The test should stub the
getAttribute()behavior to exercise the intended code path where the connection pool is retrieved from the context.modules/distribution/src/main/bin/synapse-config-migrator.sh (1)
105-105: Dead code: remove unusedJAVA_ENDORSED_DIRSconversion.This line converts a variable that is never defined or used. The past review comment correctly identified this should be removed.
🔎 Proposed fix
CLASSPATH=`cygpath --path --windows "$CLASSPATH"` - JAVA_ENDORSED_DIRS=`cygpath --path --windows "$JAVA_ENDORSED_DIRS"` fimodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java (1)
665-669: Fix grammatical error in warning message.The deprecation warning contains a grammatical error that was previously flagged.
🔎 Proposed fix
- log.warn("NashornJS support been removed. Hence defaulting to GraalVMJs"); + log.warn("NashornJS support has been removed. Hence defaulting to GraalVMJs");pom.xml (1)
1159-1160: Version mismatch between mockito-core and mockito-inline (already flagged).modules/integration/pom.xml (1)
457-474: CRITICAL: Mixing javax and jakarta JAXB namespaces breaks Java 21 migration.This issue was already identified in a previous review. These dependencies use the old
javax.xml.bindnamespace instead ofjakarta.xml.bind, which conflicts with the Jakarta migration in the root POM and will cause class loading failures.
🧹 Nitpick comments (15)
modules/integration/src/test/java/org/apache/synapse/samples/framework/TestSamplesHandlerSuite.java (2)
28-29: Consider removing unused imports.Since Sample353 and Sample354 are no longer registered in the sample class repository (lines 277-278), these imports are now unused. If these samples won't be re-enabled soon, consider removing them to keep the import list clean.
🔎 Proposed fix
-import org.apache.synapse.samples.framework.tests.advanced.Sample353; -import org.apache.synapse.samples.framework.tests.advanced.Sample354;
277-278: Consider adding an explanatory comment.The change correctly deactivates these samples as part of the JDK 21 migration. Adding an inline comment explaining why these Ruby-related samples are disabled would help future maintainers understand the context without referring back to the PR.
🔎 Proposed enhancement
+ // Samples 353 and 354 disabled - Ruby scripting not supported in JDK 21 // sampleClassRepo.put("353", Sample353.class); // sampleClassRepo.put("354", Sample354.class);modules/transports/optional/fix/src/main/java/org/apache/synapse/transport/fix/FIXTransportSender.java (1)
277-278: Visibility change to support Mockito-based testing is acceptable given JDK 21 migration.The method visibility change from
privatetoprotectedenables direct test invocation via Mockito spies, replacing PowerMockito which has compatibility issues with JDK 21. While modifying production code for testing purposes is generally a trade-off between testability and encapsulation, this approach is pragmatic given:
- PowerMockito's incompatibility with newer JDK versions
- The method contains substantial logic (~43 lines) that could legitimately be customized by subclasses
- Alternative approaches (extracting to a separate testable component, or testing exclusively through the public
sendMessageAPI) would require more extensive refactoringThe expanded API surface is minimal, and the behavior remains unchanged.
modules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.java (1)
89-89: Useany(MessageContext.class)instead of casting.The pattern
((MessageContext) any())is non-idiomatic. Prefer the type-specific matcher for clarity.🔎 Suggested refactor
- Mockito.when(value.evaluateValue(((MessageContext) any()))).thenReturn(helloKey); + Mockito.when(value.evaluateValue(any(MessageContext.class))).thenReturn(helloKey);
modules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.java (1)
57-62: Consider using a logging framework and documenting the skip behavior.The runtime guard is a pragmatic solution for handling Ruby engine unavailability on JDK 21. However, there are two areas for improvement:
- Replace
System.out.printlnwith a proper logging framework (e.g., SLF4J) for consistency with the rest of the codebase.- The early return causes the test to pass silently, which may give false confidence in test coverage metrics. Consider adding a comment explaining this behavior or migrating to JUnit 4+ where
Assume.assumeTrue()provides better skip semantics.💡 Suggested improvements
Option 1: Add logging (if SLF4J is available in the test classpath)
- // Skip this test if Ruby script engine is not available (e.g., on JDK 21 with old JRuby) - ScriptEngineManager manager = new ScriptEngineManager(); - if (manager.getEngineByName("rb") == null && manager.getEngineByName("ruby") == null) { - System.out.println("Skipping testInlineScriptMediatorSerializationScenarioTwo - Ruby script engine not available"); - return; - } + // Skip this test if Ruby script engine is not available (e.g., on JDK 21 with old JRuby) + // Note: This causes the test to pass silently. Consider migrating to JUnit 4+ for proper skip reporting. + ScriptEngineManager manager = new ScriptEngineManager(); + if (manager.getEngineByName("rb") == null && manager.getEngineByName("ruby") == null) { + log.info("Skipping testInlineScriptMediatorSerializationScenarioTwo - Ruby script engine not available"); + return; + }Option 2: Add explanatory comment about silent skip
- // Skip this test if Ruby script engine is not available (e.g., on JDK 21 with old JRuby) + // Skip this test if Ruby script engine is not available (e.g., on JDK 21 with old JRuby). + // Note: JUnit 3 lacks proper skip mechanisms, so this test will pass silently when Ruby is unavailable. ScriptEngineManager manager = new ScriptEngineManager();
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (1)
58-73: LGTM! Correct migration to MockedStatic.The try-with-resources pattern properly manages the
MockedStaticlifecycle, and the stubbing syntax is correct. The test logic is preserved from the PowerMock version.Optional: Consider adding verification
You could optionally verify that the static method was actually invoked:
targetHandler.requestReady(conn); + targetContextMock.verify(() -> TargetContext.getState(any(NHttpClientConnection.class))); }This would make the test more explicit about its expectations, though the current smoke-test approach is also valid.
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (1)
73-74: Consider reducing reliance on deep stubs.
RETURNS_DEEP_STUBSis used extensively throughout the tests. While functional, deep stubs can mask unclear test setup and make tests harder to understand. Consider explicitly stubbing the chained calls instead:// Instead of: NHttpClientConnection conn = mock(NHttpClientConnection.class, Mockito.RETURNS_DEEP_STUBS); when(conn.getContext().getAttribute("http.request")).thenReturn(httpRequest); // Consider: HttpContext context = mock(HttpContext.class); NHttpClientConnection conn = mock(NHttpClientConnection.class); when(conn.getContext()).thenReturn(context); when(context.getAttribute("http.request")).thenReturn(httpRequest);This makes the test's dependencies and interactions more explicit.
Also applies to: 107-108, 142-143, 185-186
modules/distribution/src/main/bin/synapse-config-migrator.sh (1)
107-115: Correct approach for Java 9+ endorsed JAR handling.The implementation correctly prepends endorsed JARs to the classpath instead of using the deprecated
-Djava.endorsed.dirsflag. The comments clearly document the change.Minor edge case: if
$SYNAPSE_ENDORSEDexists but contains no.jarfiles, the glob pattern won't expand and the literal string$SYNAPSE_ENDORSED/*.jargets added to the classpath. While Java typically ignores invalid classpath entries, you could prevent this by checking file existence within the loop.🔎 Optional: Guard against empty endorsed directory
if [ -d "$SYNAPSE_ENDORSED" ]; then for f in $SYNAPSE_ENDORSED/*.jar do + [ -f "$f" ] || continue SYNAPSE_CLASSPATH=$f:$SYNAPSE_CLASSPATH done fi
modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java (3)
784-785: Remove dead code: Nashorn check is unreachable.At line 668,
NASHORN_JAVA_SCRIPTis redirected toGRAAL_JAVA_SCRIPT, so this condition will never be true whengetScriptEngineSupplier()is called from line 686.🔎 Proposed fix
final Supplier<ScriptEngine> engineSupplier; if (language.equals(GRAAL_JAVA_SCRIPT) || language.equals(JAVA_SCRIPT)) { engineSupplier = () -> GraalJSScriptEngine.create( null, AccessControlUtils.createSecureGraalContext(classAccessControlConfig) ); - } else if (language.equals(NASHORN_JAVA_SCRIPT)) { - engineSupplier = () -> createNashornEnginePortable(); } else if (language.equals(RHINO_JAVA_SCRIPT)) { engineSupplier = () -> engineManager.getEngineByExtension("jsEngine"); } else {
835-836: Remove dead code: Nashorn check is unreachable.Since
NASHORN_JAVA_SCRIPTis redirected toGRAAL_JAVA_SCRIPTduring initialization (line 668), this condition will never be true.🔎 Proposed fix
ScriptEngineWrapper scriptEngineWrapper = pool.poll(); if (scriptEngineWrapper == null) { if (language.equals(GRAAL_JAVA_SCRIPT) || language.equals(JAVA_SCRIPT)) { scriptEngineWrapper = new ScriptEngineWrapper( GraalJSScriptEngine.create(null, AccessControlUtils.createSecureGraalContext(classAccessControlConfig)) ); - } else if (language.equals(NASHORN_JAVA_SCRIPT)) { - scriptEngineWrapper = new ScriptEngineWrapper(createNashornEnginePortable()); } else if (language.equals(RHINO_JAVA_SCRIPT)) { scriptEngineWrapper = new ScriptEngineWrapper(engineManager.getEngineByExtension("jsEngine")); } else {
720-759: Consider removing unused Nashorn engine creation method.If the dead code at lines 784-785 and 835-836 is removed as suggested, the
createNashornEnginePortable()method will have no callers and can be deleted.
modules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.java (1)
73-82: Consider using imports instead of fully qualified names.The JNDI Reference dereferencing logic is correct and handles cases where JNDI returns a Reference object that needs resolution. However, the code uses fully qualified names (
javax.naming.Reference,javax.naming.spi.NamingManager) instead of imports, which is inconsistent with the rest of the file.🔎 Proposed refactor to use imports
Add these imports at the top of the file (after line 27):
import javax.naming.Context; import javax.naming.InitialContext; import javax.naming.NamingException; +import javax.naming.Reference; +import javax.naming.spi.NamingManager; import javax.sql.DataSource;Then simplify the code:
if (dataSourceO != null) { // Let JNDI automatically dereference if needed - if (dataSourceO instanceof javax.naming.Reference) { - dataSourceO = javax.naming.spi.NamingManager.getObjectInstance( + if (dataSourceO instanceof Reference) { + dataSourceO = NamingManager.getObjectInstance( dataSourceO, null, null, context.getEnvironment()); } if (dataSourceO instanceof DataSource) { return (DataSource) dataSourceO; } }
modules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.java (1)
21-21: Consider modernizing JUnit assertions.The test uses the deprecated
junit.framework.Assertfrom JUnit 3/4. While functional, consider updating toorg.junit.Assert(JUnit 4) ororg.junit.jupiter.api.Assertions(JUnit 5) for better maintainability.🔎 Proposed refactor to JUnit 4 style
-import junit.framework.Assert; +import org.junit.Assert;
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (1)
65-65: Remove redundantinitMocks(this)call.With
@RunWith(MockitoJUnitRunner.class)at the class level, mocks are automatically initialized before each test method. This explicit call is unnecessary.🔎 Proposed fix
@Test public void testToAdmin() throws Exception { - initMocks(this); SessionID id = new SessionID(new BeginString("FIX.4.1"), new SenderCompID("SYNAPSE"), new TargetCompID("BANZAI"), "FIX.4.1:SYNAPSE->BANZAI");
modules/transports/core/nhttp/pom.xml (1)
243-258: Duplicatemockito-coredependency.There are two
mockito-coredependency entries (lines 244-247 and 253-257). While Maven will deduplicate them, the second entry should be removed to keep the POM clean.🔎 Proposed fix
<dependency> <groupId>org.mockito</groupId> <artifactId>mockito-inline</artifactId> <scope>test</scope> </dependency> - <dependency> - <groupId>org.mockito</groupId> - <artifactId>mockito-core</artifactId> - <scope>test</scope> - </dependency> </dependencies>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
modules/commons/pom.xmlmodules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.javamodules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.javamodules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLMapper.javamodules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProvider.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/ObjectFactory.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.javamodules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.javamodules/core/jacoco.execmodules/core/pom.xmlmodules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.javamodules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.javamodules/core/src/test/java/org/apache/synapse/core/axis2/ProxyServiceMessageReceiverTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/DynamicLoadBalanceEndpointTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/HttpEndpointTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/RecipientListEndpointTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.javamodules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.javamodules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.javamodules/core/src/test/java/org/apache/synapse/message/store/RabbitMQStoreTest.javamodules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.javamodules/coverage-report/pom.xmlmodules/distribution/pom.xmlmodules/distribution/src/main/bin/ciphertool.shmodules/distribution/src/main/bin/synapse-config-migrator.shmodules/distribution/src/main/bin/synapse.shmodules/experimental/pom.xmlmodules/extensions/pom.xmlmodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.javamodules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.javamodules/features/org.apache.synapse.samples.feature/pom.xmlmodules/features/org.apache.synapse.transport.fix.feature/pom.xmlmodules/features/org.apache.synapse.transport.nhttp.feature/pom.xmlmodules/features/org.apache.synapse.transport.vfs.feature/pom.xmlmodules/features/org.apache.synapse.transport.vfs.smb.feature/pom.xmlmodules/features/org.apache.synapse.transport.vfs.smb2.feature/pom.xmlmodules/features/org.apache.synapse.wso2.feature/pom.xmlmodules/features/pom.xmlmodules/handler/pom.xmlmodules/integration/pom.xmlmodules/integration/src/test/java/org/apache/synapse/samples/framework/TestSamplesHandlerSuite.javamodules/migrator/pom.xmlmodules/packaging/package-archetype/pom.xmlmodules/packaging/package-skeleton/pom.xmlmodules/patches/pom.xmlmodules/samples/pom.xmlmodules/samples/src/main/scripts/axis2Server/axis2server.shmodules/securevault/pom.xmlmodules/tasks/pom.xmlmodules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.javamodules/transports/core/nhttp/pom.xmlmodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/nhttp/NHttpTransportListenerTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/DeliveryAgentTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/RequestResponseTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.javamodules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/connections/HostConnectionsTest.javamodules/transports/core/nhttp/src/test/resources/log4j.propertiesmodules/transports/core/pipe/pom.xmlmodules/transports/core/vfs/pom.xmlmodules/transports/optional/fix/pom.xmlmodules/transports/optional/fix/src/main/java/org/apache/synapse/transport/fix/FIXTransportSender.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.javamodules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.javamodules/transports/pom.xmlmodules/war/pom.xmlmodules/xar-maven-plugin/pom.xmlpom.xml
💤 Files with no reviewable changes (3)
- modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.java
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/RequestResponseTest.java
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/nhttp/NHttpTransportListenerTest.java
🚧 Files skipped from review as they are similar to previous changes (34)
- modules/core/src/test/java/org/apache/synapse/message/store/RabbitMQStoreTest.java
- modules/transports/core/vfs/pom.xml
- modules/core/src/test/java/org/apache/synapse/endpoints/RecipientListEndpointTest.java
- modules/patches/pom.xml
- modules/coverage-report/pom.xml
- modules/packaging/package-skeleton/pom.xml
- modules/handler/pom.xml
- modules/features/org.apache.synapse.transport.vfs.smb2.feature/pom.xml
- modules/xar-maven-plugin/pom.xml
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/PassThroughHttpSenderTest.java
- modules/core/src/test/java/org/apache/synapse/core/axis2/ProxyServiceMessageReceiverTest.java
- modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXOutgoingMessageHandlerTest.java
- modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProvider.java
- modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportListenerTest.java
- modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/connections/HostConnectionsTest.java
- modules/samples/src/main/scripts/axis2Server/axis2server.sh
- modules/transports/optional/fix/pom.xml
- modules/features/org.apache.synapse.transport.vfs.smb.feature/pom.xml
- modules/core/src/test/java/org/apache/synapse/endpoints/resolvers/ResolverFactoryTest.java
- modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java
- modules/core/src/test/java/org/apache/synapse/aspects/flow/statistics/util/StatisticDataCollectionHelperTest.java
- modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java
- modules/distribution/src/main/bin/ciphertool.sh
- modules/experimental/pom.xml
- modules/securevault/pom.xml
- modules/distribution/src/main/bin/synapse.sh
- modules/transports/core/nhttp/src/test/resources/log4j.properties
- modules/features/org.apache.synapse.samples.feature/pom.xml
- modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/ObjectFactory.java
- modules/features/org.apache.synapse.transport.fix.feature/pom.xml
- modules/samples/pom.xml
- modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXTransportSenderTest.java
- modules/features/org.apache.synapse.wso2.feature/pom.xml
- modules/packaging/package-archetype/pom.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T11:12:52.620Z
Learnt from: shilmyhasan
Repo: wso2/wso2-synapse PR: 2427
File: modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java:268-290
Timestamp: 2025-12-12T11:12:52.620Z
Learning: In modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java, when proxy is not enabled, NoopHostnameVerifier.INSTANCE is intentionally used for backward compatibility with the previous implementation, even though it disables hostname verification.
Applied to files:
modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java
🧬 Code graph analysis (6)
modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.java (4)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java (1)
XmlRootElement(26-34)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetContext.java (1)
TargetContext(34-258)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (2)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (2)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
modules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.java (1)
modules/core/src/main/java/org/apache/synapse/config/Entry.java (1)
Entry(35-251)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.java (4)
modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleRootElement.java (1)
XmlRootElement(26-34)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
XmlType(58-60)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleType.java (1)
XmlType(21-25)modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
XmlType(21-25)
🔇 Additional comments (54)
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/DeliveryAgentTest.java (2)
31-31: LGTM!The static import for Mockito's
mockmethod is the idiomatic approach and correctly replaces the previous PowerMockito usage.
49-51: LGTM!The migration from
PowerMockito.mock(...)tomock(...)is correct. The test logic remains unchanged, and the mocks correctly default to null returns, which aligns with the test's intent to verify behavior when the connection is null.modules/core/src/test/java/org/apache/synapse/endpoints/DynamicLoadBalanceEndpointTest.java (2)
76-77: LGTM! Correct migration from PowerMockito to Mockito.The migration of the LoadBalanceMembershipHandler mock from PowerMockito to Mockito is correct and appropriate for the JDK 21 compatibility upgrade.
103-103: LGTM! Correct migration from PowerMockito to Mockito.The migration of the Axis2SynapseEnvironment mock from PowerMockito to Mockito is correct and aligns with the broader JDK 21 compatibility changes in this PR.
modules/core/src/test/java/org/apache/synapse/util/ResourceMapTest.java (4)
28-28: LGTM! Correct imports for Mockito migration.The imports are appropriate for the PowerMockito → Mockito migration.
Also applies to: 30-30
65-65: LGTM! Correct migration to Mockito.The change from PowerMockito to Mockito is correct.
76-77: LGTM! Correct migration to Mockito.The changes from PowerMockito to Mockito are correct.
88-88: LGTM! Correct migration to Mockito.The change from PowerMockito to Mockito is correct.
modules/extensions/src/test/java/org/apache/synapse/mediators/bsf/ScriptMediatorSerializationTest.java (1)
24-24: LGTM!The import is necessary for the runtime Ruby engine availability check added below.
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetHandlerTest.java (3)
38-44: LGTM! Correct imports for Mockito static mocking.The import additions properly support the migration from PowerMock to Mockito's
MockedStaticAPI.
81-112: LGTM! Correct migration with proper scoping.The migration correctly uses
MockedStaticwith try-with-resources. The approach of creating a realTargetContextinstance (line 93) and returning it from the mocked staticgetmethod (line 101) is a valid pattern for testing with partial static mocking.
120-149: LGTM! Consistent migration pattern.The migration follows the same correct pattern as
testOutputReady, usingMockedStaticwith try-with-resources and mixing real instances with mocked static getters. The stubbing ofTargetContext.get,getState, andgetResponseis properly scoped and syntactically correct.modules/core/src/test/java/org/apache/synapse/mediators/bean/Enterprise/EJBMediatorTest.java (3)
36-38: LGTM! Clean migration from PowerMock to Mockito.The import updates and runner change correctly establish Mockito-based test infrastructure compatible with JDK 21.
Also applies to: 49-49
87-93: LGTM! Standard Mockito mocking patterns.The instance mocking and stubbing follow idiomatic Mockito conventions.
95-105: Correct use of MockedStatic with try-with-resources.The static method mocking follows Mockito's recommended pattern. The try-with-resources ensures the static mock is properly scoped and cleaned up after the test block completes.
Note: The static
ejbMediatorfield (line 56) is mutated during this test. While acceptable with a single test method, if additional tests are added later, consider using a non-static field or resetting state to avoid test interdependencies.modules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.java (1)
40-40: LGTM!Import change from PowerMockito to Mockito is correct and aligns with the JDK 21 migration objective.
modules/transports/core/nhttp/src/test/java/org/apache/synapse/transport/passthru/TargetResponseTest.java (3)
65-90: Migration to Mockito looks correct.The test properly uses
MockedStaticwith try-with-resources to ensure cleanup. The stubbing on line 79 correctly handles the chained call via deep stubs.
98-122: Correct static method stubbing.The migration properly uses
targetContextMock.when()on line 111 to stub the staticTargetContext.get()method. The try-with-resources pattern ensures the static mock is cleaned up.
171-203: Excellent use of MockedConstruction for constructor mocking.The test properly demonstrates Mockito's
MockedConstructionfeature (lines 173-177) to intercept theDefaultConnectionReuseStrategyconstructor and stub itskeepAlive()method. The verification on lines 195-196 correctly ensures the static method was called exactly once.modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ScriptMediator.java (3)
435-444: LGTM: Nashorn handling correctly removed from context creation.The method now properly delegates JavaScript languages to GraalVM-based contexts only, consistent with the Nashorn deprecation strategy.
481-486: LGTM: JSON payload processing correctly updated for GraalVM.The conditional properly routes JavaScript languages to GraalVM-based JSON parsing, maintaining backward compatibility for other scripting languages.
671-673: LGTM: Engine initialization correctly targets GraalVM for JavaScript.The condition properly initializes GraalJS engines for JavaScript languages after the Nashorn redirection.
modules/tasks/src/test/java/org/apache/synapse/task/service/TaskSchedulerTest.java (2)
31-31: LGTM: PowerMock removal successful.The removal of PowerMock's
@PrepareForTestimport and annotation is appropriate for this test class, as it only mocks instance methods ofTaskManagerwhich standard Mockito handles well.
38-87: Successful migration from PowerMock to Mockito.The test class successfully migrates to standard Mockito without functionality loss. All three test methods properly mock
TaskManagerinstance methods usingMockito.mock()andMockito.when(), eliminating the need for PowerMock. No PowerMock imports or annotations remain in the code.modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java (2)
32-33: LGTM! Necessary imports for Mockito migration.The addition of
MockitoandMockitoJUnitRunnerimports correctly supports the migration from PowerMock to standard Mockito.
45-45: LGTM! Correct migration to MockitoJUnitRunner.The switch from
PowerMockRunnertoMockitoJUnitRunneris appropriate for the PowerMock migration. Note that this runner automatically initializes@Mockannotated fields, making theinitMocks(this)call at line 65 redundant.modules/transports/core/pipe/pom.xml (1)
26-26: LGTM! Parent version bump is consistent.The parent version update to
4.1.0-wso2v1-SNAPSHOTaligns with the repository-wide version uplift for JDK 21 support.modules/features/pom.xml (1)
25-25: LGTM! Parent version bump is consistent.The parent version update to
4.1.0-wso2v1-SNAPSHOTaligns with the repository-wide version uplift for JDK 21 support.modules/core/src/test/java/org/apache/synapse/endpoints/HttpEndpointTest.java (1)
162-162: LGTM! PowerMock to Mockito migration is correct.The change from
PowerMockito.mock()toMockito.mock()is appropriate here. For standard interface mocking (Axis2SynapseEnvironment), Mockito's standard mock capabilities are sufficient. PowerMock is only needed for mocking static methods, final classes, or private methods, none of which apply in this test.modules/migrator/pom.xml (1)
27-27: LGTM! Parent version bump is consistent.The parent version update to
4.1.0-wso2v1-SNAPSHOTaligns with the repository-wide version uplift for JDK 21 support.modules/features/org.apache.synapse.transport.vfs.feature/pom.xml (1)
22-22: LGTM! Parent version bump is consistent.The parent version update to
4.1.0-wso2v1-SNAPSHOTaligns with the repository-wide version uplift for JDK 21 support.modules/features/org.apache.synapse.transport.nhttp.feature/pom.xml (1)
22-22: LGTM! Parent version bump is consistent.The parent version update to
4.1.0-wso2v1-SNAPSHOTaligns with the repository-wide version uplift for JDK 21 support.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/sample/SampleTypeWithNamespace.java (1)
18-19: LGTM! JAXB namespace migration is correct.The migration from
javax.xml.bind.annotationtojakarta.xml.bind.annotationis required for JDK 11+ compatibility. The Jakarta JAXB annotations are functionally identical to their javax counterparts.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLRootProviderTest.java (1)
21-25: LGTM! JAXB namespace migration is correct.The migration from
javax.xml.bindtojakarta.xml.bindis required because JAXB was removed as of Java SE 11. The Jakarta JAXB API is the direct successor with identical signatures. The project's configured version, jakarta.xml.bind-api 4.0.4, is fully compatible with JDK 21 (which requires Java SE 11 or higher support).modules/core/src/test/java/org/apache/synapse/endpoints/auth/oauth/OAuthClientTest.java (1)
64-104: Clean migration from PowerMockito to Mockito.The test correctly uses
MockedStatic<HttpClientBuilder>with try-with-resources to scope the static mock, ensuring proper cleanup. The mock chain forHttpClientBuilderis well-structured, and the test logic is preserved.pom.xml (3)
1536-1537: Verify axis2 version downgrade is intentional.The
axis2.versionchanged from1.6.1-wso2v117to1.6.1-wso2v114andaxis2.transport.versionfrom2.0.0-wso2v80to2.0.0-wso2v79. These appear to be version downgrades. Please confirm this is intentional and required for JDK 21 compatibility.
221-222: Java 21 compilation configuration looks correct.The
maven-compiler-pluginis properly configured with source and target set to21for JDK 21 support.
1354-1358: JAXB to Jakarta migration looks correct.The migration from
javax.xml.bind:jaxb-apitojakarta.xml.bind:jakarta.xml.bind-apiis appropriate for JDK 21 compatibility, as thejavax.xml.bindnamespace is no longer included in the JDK.modules/war/pom.xml (1)
23-27: Parent version update looks correct.Standard parent POM version bump to
4.1.0-wso2v1-SNAPSHOTas part of the JDK 21 migration.modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinder.java (1)
24-29: JAXB to Jakarta namespace migration is correct.The imports are properly migrated from
javax.xml.bind.*tojakarta.xml.bind.*. This aligns with the POM dependency change fromjaxb-apitojakarta.xml.bind-apiand is required for JDK 21 compatibility wherejavax.xml.bindis no longer available.modules/commons/src/test/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLBinderTest.java (1)
26-30: Test file JAXB imports correctly migrated to Jakarta namespace.The test imports are consistently migrated to
jakarta.xml.bind.*, matching the changes in the mainJsonXMLBinder.javaclass.modules/transports/pom.xml (1)
23-27: Parent version update looks correct.Standard parent POM version bump to
4.1.0-wso2v1-SNAPSHOTconsistent with other modules in this PR.modules/distribution/pom.xml (1)
22-27: Parent version update looks correct.Standard parent POM version bump to
4.1.0-wso2v1-SNAPSHOTconsistent with other modules in this PR.modules/extensions/pom.xml (2)
26-26: LGTM: Parent version bump aligns with JDK 21 migration.The parent version update to 4.1.0-wso2v1-SNAPSHOT is consistent with the project-wide baseline shift for Java 21 support.
55-58: LGTM: JaCoCo plugin configuration simplified.The removal of JaCoCo execution blocks (instrumentation, restore, reporting) is consistent with similar changes across other modules (commons, core) as part of the build configuration cleanup.
modules/integration/pom.xml (2)
26-26: LGTM: Parent version bump aligns with JDK 21 migration.The parent version update is consistent across all modules in this PR.
295-295: LGTM: Metaspace configuration updated for Java 9+.The argLine update replaces deprecated PermSize parameters with MetaspaceSize, which is correct for Java 9 and later.
modules/commons/src/main/java/org/apache/synapse/commons/staxon/core/json/jaxb/JsonXMLMapper.java (1)
28-29: LGTM: JAXB namespace correctly migrated to Jakarta.The import updates from
javax.xml.bindtojakarta.xml.bindare correct for Jakarta EE 9+ and align with the dependency change tojakarta.xml.bind-apiin modules/commons/pom.xml.modules/commons/pom.xml (2)
26-26: LGTM: Parent version bump is consistent.The version update aligns with the project-wide migration to 4.1.0-wso2v1-SNAPSHOT.
237-238: LGTM: JAXB dependency correctly migrated to Jakarta namespace.The dependency change from
javax.xml.bind:jaxb-apitojakarta.xml.bind:jakarta.xml.bind-apiis correct and aligns with the code changes in JsonXMLMapper.java and related classes.modules/tasks/pom.xml (2)
26-26: LGTM: Parent version bump is consistent.The version update to 4.1.0-wso2v1-SNAPSHOT aligns with other modules.
56-81: Verify JaCoCo execution retention is intentional.This module retains the jacoco-maven-plugin executions (instrument, restore-instrumented-classes, report) while extensions, commons, and core modules have the plugin declared without executions. Confirm whether this difference is intentional or if these executions should also be removed here for consistency.
modules/core/pom.xml (2)
19-19: LGTM: Parent version bump is consistent.The version update to 4.1.0-wso2v1-SNAPSHOT aligns with the project-wide JDK 21 migration baseline.
108-110: LGTM: JaCoCo plugin configuration simplified.The plugin declaration without executions is consistent with the cleanup pattern observed in modules/extensions/pom.xml and modules/commons/pom.xml.
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Improve exception handling to preserve context.
The generic Exception catch wraps the exception in a RuntimeException without preserving the error message or logging the issue. This makes debugging difficult and loses valuable diagnostic information when NamingManager.getObjectInstance() throws non-NamingException errors.
🔎 Proposed fix to improve exception handling
} catch (NamingException e) {
throw new SynapseCommonsException("Error looking up DataSource : " + dsName
+ " using JNDI" + " properties : " + context, e);
} catch (Exception e) {
- throw new RuntimeException(e);
+ throw new SynapseCommonsException("Unexpected error resolving DataSource : "
+ + dsName + " from JNDI", e, log);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
modules/commons/src/main/java/org/apache/synapse/commons/datasource/DataSourceFinder.java
around lines 90-92, the catch block catches a generic Exception and rethrows it
as new RuntimeException(e) which loses context; replace this with richer
exception handling: catch specific exceptions where possible (e.g.,
NamingException) and for the general catch include the original message and
cause when rethrowing (for example construct a RuntimeException with a
descriptive message plus e as the cause) and also log the error using the class
logger before rethrowing to preserve diagnostic details.
| Entry entry = Mockito.mock(Entry.class); | ||
| entry.setType(Entry.URL_SRC); | ||
| doReturn(new URL("https://test")).when(entry).getSrc(); | ||
| doReturn("testKey").when(entry).getKey(); | ||
| Mockito.doReturn(new URL("https://test")).when(entry).getSrc(); | ||
| Mockito.doReturn("testKey").when(entry).getKey(); |
There was a problem hiding this comment.
setType() call on Mockito mock has no effect.
Line 178 calls entry.setType(Entry.URL_SRC) on a Mockito mock. Unlike real objects, Mockito mocks don't execute actual method implementations—they just record interactions and return defaults. This means the type is never actually set.
If the serialization logic calls getType() or isURLSource(), it will receive default values (0 or false), not Entry.URL_SRC.
🔎 Proposed fix: Stub the type-related methods
Entry entry = Mockito.mock(Entry.class);
- entry.setType(Entry.URL_SRC);
+ Mockito.doReturn(Entry.URL_SRC).when(entry).getType();
+ Mockito.doReturn(true).when(entry).isURLSource();
Mockito.doReturn(new URL("https://test")).when(entry).getSrc();
Mockito.doReturn("testKey").when(entry).getKey();Alternatively, consider using a real Entry object instead of a mock if the class is straightforward to instantiate, which would avoid needing to stub multiple methods.
📝 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.
| Entry entry = Mockito.mock(Entry.class); | |
| entry.setType(Entry.URL_SRC); | |
| doReturn(new URL("https://test")).when(entry).getSrc(); | |
| doReturn("testKey").when(entry).getKey(); | |
| Mockito.doReturn(new URL("https://test")).when(entry).getSrc(); | |
| Mockito.doReturn("testKey").when(entry).getKey(); | |
| Entry entry = Mockito.mock(Entry.class); | |
| Mockito.doReturn(Entry.URL_SRC).when(entry).getType(); | |
| Mockito.doReturn(true).when(entry).isURLSource(); | |
| Mockito.doReturn(new URL("https://test")).when(entry).getSrc(); | |
| Mockito.doReturn("testKey").when(entry).getKey(); |
🤖 Prompt for AI Agents
In
modules/core/src/test/java/org/apache/synapse/config/xml/SynapseXMLConfigurationSerializerTest.java
around lines 177 to 180, calling entry.setType(Entry.URL_SRC) on a Mockito mock
does nothing — you must stub the type-related getters or use a real Entry
instance; fix by replacing the no-op setter with explicit stubs such as
Mockito.doReturn(Entry.URL_SRC).when(entry).getType() and/or
Mockito.doReturn(true).when(entry).isURLSource() (or construct a real Entry and
set its type and src) so the serializer sees the expected type during the test.
| Mockito.when(service.getParameter(FIXConstants.FIX_USERNAME)) | ||
| .thenReturn(new Parameter(FIXConstants.FIX_USERNAME, "wos2")); | ||
| PowerMockito.when(service.getParameter(FIXConstants.FIX_PASSWORD)) | ||
| Mockito.when(service.getParameter(FIXConstants.FIX_PASSWORD)) | ||
| .thenReturn(new Parameter(FIXConstants.FIX_PASSWORD, "wos2")); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the FIXIncomingMessageHandlerTest.java file
find . -name "FIXIncomingMessageHandlerTest.java" -type f 2>/dev/null | head -5Repository: wso2/wso2-synapse
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Read the test file to verify the claims in the review comment
cat -n ./modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java | head -150Repository: wso2/wso2-synapse
Length of output: 6318
Migration from Mockito.when() is correct; remove redundant initMocks() call in testToAdmin.
The replacement of PowerMockito.when with Mockito.when is appropriate and follows the standard migration pattern. However, the explicit initMocks(this) call on line 65 in testToAdmin is redundant since the class uses @RunWith(MockitoJUnitRunner.class), which automatically initializes all @Mock fields before each test method. This redundant initialization should be removed for consistency.
🤖 Prompt for AI Agents
In
modules/transports/optional/fix/src/test/java/org/apache/synapse/transport/fix/FIXIncomingMessageHandlerTest.java
around lines 76 to 79, the test still calls MockitoAnnotations.initMocks(this)
on line 65 even though the class is annotated with
@RunWith(MockitoJUnitRunner.class); remove the redundant initMocks(this) call
from testToAdmin so mock initialization is only handled by MockitoJUnitRunner
and keep the Mockito.when(...) replacements as-is.
| <fasterxml.jackson-databind.version>2.17.2</fasterxml.jackson-databind.version> | ||
| <google.guava.version>33.0.0-jre</google.guava.version> | ||
| <failureaccess.version>1.0.3</failureaccess.version> | ||
| <failureaccess.version>1.0.2</failureaccess.version> |
There was a problem hiding this comment.
Why do we need to downgrade this?
There was a problem hiding this comment.
I upgraded this version yesterday, shall we keep this as 1.0.3?
3 participants
Related to wso2/product-micro-integrator#4494
As part of the effort, the following changes have been made.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.