#369 and #370: Add support for the clear chat room history event #408
Conversation
Database creation script and update script changes. - Add `ofMucRoomStatus` table to store the mapping between roomID and roomJID together with whether a room was destroyed. - Add `roomID` column to `ofConversation`, `ofConParticipant`, and `ofMessageArchive` tables to enable the removal of data in these table using a roomID. - Create indexes on `roomID` for `ofConversation`, `ofConParticipant`, and `ofMessageArchive` tables to speed up matching operations. - Bump version to 9 in `ofVersion` table.
Add roomID to ArchivedMessage class. - Update constructors to include `roomID` parameter. - Add `getRoomID` method to retrieve the room ID.
Add roomID to Conversation class - Update constructors to include `roomID` parameter. - Add `getRoomID` method to retrieve the room ID. - Update `ConversationDAO.insertIntoDb` calls to include `roomID`.
Refactor ConversationDAO to include roomID in database operations and update method signatures accordingly. - Add roomID to SQL queries for inserting and loading conversations and participants. - Modify createConversation method to handle roomID and insert new room into ofMucRoomStatus table if it doesn’t exist. - Update getMessages method to include roomID in the result set and ArchivedMessage constructor. - Adjust loadFromDb method to retrieve roomID from the database and use it in the Conversation constructor. - Add insertNewRoomIntoStatusTable method to insert a new room into the ofMucRoomStatus table. - Modify insertIntoDb methods to include roomID in the SQL insert statements.
Add roomClearChatHistory event to ConversationEvent - Add new event type roomClearChatHistory to handle clearing chat history for a room. - Update run method to process roomClearChatHistory event. - Add static method roomClearChatHistory to create a new ConversationEvent for clearing chat history.
Enhance ConversationManager to support roomID and room chat history management - Add NO_ROOM_ID constant to represent the absence of a room ID. - Add SQL queries for updating room destroyed status and deleting room chat history, participants, and conversations. - Introduce roomJIDToIDMap to store mappings between room JIDs and unique room IDs. - Introduce conversationIDToRoomIDMap to store mappings between conversation IDs and room IDs. - Add roomIDSequenceManager to generate unique room IDs. - Implement populateRoomJIDToIDMap to load room ID mappings from the database. - Add methods hasRoomJIDToIDMap and getRoomIDFromRoomJID to manage room ID mappings. - Add getRoomIDFromConversationID to retrieve room ID from conversation ID. - Implement clearChatHistory to delete all chat history, participants, and conversations for a room. - Update methods to include roomID in database operations and mappings. - Add roomDestroyed method to handle room destruction and update room status in the database. - Update removeConversation to remove conversation ID to room ID mapping.
Add roomClearChatHistory event handling to GroupConversationInterceptor - Update roomDestroyed method to call conversationManager.roomDestroyed instead of roomConversationEnded. We are now keeping track of rooms being destroyed so roomConversationEnded is not sufficient. - Add roomClearChatHistory method to handle clearing chat history for a room.
Update XmlSerializerTest to include roomID in Conversation constructor.
Add roomID to javadoc for ArchivedMessage constructor.
Getting the room JID data from the correct resultset column in the loadFromDb method.
There was a problem hiding this comment.
Am I right to think that this will assign a room ID of -1 to all conversations that preexist in the database? That would identify those conversations as being one-on-one chats, instead of group chats, right? I think that the database update script should do a 'migration' of sorts, so that all preexisting data (where needed) gets a valid identifier.
It also seems a bit awkward to have a roomJIDtoIDMap. Keeping this map in memory may introduce issues when clustering (where each of the cluster nodes would have it's own in-memory map). Maybe it's good to consider refactoring this a bit, and use a MUCRoomStatus object, that is a proper representation of a room, plus associated DAO objects?
|
|
||
| CREATE TABLE ofConversation ( | ||
| roomID INTEGER NOT NULL, | ||
| conversationID INTEGER NOT NULL, | ||
| room VARCHAR(512), |
There was a problem hiding this comment.
Should the room and isExernal columns be moved to the new table?
Yes, all existing chats will have a room ID of -1. Do you do complex data migration using the sql update scripts? The things we would need to do would include:
Will need to check with you on whether a data migration script is the best solution. |
Added MUCRoomStatusDAO to house all database access procedures for ofMucRoomStatus: Refactor ConversationManager to use MUCRoomStatusDAO for room status management - Moved `UPDATE_ROOM_DESTROYED_STATUS` and `LOAD_ROOM_INFO` SQL queries from `ConversationManager` into MUCRoomStatusDAO. - Updated `getRoomIDFromRoomJID` method to handle exceptions and return `NO_ROOM_ID` if room ID is not found. - Added `roomCreated` method to insert new room info using `MUCRoomStatusDAO`. - Updated `roomDestroyed` method to use `MUCRoomStatusDAO` for updating room destroyed status and removing room from `roomJIDToIDMap`. - Added code to `clearChatHistory` method to rebuild archive and MUC Lucene index after clearing chat history.
I've updated the code to group the access methods for ofMucRoomStatus into MUCRoomStatusDAO. The roomJIDtoIDMap is similar to ConversationManager.conversations. I’ll need to check with you about clustering. |
I'd prefer to have an SQL script able to do this, but I recognize that this might not be practically achievable. I don't think that not doing a migration at all is an option: I expect that this basically corrupts preexisting data, leading to broken end-user functionality. There is some precedent of having bits of Java code for one-time data migration. For examples of that, see the code in the |
org.jivesoftware.database.bugfix is in the openfire repo. Should I create a org.jivesoftware.openfire.database.bugfix, or more accurately org.jivesoftware.openfire.database.migration, package in the monitoring plugin to house a Java script if needed? |
Updated SQL scripts to: - Add the automatic generation of room IDs during upgrades - Create the appropriate tables containing room IDs during a clean setup
Removing the usage of roomID due to database schema changes. We are no longer storing room IDs in ofConParticipant and ofMessageArchive: - Removed the `roomID` field from the `ArchivedMessage` class. - Updated constructors of `ArchivedMessage` to remove the `roomID` parameter. - Removed the `getRoomID` method from `ArchivedMessage`. - Updated SQL queries in `ConversationDAO` to remove references to `roomID` in `ofConParticipant` and `ofMessageArchive` tables. - Modified the `ConversationManager` to handle the removal of `roomID` from `ArchivedMessage`. - Updated the `INSERT_MESSAGE` and `INSERT_PARTICIPANT` SQL statements to exclude `roomID`. - Adjusted the `DELETE_ARCHIVE_ROOM_CHAT_HISTORY` and `DELETE_ALL_ROOM_PARTICIPANTS` queries to use `conversationID` instead of `roomID`. - Ensured all instances of `ArchivedMessage` creation and usage reflect the removal of `roomID`. Also added roomJID info to an error log for ConversationManager.
Add the use of string concatenation for hsqldb.
There was a problem hiding this comment.
I have serious concerns about this. On the one hand it out of the box generates errors upon first use, and has a SQL call which is clearly not correct (mismatched number of query parameters vs arguments supplied), but I've also got concerns about the overarching approach.
In particular the various maps that retain identifier-to-object relationships seem fragile. I believe that they duplicate state (from either other objects or the database), and their positioning/usage isn't well defined, leading to confusing usage patterns (where unnecessary room for issues is introduced).
There also is a scary blunt rebuild of Lucene indices when a room is removed. That is a non-trivial action, which basically does a full text scan of all archived messages in the database.
Finally, there are a number of other inconsistencies that I've flagged in individual review items.
Let's discuss how to move forward.
| /** | ||
| * Constant representing the absence of a room ID. | ||
| */ | ||
| public static final long NO_ROOM_ID = -1; |
There was a problem hiding this comment.
Why not use NULL or an absent Optional or something like that?
| * A room JIDs can be reuse if a room is destroyed and then recreated. | ||
| * The unique room IDs however are kept unique using a SequenceManager instance. | ||
| */ | ||
| private static Map<JID, Long> roomJIDToIDMap = MUCRoomStatusDAO.loadRoomJIDToIDMap(); |
There was a problem hiding this comment.
There are several issues with this line:
- Initialization prior to the plugin's initialization causes database connection errors (as the database has not been installed yet, if this was the first time that the plugin was loaded.
- Usage of this collection implies that all mappings (for all rooms) are always stored in memory. That's a risky assumption: any mismatch in state will potentially break things. Also: is this cluster-safe? I release that most of the operations are delegated to the senior cluster node, but does that hold true for all operations that potentially use this map?
- Isn't this really just a cache for a call to a to-be-created
MUCRoomStatusDAO.getLastIDFor(JID roomAddress)? As such, it should probably live in MUCRoomStatusDAO (as that class is responsible for this data).
| * @param roomJID the JID of the room to check. | ||
| * @return true if the map contains the room JID, false otherwise. | ||
| */ | ||
| public static boolean hasRoomJIDToIDMap(JID roomJID) { |
| * @param roomJID the JID of the room. | ||
| * @return the unique room ID. | ||
| */ | ||
| public static long getRoomIDFromRoomJID(JID roomJID) { |
There was a problem hiding this comment.
This method is basically just a call to Map.get(). Why wrap this in a try/catch?
| } | ||
|
|
||
| // Rebuild the archive and muc lucene index | ||
| MonitoringPlugin plugin = MonitoringPlugin.getInstance(); |
There was a problem hiding this comment.
This introduces a potentially (very) expensive operation each time a room is deleted. Rebuilding the index literally throws away the index, and reads all data from the database.
There is an update mechanims for indexes, which already has a bit of functionality to remove selected documents from the index. Hopefully we can use that to optimize this call.
| } | ||
|
|
||
| /** | ||
| * Returns the unique room ID for the given room JID. If no mapping exists, a new one is created. |
There was a problem hiding this comment.
If no mapping exists, a new one is created
That's not what the implementation seems to do. Is there code that depends on this behavior?
| con = DbConnectionManager.getConnection(); | ||
| pstmt = con.prepareStatement(INSERT_NEW_ROOM); | ||
| pstmt.setLong(1, roomID); | ||
| pstmt.setString(2, roomJID.toString()); |
There was a problem hiding this comment.
Room JIDs should always be bare JIDs. The provided value probably is, but it's good to either check for this and throw an exception if it isn't (be fail fast), or downcast the JID into a bare one (be defensive).
| @@ -0,0 +1,42 @@ | |||
| DECLARE GLOBAL TEMPORARY TABLE SESSION.ofTmpStatus ( | |||
| roomID INTEGER GENERATED ALWAYS AS IDENTITY PRIMARY KEY, | |||
There was a problem hiding this comment.
I don't have a DB2 instance to test this again, but a syntax checker dislikes the GENERATED ALWAYS AS IDENTITY PRIMARY KEY part.
| ALTER TABLE ofConversation ADD COLUMN roomID BIGINT DEFAULT -1 NOT NULL; | ||
| CREATE INDEX ofConversation_room_idx ON ofConversation (roomID); | ||
|
|
||
| UPDATE ofConversation c |
There was a problem hiding this comment.
HSQLDB does not accept this UPDATE query syntax.
| roomDestroyed TINYINT DEFAULT 0 | ||
| ); | ||
|
|
||
| INSERT INTO ofMucRoomStatus (SELECT * FROM #ofTmpStatus); |
There was a problem hiding this comment.
Syntax checker does not like this INSERT query, and fails on the * of the sub-select.
|
|
||
| CREATE TABLE ofConversation ( | ||
| roomID INTEGER NOT NULL, | ||
| conversationID INTEGER NOT NULL, | ||
| room VARCHAR2(1024) NULL, | ||
| isExternal NUMBER(2) NOT NULL, |
There was a problem hiding this comment.
Seems to miss the index that's created for other databases.
|
Closing in favor of #411 |
fixes #369
fixes #370
We can address both #369 and #370 by implementing the handling of the "clear chat history" event from Openfire. Openfire sends a "clear chat history" event in the following scenarios:
A room is destroyed, and no chat history should be preserved.
A "clear room history" command is issued during the lifetime of a chat room.
By tracking the archive, conversation, and participant data using a roomID, we can remove the relevant data when a "clear chat history" event is received.
An ofMucRoomStatus table is used to persistently store mappings between roomID and roomJID. This table also stores information on whether a room was destroyed, ensuring the same room ID cannot be reused and allowing the plugin to determine which rooms are still active. This data is loaded into ConversationManager.roomJIDToIDMap during the static initialisation of the class. Additionally, ofMucRoomStatus is kept up to date whenever roomJIDToIDMap changes. If the Openfire server instance is restarted, the correct and up-to-date room status data will be reloaded.
When a room is destroyed, in addition to ending the room conversation, both ofMucRoomStatus and ConversationManager.roomJIDToIDMap are updated.