fixes #139: Refactor stanza construction

guusdk · guusdk · commit 52ba880409b6 · 2020-12-08T14:56:55.000+01:00
When a previously archived message is being retrieved, the optional 'stanza' database value is eventually parsed as XMPP.

This commit brings forward this process (to a place immediately after the data is obtained from the database), replacing the existing mechanism that did it just before the message was being routed to the intended recipient.

The benefit of this is that the code that it is more fail-fast, and operations that follow the database retrieval can work with a proper stanza, instead of having to work with its String representation. This brings type-safety. Lastly, there's less ambiguity around where parsing occurs (and at what point the parsing can be expected to have been completed).
diff --git a/src/java/com/reucon/openfire/plugin/archive/impl/JdbcPersistenceManager.java b/src/java/com/reucon/openfire/plugin/archive/impl/JdbcPersistenceManager.java
@@ -7,10 +7,7 @@
 import com.reucon.openfire.plugin.archive.model.Participant;
 import com.reucon.openfire.plugin.archive.util.StanzaIDUtil;
 import com.reucon.openfire.plugin.archive.xep0059.XmppResultSet;
-import org.dom4j.Document;
-import org.dom4j.DocumentFactory;
-import org.dom4j.DocumentHelper;
-import org.dom4j.Element;
+import org.dom4j.*;
 import org.jivesoftware.database.DbConnectionManager;
 import org.jivesoftware.openfire.archive.ConversationManager;
 import org.jivesoftware.util.JiveConstants;
@@ -31,7 +28,6 @@
  */
 public class JdbcPersistenceManager implements PersistenceManager {
     private static final Logger Log = LoggerFactory.getLogger( JdbcPersistenceManager.class );
-    protected static final DocumentFactory docFactory = DocumentFactory.getInstance();
     public static final int DEFAULT_MAX = 1000;
 
     public static final String SELECT_MESSAGES_BY_CONVERSATION = "SELECT DISTINCT ofConversation.conversationID, ofConversation.room, "
@@ -474,7 +470,7 @@ public Conversation getConversation(JID owner, JID with, Date start) {
                 message = extractMessage(rs);
                 conversation.addMessage(message);
             }
-        } catch (SQLException sqle) {
+        } catch (SQLException | DocumentException sqle) {
             Log.error("Error selecting conversation", sqle);
         } finally {
             DbConnectionManager.closeConnection(rs, pstmt, con);
@@ -542,7 +538,7 @@ private Collection<Participant> extractParticipant(ResultSet rs) throws SQLExcep
         return participants;
     }
 
-    static ArchivedMessage extractMessage(ResultSet rs) throws SQLException {
+    static ArchivedMessage extractMessage(ResultSet rs) throws SQLException, DocumentException {
         Date time = millisToDate(rs.getLong("sentDate"));
         String body = rs.getString("body");
         String stanza = rs.getString("stanza");
@@ -604,46 +600,26 @@ public static ArchivedMessage getArchivedMessage( long messageId, JID owner )
         } catch (SQLException ex) {
             Log.warn("SQL failure while trying to get message with ID {} from the archive of {}.", messageId, owner, ex);
             return null;
+        } catch (DocumentException ex) {
+            Log.warn("Failure to parse 'stanza' value as XMPP for the message with ID {} from the archive of {}.", messageId, owner, ex);
+            return null;
         } finally {
             DbConnectionManager.closeConnection(rs, pstmt, connection);
         }
     }
 
