toString for SearchRequestBuilder and CountRequestBuilder #9944

javanna · 2015-03-02T11:42:38Z

Fixed SearchRequestBuilder#toString to not wipe the request source when called.

Improved SearchRequestBuilder#toString to support the different ways a query can be set to it. Also printed out a merged version of source and extraSrouce in case there in case any extraSource is set, to reflect what will happen when executing the request builder.

Implemented toString in CountRequestBuilder

Closes #5576
Closes #5555

kimchy · 2015-03-02T19:32:32Z

I think the toString here looses the meaning of the request? I wonder if actually someone won't wish to see something like source={}, extra_source={} then its clearer what happens in the context of this builder? It will also simplify the toString implementation quite a bit I would hope?

javanna · 2015-03-03T08:47:47Z

I see what you mean @kimchy . I think that there are a couple of different problems with this toString implementation.

First of all it prints out only part of the request (index, type, routing etc. are ignored) which in my mind relates to the request body (when the request comes in through REST layer). This hasn't changed in this PR, but it did get more convoluted since I am now attempting to print out what is actually going to be executed (hence merging extra_source and source, cause that is what happen at execution time anyway).

Also, I wonder if users expect the output to be a valid json query that can be sent back to elasticsearch or not. If we add the extraSource to the current json, we break this "contract". In general I think a toString should just print out the content of the request, it doesn't need to be a json either and the fact that it is a json is misleading. I would consider, on the long term, moving away from json and just print out everything that the request contains, not only what we can represent as json.

That said, what I am proposing is a bigger (breaking?) change that deserves its own issue and discussion, and should be applied to every request consistently (after #9201). I am leaning towards fixing the actual bug only in this PR, the fact that the toString modifies the content of the request. Shall I take take out the merging then? I think that printing out the extraSource in the current json might not make users happy, we should just move away from json if we do that. Makes sense?

kimchy · 2015-03-03T11:20:43Z

@javanna yea, I think fixing the actual bug here is the best way forward. Personally, I am leaning towards not adding the behavior of being able to take a request toString and using it as the body, since its more than that..., sometimes the request also have parameters that go over the URI when its a RESTful request, ... . I think we should keep toString into being a nice way to debug requests if you log them, with all the relevant information to be able to do so.

If we want to take such a project of having the request be represented as rest requests, then we need to have something like RestRequest toRestRequest(), but I personally, at least now, view it as a low priority task because of the combination of its scope of change and the added value.

javanna · 2015-03-03T12:07:52Z

I just opened #9962 to keep track of the bigger change we've been discussing. As for fixing this bug, what should we do?

Simplify the PR by ignoring the extra_source part of the request, which we currently merge to the source (keeping the output as a valid json that one can send to elasticsearch). It was previously ignored as well...
Simplify the PR by printing out the extra_source separately, which we currently merge to the source, as part of the same json object
Leave the PR as is, print out the merged source and extra_source, so that the output is a json that represent the query that will get executed when the builder is run
your idea goes here

javanna · 2015-03-03T12:09:50Z

if I understand correctly @kimchy , you vote for

Simplify the PR by printing out the extra_source separately, which we currently merge to the source, as part of the same json object

Is that right?

kimchy · 2015-03-03T12:11:35Z

I think we need to fix the bug first of all..., we can improve toString while we are at it, but maybe in a different pull request?

javanna · 2015-03-07T14:43:17Z

Agreed @kimchy I updated the PR to only fix the bug. Removed the extra_source merge. Note that extra_source is ignored, as before the fix. But the right source gets now printed out depending on what's set. Let me know what you think

kimchy · 2015-03-08T17:51:24Z

src/main/java/org/elasticsearch/action/search/SearchRequestBuilder.java

+            return sourceBuilder.toString();
+        }
+        if (request.source() != null) {
+            return request.source().toUtf8();


the source might be SMILE or CBOR, and then we can't convert it to utf8?

oh boy I totally missed this, fixing

kimchy · 2015-03-08T17:51:52Z

@javanna left a minor comment

… without wiping the request source Broaden SearchRequestBuilder#toString to support the different ways a query can be set to it. Also printed out a merged version of source and extraSrouce in case there in case any extraSource is set, to reflect what will happen when executing the request builder. Closes elastic#5576

Similarly to what SearchRequestBuilder does, we print out a string representation of they query that is going to be executed when executing the request builder. Closes elastic#5555

javanna · 2015-03-13T00:30:25Z

Updated to properly handle binary formats as well and added tests for it, can you have another look please @kimchy ?

javanna · 2015-03-19T11:46:06Z

@kimchy @s1monw what do you think? would like to get this in ;)

cooniur · 2015-03-23T19:42:21Z

Hey @javanna , how is the bug fixing going? I saw 1.5.0 has been released. Does it include the fix of this toString() bug?

I noticed you guys talking about the use of toString(). IMHO, I feel the proper use case of toString() would be only for logging and debugging. In this case, developers probably wouldn't care too much about whether toString() returns a proper JSON string or something else, as long as it returns correct and enough information about the QueryBuilder object.

The most important thing is to make sure that calling toString() will never change the state of QueryBuilder, which is probably something you guys could start with to fix the bug, and then collect feedback, and keep improving it.

Thanks!

javanna · 2015-03-23T19:53:12Z

Hi @cooniur the PR is still open which means that it didn't make it into 1.5.0. It will most likely make it into the next release, I am just waiting for a final review here, have a look at the code and tell me what you think if you feel like it. It addresses exactly the issue of changing the content of the request. The side discussions we had were more generic around toString and what it should do (I agree with you btw) which I opened another issue for (#9962).

