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

Update query-string-syntax.asciidoc #46863

Merged
merged 5 commits into from
Sep 30, 2019
Merged

Update query-string-syntax.asciidoc #46863

merged 5 commits into from
Sep 30, 2019

Conversation

ndrwnaguib
Copy link
Contributor

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@gwbrown gwbrown added :Search/Search Search-related issues that do not fall into other categories >docs General docs changes labels Sep 19, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs

@cbuescher
Copy link
Member

@andrewnaguib thanks for opening the issue, I think you touch an important point because proper escaping isn't really obvious here. I left a few comments where I think this could be made even clearer but maybe @jrodewig has some opinion about this as well?

@@ -278,6 +278,9 @@ The reserved characters are: `+ - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /`
Failing to escape these special characters correctly could lead to a syntax
error which prevents your query from running.

NOTE: Since \ (backslash) is a special character in JSON strings, it needs
to be escaped, hence two backslashes should be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better than having two notes would be to make the former example even clearer. We could first extend the paragraph with the search for "(1+1)=2" by stating this would apply to keyword-field search mostly ("... to search for (1+1)=2 in an unanalyzed keyword field ...") and then add another sentence that mentions the need for additional backslash escaping when using JSON (which is probably the most common way to use the REST API). Maybe even add an example query like:

GET /_search
{
  "query": {
    "query_string": {
      "query": "\\(1\\+1\\)\\=2"
    }
  }
}

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a simpler example might illustrate the point better. Maybe something more like this:

====== Reserved characters

Query string syntax reserves the following characters as operators:

....
+ - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /
....

To use one of these characters literally,
escape it with two preceding backlashes (`\\`).

For example,
the following search request uses a query string query
to match `kimchy!` in the `user` <<keyword,keyword>> field.
Two backslashes are required to escape `!` in `kimchy!`.

[source,console]
----
GET /twitter/_search
{
  "query" : {
    "query_string" : {
      "query" : "kimchy\\!",
      "fields"  : ["user"]
    }
  }
}
----
// TEST[setup:twitter]

NOTE: `>` and `<` can't be escaped at all. The only way to prevent them from
attempting to create a range query is to remove them from the query string
entirely.

@andrewnaguib Let me know if you think that addresses the issue. If so, please feel free to add it to your PR.

I think it might also be helpful to move this section higher in the file.

Copy link
Contributor Author

@ndrwnaguib ndrwnaguib Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jrodewig and @cbuescher for your response. I believe both of you have provided a much neater way on saying the same information; I will add them to the PR. I have a question tho, do you have internal policies for the documentation that I should be aware of, i.e., is it preferred, for instance, to adopt single example across the topic and so on?

Moreover, @jrodewig you were suggesting moving the section in a higher position, I'm not sure where is the appropriate place except right after Regular Expression and before Fuzziness (I think its current place is better tho because all operators would have been mentioned already, i.e., a context of why they would be reserved was already shown. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @andrewnaguib.

We don't have any specific policies about adopting the examples across topics. Generally, you should use an example you feel best fits the context. The twitter example provided is kind of an unofficial default, but please feel free to use your own if you feel it is a better fit.

I don't feel strongly about relocating the section. Your point about this section following others that mention operators is a good one. Let's just leave the section where it is for now.

Thanks again for your work on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your suggestions; I've added them.

@cbuescher cbuescher self-assigned this Sep 25, 2019
@jrodewig
Copy link
Contributor

Thanks @andrewnaguib. Left a few other minor suggestions.

@ndrwnaguib
Copy link
Contributor Author

@jrodewig Thanks for your comments. BTW, I didn't use accurate commit messages in the favor of squashing and writing your own preferred message.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks again @andrewnaguib.

We'll merge and backport this as needed once @cbuescher has had a chance to review.

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants