[ECO-5193] Support message edits and deletes

[sacOO7]

• Fixed Support message edits and deletes  #1058

TODO

• TM2l = refSerial string – an opaque string that uniquely identifies some referenced message.
• TM2m = refType string – an opaque string that identifies the type of this reference.
• TM2n = operation – object that contain clientId, description and metadata.
• Add integration tests for message create, update and delete to check if fields are being populated correctly.
• Test for Update/ delete op for existing deleted message.

Summary by CodeRabbit

• New Features

  • Added support for message operations with new fields like refSerial, refType, and operation.
  • Introduced enhanced message handling capabilities for chat rooms.
  • Implemented new serialization and deserialization methods for message operations.

• Tests

  • Added comprehensive test coverage for message creation, updating, and deletion.
  • Verified message serialization and deserialization processes, including the new operation details.
  • Created a test suite for chat message functionality.

[coderabbitai]

Walkthrough

The pull request introduces support for message edits and deletes in the Ably library by extending the Message class with new fields and methods. A new Operation inner class is added to capture metadata about message modifications, including client ID, description, and additional metadata. The changes enable serialization and deserialization of these new message operation details through both JSON and MessagePack formats. Corresponding test classes are added to validate the new functionality for sending, updating, and deleting messages in a chat room context.

Changes

File	Change Summary
lib/src/main/java/io/ably/lib/types/Message.java	Added new fields refSerial, refType, and operation to support message operations. Implemented serialization and deserialization methods for the new Operation class.
lib/src/test/java/io/ably/lib/chat/ChatMessagesTest.java	New test class for validating message operations like sending, updating, and deleting messages.
lib/src/test/java/io/ably/lib/chat/ChatRoom.java	Added methods for sending, updating, and deleting messages with parameter classes and request handling.
lib/src/test/java/io/ably/lib/types/MessageTest.java	Added test methods to verify serialization and deserialization of messages with operation details.

Assessment against linked issues

