ARTEMIS-5099 fix impossible cast in AMQPLargeMessage #5448
Conversation
I believe this was likely a copy-and-paste error involving o.a.a.a.c.p.i.j.LargeServerMessageImpl#referenceOriginalMessage.
jbertram
force-pushed
the
ARTEMIS-5099
branch
from
42a525d to
2e7c7fc
Compare
| @@ -660,7 +659,7 @@ public void referenceOriginalMessage(final Message original, final SimpleString | |||
|
|
|||
| super.referenceOriginalMessage(original, originalQueue); | |||
|
|
|||
| if (original instanceof LargeServerMessageImpl) { | |||
| if (original instanceof AMQPLargeMessage) { | |||
There was a problem hiding this comment.
My main question would be what is the impact of adding this call into the mix given that it wasn't happening before and I guess didn't cause anything to break? Would something break now?
There was a problem hiding this comment.
When you say, "it wasn't happening before," what exactly do you mean? Do you mean that org.apache.activemq.artemis.core.persistence.impl.journal.LargeBody#referenceOriginalMessage wasn't being invoked since this instanceof would never succeed?
There was a problem hiding this comment.
I'm not sure what else I could have meant but let me be more clear. What will happen now that an AMQPLargeMessage cast would succeed when this method is called? Should something have been testing this? From a brief look at the broker code calling this it seems like the source message would never be anything other than the AMQPLargeMessage itself so I doubt the cast to ServerLargeMessageImpl would have succeeded in the past.
There was a problem hiding this comment.
I think this is supposed to be kept as it was...
it's being tested from TestConversions.. so it is for conversions...
So when you convert from AMQP to Core
It probably needs to add coverage
There was a problem hiding this comment.
These two lines break the existing code:
AMQPLargeMessage m = new AMQPLargeMessage(0, 0, null, null, null);
m.referenceOriginalMessage(new LargeServerMessageImpl((byte) 0, 0, null, null), SimpleString.of("foo"));Causing this to be thrown:
java.lang.ClassCastException: class org.apache.activemq.artemis.core.persistence.impl.journal.LargeServerMessageImpl cannot be cast to class org.apache.activemq.artemis.protocol.amqp.broker.AMQPLargeMessage (org.apache.activemq.artemis.core.persistence.impl.journal.LargeServerMessageImpl and org.apache.activemq.artemis.protocol.amqp.broker.AMQPLargeMessage are in unnamed module of loader 'app')
It's not clear to me that this code is directly related to any conversion. If it was I would expect org.apache.activemq.artemis.core.persistence.impl.journal.LargeServerMessageImpl#referenceOriginalMessage to behave in a similar way.
I believe this was likely a copy-and-paste error involving o.a.a.a.c.p.i.j.LargeServerMessageImpl#referenceOriginalMessage.