Copilot commented on code in PR #42:
URL:
https://github.com/apache/activemq-nms-openwire/pull/42#discussion_r2199797839
##########
src/Transport/InactivityMonitor.cs:
##########
@@ -230,32 +231,44 @@ protected override async System.Threading.Tasks.Task
OnCommand(ITransport sender
inRead.Value = true;
try
{
- if(command.IsKeepAliveInfo)
+ if (command is ExceptionResponse)
+ {
+ ExceptionResponse error = command as ExceptionResponse;
+ NMSException exception =
ExceptionFromBrokerError.CreateExceptionFromBrokerError(error.Exception);
+ if (exception is NMSSecurityException)
+ {
+ OnException(this, exception);
+ }
+ else
+ {
+ Tracer.WarnFormat("ExceptionResponse received from the
broker:", command.GetType());
Review Comment:
The format string has no placeholder for the command type argument, so
command.GetType() will be ignored. Add a placeholder, e.g., "ExceptionResponse
received from the broker: {0}".
```suggestion
Tracer.WarnFormat("ExceptionResponse received from
the broker: {0}", command.GetType());
```
##########
src/Util/ExceptionFromBrokerError.cs:
##########
@@ -0,0 +1,78 @@
+using Apache.NMS.ActiveMQ.Commands;
+using System;
+using System.Reflection;
+
+
+namespace Apache.NMS.ActiveMQ.Util
+{
+ internal class ExceptionFromBrokerError
+ {
+ public static NMSException CreateExceptionFromBrokerError(BrokerError
brokerError)
+ {
+ String exceptionClassName = brokerError.ExceptionClass;
+
+ if (String.IsNullOrEmpty(exceptionClassName))
+ {
+ return new BrokerException(brokerError);
+ }
+
+ NMSException exception = null;
+ String message = brokerError.Message;
+
+ // We only create instances of exceptions from the NMS API
+ Assembly nmsAssembly = Assembly.GetAssembly(typeof(NMSException));
+
+ // First try and see if it's one we populated ourselves in which
case
+ // it will have the correct namespace and exception name.
+ Type exceptionType = nmsAssembly.GetType(exceptionClassName,
false, true);
Review Comment:
[nitpick] ExceptionFromBrokerError performs reflection lookups on every
call, which can be expensive. Consider caching the resolved exception types or
constructors to improve performance under high load.
##########
test/Transport/Inactivity/InactivityMonitorTest.cs:
##########
@@ -133,6 +133,51 @@ public void TestWriteMessageFail()
{
}
}
+ public class TestableInactivityMonitor : InactivityMonitor
+ {
+ public TestableInactivityMonitor(ITransport next) : base(next) { }
+
+ // Expose protected method for testing
+ public async Task TestOnCommand(ITransport sender, Command command)
+ {
+ await OnCommand(sender, command);
+ }
+ }
+ [Test]
+ public void OnCommand_WithNMSSecurityException_ShouldCallOnException()
+ {
+ // Arrange
+ var brokerError = new BrokerError
+ {
+ ExceptionClass = "javax.jms.JMSSecurityException",
+ Message = "Authentication failed"
+ };
+
+ var exceptionResponse = new ExceptionResponse
+ {
+ Exception = brokerError
+ };
+
+ // Mock the static method call - this would require making
ExceptionFromBrokerError testable
+ // For this test, we'll assume it returns an NMSSecurityException
+ var securityException = new NMSSecurityException("Authentication
failed");
+ TestableInactivityMonitor monitor = new
TestableInactivityMonitor(this.transport);
+ monitor.Exception += new ExceptionHandler(OnException);
+ bool exceptionHandlerCalled = false;
+ Exception caughtException = null;
+ monitor.Exception += (sender, args) =>
+ {
+ exceptionHandlerCalled = true;
+ caughtException = args;
+ };
+ // Act
+ _ = monitor.TestOnCommand(transport, exceptionResponse);
Review Comment:
The async TestOnCommand call is not awaited, so the test may complete before
the command handling finishes. Consider awaiting the Task to ensure the event
is raised before assertions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
