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

Search Template - conditional clauses not rendering correctly #8308

Closed
lukas-vlcek opened this issue Oct 31, 2014 · 17 comments
Closed

Search Template - conditional clauses not rendering correctly #8308

lukas-vlcek opened this issue Oct 31, 2014 · 17 comments
Assignees
Labels
>bug :Search/Search Search-related issues that do not fall into other categories

Comments

@lukas-vlcek
Copy link
Contributor

Is the example of filtered query template [1] working? It seems to contain extra </6> string which seems as an issue to me. I am unable to test this example even after I remove this extra string.

[1] http://www.elasticsearch.org/guide/en/elasticsearch/reference/1.3/search-template.html#_conditional_clauses

@lukas-vlcek
Copy link
Contributor Author

I was testing it with ES 1.3.0

clintongormley added a commit that referenced this issue Oct 31, 2014
Fixed asciidoc typo

Closes #8308
clintongormley added a commit that referenced this issue Oct 31, 2014
Fixed asciidoc typo

Closes #8308
@lukas-vlcek
Copy link
Contributor Author

@clintongormley thanks for clarifying this. Out of curiosity, does the template example in question work for you?

@clintongormley
Copy link
Contributor

@lukas-vlcek did you wrap it in a query element and pass it as an escaped string?

I've just pushed another improvement to the docs to include both of the above elements

@lukas-vlcek
Copy link
Contributor Author

@clintongormley thanks for this, but still I am unable to get search template with section work. Here is trivial recreation https://gist.github.com/lukas-vlcek/56540a7e8206d122d55c
Is there anything I do wrong? Am I missing some escaping?

@lukas-vlcek
Copy link
Contributor Author

Just in case here is excerpt from server log:

[2014-10-31 19:03:29,575][DEBUG][action.search.type       ] [Jester] [twitter][4], node[ccMkzywSSfi-4ttibpHh9w], [P], s[STARTED]: Failed to execute [org.elasticsearch.action.search.SearchRequest@7de37772] lastShard [true]
org.elasticsearch.ElasticsearchParseException: Failed to parse template
    at org.elasticsearch.search.SearchService.parseTemplate(SearchService.java:612)
    at org.elasticsearch.search.SearchService.createContext(SearchService.java:514)
    at org.elasticsearch.search.SearchService.createAndPutContext(SearchService.java:487)
    at org.elasticsearch.search.SearchService.executeQueryPhase(SearchService.java:256)
    at org.elasticsearch.search.action.SearchServiceTransportAction$5.call(SearchServiceTransportAction.java:206)
    at org.elasticsearch.search.action.SearchServiceTransportAction$5.call(SearchServiceTransportAction.java:203)
    at org.elasticsearch.search.action.SearchServiceTransportAction$23.run(SearchServiceTransportAction.java:517)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
    at java.lang.Thread.run(Thread.java:745)
