Handle bad regexs in search.findTopLevelDeclarations.

Handle bad regexs in search.findTopLevelDeclarations.

BUG=

[devoncarew, 2016-07-13 23:23:44]

devoncarew@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[devoncarew, 2016-07-13 23:23:45]

Fix an issue where an issue with search.findTopLevelDeclarations. When passed a
string that was an invalid regex, the command would return a valid search id.
The analysis server would then report an error later via the server.error
notification, when constructing the RegExp would throw an exception.

We instead return a parameter error on the call to
search.findTopLevelDeclarations.

---

Failed to handle request: {params: {pattern: [f}, method:
search.findTopLevelDeclarations, id: 9}
FormatException: Unterminated character class[f
#0      _RegExp._RegExp (dart:core-patch/regexp_patch.dart:139)
#1      RegExp.RegExp (dart:core-patch/regexp_patch.dart:23)
#2      SearchEngineImpl._searchDefinedNames.<_searchDefinedNames_async_body>
(package:analysis_server/src/services/search/search_engine_internal.dart:141)
#3      Future.Future.microtask.<anonymous closure> (dart:async/future.dart:144)

[Brian Wilkerson, 2016-07-13 23:42:38]

We should update the spec for the request to include a description of the error
condition, something like:

It is an error if the pattern is not a valid regular expression (as defined by
the class RegExp in dart:core).

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/search/search_domain.dart (right):

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/search/search_domain.dart:118: void
findTopLevelDeclarations(protocol.Request request) {
Removing the `async` makes this much harder to read. What was the motivation?

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/test/se...
File pkg/analysis_server/test/search/top_level_declarations_test.dart (right):

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/search/top_level_declarations_test.dart:85:
expect(true, result is RequestError);
The arguments are backward. The better way to write this is:

  expect(result, new isInstanceOf<RequestError>());

[devoncarew, 2016-07-14 00:01:40]

> We should update the spec for the request to include a description of the
error
> condition, something like:
> 
> It is an error if the pattern is not a valid regular expression (as defined by
> the class RegExp in dart:core).

Yeah, I wasn't sure whether it was better to return an error, or let the search
run, but return 0 results for a bad term. It seems unfortunate to return an
error code for a bad regex when the user may not have even known they were
typing a regex into a field.

For some background, what I want to do is implement a find type dialog. What
would be great is a fuzzy case insensitive match on the search term (fobr would
match foobar). A case insensitive substring match would be a close second. I
don't really know that user's will expect to have the power of a regex. I could
just sanitize any special regexs chars on the frontend.

> Removing the `async` makes this much harder to read. What was the motivation?

With the async, the body of the method runs in a separate future, so by the time
our parameter validation runs we've already returned the DELAYED_RESPONSE
response from the caller.

[scheglov, 2016-07-14 00:18:07]

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/search/search_domain.dart (right):

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/search/search_domain.dart:118: void
findTopLevelDeclarations(protocol.Request request) {
On 2016/07/13 23:42:37, Brian Wilkerson wrote:
> Removing the `async` makes this much harder to read. What was the motivation?

We could use something like this instead.

    () async {
      await server.onAnalysisComplete;
      // respond
      String searchId = (_nextSearchId++).toString();
      _sendSearchResult(
          request, new protocol.SearchFindTopLevelDeclarationsResult(searchId));
      // search
      List<SearchMatch> matches =
          await searchEngine.searchTopLevelDeclarations(params.pattern);
      _sendSearchNotification(searchId, true, matches.map(toResult));
    }();

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/test/se...
File pkg/analysis_server/test/search/top_level_declarations_test.dart (right):

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/search/top_level_declarations_test.dart:63:
addTestFile('''
This test file is specifically for the corresponding test.
Don't put it into the whole class setUp().

It seems to me that the test_invalidRegex() does not need any file at all.

https://codereview.chromium.org/2149853002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/search/top_level_declarations_test.dart:85:
expect(true, result is RequestError);
On 2016/07/13 23:42:37, Brian Wilkerson wrote:
> The arguments are backward. The better way to write this is:
> 
>   expect(result, new isInstanceOf<RequestError>());

See also isResponseFailure in mocks.dart

[Brian Wilkerson, 2016-07-14 13:20:50]

> Yeah, I wasn't sure whether it was better to return an error, or let the
> search run, but return 0 results for a bad term.

I think it's best to give the client as much information as possible and let
them decide how to handle the error. If they've made it clear in the UI that the
expected field is a regexp, then they can display an "invalid regexp" message.
If they want to hide that, then they can pretend they got back an empty
response.

[devoncarew, 2016-07-15 15:16:45]

On 2016/07/14 13:20:50, Brian Wilkerson wrote:
> > Yeah, I wasn't sure whether it was better to return an error, or let the
> > search run, but return 0 results for a bad term.
> 
> I think it's best to give the client as much information as possible and let
> them decide how to handle the error. If they've made it clear in the UI that
the
> expected field is a regexp, then they can display an "invalid regexp" message.
> If they want to hide that, then they can pretend they got back an empty
> response.

sgtm! Will clean up this CL to address the async and testing comments.

[devoncarew, 2016-07-18 15:24:11]

On 2016/07/15 15:16:45, devoncarew wrote:
> On 2016/07/14 13:20:50, Brian Wilkerson wrote:
> > > Yeah, I wasn't sure whether it was better to return an error, or let the
> > > search run, but return 0 results for a bad term.
> > 
> > I think it's best to give the client as much information as possible and let
> > them decide how to handle the error. If they've made it clear in the UI that
> the
> > expected field is a regexp, then they can display an "invalid regexp"
message.
> > If they want to hide that, then they can pretend they got back an empty
> > response.
> 
> sgtm! Will clean up this CL to address the async and testing comments.

ptal - changed back to using async in the findTopLevelDeclarations handler, and
reverted some changes to the test file.

[scheglov, 2016-07-18 16:04:18]

lgtm

[Brian Wilkerson, 2016-07-18 16:45:53]

lgtm

[devoncarew, 2016-07-18 19:02:16]

Description was changed from

==========
Handle bad regexs in search.findTopLevelDeclarations.

BUG=
==========

to

==========
Handle bad regexs in search.findTopLevelDeclarations.

BUG=
==========