Skip to content
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

ByteBuffer serialization is broken if offset is not 0 #1662

Closed
j-baker opened this issue Jun 15, 2017 · 2 comments
Closed

ByteBuffer serialization is broken if offset is not 0 #1662

j-baker opened this issue Jun 15, 2017 · 2 comments
Milestone

Comments

@j-baker
Copy link

j-baker commented Jun 15, 2017

This happens with all Jackson versions. The below test sadly passes:

    @Test
    public void byteBufferInJacksonIsBroken() throws IOException {
        ByteBuffer byteBuffer = ByteBuffer.allocate(2);
        byteBuffer.put((byte) 1);
        byteBuffer.put((byte) 2);
        byteBuffer.position(1);
        ByteBuffer slice = byteBuffer.slice();
        assertThat(slice.position()).isZero();
        assertThat(slice.capacity()).isEqualTo(1);
        ByteBuffer serde = serde(slice);
        assertThat(serde.position()).isZero();
        assertThat(serde.capacity()).isEqualTo(2);
        assertThat(serde.get()).isEqualTo((byte) 1);
        assertThat(slice.get()).isEqualTo((byte) 2);
    }

    private ByteBuffer serde(ByteBuffer t) throws IOException {
        ObjectMapper mapper = new ObjectMapper();
        return mapper.readValue(mapper.writeValueAsBytes(t), ByteBuffer.class);
    }

There are two bugs, really:

  • Java ByteBuffer equality is defined by the bytes remaining being equivalent, but Jackson rewinds the buffer so Jackson will happily serialise bytes which are not in the view. This is not visible above, but if instead of calling byteBuffer.slice() you call byteBuffer.duplicate() you will have the same issue (but slice.position() is not zero and the lengths are the same).
  • Secondly, if the ByteBuffer has wrapped an array, Jackson assumes that the ByteBuffer starts at the start of the array, which is not true.

What kind of fix would be accepted here?

My ideal scenario is that ByteBufferSerializer is rewritten to

public class ByteBufferSerializer extends StdScalarSerializer<ByteBuffer>
{
    public ByteBufferSerializer() { super(ByteBuffer.class); }

    @Override
    public void serialize(ByteBuffer bbuf, JsonGenerator gen, SerializerProvider provider) throws IOException
    {
        if (bbuf.hasArray()) {
            gen.writeBinary(bbuf.array(), bbuf.arrayOffset() + bbuf.position(), bbuf.arrayOffset() + bbuf.limit());
            return;
        }
        ByteBuffer copy = bbuf.asReadOnlyBuffer();
        InputStream in = new ByteBufferBackedInputStream(copy);
        gen.writeBinary(in, copy.remaining());
        in.close();
    }
}

but I could also see it being rewritten to

public class ByteBufferSerializer extends StdScalarSerializer<ByteBuffer>
{
    public ByteBufferSerializer() { super(ByteBuffer.class); }

    @Override
    public void serialize(ByteBuffer bbuf, JsonGenerator gen, SerializerProvider provider) throws IOException
    {
        if (bbuf.hasArray()) {
            gen.writeBinary(bbuf.array(), bbuf.arrayOffset(), bbuf.arrayOffset() + bbuf.limit());
            return;
        }
        ByteBuffer copy = bbuf.asReadOnlyBuffer();
        if (copy.position() > 0) {
            copy.rewind();
        }
        InputStream in = new ByteBufferBackedInputStream(copy);
        gen.writeBinary(in, copy.remaining());
        in.close();
    }
}

- I accept that my former solution changes behaviour a little more drastically (but makes it consistent with equals()).

@cowtowncoder
Copy link
Member

This might be something to bring up on mailing lists -- with a link to this issue. Since I don't use ByteBuffer for anything myself, I am not the best person to comment on how interaction ideally should work. I assume your suggestions would be an improvement, but it would be great to get additional feedback from users that may be trying to use the functionality (unless it's broken enough not to really be usable).

marc-christian-schulze added a commit to marc-christian-schulze/jackson-databind that referenced this issue Sep 24, 2018
@cowtowncoder cowtowncoder added 2.9 and removed 2.10 labels Oct 22, 2018
@cowtowncoder cowtowncoder changed the title ByteBuffer serialization is broken ByteBuffer serialization is broken if offset is not 0 Oct 22, 2018
cowtowncoder added a commit that referenced this issue Oct 23, 2018
@cowtowncoder cowtowncoder added this to the 2.9.8 milestone Oct 24, 2018
@cowtowncoder
Copy link
Member

Was included in 2.9.8

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants