Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the message queue system to use byte arrays instead of strings for payload storage, enabling better handling of binary data. The changes also remove message ID functionality for simplification and implement Base64 encoding for file persistence.
Key changes:
- Refactored
Messageclass to store payloads asbyte[]with helper methods for string conversion - Updated serialization to use Base64 encoding for safe file storage of binary data
- Removed message ID field and related functionality from the entire codebase
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/ofek/queue/Message.java | Core refactor to byte array storage, removed ID field, added string conversion methods |
| src/main/java/com/ofek/queue/MessageQueue.java | Updated file persistence to use Base64 encoding/decoding for binary data |
| src/main/java/com/ofek/queue/Producer.java | Added overload method to accept byte array payloads directly |
| src/test/java/com/ofek/queue/MessageTest.java | Comprehensive test updates for byte array functionality and removed ID-related tests |
| src/test/java/com/ofek/queue/MessageQueueTest.java | Updated assertions to use getPayloadAsString() and Base64 decoding for file validation |
| src/test/java/com/ofek/queue/ProducerTest.java | Updated test assertions and removed ID uniqueness tests |
| src/test/java/com/ofek/queue/ConsumerTest.java | Updated all payload assertions to use new string conversion method |
| src/test/java/com/ofek/queue/IntegrationTest.java | Updated integration test assertions for new payload access pattern |
| Message message = new Message(payload); | ||
| queue.offer(message); | ||
| loadedCount++; | ||
| } |
There was a problem hiding this comment.
There is an unmatched closing brace. The if statement on line 189 that checks parts.length == 2 was removed, but its closing brace remains, creating a syntax error.
| String line = lines.get(i); | ||
| assertTrue(line.contains("Batch message " + i)); | ||
| assertTrue(line.contains("|")); // ID|payload format | ||
| String decodedMessage = new String(Base64.getDecoder().decode(lines.get(i))); |
There was a problem hiding this comment.
The Base64 decoding should specify the character encoding explicitly. Use new String(Base64.getDecoder().decode(lines.get(i)), StandardCharsets.UTF_8) to ensure consistent encoding behavior across different platforms.
| for (int i = 0; i < messageCount; i++) { | ||
| String line = lines.get(i); | ||
| assertTrue(line.contains("Small batch message " + i)); | ||
| String decodedMessage = new String(Base64.getDecoder().decode(line)); |
There was a problem hiding this comment.
The Base64 decoding should specify the character encoding explicitly. Use new String(Base64.getDecoder().decode(line), StandardCharsets.UTF_8) to ensure consistent encoding behavior across different platforms.
| String decodedMessage = new String(Base64.getDecoder().decode(line)); | |
| String decodedMessage = new String(Base64.getDecoder().decode(line), StandardCharsets.UTF_8); |
| for (int i = 0; i < messageCount; i++) { | ||
| String line = lines.get(i); | ||
| assertTrue(line.contains("Large batch message " + i)); | ||
| String decodedMessage = new String(Base64.getDecoder().decode(line)); |
There was a problem hiding this comment.
The Base64 decoding should specify the character encoding explicitly. Use new String(Base64.getDecoder().decode(line), StandardCharsets.UTF_8) to ensure consistent encoding behavior across different platforms.
| String decodedMessage = new String(Base64.getDecoder().decode(line)); | |
| String decodedMessage = new String(Base64.getDecoder().decode(line), StandardCharsets.UTF_8); |
| List<String> lines = Files.readAllLines(testFile); | ||
| assertEquals(1, lines.size()); | ||
| assertTrue(lines.get(0).contains("Before shutdown")); | ||
| String decodedMessage = new String(Base64.getDecoder().decode(lines.get(0))); |
There was a problem hiding this comment.
The Base64 decoding should specify the character encoding explicitly. Use new String(Base64.getDecoder().decode(lines.get(0)), StandardCharsets.UTF_8) to ensure consistent encoding behavior across different platforms.
| String decodedMessage = new String(Base64.getDecoder().decode(lines.get(0))); | |
| String decodedMessage = new String(Base64.getDecoder().decode(lines.get(0)), StandardCharsets.UTF_8); |
1 participant
This pull request refactors the
MessageandMessageQueueclasses to improve payload handling by switching fromStringtobyte[]for payload storage. It also updates serialization to use Base64 encoding for safer storage and modifies tests to reflect these changes.Core Refactoring and Enhancements:
Messageclass to store payloads asbyte[]instead ofString, added helper methods for encoding/decoding payloads as strings, and removed theidfield for simplicity. (src/main/java/com/ofek/queue/Message.java)MessageQueueto encode message payloads as Base64 when saving to files and decode them when loading. (src/main/java/com/ofek/queue/MessageQueue.java) [1] [2]API Changes:
produce: Added a method inProducerto acceptbyte[]payloads directly. (src/main/java/com/ofek/queue/Producer.java)Test Updates:
getPayloadAsString()for assertions instead of directly accessinggetPayload(). (src/test/java/com/ofek/queue/ConsumerTest.java,src/test/java/com/ofek/queue/IntegrationTest.java,src/test/java/com/ofek/queue/MessageQueueTest.java) [1] [2] [3]src/test/java/com/ofek/queue/MessageQueueTest.java) [1] [2] [3]Cleanup:
idfield and related logic fromMessageand associated tests, simplifying the class. (src/main/java/com/ofek/queue/Message.java,src/test/java/com/ofek/queue/MessageQueueTest.java) [1] [2]These changes improve the flexibility of payload handling, ensure safe storage of binary data, and streamline the codebase.