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

NPE when closing XContentBuilder and using 'pretty' query parameter #4100

Closed
seallison opened this Issue Nov 5, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@seallison

seallison commented Nov 5, 2013

This is happening in 0.90.6 and didn't happen in 0.90.5.

I created a gist to demonstrate the problem. https://gist.github.com/seallison/7325931

If you don't close the XContentBuilder, the pretty query parameter works fine.

If you do close w/ pretty, the close() method attempts to write a newline character to the end of the closed generator. This creates the NullPointerException as shown in the gist. This line appears to be the culprit:

generator.writeRaw(LF);
@salyh

This comment has been minimized.

Show comment
Hide comment
@salyh

salyh Nov 5, 2013

Contributor

i guess you should not close the builder:


try {
            XContentBuilder builder = RestXContentBuilder.restContentBuilder(request).startObject();
            builder.field("foo", "bar");
            builder.endObject();
            channel.sendResponse(new XContentRestResponse(request, RestStatus.OK, builder));
        }
        catch (Exception e) {
            logger.error("error", e);
        }

this works for me

Contributor

salyh commented Nov 5, 2013

i guess you should not close the builder:


try {
            XContentBuilder builder = RestXContentBuilder.restContentBuilder(request).startObject();
            builder.field("foo", "bar");
            builder.endObject();
            channel.sendResponse(new XContentRestResponse(request, RestStatus.OK, builder));
        }
        catch (Exception e) {
            logger.error("error", e);
        }

this works for me

@seallison

This comment has been minimized.

Show comment
Hide comment
@seallison

seallison Nov 5, 2013

Yes, not closing the builder solves the problem, as I noted in the comment in my gist.

The point is the builder isClosed() should be tested before blindly writing to it.

seallison commented Nov 5, 2013

Yes, not closing the builder solves the problem, as I noted in the comment in my gist.

The point is the builder isClosed() should be tested before blindly writing to it.

@salyh

This comment has been minimized.

Show comment
Hide comment
@salyh

salyh Nov 5, 2013

Contributor

If builder is closed explicitly then sendResponse() failed with a NPE on UTF8JsonGenerator._writeBytes(byte[]) line: 1125 because _outputBuffer in System.arraycopy(bytes, 0, _outputBuffer, _outputTail, len) is null because its already closed.

Maybe a fix could look like this in JsonXContentGenerator


 @Override
    public void close() throws IOException {
        if (writeLineFeedAtEnd && !generator.isClosed()) {
            flush();
            generator.writeRaw(LF);
        }
        generator.close();
    }
Contributor

salyh commented Nov 5, 2013

If builder is closed explicitly then sendResponse() failed with a NPE on UTF8JsonGenerator._writeBytes(byte[]) line: 1125 because _outputBuffer in System.arraycopy(bytes, 0, _outputBuffer, _outputTail, len) is null because its already closed.

Maybe a fix could look like this in JsonXContentGenerator


 @Override
    public void close() throws IOException {
        if (writeLineFeedAtEnd && !generator.isClosed()) {
            flush();
            generator.writeRaw(LF);
        }
        generator.close();
    }
@seallison

This comment has been minimized.

Show comment
Hide comment
@seallison

seallison Nov 5, 2013

Sure, if you don't want the newline character to be added - which was the point of the feature.. maybe leveraging Jackson's pretty printing features would be a solution.

This really is a simple/minor problem, I'm just complaining because I closed all my objects and now need to remove that just to get the same queries that worked in 0.90.5 to work in 0.90.6. Because this wasn't documented as a breaking change, it caught me by surprise.

seallison commented Nov 5, 2013

Sure, if you don't want the newline character to be added - which was the point of the feature.. maybe leveraging Jackson's pretty printing features would be a solution.

This really is a simple/minor problem, I'm just complaining because I closed all my objects and now need to remove that just to get the same queries that worked in 0.90.5 to work in 0.90.6. Because this wasn't documented as a breaking change, it caught me by surprise.

@kimchy

This comment has been minimized.

Show comment
Hide comment
@kimchy

kimchy Nov 6, 2013

Member

yea, unexpected to be honest..., but we can fix it nonetheless..., will push a fix.

Member

kimchy commented Nov 6, 2013

yea, unexpected to be honest..., but we can fix it nonetheless..., will push a fix.

@kimchy kimchy closed this in ebc8975 Nov 6, 2013

mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment