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

Add fromXContent method to SearchResponse #24720

Conversation

@javanna
Copy link
Member

commented May 16, 2017

SearchResponse#fromXContent allows to parse a search response, including search hits, aggregations, suggestions and profile results. Only the aggs that we can parse today are supported (which means all of them but a couple that are left to support). SearchResponseTests reuses the existing test infra to randomize aggregations, suggestions and profile response.

This PR contains some of the changes proposed with #22533 and add extensive tests for it based on the existing test infra for parsing responses.

Relates to #23331

Add fromXContent method to SearchResponse
SearchResponse#fromXContent allows to parse a search response, including search hits, aggregations, suggestions and profile results. Only the aggs that we can parse today are supported (which means all of them but a couple that are left to support). SearchResponseTests reuses the existing test infra to randomize aggregations, suggestions and profile response.

Relates to #23331

@javanna javanna force-pushed the javanna:enhancement/search_response_parsing branch to b8ad84e May 16, 2017

}
RestActions.buildBroadcastShardsHeader(builder, params, getTotalShards(), getSuccessfulShards(), getFailedShards(),
getShardFailures());
internalResponse.toXContent(builder, params);
return builder;
}

public static SearchResponse fromXContent(XContentParser parser) throws IOException {
XContentParser.Token token;
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.currentToken(), parser::getTokenLocation);

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

nit: can you move the ensureExpectedToken() first?

XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
SearchResponse parsed = SearchResponse.fromXContent(parser);
// check that we at least get the same number of shardFailures
assertEquals(response.getShardFailures().length, parsed.getShardFailures().length);

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

We have some tests that create a failure instance and its expected instance, maybe we could have something similar here and use ElasticsearchExceptionTests.randomExceptions()?

boolean humanReadable = randomBoolean();
final ToXContent.Params params = new ToXContent.MapParams(singletonMap(RestSearchAction.TYPED_KEYS_PARAM, "true"));
BytesReference originalBytes = toShuffledXContent(response, xcontentType, params, humanReadable);
XContentParser parser = createParser(xcontentType.xContent(), originalBytes);

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

Can you use try-with-resources here so that resources are released asap?

@javanna

This comment has been minimized.

Copy link
Member Author

commented May 17, 2017

@tlrx can you please check if my latest commit addresses your concerns?

@cbuescher
Copy link
Member

left a comment

@javanna left one general question about ignoring unknown fields in the parser and a small nit about a test, otherwise LGTM

} else if (NUM_REDUCE_PHASES.match(currentFieldName)) {
numReducePhases = parser.intValue();
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Member

We started being lenient with other parts of the response in order to allow new fields in the future to work with an older client. Should we also do this on this level? This applies to all similar checks in this PR.

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Author Member

I think we are free to do whatever we want to do until we test that leniency. I went for using throwUnknownField as that one may one day conditionally throw. Or maybe we have to remove it. But once we test this behaviour, we will have to go back to each parsing method and check that we do the right thing

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Member

fine with me.

+ "\"max_score\":1.5,"
+ "\"hits\":[{\"_type\":\"type\",\"_id\":\"id1\",\"_score\":2.0}]"
+ "}"
+ "}", Strings.toString(response));

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Member

Although I liked it at first, I'm starting to doubt the usefulness of these tests. And formatting makes it unreadable. If we keep it, can you change this to something like:

StringBuilder expectedString = new StringBuilder();
        expectedString.append("{");
        {
            expectedString.append("\"took\":0,");
            expectedString.append("\"timed_out\":false,");
            expectedString.append("\"_shards\":");
            {
                expectedString.append("{\"total\":0,");
                expectedString.append("\"successful\":0,");
                expectedString.append("\"failed\":0},");
            }
            expectedString.append("\"hits\":");
            {
                expectedString.append("{\"total\":100,");
                expectedString.append("\"max_score\":1.5,");
                expectedString.append("\"hits\":[{\"_type\":\"type\",\"_id\":\"id1\",\"_score\":2.0}]}");
            }
        }
        expectedString.append("}");
        assertEquals(expectedString.toString(), Strings.toString(response));

That should be more robust against auto-formatting.

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Author Member

I don't find it much more readable when using a string builder though :)

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Member

Maybe not much, but not even a tiny bit better?

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Member

Just a suggestion, I don't mind if we remove it here entirely. I used to like those tests because they show how a typical xContent output of those classes looks like. But with these large ones its kind of debatable whether it is useful.

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Author Member

I reformatted, was just a copy paste, thanks for doing it ;)

@tlrx
tlrx approved these changes May 17, 2017
Copy link
Member

left a comment

LGTM, Thanks

@javanna javanna merged commit da669f0 into elastic:feature/client_aggs_parsing May 17, 2017

1 check passed

CLA Commit author is a member of Elasticsearch
Details
javanna added a commit to javanna/elasticsearch that referenced this pull request May 23, 2017
Add fromXContent method to SearchResponse (elastic#24720)
SearchResponse#fromXContent allows to parse a search response, including search hits, aggregations, suggestions and profile results. Only the aggs that we can parse today are supported (which means all of them but a couple that are left to support). SearchResponseTests reuses the existing test infra to randomize aggregations, suggestions and profile response.

Relates to elastic#23331
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.