-    static protected ArchivedMessage asArchivedMessage(JID owner, String fromJID, String fromJIDResource, String toJID, String toJIDResource, Date sentDate, String body, String stanza, Long id)
-    {
-        if (stanza == null) {
-            Message message = new Message();
-            message.setFrom(fromJID);
-            message.setTo(toJID);
-            message.setType(Message.Type.normal);
-            message.setBody(body);
-            stanza = message.toString();
-        }
-
+    static protected ArchivedMessage asArchivedMessage(JID owner, String fromJID, String fromJIDResource, String toJID, String toJIDResource, Date sentDate, String body, String stanza, Long id) throws DocumentException {
         String sid;
-        try
-        {
-            if ( !JiveGlobals.getBooleanProperty( "conversation.OF-1804.disable", false ) )
-            {
-                // Prior to OF-1804 (Openfire 4.4.0), the stanza was logged with a formatter applied.
-                // This causes message formatting to be modified (notably, new lines could be altered).
-                // This workaround restores the original body text, that was stored in a different column.
-                final int pos1 = stanza.indexOf( "<body>" );
-                final int pos2 = stanza.indexOf( "</body>" );
-
-                if ( pos1 > -1 && pos2 > -1 )
-                {
-                    // Add the body value to a proper XML element, so that the strings get XML encoded (eg: ampersand is escaped).
-                    final Element bodyEl = docFactory.createDocument().addElement("body");
-                    bodyEl.setText(body);
-                    stanza = stanza.substring( 0, pos1 ) + bodyEl.asXML() + stanza.substring( pos2 + 7 );
-                }
+        if (stanza != null) {
+            try {
+                final Document doc = DocumentHelper.parseText(stanza);
+                final Message message = new Message(doc.getRootElement());
+                sid = StanzaIDUtil.findFirstUniqueAndStableStanzaID(message, owner.toBareJID());
+            } catch (Exception e) {
+                Log.warn("An exception occurred while parsing message with ID {}", id, e);
+                sid = null;
             }
-            final Document doc = DocumentHelper.parseText( stanza );
-            final Message message = new Message( doc.getRootElement() );
-            sid = StanzaIDUtil.findFirstUniqueAndStableStanzaID( message, owner.toBareJID() );
-        } catch ( Exception e ) {
-            Log.warn( "An exception occurred while parsing message with ID {}", id, e );
+        } else {
             sid = null;
         }
 
diff --git a/src/java/com/reucon/openfire/plugin/archive/impl/MucMamPersistenceManager.java b/src/java/com/reucon/openfire/plugin/archive/impl/MucMamPersistenceManager.java
@@ -217,13 +217,15 @@ public static ArchivedMessage getArchivedMessage( long messageId, MUCRoom room )
         } catch (SQLException ex) {
             Log.warn("SQL failure while trying to get message with ID {} from the archive of MUC room {}.", messageId, room, ex);
             return null;
+        } catch (DocumentException ex) {
+            Log.warn("Failure to parse 'stanza' value as XMPP for the message with ID {} from the archive of MUC room {}.", messageId, room, ex);
+            return null;
         } finally {
             DbConnectionManager.closeConnection(rs, pstmt, connection);
         }
     }
 
-    static protected ArchivedMessage asArchivedMessage(JID roomJID, String senderJID, String nickname, Date sentDate, String subject, String body, String stanza, long id)
-    {
+    static protected ArchivedMessage asArchivedMessage(JID roomJID, String senderJID, String nickname, Date sentDate, String subject, String body, String stanza, long id) throws DocumentException {
         if (stanza == null) {
             Message message = new Message();
             message.setType(Message.Type.groupchat);
@@ -242,29 +244,16 @@ static protected ArchivedMessage asArchivedMessage(JID roomJID, String senderJID
         }
 
         String sid;
-        try
-        {
-            if ( !JiveGlobals.getBooleanProperty( "conversation.OF-1804.disable", false ) )
-            {
-                // Prior to OF-1804 (Openfire 4.4.0), the stanza was logged with a formatter applied.
-                // This causes message formatting to be modified (notably, new lines could be altered).
-                // This workaround restores the original body text, that was stored in a different column.
-                final int pos1 = stanza.indexOf( "<body>" );
-                final int pos2 = stanza.indexOf( "</body>" );
-
-                if ( pos1 > -1 && pos2 > -1 )
-                {
-                    // Add the body value to a proper XML element, so that the strings get XML encoded (eg: ampersand is escaped).
-                    final Element bodyEl = docFactory.createDocument().addElement("body");
-                    bodyEl.setText(body);
-                    stanza = stanza.substring( 0, pos1 ) + bodyEl.asXML() + stanza.substring( pos2 + 7 );
-                }
+        if (stanza != null) {
+            try {
+                final Document doc = DocumentHelper.parseText(stanza);
+                final Message message = new Message(doc.getRootElement());
+                sid = StanzaIDUtil.findFirstUniqueAndStableStanzaID(message, roomJID.toBareJID());
+            } catch (Exception e) {
+                Log.warn("An exception occurred while parsing message with ID {}", id, e);
+                sid = null;
             }
-            final Document doc = DocumentHelper.parseText( stanza );
-            final Message message = new Message( doc.getRootElement() );
-            sid = StanzaIDUtil.findFirstUniqueAndStableStanzaID( message, roomJID.toBareJID() );
-        } catch ( Exception e ) {
-            Log.warn( "An exception occurred while parsing message with ID {}", id, e );
+        } else {
             sid = null;
         }
 
diff --git a/src/java/com/reucon/openfire/plugin/archive/impl/PaginatedMessageDatabaseQuery.java b/src/java/com/reucon/openfire/plugin/archive/impl/PaginatedMessageDatabaseQuery.java
@@ -15,6 +15,7 @@
 package com.reucon.openfire.plugin.archive.impl;
 
 import com.reucon.openfire.plugin.archive.model.ArchivedMessage;
+import org.dom4j.DocumentException;
 import org.jivesoftware.database.DbConnectionManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -134,7 +135,9 @@ protected List<ArchivedMessage> getPage( final Long after, final Long before, fi
                 archivedMessages.add(archivedMessage);
             }
         } catch (SQLException e) {
-            Log.error("SQL failure during MAM: ", e);
+            Log.error("SQL failure during MAM for owner: {}", this.owner, e);
+        } catch (DocumentException e) {
+            Log.error("Unable to parse 'stanza' value as valid XMMP for owner {}", this.owner, e);
         } finally {
             DbConnectionManager.closeConnection(rs, pstmt, connection);
         }
diff --git a/src/java/com/reucon/openfire/plugin/archive/impl/PaginatedMucMessageDatabaseQuery.java b/src/java/com/reucon/openfire/plugin/archive/impl/PaginatedMucMessageDatabaseQuery.java
@@ -15,6 +15,7 @@
 package com.reucon.openfire.plugin.archive.impl;
 
 import com.reucon.openfire.plugin.archive.model.ArchivedMessage;
+import org.dom4j.DocumentException;
 import org.jivesoftware.database.DbConnectionManager;
 import org.jivesoftware.openfire.muc.MUCRoom;
 import org.jivesoftware.util.StringUtils;
@@ -136,7 +137,9 @@ protected List<ArchivedMessage> getPage( final Long after, final Long before, fi
                 msgs.add( MucMamPersistenceManager.asArchivedMessage(room.getJID(), senderJID, nickname, sentDate, subject, body, stanza, id) );
             }
         } catch (SQLException e) {
-            Log.error("SQL failure during MAM-MUC: ", e);
+            Log.error("SQL failure during MAM-MUC for room: {}", this.room, e);
+        } catch (DocumentException e) {
+            Log.error("Unable to parse 'stanza' value as valid XMMP for MAM-MUC room {}", this.room, e);
         } finally {
             DbConnectionManager.closeConnection(rs, pstmt, connection);
         }
diff --git a/src/java/com/reucon/openfire/plugin/archive/model/ArchivedMessage.java b/src/java/com/reucon/openfire/plugin/archive/model/ArchivedMessage.java
@@ -1,7 +1,13 @@
 package com.reucon.openfire.plugin.archive.model;
 
+import org.dom4j.*;
 import org.jivesoftware.database.JiveID;
+import org.jivesoftware.openfire.XMPPServer;
+import org.jivesoftware.util.JiveGlobals;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.xmpp.packet.JID;
+import org.xmpp.packet.Message;
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -14,6 +20,9 @@
 @JiveID(601)
 @Immutable
 public class ArchivedMessage {
+
+    public static final Logger Log = LoggerFactory.getLogger( ArchivedMessage.class );
+
     public enum Direction {
         /**
          * A message sent by the owner.
@@ -42,19 +51,33 @@ public enum Direction {
     private final JID with;
 
     @Nullable
-    private final String stanza;
+    private final Message stanza;
 
     @Nullable
     private final String stableId;
 
-    public ArchivedMessage(@Nullable final Long id, @Nonnull final Date time, @Nonnull final Direction direction, @Nullable final JID with, @Nullable final String stableId, @Nullable final String body, @Nullable final String stanza) {
+    public ArchivedMessage(@Nullable final Long id, @Nonnull final Date time, @Nonnull final Direction direction, @Nullable final JID with, @Nullable final String stableId, @Nullable final String body, @Nullable final String stanza) throws DocumentException {
         this.id = id;
         this.time = time;
         this.direction = direction;
         this.with = with;
         this.stableId = stableId;
         this.body = body;
-        this.stanza = stanza;
+
+        if ( stanza != null) {
+            final Document doc = DocumentHelper.parseText( stanza );
+            this.stanza = new Message( doc.getRootElement() );
+        } else {
+            this.stanza = null;
+        }
+
+        if ( this.stanza != null && !JiveGlobals.getBooleanProperty( "conversation.OF-1804.disable", false ) )
+        {
+            // Prior to OF-1804 (Openfire 4.4.0), the stanza was logged with a formatter applied.
+            // This causes message formatting to be modified (notably, new lines could be altered).
+            // This workaround restores the original body text, that was stored in a different column.
+            this.stanza.setBody( body );
+        }
     }
 
     /**
@@ -107,7 +130,7 @@ public String getBody() {
      * @return XMPP packet
      */
     @Nullable
-    public String getStanza() {
+    public Message getStanza() {
         return stanza;
     }
 
@@ -126,7 +149,8 @@ public JID getWith() {
     }
 
     /**
-     * The first stable and unique stanza-id value in the stanza, if the stanza contains such a value.
+     * The first stable and unique stanza-id value in the stanza that was set by owner of the message archive, if the
+     * stanza contains such a value.
      *
      * @return a stable and unique stanza-id value.
      */
@@ -146,4 +170,54 @@ public String toString() {
 
         return sb.toString();
     }
+
+    /**
+     * When the archived message does not include the original stanza, this method can be used to recreate a stanza from
+     * the individual parts that did get persisted.
+     *
+     * Note that the result of this method will not include most of the metadata that would have been sent in the original
+     * stanza. When the original stanza is available, that should be preferred over the result of this method. This
+     * method explicitly does not return or use a stanza that is available in the archivedMessage argument, assuming
+     * that the caller has evaluated that, and choose (for whatever reason) to not use that (eg: it might be malformed?)
+     *
+     * When the archived message does not contain certain optional data (such as a body), this method cannot recreate a
+     * message stanza. In such cases, this method returns null.
+     *
+     * @param archivedMessage The archived message for which to recreate a stanza.
+     * @param archiveOwner The owner of the archive from which to recreate a stanza
+     * @return A recreated stanza.
+     */
+    @Nullable
+    public static Message recreateStanza( @Nonnull final ArchivedMessage archivedMessage, @Nonnull final JID archiveOwner )
+    {
+        if ( archivedMessage.getBody() == null || archivedMessage.getBody().isEmpty() ) {
+            Log.trace("Cannot reconstruct stanza for archived message ID {}, as it has no body.", archivedMessage.getId());
+            return null;
+        }
+
+        // Try creating a fake one from the body.
+        final JID to;
+        final JID from;
+        if (archivedMessage.getDirection() == ArchivedMessage.Direction.to) {
+            // message sent by the archive owner;
+            to = archivedMessage.getWith();
+            from = archiveOwner;
+        } else {
+            // message received by the archive owner;
+            to = archiveOwner;
+            from = archivedMessage.getWith();
+        }
+
+        final boolean isMuc = (to != null && XMPPServer.getInstance().getMultiUserChatManager().getMultiUserChatService( to ) != null)
+            || (from != null && XMPPServer.getInstance().getMultiUserChatManager().getMultiUserChatService( from ) != null);
+
+        final Message result = new Message();
+        result.setFrom(from);
+        result.setTo(to);
+        result.setType( isMuc ? Message.Type.groupchat : Message.Type.chat );
+        result.setBody(archivedMessage.getBody());
+
+        Log.trace( "Reconstructed stanza for archived message with ID {} (only a body was stored): {}", archivedMessage.getId(), result );
+        return result;
+    }
 }
diff --git a/src/java/com/reucon/openfire/plugin/archive/xep0313/IQQueryHandler.java b/src/java/com/reucon/openfire/plugin/archive/xep0313/IQQueryHandler.java