javanna · 2015-03-25T12:14:22Z

ping @kimchy can you have a look please?

javanna · 2015-04-07T16:59:54Z

ping @s1monw @kimchy can either of you have a look please? this bug has been around for too long, would love to get it fixed.

javanna · 2015-04-16T16:07:17Z

@jpountz maybe you can have a look?

jpountz · 2015-04-23T19:58:48Z

LGTM

javanna · 2015-04-28T09:23:44Z

Merged to master, 1.x and 1.5.

dadoonet · 2015-05-29T20:28:35Z

src/main/java/org/elasticsearch/action/count/CountRequestBuilder.java

+                return "{ \"error\" : \"" + ExceptionsHelper.detailedMessage(e) + "\"}";
+            }
+        }
+        return new QuerySourceBuilder().toString();


This is printing an empty thing.

SearchRequestBuilder builder = client().prepareSearch("index").setTypes("type"); System.out.println("builder = " + builder);

Gives

builder = { }

Is that expected @javanna? Or should we wait for #9962 for a fix?

yes it is expected

cooniur · 2015-06-11T07:46:41Z

Hey @javanna , thanks for fixing this issue in 1.6! Being waiting for it for a long time but eventually get it!

javanna added >bug >enhancement v1.5.0 v1.4.5 v2.0.0-beta1 v1.3.8 labels Mar 2, 2015

javanna self-assigned this Mar 2, 2015

javanna mentioned this pull request Mar 2, 2015

SearchRequestBuilder.toString modifies the SearchRequestBuilder wiping any source set. #7334

Closed

javanna added :Core/Infra/Transport API Transport client API review labels Mar 2, 2015

javanna mentioned this pull request Mar 3, 2015

Java Api: every request, builder and response to implement toString() consistently #9962

Closed

javanna force-pushed the fix/search_request_builder_to_string branch from e0a763b to 6067cda Compare March 7, 2015 14:39

kimchy reviewed Mar 8, 2015
View reviewed changes

javanna force-pushed the fix/search_request_builder_to_string branch 3 times, most recently from 1ab93db to 7f8dad8 Compare March 12, 2015 23:46

javanna added 2 commits March 12, 2015 17:21

Java api: implement toString in CountRequestBuilder

e0f9ec6

Similarly to what SearchRequestBuilder does, we print out a string representation of they query that is going to be executed when executing the request builder. Closes elastic#5555

javanna force-pushed the fix/search_request_builder_to_string branch from 7f8dad8 to e0f9ec6 Compare March 13, 2015 00:23

s1monw added v1.6.0 and removed v1.6.0 labels Mar 17, 2015

s1monw removed the v1.5.0 label Mar 17, 2015

javanna added v1.5.0 v1.3.10 and removed v1.6.0 v1.3.8 labels Mar 18, 2015

javanna added v1.6.0 and removed v1.5.0 labels Mar 20, 2015

javanna added v1.5.1 and removed v1.3.10 labels Mar 23, 2015

clintongormley added v1.5.2 and removed v1.5.1 labels Apr 9, 2015

clintongormley added v1.5.3 and removed v1.5.2 v1.4.5 labels Apr 26, 2015

javanna closed this Apr 28, 2015

kevinkluge removed the review label Apr 28, 2015

dadoonet reviewed May 29, 2015
View reviewed changes

clintongormley changed the title ~~Java api: toString for SearchRequestBuilder and CountRequestBuilder~~ toString for SearchRequestBuilder and CountRequestBuilder May 30, 2015

clintongormley removed the >enhancement label Jun 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

toString for SearchRequestBuilder and CountRequestBuilder #9944

toString for SearchRequestBuilder and CountRequestBuilder #9944

javanna commented Mar 2, 2015

kimchy commented Mar 2, 2015

javanna commented Mar 3, 2015

kimchy commented Mar 3, 2015

javanna commented Mar 3, 2015

javanna commented Mar 3, 2015

kimchy commented Mar 3, 2015

javanna commented Mar 7, 2015

kimchy Mar 8, 2015

javanna Mar 13, 2015

kimchy commented Mar 8, 2015

javanna commented Mar 13, 2015

javanna commented Mar 19, 2015

cooniur commented Mar 23, 2015

javanna commented Mar 23, 2015

javanna commented Mar 25, 2015

javanna commented Apr 7, 2015

javanna commented Apr 16, 2015

jpountz commented Apr 23, 2015

javanna commented Apr 28, 2015

dadoonet May 29, 2015

javanna May 30, 2015

cooniur commented Jun 11, 2015

toString for SearchRequestBuilder and CountRequestBuilder #9944

toString for SearchRequestBuilder and CountRequestBuilder #9944

Conversation

javanna commented Mar 2, 2015

kimchy commented Mar 2, 2015

javanna commented Mar 3, 2015

kimchy commented Mar 3, 2015

javanna commented Mar 3, 2015

javanna commented Mar 3, 2015

kimchy commented Mar 3, 2015

javanna commented Mar 7, 2015

kimchy Mar 8, 2015

Choose a reason for hiding this comment

javanna Mar 13, 2015

Choose a reason for hiding this comment

kimchy commented Mar 8, 2015

javanna commented Mar 13, 2015

javanna commented Mar 19, 2015

cooniur commented Mar 23, 2015

javanna commented Mar 23, 2015

javanna commented Mar 25, 2015

javanna commented Apr 7, 2015

javanna commented Apr 16, 2015

jpountz commented Apr 23, 2015

javanna commented Apr 28, 2015

dadoonet May 29, 2015

Choose a reason for hiding this comment

javanna May 30, 2015

Choose a reason for hiding this comment

cooniur commented Jun 11, 2015