search API def return more than needed

[chkp-baselz]

Hey,
My Opengrok version is 1-5-11
When I try to search for definitions I got each appears on the file the variable is defined there, for example,
https://opengrok.checkpoint.com:8443/source/api/v1/search?def=ws_http2_stream_error_action&projects=ivory_main
This is what I get, it should return only line 105, on UI it works and jumps me direct to line 105

{
  "time": 11,
  "resultCount": 1,
  "startDocument": 0,
  "endDocument": 0,
  "results": {
    "/ivory_main/ws/http2/ws_http2_parser.c": [
      {
        "line": " * fw ctl set int <b>ws_http2_stream_error_action</b> 0/1",
        "lineNumber": "103"
      },
      {
        "line": "int <b>ws_http2_stream_error_action</b> = E_WS_HTTP2_STREAM_ERROR_HANDLE;",
        "lineNumber": "105"
      },
      {
        "line": "\t\tswitch(<b>ws_http2_stream_error_action</b>)",
        "lineNumber": "5009"
      },
      {
        "line": "\t\t\t\tWS_DEBUG(WS_HTTP2, WS_ERROR, (\"invalid stream error action %d\", <b>ws_http2_stream_error_action</b>));",
        "lineNumber": "5035"
      }
    ]
  }
}

[vladak]

I assume that the search in the UI is performed by using the 'Definition' field in the search form, correct ?

[vladak]

Tried that with search for the USERNAME_FIELD definition in OpenGrok source code. Using the search API returns 5 hits in plugins/src/main/java/opengrok/auth/plugin/UserWhiteListPlugin.java which is where the actual definition is located. The API search results contain just this file even though there are some references to this symbol also in plugins/src/test/java/opengrok/auth/plugin/UserWhiteListPluginTest.java, otherwise I'd say this is wrongly formed query inside the SearchController somewhere, however this looks more like a problem with interpreting the search results.

[vladak]

Getting the results of search to the client is actually pretty complex.

There are major differences in how the search is done in the UI and via the API. I will try to summarize it here for future reference since I don't know whole lot about it myself.

When going via the API, it starts in SearchController#search(). This uses SearchEngineWrapper, a private static class located in the same source code file. As suggested by the name, this class is a wrapper to the SearchEngine class. The search() method then calls engine.search() (where engine is instance of SearchEngineWrapper) which returns the list of Hit objects which are then wrapped inside the SearchResult object and this gets converted automatically to the JSON representation returned to the client.

Now, for the context (sic!) of this bug, SearchEngineWrapper#search() calls SearchEngine#search() to get the search results and then calls SearchEngine#results() to convert the search results to the list of Hit objects. The SearchEngine object keeps the list of hits in an array of ScoreDoc (Lucene) objects. The SearchEngine#results() traverses this array and in this particular case uses the Context#getContext() that fills the list of Hit objects:

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/SearchEngine.java

Lines 467 to 476 in 33eb38a

	if (sourceContext != null) {
	sourceContext.toggleAlt();
	try {
	if (AbstractAnalyzer.Genre.PLAIN == genre && (source != null)) {
	// SRCROOT is read with UTF-8 as a default.
	hasContext = sourceContext.getContext(
	new InputStreamReader(new FileInputStream(
	source + filename), StandardCharsets.UTF_8),
	null, null, null, filename, tags, nhits > 100,
	false, ret, scopes);

This method then uses PlainLineTokenizer:

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/context/Context.java

Line 364 in 33eb38a

PlainLineTokenizer tokens = new PlainLineTokenizer(null);

which might add Hit objects to the list for the non-definition tokens in dumpRest():

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/context/Context.java

Lines 404 to 406 in 33eb38a

	if (hits != null) {
	tokens.setAlt(alt);
	tokens.setHitList(hits);

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/context/Context.java

Lines 420 to 424 in 33eb38a

	if (!isDefSearch) {
	tokens.printContext();
	} else if (tokens.tags.containsKey(tokens.markedLine)) {
	tokens.printContext();
	}

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/context/Context.java

Line 435 in 33eb38a

tokens.dumpRest();

which in this case is actually unwanted.

[vladak]

Searching from the UI is a tad different as it uses different search wraper:

opengrok/opengrok-web/src/main/webapp/search.jsp

Lines 81 to 86 in 701f162

	SearchHelper searchHelper = cfg.prepareSearch();
	// N.b. searchHelper.destroy() is called via
	// WebappListener.requestDestroyed() on presence of the following
	// REQUEST_ATTR.
	request.setAttribute(SearchHelper.REQUEST_ATTR, searchHelper);
	searchHelper.prepareExec(cfg.getRequestedProjects()).executeQuery().prepareSummary();

It builds IndexSearcher in prepareExec() and calls search() in executeQuery - the approach is similar to what SearchEngine does for the API based search.

The results are then presented via

opengrok/opengrok-web/src/main/webapp/search.jsp

Line 228 in 701f162

Results.prettyPrint(out, searchHelper, start, start + thispage);

which then calls Results#printPlain(). Similarly to SearchEngine used by the API, the SearchHelper uses Context (at least in our case of plaintext) however different method:

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/Results.java

Lines 265 to 270 in 701f162

	private static void printPlain(PrintPlainFinalArgs fargs, Document doc,
	int docId, String rpath) throws ClassNotFoundException, IOException {
	fargs.shelp.sourceContext.toggleAlt();
	boolean didPresentNew = fargs.shelp.sourceContext.getContext2(fargs.env,

which then uses OGKUnifiedHighlighter that extends Lucene's UnifiedHighlighter to display the results. It uses the query to filter the results, I think.

So, while SearchEngine (used by API) and SearchHelper (used by UI) both use the Context, they use radically different pieces of it in the generic case. There is one interesting tidbit where printPlain() falls back to using getContext() after getContext2() indicated no matching context:

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/Results.java

Lines 274 to 279 in 701f162

	if (!didPresentNew) {
	/*
	* Fall back to the old view, which re-analyzes text using
	* PlainLinetokenizer. E.g., when source code is updated (thus
	* affecting timestamps) but re-indexing is not yet complete.
	*/

however in that case it sets the isDefSearch properly:

opengrok/opengrok-indexer/src/main/java/org/opengrok/indexer/search/Results.java

Lines 292 to 299 in 701f162

	boolean isDefSearch = fargs.shelp.builder.isDefSearch();
	// SRCROOT is read with UTF-8 as a default.
	File sourceFile = new File(fargs.shelp.sourceRoot, rpath);
	try (FileInputStream fis = new FileInputStream(sourceFile);
	Reader r = IOUtils.createBOMStrippedReader(fis, StandardCharsets.UTF_8.name())) {
	fargs.shelp.sourceContext.getContext(r, fargs.out,
	fargs.xrefPrefix, fargs.morePrefix, rpath, tags, true,
	isDefSearch, null, scopes);

[vladak]

The use of Context to present the results from the /search API call likely leads to problems such as #2612, #3090, #3170.

[vladak]

Looking at #2732, this bug could be fixed if we merged that PR since it replaces the offending sourceContext.getContext() call with something else, however it might as well reintroduce the bug - need to check this.