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 query with multiple case:yes|no is silently modified #13958

Closed
sqs opened this issue Sep 18, 2020 · 7 comments · Fixed by #16346
Closed

Search query with multiple case:yes|no is silently modified #13958

sqs opened this issue Sep 18, 2020 · 7 comments · Fixed by #16346
Assignees
Labels
bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior.

Comments

@sqs
Copy link
Member

sqs commented Sep 18, 2020

Repro:

  1. Type repo:^github\.com/sourcegraph/sourcegraph$ (Github case:yes) or (organisation case:no) into the search bar on Sourcegraph.com
  2. Press Enter

Expected: the query remains as I typed it in, and Github is matched case-sensitively and organization is matched case-insensitively

Actual: the query changes (when I press Enter) to repo:^github\.com/sourcegraph/sourcegraph$ (Github ) or (organisation case:no), which is surprising and means my search doesn't do what I expected

I think I know why this happens (because we pull out case: tokens and reflect them in the input toggles), but this is still surprising. It also makes a legit use case harder (finding typos*). BTW, I am not sure if case: can even be applied within a parenthetical sub-expression.

(Not high priority.)

  • I know organisation is not a typo universally, but from the POV of US English it is. 😄
@rvantonder
Copy link
Contributor

BTW, I am not sure if case: can even be applied within a parenthetical sub-expression.

In the current state: no, this shouldn't be allowed. What you should have gotten was a "Can't process that query" notification :-). I think the reason you didn't is because the web app actually transforms the query based on the presence of case.

But, when #13126 is done, a query like (Github case:yes) or (organisation case:no) is totally reasonable and will behave like expected (this is a cool example by the way). Once that functionality is available, it will be inconsistent with the toggle state (the webapp parser assumes there is only one case in the query ever, and will change state on that). How we resolve UI state with subexpressions is a known, but open issue. I am thinking about it.

@rvantonder
Copy link
Contributor

You should have gotten was a "Can't process that query" notification :-). I think the reason you didn't is because the web app actually transforms the query based on the presence of case.

Indeed, I can confirm that the backend will reject that raw query, you can see here in the console. So unfortunately the webapp transforms it before it hits the backend validation. There are a couple of ways to resolve this (e.g., I implement an improved webapp parser detects invalid queries), but that doesn't fix the toggle state issue.

@rvantonder
Copy link
Contributor

Shower thought: @attfarhan if I add a webapp parser, it would be straightforward to just disable (gray out) the case toggle if we see more than one case in a subexpression. I.e., we assume "the user is doing some advanced thing and knows what they're doing, these toggle states are now undefined/disabled". Similar if we see multiple patterntype, we just disable the patterntype toggles.

@rvantonder
Copy link
Contributor

rvantonder commented Sep 18, 2020

The more involved solution would be to add a UI button per search pattern with case toggles, like:

(Github case:yes) or (organisation case:no) is transformed to:
Github[X] or organisation[Y] where [X] is a UI case button that is toggled on, and [X] is a UI case button toggled off. This would probably need a bunch of changes, so just disabling the current states is a good intermediate solution.

This UI approach becomes problematic for queries like (case:yes (foo or bar)) though, so I'm not convinced it pans out in general.

@attfarhan
Copy link
Contributor

Shower thought: @attfarhan if I add a webapp parser, it would be straightforward to just disable (gray out) the case toggle if we see more than one case in a subexpression. I.e., we assume "the user is doing some advanced thing and knows what they're doing, these toggle states are now undefined/disabled". Similar if we see multiple patterntype, we just disable the patterntype toggles.

Yeah, this sounds like a good solution for now. If we add a separate page for multi-line editing or something along those lines we could explore more advanced UI, but that seems like it could be quite far down the line.

@sqs
Copy link
Member Author

sqs commented Sep 21, 2020

Given https://sourcegraph.slack.com/archives/CHEKCRWKV/p1600443200010300 I don't think we will want to add more advanced UI anytime soon, so that simple solution to disable (gray out) the case toggle seems good to me as well.

@rvantonder
Copy link
Contributor

rvantonder commented Sep 22, 2020

Sounds good to me. I've filed #14016 and referenced this issue in #13126.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants