-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
imp: ByteBufferSerde #604
imp: ByteBufferSerde #604
Conversation
Based on Jackson implementation: https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/ser/std/ByteBufferSerializer.java https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/deser/std/ByteBufferDeserializer.java Not 100% complete see NativeTest excluded in the serde tck
import org.junit.platform.suite.api.Suite; | ||
import org.junit.platform.suite.api.SelectPackages; | ||
import org.junit.platform.suite.api.SuiteDisplayName; | ||
|
||
@Suite | ||
@ExcludeClassNamePatterns( | ||
"io.micronaut.serde.tck.tests.bytebuffer.ByteBufferNativeTest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is excluded. I don't see how to easily port this port this part of the serializer
https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/ser/std/ByteBufferSerializer.java#L27-L35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since youre copying the data anyway, you can just copy it for direct buffers too. but ideally there should be overloads for encodeBinary for ByteBuffers.
|
||
private void encodeByteBuffer(@NonNull Encoder encoder, | ||
@NonNull ByteBuffer value) throws IOException { | ||
if (value.hasArray()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and what if not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could take inspiration from Jackson ad do something like
diff --git a/serde-support/src/main/java/io/micronaut/serde/support/serdes/ByteBufferSerde.java b/serde-support/src/main/java/io/micronaut/serde/support/serdes/ByteBufferSerde.java
index 06e03a2f..12bc6b8d 100644
--- a/serde-support/src/main/java/io/micronaut/serde/support/serdes/ByteBufferSerde.java
+++ b/serde-support/src/main/java/io/micronaut/serde/support/serdes/ByteBufferSerde.java
@@ -58,6 +58,43 @@ public class ByteBufferSerde implements Serde<ByteBuffer> {
byte[] copy = new byte[len];
System.arraycopy(value.array(), offset, copy, 0, len);
encoder.encodeBinary(copy);
+ return;
+ }
+ try (InputStream s = new ByteBufferInputStream(value.asReadOnlyBuffer().rewind())) {
+ encoder.encodeBinary(s.readAllBytes());
+ }
+ }
+
+ private static class ByteBufferInputStream extends InputStream {
+
+ private static final int EOS = -1;
+ private final ByteBuffer buffer;
+
+ private ByteBufferInputStream(ByteBuffer buffer) {
+ this.buffer = buffer;
+ }
+
+ @Override
+ public int available() {
+ return buffer.remaining();
+ }
+
+ @Override
+ public int read() {
+ if (buffer.hasRemaining()) {
+ return buffer.get() & 0xFF;
+ }
+ return EOS;
+ }
+
+ @Override
+ public int read(byte[] b, int off, int len) {
+ if (buffer.hasRemaining()) {
+ len = Math.min(len, buffer.remaining());
+ buffer.get(b, off, len);
+ return len;
+ }
+ return EOS;
}
}
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'm not sure my use of rewind
is correct here... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding the following commit efe6b37
theres easier ways, i will look at it tomorrow. |
ive implemented a smaller copy mechanism. also i discovered a jackson-databind bug which might break the tck: FasterXML/jackson-databind#4164 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timyates can you review and approve. I cannot since I created the PR
...uite-tck-serde/src/test/java/io/micronaut/serde/tck/tests/serde/SerializationSerdeSuite.java
Outdated
Show resolved
Hide resolved
…s/serde/SerializationSerdeSuite.java Co-authored-by: Sergio del Amo <[email protected]>
SonarCloud Quality Gate failed. 0 Bugs 2.1% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Close: #603
Based on Jackson implementation:
https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/ser/std/ByteBufferSerializer.java
https://github.com/FasterXML/jackson-databind/blob/2.16/src/main/java/com/fasterxml/jackson/databind/deser/std/ByteBufferDeserializer.java
Not 100% complete see NativeTest excluded in the serde tck