[GitHub] [activemq] gemmellr commented on a diff in pull request #979: AMQ-7309 - Implement Message#isBodyAssignableTo and Message#getBody methods

["gemmellr (via GitHub)" <gi...@apache.org>]

gemmellr commented on code in PR #979:
URL: https://github.com/apache/activemq/pull/979#discussion_r1108708010


##########
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQTextMessage.java:
##########
@@ -210,4 +210,22 @@ public String toString() {
         }
         return super.toString();
     }
+
+    @SuppressWarnings("unchecked")
+    public boolean isBodyAssignableTo(Class c) throws JMSException {
+        /*
+         *If the message is a TextMessage, ObjectMessage, MapMessage or BytesMessage
+         * and the message has no body, then the above does not apply and this method
+         *  will return true irrespective of the value of this parameter.
+         */
+        if (getText() == null) {
+            return true;
+        }
+        return c.isAssignableFrom(java.lang.String.class);
+    }
+
+    @SuppressWarnings("unchecked")
+    protected <T> T doGetBody(Class<T> asType) throws JMSException {
+        return getText() != null ? (T) getText() : null;

Review Comment:
   Is the simpler "return (T) getText();" not equivalent?



##########
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQMapMessage.java:
##########
@@ -827,4 +839,26 @@ public Map<String, Object> getContentMap() throws JMSException {
         initializeReading();
         return map;
     }
+
+    @Override
+    @SuppressWarnings("unchecked")
+    public boolean isBodyAssignableTo(Class c) throws JMSException {
+        final Map<String, Object> map = getContentMap();
+        if (map == null || map.isEmpty()) {

Review Comment:
   doGetBody doesnt seem to special case the empty map as this does...should it? (since it special casing saying true while being given any class here)



##########
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQMessage.java:
##########
@@ -795,14 +795,22 @@ public void setJMSDeliveryTime(long deliveryTime) throws JMSException {
         this.deliveryTime = deliveryTime;
 	}
 
-	@Override
-	public <T> T getBody(Class<T> c) throws JMSException {
-        throw new UnsupportedOperationException("getBody(Class<T>) is not supported");
-	}
+    @Override
+    public final <T> T getBody(Class<T> asType) throws JMSException {
+        if (isBodyAssignableTo(asType)) {
+            return doGetBody(asType);
+        }
+
+        throw new MessageFormatException("Message body cannot be read as type: " + asType);
+    }
 
 	@Override
 	public boolean isBodyAssignableTo(Class c) throws JMSException {
-        throw new UnsupportedOperationException("isBodyAssignableTo(Class) is not supported");
+        return true;

Review Comment:
   indentation is off (on the method itself...and some others above)



##########
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQTextMessage.java:
##########
@@ -210,4 +210,22 @@ public String toString() {
         }
         return super.toString();
     }
+
+    @SuppressWarnings("unchecked")
+    public boolean isBodyAssignableTo(Class c) throws JMSException {
+        /*
+         *If the message is a TextMessage, ObjectMessage, MapMessage or BytesMessage
+         * and the message has no body, then the above does not apply and this method
+         *  will return true irrespective of the value of this parameter.
+         */

Review Comment:
   Could probably make the comment make more sense by not using the full spec sentence :)



##########
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQBytesMessage.java:
##########
@@ -956,4 +956,22 @@ protected void doCompress() throws IOException {
             }
         }
     }
+
+    @Override
+    @SuppressWarnings("unchecked")
+    public boolean isBodyAssignableTo(Class c) {
+        return getContent() == null || c.isAssignableFrom(byte[].class);
+    }
+
+    @SuppressWarnings("unchecked")
+    protected <T> T doGetBody(Class<T> asType) {
+        //Make sure the bytes are stored before trying to copy and return
+        if (dataOut != null && getContent() == null) {
+            storeContent();
+        }
+
+        final ByteSequence content = getContent();
+        return content != null ? (T) new ByteSequence(content.getData(), content.getOffset(),
+            content.getLength()).getData() : null;

Review Comment:
   (also if there is somehow any offset/length diff then the returned bytes would actually be invalid as they dont account for those)



-- 
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-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org