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 dedicated /_search/template endpoint for query templates #5353

Closed

Conversation

@spinscale
Copy link
Member

commented Mar 6, 2014

In order to simplify query template execution an own endpoint has been added

  • Rest tests and another unit test added
  • Changed the RestApiSpecParser to throw a more useful exception

Open question: Should the JAVA APIs change as well?

@javanna
javanna reviewed Mar 6, 2014
View changes
rest-api-spec/api/search.template.json Outdated
}
},
"body": {
"description": "The search definiation template"

This comment has been minimized.

Copy link
@javanna

javanna Mar 6, 2014

Member

typo :)

@javanna
javanna reviewed Mar 6, 2014
View changes
src/main/java/org/elasticsearch/action/search/SearchRequest.java Outdated
@@ -471,6 +503,9 @@ public void readFrom(StreamInput in) throws IOException {

types = in.readStringArray();
indicesOptions = IndicesOptions.readIndicesOptions(in);

templateSourceUnsafe = in.readBoolean();

This comment has been minimized.

Copy link
@javanna

javanna Mar 6, 2014

Member

don't we need version checks here? This is going to go to 1.x right?

@@ -497,5 +532,8 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeBytesReference(extraSource);
out.writeStringArray(types);
indicesOptions.writeIndicesOptions(out);

This comment has been minimized.

Copy link
@javanna

javanna Mar 6, 2014

Member

same as above

@javanna
javanna reviewed Mar 6, 2014
View changes
src/test/java/org/elasticsearch/test/rest/spec/RestApiParser.java Outdated
@@ -65,7 +65,9 @@ public RestApi parse(XContentParser parser) throws IOException {
while (parser.nextToken() == XContentParser.Token.FIELD_NAME) {

This comment has been minimized.

Copy link
@javanna

javanna Mar 6, 2014

Member

can we have the rest api parser change + test as a separate commit? You can push it straight-away I'd say, looks good!

This comment has been minimized.

Copy link
@spinscale

spinscale Mar 6, 2014

Author Member

makes sense, will put it in separately and move out here

public BytesReference templateSource() {
return this.templateSource;
}

public ShardSearchRequest nowInMillis(long nowInMillis) {
this.nowInMillis = nowInMillis;
return this;

This comment has been minimized.

Copy link
@javanna

javanna Mar 6, 2014

Member

One thing I noticed here is that you added a templateSource object which doesn't get serialized. Is that correct?

@s1monw
s1monw reviewed Mar 13, 2014
View changes
src/main/java/org/elasticsearch/action/search/SearchRequest.java Outdated
@@ -471,6 +534,15 @@ public void readFrom(StreamInput in) throws IOException {

types = in.readStringArray();
indicesOptions = IndicesOptions.readIndicesOptions(in);

if (in.getVersion().onOrAfter(Version.V_1_1_0)) {
templateSourceUnsafe = in.readBoolean();

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 13, 2014

Contributor

this should be just templateSourceUnsafe = false; no?

@s1monw
s1monw reviewed Mar 13, 2014
View changes
src/main/java/org/elasticsearch/action/search/SearchRequest.java Outdated
@@ -497,5 +569,17 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeBytesReference(extraSource);
out.writeStringArray(types);
indicesOptions.writeIndicesOptions(out);

if (out.getVersion().onOrAfter(Version.V_1_1_0)) {
out.writeBoolean(templateSourceUnsafe);

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 13, 2014

Contributor

no need to serialize the templateSourceUnsafe value

@s1monw
s1monw reviewed Mar 13, 2014
View changes
src/main/java/org/elasticsearch/search/SearchService.java Outdated
} catch (IOException e) {
logger.error("Error trying to parse template: ", e);
} finally {
if (parser != null) {

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 13, 2014

Contributor

use IOUtils here to close? or try-with on master?

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2014

I like it a lot! I think it looks pretty cool left some minor comments

@spinscale

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2014

updated the PR based on your comments

spinscale added a commit to spinscale/elasticsearch that referenced this pull request Mar 14, 2014
In order to simplify query template execution an own endpoint has been added

Closes elastic#5353
"size" : {{mySize}}
},
"params" : {
"myField" : "foo",

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 14, 2014

Contributor

I'd like to have more reasonable names / values here maybe use an example that is more realistic like filter for a username or so?

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 14, 2014

Contributor

I also suggest that we use my_value which is less javaish?

private BytesReference templateSource;
private boolean templateSourceUnsafe;
private String templateName;
private Map<String, String> templateParams;

This comment has been minimized.

Copy link
@s1monw

s1monw Mar 14, 2014

Contributor

maybe we make sure that this is always an empty map? rather than null?

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2014

a small amount of comments but LGTM !!

}
}
------------------------------------------


For more information on how Mustache templating and what kind of templating you
can do with it check out the [online

This comment has been minimized.

Copy link
@clintongormley

clintongormley Mar 14, 2014

Member

Incorrect asciidoc link syntax here. Use: http://foo.com[Name]

@clintongormley

This comment has been minimized.

Copy link
Member

commented Mar 14, 2014

I'd love to see some examples of more complicated templates, including mustache sections, use of arrays etc.

Also, is the automatic html encoding of output turned off?

@spinscale

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2014

hrm...

{
    "template" : {
        "query": { "filtered" : {
            "query" : { "match_all" : {} },
            "filter" : { "owner" : "{{user}}" }
         },
        "aggs" : {
        {{#aggs}}
          "{{.}}" : { "terms" : { "field" : "{{.}}" }  }
        {{/aggs}}
        }
    },
    "params" : {
        "user" : "kimchy",
        "aggs" : [ "category_id", "sub_category_id" ]
    }
}

something like this will not produce valid JSON, as you cannot check for the last element of an array with mustache, thanks to logicless template... need to think about this in order to solve it nicely, makes mustache somewhat limited here for our needs...

@clintongormley

This comment has been minimized.

Copy link
Member

commented Mar 14, 2014

i do wonder if mustache is the right answer here... You can certainly make it do the right thing, but I wonder how easy it will be for the user to write.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2014

it's just a scripting engine so you can plug in whatever produces a string?

@clintongormley

This comment has been minimized.

Copy link
Member

commented Mar 14, 2014

yeah, but i'm thinking more of the existing dsl with some added operators... not sure. need to play with it a bit

@clintongormley

This comment has been minimized.

Copy link
Member

commented Mar 15, 2014

OK I've worked through various non-trivial examples of templating using mustache, and added a number of worked examples to the docs page: See spinscale#2

There are some things (conditional clauses) which can't be expressed using the JSON form of templates, but can be expressed as strings. I've included an example in the PR.

One thing that needs to be fixed is the escape method. Currently it escapes characters as expected for HTML, eg " -> &quot;. This is clearly wrong. Instead, we should be using these escapes:

\b  Backspace (ascii code 08)
\f  Form feed (ascii code 0C)
\n  New line
\r  Carriage return
\t  Tab
\v  Vertical tab
\"  Double quote
\\  Backslash 
In order to simplify query template execution an own endpoint has been added

Closes #5353
@s1monw

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2014

LGTM lets push this @spinscale

@clintongormley

This comment has been minimized.

Copy link
Member

commented Mar 20, 2014

I've opened the escaping bug as a separate issue #5473

@spinscale spinscale closed this in 8f6e1d4 Mar 20, 2014
spinscale added a commit that referenced this pull request Mar 20, 2014
In order to simplify query template execution an own endpoint has been added

Closes #5353
@s1monw s1monw added v2.0.0 labels Mar 20, 2014
@s1monw s1monw added the enhancement label Mar 20, 2014
@clintongormley clintongormley added v1.1.0 and removed v1.2.0 labels Mar 21, 2014
@spinscale spinscale referenced this pull request Mar 28, 2014
karmi added a commit to elastic/elasticsearch-ruby that referenced this pull request Apr 9, 2014
@clintongormley clintongormley changed the title Query Templates: Adding dedicated /_search/template endpoint Add dedicated /_search/template endpoint for query templates Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.