[ https://issues.apache.org/jira/browse/ARTEMIS-3576?focusedWorklogId=681907&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-681907 ]
ASF GitHub Bot logged work on ARTEMIS-3576: ------------------------------------------- Author: ASF GitHub Bot Created on: 16/Nov/21 10:07 Start Date: 16/Nov/21 10:07 Worklog Time Spent: 10m Work Description: gemmellr commented on a change in pull request #3854: URL: https://github.com/apache/activemq-artemis/pull/3854#discussion_r750080638 ########## File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java ########## @@ -466,7 +466,7 @@ private boolean isLocalhost(String hostname) { @Override public final String toString() { - return super.toString() + "[ID=" + getID() + ", local= " + channel.localAddress() + ", remote=" + channel.remoteAddress() + "]"; + return super.toString() + (channel != null ? "[ID=" + channel.id() + ", local= " + channel.localAddress() + ", remote=" + channel.remoteAddress() + "]" : "[]"); Review comment: This seems wrong, lots of other NPE opportunities remain since the whole class clearly expects it simpy isnt meant to be null. Seems like a better change /actual fix would be ensuring at construction that it isnt. Specific unit tests for NettyConnection should then verify that explicitly. ########## File path: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ########## @@ -853,10 +853,14 @@ public boolean waitCompletionOnStream(final long timeWait) throws JMSException { @Override public String toString() { StringBuffer sb = new StringBuffer("ActiveMQMessage["); - sb.append(getJMSMessageID()); - sb.append("]:"); - sb.append(message.isDurable() ? "PERSISTENT" : "NON-PERSISTENT"); - sb.append("/" + message.toString()); + if (message != null) { Review comment: Similarly here, its clearly reasonably assumed it never is null, since it is a wrapper for a message that should never be null. We should enforce and test that that instead of 'fixing' only one of the many many possible NPEs in the class that dont seem like they should even happen outwith broken test usage. ########## File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java ########## @@ -1779,11 +1779,16 @@ public final SimpleString getLastValueProperty() { @Override public String toString() { + MessageDataScanningStatus scanningStatus = getDataScanningStatus(); + Map<String, Object> applicationProperties = scanningStatus == MessageDataScanningStatus.SCANNED ? + getApplicationPropertiesMap(false) : Collections.EMPTY_MAP; Review comment: This is probably the first one I could believe there might be a valid path to hitting, though I could also believe its again just a situation that we should enforce doesnt occur since it isnt expected to be possible in actual (vs broken test) usage. ########## File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/util/NettyReadable.java ########## @@ -222,7 +222,7 @@ public ReadableBuffer get(WritableBuffer target) { @Override public String toString() { - return buffer.toString(); + return buffer != null ? buffer.toString() : this.getClass().getSimpleName(); Review comment: Seems like fixing the constuctor to reject the null buffer would be the actual fix. Essentially every method here NPEs, including this one, because its just not considered valid for wrapped buffer to be null. Should just enforce and properly test that. -- 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 Issue Time Tracking ------------------- Worklog Id: (was: 681907) Remaining Estimate: 0h Time Spent: 10m > Fix toString methods throwing exceptions > ---------------------------------------- > > Key: ARTEMIS-3576 > URL: https://issues.apache.org/jira/browse/ARTEMIS-3576 > Project: ActiveMQ Artemis > Issue Type: Bug > Reporter: Domenico Francesco Bruscino > Assignee: Domenico Francesco Bruscino > Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > A toString method should never throw any exceptions. -- This message was sent by Atlassian Jira (v8.20.1#820001)