Objective	Addressed	Explanation
Support message edits and deletes [#1058]	✅
Implement TM* field changes [ECO-5193]	✅

Possibly related PRs

• [ECO-5188] MessageAction enum changes #1056: Changes in MessageTest.java regarding the expected values for the "action" property during serialization and deserialization are directly related to the modifications in the Message class.

Suggested reviewers

• ttypic

Poem

🐰 In the realm of chat, messages dance and play,
Edits and deletes, now find their way!
With Operation magic, we rewrite the tale,
Serialized bytes that never fail!
CodeRabbit's wisdom, in every line we see! 🚀

✨ Finishing Touches

• 📝 Generate Docstrings (Beta)

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (7)

lib/src/test/java/io/ably/lib/chat/ChatRoom.java (1)

3-5: Reuse Gson instance to improve performance

Currently, a new Gson instance is created in each method. Consider defining a single Gson instance as a class-level field to reuse across methods, which can improve performance by reducing object creation overhead.

Apply this diff to refactor the Gson instance:

+import static io.ably.lib.util.Serialisation.gson;

...

-public JsonElement sendMessage(SendMessageParams params) throws AblyException {
+public JsonElement sendMessage(SendMessageParams params) throws AblyException {
     return makeAuthorizedRequest("/chat/v2/rooms/" + roomId + "/messages", "POST", gson.toJsonTree(params))
         .orElseThrow(() -> new AblyException("Failed to send message", 500, 50000));
}

-public JsonElement updateMessage(String serial, UpdateMessageParams params) throws AblyException {
+public JsonElement updateMessage(String serial, UpdateMessageParams params) throws AblyException {
     return makeAuthorizedRequest("/chat/v2/rooms/" + roomId + "/messages/" + serial, "PUT", gson.toJsonTree(params))
         .orElseThrow(() -> new AblyException("Failed to update message", 500, 50000));
}

-public JsonElement deleteMessage(String serial, DeleteMessageParams params) throws AblyException {
+public JsonElement deleteMessage(String serial, DeleteMessageParams params) throws AblyException {
     return makeAuthorizedRequest("/chat/v2/rooms/" + roomId + "/messages/" + serial + "/delete", "POST", gson.toJsonTree(params))
         .orElseThrow(() -> new AblyException("Failed to delete message", 500, 50000));
}

lib/src/test/java/io/ably/lib/types/MessageTest.java (1)

Line range hint 99-232: Add test cases with null fields to ensure robustness

In the tests serialize_message_with_operation, deserialize_message_with_operation, and serialize_and_deserialize_with_msgpack, all fields of the Operation object are populated. To ensure that serialization and deserialization handle null values correctly, consider adding test cases where optional fields (clientId, description, metadata) are set to null. This will help validate that your implementation gracefully handles missing data.

Example of modifying the test:

@Test
public void serialize_and_deserialize_with_msgpack() throws Exception {
    // Given
    Message message = new Message("test-name", "test-data");
    message.clientId = "test-client-id";
    message.connectionKey = "test-key";
    message.refSerial = "test-ref-serial";
    message.refType = "test-ref-type";
    message.action = MessageAction.MESSAGE_CREATE;
    message.serial = "01826232498871-001@abcdefghij:001";
    Message.Operation operation = new Message.Operation();
    operation.clientId = null; // Set to null
    operation.description = "operation-description";
    operation.metadata = null; // Set to null
    message.operation = operation;

    // When Encode to MessagePack
    ...

    // Then
    assertEquals(null, unpacked.operation.clientId);
    assertEquals("operation-description", unpacked.operation.description);
    assertNull(unpacked.operation.metadata);
}

lib/src/test/java/io/ably/lib/chat/ChatMessagesTest.java (5)

65-65: Consider extracting the magic number timeout value.

The timeout value of 10,000ms should be extracted as a constant to improve maintainability.

+ private static final int MESSAGE_WAIT_TIMEOUT = 10_000;
- Exception err = new Helpers.ConditionalWaiter().wait(() -> !receivedMsg.isEmpty(), 10_000);
+ Exception err = new Helpers.ConditionalWaiter().wait(() -> !receivedMsg.isEmpty(), MESSAGE_WAIT_TIMEOUT);

---

107-109: Enhance resource cleanup in finally block.

The ably.close() call should be wrapped in a try-catch block to ensure any cleanup exceptions are logged but don't mask the original test exception.

 finally {
-    if(ably != null)
-        ably.close();
+    if(ably != null) {
+        try {
+            ably.close();
+        } catch (Exception e) {
+            e.printStackTrace();
+        }
+    }
 }

---

323-438: Consider splitting this test into smaller, focused tests.

This test method is quite long and tests multiple aspects of message lifecycle. Consider splitting it into smaller tests using @TestFactory or parameterized tests to improve maintainability and readability.

Example approach:

@TestFactory
Stream<DynamicTest> messageLifecycleTests() {
    return Stream.of(
        DynamicTest.dynamicTest("Message creation", () -> {
            // Test message creation
        }),
        DynamicTest.dynamicTest("Message update after creation", () -> {
            // Test message update
        }),
        DynamicTest.dynamicTest("Message deletion after update", () -> {
            // Test message deletion
        })
    );
}

---

511-513: Enhance assertions for message state verification.

The current assertion only verifies the serial number. Consider adding more specific assertions to verify the state and content of messages at each stage.

 for (Message msg : receivedMsg) {
     Assert.assertEquals("Serial should match original serial", originalSerial, msg.serial);
+    switch(msg.action) {
+        case MESSAGE_CREATE:
+            Assert.assertEquals("hello there", ((JsonObject)msg.data).get("text").getAsString());
+            break;
+        case MESSAGE_UPDATE:
+            Assert.assertEquals("updated text", ((JsonObject)msg.data).get("text").getAsString());
+            break;
+        case MESSAGE_DELETE:
+            Assert.assertNotNull("Delete operation should be present", msg.operation);
+            Assert.assertEquals("message deleted by clientId1", msg.operation.description);
+            break;
+    }
 }

---

1-523: Consider implementing a custom test framework for chat operations.

Given the repetitive nature of these tests and common patterns, consider creating a custom test framework or JUnit extensions that would:

1. Handle common setup/teardown operations
2. Provide fluent assertions for chat messages
3. Manage connection lifecycle
4. Handle async operations with proper timeouts

This would significantly reduce code duplication and make tests more maintainable.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bbb3ec2 and 4610d4d.

📒 Files selected for processing (4)

• lib/src/main/java/io/ably/lib/types/Message.java (9 hunks)
• lib/src/test/java/io/ably/lib/chat/ChatMessagesTest.java (1 hunks)
• lib/src/test/java/io/ably/lib/chat/ChatRoom.java (1 hunks)
• lib/src/test/java/io/ably/lib/types/MessageTest.java (4 hunks)

🔇 Additional comments (1)

lib/src/test/java/io/ably/lib/chat/ChatMessagesTest.java (1)

1-20: LGTM! Class structure and imports are well organized.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

lib/src/main/java/io/ably/lib/types/Message.java (1)

96-98: Consider using a more flexible metadata type.

The current Map<String, String> type for metadata might be limiting. Consider using Map<String, Object> or JsonObject to support various data types in metadata, improving flexibility for future use cases.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4610d4d and 6f7a983.

📒 Files selected for processing (2)

• lib/src/main/java/io/ably/lib/types/Message.java (9 hunks)
• lib/src/test/java/io/ably/lib/chat/ChatRoom.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/src/test/java/io/ably/lib/chat/ChatRoom.java

⏰ Context from checks skipped due to timeout of 90000ms (10)

• GitHub Check: check-rest-okhttp
• GitHub Check: check (29)
• GitHub Check: check (24)
• GitHub Check: check-rest
• GitHub Check: check-realtime-okhttp
• GitHub Check: check (21)
• GitHub Check: build
• GitHub Check: check (19)
• GitHub Check: check-realtime
• GitHub Check: check

🔇 Additional comments (5)

lib/src/main/java/io/ably/lib/types/Message.java (5)

80-94: LGTM! Well-documented field declarations.

The new fields are properly documented with spec references and maintain consistency with the existing codebase.

---

95-172: Well-implemented Operation class with forward compatibility.

The Operation class implementation is robust with:

• Efficient serialization of non-null fields
• Proper handling of null values
• Forward compatibility through default cases in deserialization

---

Line range hint 263-313: LGTM! Consistent serialization implementation.

The MessagePack serialization changes maintain consistency with the existing pattern and include proper null checks.

Also applies to: 343-349

---

516-522: LGTM! Robust JSON handling implementation.

The JSON serialization/deserialization changes include:

• Proper type checking
• Clear error messages
• Consistent handling with existing fields

Also applies to: 550-558

---

95-172: Verify unit test coverage for Operation class.

Please ensure there are comprehensive unit tests for the Operation class, covering:

• Serialization/deserialization of all fields
• Handling of null values
• Edge cases with empty/null metadata

✅ Verification successful

Operation class has sufficient test coverage

The Operation class is well-tested through:

• Comprehensive serialization tests (JSON and MessagePack)
• Full coverage of all fields (clientId, description, metadata)
• Integration tests in chat functionality
• Edge cases via Message class tests

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Operation class tests
rg -l "class.*Test.*Operation" "lib/src/test"

Length of output: 45

---

Script:

#!/bin/bash
# Search for Message.Operation usage in test files
echo "=== Searching for Message.Operation usage in tests ==="
rg "Message\.Operation" "lib/src/test" -B 1 -A 1

echo -e "\n=== Searching for test methods related to Operation ==="
rg "void.*test.*[Oo]peration.*\(" "lib/src/test" -B 1 -A 1

echo -e "\n=== Searching for Message test files ==="
rg -l "class.*MessageTest" "lib/src/test"

Length of output: 2426

---

Script:

#!/bin/bash
# Search for relevant test methods in MessageTest.java
echo "=== Searching MessageTest.java for serialization tests ==="
rg "test.*serializ|test.*json|test.*msgpack" "lib/src/test/java/io/ably/lib/types/MessageTest.java" -B 1 -A 10

echo -e "\n=== Showing context around Operation usage in MessageTest.java ==="
rg -A 20 -B 2 "Operation operation = new Message\.Operation\(\)" "lib/src/test/java/io/ably/lib/types/MessageTest.java"

Length of output: 6116

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

lib/src/main/java/io/ably/lib/types/Message.java (2)

98-98: Consider making the metadata map immutable.

The metadata map could be modified after creation, which might lead to unexpected behavior. Consider making it immutable to prevent modifications after creation.

-    public Map<String, String> metadata;
+    public final Map<String, String> metadata;

Also, consider returning an unmodifiable view in the read methods:

-            operation.metadata = new HashMap<>(mapSize);
+            Map<String, String> modifiableMap = new HashMap<>(mapSize);
             // ... populate the map ...
+            operation.metadata = Collections.unmodifiableMap(modifiableMap);

---

343-349: Consider using a switch statement for field deserialization.

The current if-else chain could be refactored to use a switch statement for better maintainability and consistency with the Operation class.

-            }  else if (fieldName.equals(REF_SERIAL)) {
-                refSerial = unpacker.unpackString();
-            } else if (fieldName.equals(REF_TYPE)) {
-                refType = unpacker.unpackString();
-            } else if (fieldName.equals(OPERATION)) {
-                operation = Operation.read(unpacker);
+            } else {
+                switch (fieldName) {
+                    case REF_SERIAL:
+                        refSerial = unpacker.unpackString();
+                        break;
+                    case REF_TYPE:
+                        refType = unpacker.unpackString();
+                        break;
+                    case OPERATION:
+                        operation = Operation.read(unpacker);
+                        break;
+                    default:
+                        Log.v(TAG, "Unexpected field: " + fieldName);
+                        unpacker.skipValue();
+                }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f7a983 and c3264ea.

📒 Files selected for processing (2)

• lib/src/main/java/io/ably/lib/types/Message.java (9 hunks)
• lib/src/test/java/io/ably/lib/chat/ChatRoom.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/src/test/java/io/ably/lib/chat/ChatRoom.java

⏰ Context from checks skipped due to timeout of 90000ms (10)

• GitHub Check: check-rest-okhttp
• GitHub Check: check (29)
• GitHub Check: check (24)
• GitHub Check: check-rest
• GitHub Check: check (21)
• GitHub Check: check-realtime-okhttp
• GitHub Check: build
• GitHub Check: check (19)
• GitHub Check: check-realtime
• GitHub Check: check

🔇 Additional comments (7)

lib/src/main/java/io/ably/lib/types/Message.java (7)

80-94: LGTM! Well-documented fields with clear spec references.

The new fields refSerial, refType, and operation are properly documented with their respective spec references (TM2l, TM2m, TM2n).

---

95-172: LGTM! Well-structured Operation class with robust serialization.

The Operation class implementation is solid with proper null checks, efficient map initialization, and graceful handling of unknown fields.

---

181-183: LGTM! Constants follow existing naming pattern.

The new constants REF_SERIAL, REF_TYPE, and OPERATION are consistent with the existing naming convention.

---

268-271: LGTM! Proper MessagePack serialization for new fields.

The serialization logic correctly handles null checks and follows the existing pattern for writing fields.

Also applies to: 302-313

---

513-522: LGTM! Robust JSON deserialization with proper type checking.

The JSON deserialization properly validates the operation field type and handles errors appropriately.

---

550-558: LGTM! Proper JSON serialization for new fields.

The serialization logic correctly handles null checks and follows the existing pattern for writing fields.

---

Line range hint 1-583: Verify test coverage for new message operations.

Please ensure comprehensive test coverage for the new message operations functionality, including:

1. Serialization/deserialization of messages with operations
2. Edge cases with null/empty operation fields
3. Invalid operation JSON/MessagePack formats

✅ Verification successful

Test coverage verified for Message Operation functionality

The codebase has comprehensive test coverage for the new Message Operation functionality:

• Core Operation class functionality in MessageTest.java
• Integration testing in ChatMessagesTest.java
• Both JSON and MessagePack serialization formats
• All Operation fields (clientId, description, metadata)
• Edge cases and error handling

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check test coverage for new message operations

# Look for test files
echo "Searching for relevant test files..."
fd -e java -e kt test.*Message

# Search for operation-related tests
echo -e "\nSearching for operation-related tests..."
rg -l "operation.*test" -t java -t kotlin

# Look for specific test cases
echo -e "\nSearching for specific test cases..."
rg "test.*operation" -A 10 -t java -t kotlin

Length of output: 1518

---

Script:

#!/bin/bash
# Broader search for Message-related tests

echo "Searching for Message class usage in tests..."
rg "class.*Message.*Test|extends.*Message" -t java

echo -e "\nSearching for Message serialization/deserialization tests..."
rg "fromEncoded|writeMsgpack|readMsgpack.*Message" -B 2 -A 5 -t java

echo -e "\nSearching for Operation-related code in tests..."
rg "Operation.*\{|new Operation\(|operation\." -B 2 -A 5 -t java

echo -e "\nLooking for test files that might contain Message tests..."
fd -e java test

Length of output: 67090