james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <server-...@james.apache.org>
Subject [jira] [Commented] (JAMES-2167) Serializable attributes are not preserved by enqueue/dequeue on a JMS queue
Date Wed, 11 Jul 2018 04:32:00 GMT

    [ https://issues.apache.org/jira/browse/JAMES-2167?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16539540#comment-16539540
] 

ASF GitHub Bot commented on JAMES-2167:
---------------------------------------

Github user chibenwa commented on a diff in the pull request:

    https://github.com/apache/james-project/pull/126#discussion_r201560161
  
    --- Diff: server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueue.java
---
    @@ -442,16 +426,39 @@ protected void populateMail(Message message, MailImpl mail) throws
JMSException
         }
     
         /**
    -     * Convert the attribute value if necessary.
    +     * Retrieves the attribute by {@code name} form {@code message} and tries to add
it on {@code mail}.
    +     *
    +     * @param message The attribute source.
    +     * @param mail    The mail on which attribute should be set.
    +     * @param name    The attribute name.
    +     */
    +    private void setMailAttribute(Message message, Mail mail, String name) {
    +        // Now cast the property back to Serializable and set it as attribute.
    +        // See JAMES-1241
    +        Object attrValue = Throwing.function(message::getObjectProperty).apply(name);
    +
    +        // can be a base64 representation of serialized object try decode and deserialize.
See JAMES-2167.
    +        if (attrValue instanceof String) {
    +            mail.setAttribute(name, tryDeserialize((String) attrValue));
    +        } else if (attrValue instanceof Serializable) {
    +            mail.setAttribute(name, (Serializable) attrValue);
    +        } else {
    +            LOGGER.error("Not supported mail attribute {} of type {} for mail {}", name,
attrValue, mail.getName());
    +        }
    +    }
    +
    +    /**
    +     * Tries to deserialize given argument and when fails returns itself.
          *
    -     * @param value
    -     * @return convertedValue
    +     * @param attrValue The input string.
    +     * @return The deserialized object or itself.
          */
    -    protected Object convertAttributeValue(Object value) {
    -        if (value == null || value instanceof String || value instanceof Byte || value
instanceof Long || value instanceof Double || value instanceof Boolean || value instanceof
Integer || value instanceof Short || value instanceof Float) {
    -            return value;
    +    private Serializable tryDeserialize(String attrValue) {
    --- End diff --
    
    This should be moved to `JMSSerializationUtils`.
    
    Also, you should not rely on an exception of deserializing to handle "native" values.
If doing so, a user providing a `base64 serialized integer string` as an input will get an
`Integer` as an output.
    
    We miss the `bijection` behaviour that we would expect from such a feature. That may lead
to subtle bugs.
    
    My opinion on this is to promote correctness over performances. Hence I'd advocate**use
base64 serialization even for native values**.
    
    This would also have some cool benefits:
     - We would stop assuming ActiveMQ behaviour in James code as it is done in `JMSSerializationUtils::hasJMSNativeSupport`
     - We will simplify the code by removing one conditional branch.
     - `JMSSerializationUtils` would not be JMS specific anymore. We will be able to change
its name, & in a near future use it in another context.
     - This would also remove the `if (X instanceOf String)` in `this::setMailAttribute`.


> Serializable attributes are not preserved by enqueue/dequeue on a JMS queue
> ---------------------------------------------------------------------------
>
>                 Key: JAMES-2167
>                 URL: https://issues.apache.org/jira/browse/JAMES-2167
>             Project: James Server
>          Issue Type: Bug
>          Components: Queue
>    Affects Versions: master
>            Reporter: Tellier Benoit
>            Priority: Major
>
> A call to toString breaks convertion for generic serializable attributes. 
> The dequeued email will have only a toString version of it. We are expecting the exact
same value, just deserialized.
> We should ensure the value of Serializable attributes gets preserved by enqueue/dequeue
operations. We should add a unit test for this, and fix it.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message