Caused by: org.elasticsearch.common.jackson.core.JsonParseException: Unexpected character ('{' (code 123)): was expecting either valid name character (for unquoted name) or double-quote (for quoted) to start field name
 at [Source: [B@438d7c6b; line: 1, column: 4]
    at org.elasticsearch.common.jackson.core.JsonParser._constructError(JsonParser.java:1419)
    at org.elasticsearch.common.jackson.core.base.ParserMinimalBase._reportError(ParserMinimalBase.java:508)
    at org.elasticsearch.common.jackson.core.base.ParserMinimalBase._reportUnexpectedChar(ParserMinimalBase.java:437)
    at org.elasticsearch.common.jackson.core.json.UTF8StreamJsonParser._handleOddName(UTF8StreamJsonParser.java:1808)
    at org.elasticsearch.common.jackson.core.json.UTF8StreamJsonParser._parseName(UTF8StreamJsonParser.java:1496)
    at org.elasticsearch.common.jackson.core.json.UTF8StreamJsonParser.nextToken(UTF8StreamJsonParser.java:693)
    at org.elasticsearch.common.xcontent.json.JsonXContentParser.nextToken(JsonXContentParser.java:50)
    at org.elasticsearch.index.query.TemplateQueryParser.parse(TemplateQueryParser.java:113)
    at org.elasticsearch.index.query.TemplateQueryParser.parse(TemplateQueryParser.java:103)
    at org.elasticsearch.search.SearchService.parseTemplate(SearchService.java:605)
    ... 9 more

@clintongormley
Copy link
Contributor

@lukas-vlcek I can't get this working either. No idea what is going on here :(

@lukas-vlcek
Copy link
Contributor Author

@clintongormley thanks for look at this. Shall I change the title of this issue and remove the [DOC] part? It is probably not a DOC issue anymore, is it?

@clintongormley clintongormley changed the title [DOC] Search Template - conditional clauses example not valid? Search Template - conditional clauses not rendering correctly Nov 3, 2014
@wwken
Copy link
Contributor

wwken commented Nov 6, 2014

After looking deep inside the code, I am wondering has this (i.e. the conditional clause thing) been ever working? The root clause is, inside the JsonXContentParser.java:nextToken() method, the parser.nextToken() throws an exception straight out of the box when it encounters the clause like {{#use_size}}. I guess such clause has not been implemented to be recognized yet. Notice that the parser is of type org.elasticsearch.common.jackson.core.json.UTF8StreamJsonParser .

It looks like we have to override the UTF8StreamJsonParser's nextToken() method and do some look ahead for the conditional clauses.

@clintongormley
Copy link
Contributor

@wwken As I understand it, the JSON parser should never see this the {{#use_size}}. The template is passed as a string, not as JSON, so the Mustache should render the template (thus removing the conditionals) to generate JSON which can then be parsed.

This would be much easier to debug if we had #6821

wwken added a commit to wwken/elasticsearch that referenced this issue Nov 7, 2014
…lastic#8308

   - implemented the conditional parsing capabilities
   - attached few junit test cases to test it
wwken added a commit to wwken/elasticsearch that referenced this issue Nov 7, 2014
…lastic#8308

   - implemented the conditional parsing capabilities
   - attached few junit test cases to test it (reverted from commit bf1ae31)
@wwken
Copy link
Contributor

wwken commented Nov 7, 2014

I fixed it. Please refer to this pull request: https://github.com/elasticsearch/elasticsearch/pull/8376/files
(P.S. plz ignore the above few links as i messed up checking in some incorrect commit files before by mistake)

@clintongormley
Copy link
Contributor

thanks @wwken - we'll take a look and get back to you

@lukas-vlcek
Copy link
Contributor Author

@clintongormley #8393 is my try to fix this issue. If you find this relevant and to the point then it might make sense to consider renaming this issue again to more general description (I think the main issue here is broken parser logic of TemplateQueryParser in case the template value is a single string token. In other words I think there is more general issue not directly related to conditional clauses only).

@wwken
Copy link
Contributor

wwken commented Nov 7, 2014

@lukas-vlcek I think for sure your solution is better! I have two suggestions on it though (minor, not any important):

  1. In this file, https://github.com/lukas-vlcek/elasticsearch/blob/8308/src/main/java/org/elasticsearch/search/SearchService.java, you can omit the lines from 617-635 since they are not needed

  2. in this file, https://github.com/lukas-vlcek/elasticsearch/blob/8308/src/main/java/org/elasticsearch/index/query/TemplateQueryParser.java, you can separate the single if in line 113 to two if statements to make it more elegant :)

thanks

@lukas-vlcek
Copy link
Contributor Author

@wwken thanks for looking at this. I am going to wait if ES devs give any feedback. May be they will want to fix it in very different approach. Who knows...

@lukas-vlcek
Copy link
Contributor Author

Bump.

Is there any plan to have this fixed in 1.4? Or even backporting it to 1.3? We would like to make use of Search Templates but this issue is somehow limiting us. How can I help with this?

@clintongormley clintongormley removed the help wanted adoptme label Nov 12, 2014
lukas-vlcek added a commit to lukas-vlcek/elasticsearch-old-repo that referenced this issue Nov 25, 2014
…d string. Closes elastic#8308

- Search template is correctly parsed if it is provided as single escaped string
- Ignore exceptions in case of second parsing of template content (assuming it is escaped template string content)
- New tests for relevant use cases
lukas-vlcek added a commit to lukas-vlcek/elasticsearch-old-repo that referenced this issue Dec 8, 2014
…d string. Closes elastic#8308

- Search template is correctly parsed if it is provided as single escaped string
- Ignore exceptions in case of second parsing of template content (assuming it is escaped template string content)
- New tests for relevant use cases

Conflicts:
	src/main/java/org/elasticsearch/search/SearchService.java
@javanna javanna self-assigned this Mar 24, 2015
@MaineC MaineC assigned MaineC and unassigned javanna Apr 7, 2015
@clintongormley
Copy link
Contributor

Closed by #11512

@lukas-vlcek
Copy link
Contributor Author

@clintongormley correct, this issue was created before I opened more general PR. Closing...

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

Closes elastic#8308
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Search Templates labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants