Conversation
kapybro
Bot
added
status/triage
|
@YeeaaahMan hi, thanks for your contirbution. As this serde is not quite a popular unlike schem registry or glue and it requires external dependencies, we'd rather see this as a separate standalone serde, as I suggested here. |
|
I see. It sounds logical; this should be moved to a separate repository. I’d prefer to leave this to you, as I’m completely unfamiliar with the Java ecosystem and can’t quite estimate the scope of work right now. Alternatively, we could hop on a call to outline a plan, and then I can try to handle it myself. In any case, I’d be happy to help at least partially. |
discussed with @germanosin — messagepack is quite popular and isn't dependency-heavy, we'll include it as a built-in serde. Please take a look at the inline comment I left, the rest is good to go. |
Haarolean
added
scope/backend
|
@YeeaaahMan also please add some tests |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new built-in MessagePack serde, registers it in the serde initializer, updates Gradle version catalog and Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Registry as SerdeRegistry
participant Serde as MessagePackSerde
participant Unpacker as MessageUnpacker
participant Mapper as ObjectMapper
Client->>Registry: request serde (by name or auto-select)
Registry->>Serde: return MessagePackSerde instance
alt Deserialization
Client->>Serde: provide MessagePack bytes
Serde->>Unpacker: create unpacker from bytes
Unpacker-->>Serde: unpack Value
Serde->>Mapper: convert Value -> JSON string
Serde-->>Client: return DeserializeResult(type=STRING, payload=string)
else Serialization
Client->>Serde: provide JSON string
Serde->>Mapper: parse JSON -> JsonNode
Serde->>Mapper: write JsonNode -> MessagePack bytes
Serde-->>Client: return MessagePack bytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/test/java/io/kafbat/ui/serdes/builtin/MessagePackSerdeTest.java (1)
59-67: Add one malformed-payload test for error-path regression safety.Current tests cover only valid bytes; adding an invalid payload assertion will lock down failure behavior.
Proposed test addition
import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import io.kafbat.ui.serde.api.DeserializeResult; import io.kafbat.ui.serde.api.Serde; import io.kafbat.ui.serdes.PropertyResolverImpl; import io.kafbat.ui.serdes.RecordHeadersImpl; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; @@ `@ParameterizedTest` `@EnumSource` void deserializesDataAsMessagePackBytes(Serde.Target type) { var deserializer = msgPackSerde.deserializer("anyTopic", type); var result = deserializer.deserialize(new RecordHeadersImpl(), TEST_BYTES); assertThat(result.getResult()).isEqualTo(TEST_STRING); assertThat(result.getType()).isEqualTo(DeserializeResult.Type.STRING); assertThat(result.getAdditionalProperties()).isEmpty(); } + + `@Test` + void throwsOnMalformedMessagePackPayload() { + var deserializer = msgPackSerde.deserializer("anyTopic", Serde.Target.VALUE); + assertThatThrownBy(() -> deserializer.deserialize(new RecordHeadersImpl(), new byte[] {(byte) 0xc1})) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Failed to deserialize MessagePack payload"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/test/java/io/kafbat/ui/serdes/builtin/MessagePackSerdeTest.java` around lines 59 - 67, Add a new parameterized test in MessagePackSerdeTest (alongside deserializesDataAsMessagePackBytes) that uses msgPackSerde.deserializer("anyTopic", type) to deserialize a deliberately malformed byte array (e.g., invalid MessagePack bytes) via deserializer.deserialize(new RecordHeadersImpl(), MALFORMED_BYTES) and assert the failure path: result.getType() equals DeserializeResult.Type.FAIL, result.getResult() is null, and result.getAdditionalProperties() is not empty (or contains an error key/message) to lock down error behavior for malformed payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/test/java/io/kafbat/ui/serdes/builtin/MessagePackSerdeTest.java`:
- Around line 59-67: Add a new parameterized test in MessagePackSerdeTest
(alongside deserializesDataAsMessagePackBytes) that uses
msgPackSerde.deserializer("anyTopic", type) to deserialize a deliberately
malformed byte array (e.g., invalid MessagePack bytes) via
deserializer.deserialize(new RecordHeadersImpl(), MALFORMED_BYTES) and assert
the failure path: result.getType() equals DeserializeResult.Type.FAIL,
result.getResult() is null, and result.getAdditionalProperties() is not empty
(or contains an error key/message) to lock down error behavior for malformed
payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 976bed64-d085-4669-8310-91864bc55026
📒 Files selected for processing (5)
api/build.gradleapi/src/main/java/io/kafbat/ui/serdes/SerdesInitializer.javaapi/src/main/java/io/kafbat/ui/serdes/builtin/MessagePackSerde.javaapi/src/test/java/io/kafbat/ui/serdes/builtin/MessagePackSerdeTest.javagradle/libs.versions.toml
|
I found some inconsistencies in the serializer's behavior and removed it. If we decide to add a serializer (used in the "Produce Message" dialog), the logic should be: This would require a new dependency: org.msgpack.jackson-dataformat-msgpack. But is it worth it? My main goal was to provide a convenient way to view messages in msgpack format, and the current deserializer handles that perfectly. Any other functionality can be added later. Guys, what do you think? |
I don't mind either personally. We could start with deserialization and implement serialization in a separate task later if requested |
…ation + update tests
|
Serialization was added. Many corner cases were tested and covered by unit tests. De-serialization also is ok. Ready to merge. |
…po, and add tests
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
3 participants
What changes did you make? (Give an overview)
Fixes #951
Add MessagePackSerde which supports msgpack format. https://msgpack.org/
Serialization and de serialization works, new option is available in Messages preview tab.
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
I used https://msgpack.solder.party/ to define basic objects: string, list, dict.
Then copied HEX representation to the Produce Message dialog.
As result all messages are displayed correctly when MessagePack serde is selected.
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Summary by CodeRabbit
New Features
Tests