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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
