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.

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

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.

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

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();
+                }

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