This is an automated email from the ASF dual-hosted git repository.
tbonelee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new e960f7c0d0 [ZEPPELIN-6414] Apply the authorization filter before the
search cutoff
e960f7c0d0 is described below
commit e960f7c0d00d2c52dd22e1cf5c7520964d177142
Author: 김동환 <[email protected]>
AuthorDate: Sun Sep 13 15:43:34 2026 +0900
[ZEPPELIN-6414] Apply the authorization filter before the search cutoff
### What is this PR for?
`/notebook/search` asked the search service for results and then dropped
the ones the caller may not read. Both search services cut their results down
to twenty before returning them, so that removal ran on an already shortened
list:
* A caller with access to few notes is served fewer results than it is
allowed to see. If the twenty highest scoring hits all belong to notes it
cannot read, the search comes back empty while its own matching notes sit just
below the cut.
* In `EmbeddingSearch` the entries the caller cannot read are also counted
into the table boost of Phase 1, so notes that never reach the caller still
move the ranking of the ones that do.
The read check now travels with the query, as a predicate over the note id,
and every implementation applies it before it cuts anything.
| | before | after |
| --- | --- | --- |
| `EmbeddingSearch` | scores every entry, cuts to `MAX_RESULTS`, REST
filters | skips unreadable entries while scoring, so neither the table weights
nor the cutoff see them |
| `LuceneSearch` | `searcher.search(query, 20)`, REST filters | walks the
hits in score order with `searchAfter` until a full page of readable hits is
collected, or the hits run out |
| `NotebookRestApi` | four permission calls per result, list mutated while
iterating | passes the predicate, no post-filtering |
`LuceneSearch` reads only the id field to decide on a hit and loads the
whole document for the hits it keeps, so hits that are dropped cost one stored
field read and no highlighting.
`query(String)` is removed rather than kept next to the new method: leaving
it would leave a way to search without saying who is asking, which is the
defect this issue is about. `SearchService` is bound in `ZeppelinServer` to the
three implementations in the repository and is not reachable as an extension
point, so nothing outside the tree implements it.
Note that this changes what the result limit means: it is now the top
twenty results the caller may read, rather than what is left of the top twenty
overall after filtering.
### What type of PR is it?
Bug Fix
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-6414
### How should this be tested?
New tests in both search services,
`keepsReadableResultsThatTheCutWouldHide`, put twenty-eight matching notes in
the index and let the caller read three of them. Each test first checks its own
fixture: it queries with an allow-all predicate and asserts that the readable
notes really do fall outside the cut, so that the test cannot quietly stop
testing anything if the limit or the scoring changes later. It then queries
with the real predicate and expects all three readable notes back a [...]
`LuceneSearchTest.returnsNothingWhenTheCallerMayReadNothing` covers the
walk ending on its own when no hit is readable.
Run locally:
* `LuceneSearchTest` - 12 tests, including the two new ones
* `EmbeddingSearchTest` - 12 tests, including the new one, run with
`ZEPPELIN_EMBEDDING_TEST=true` after `bin/install-search-model.sh`
The existing call sites in both test classes now pass `id -> true`
explicitly.
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions?
`SearchService.query(String)` is replaced by `query(String,
Predicate<String>)`. Only the three in-tree implementations and the REST
resource use it.
* Does this needs documentation? No
### Possible follow-up
Indexing the note id as its own field would let `LuceneSearch` hand the
permission filter to Lucene instead of walking the hits, but it needs an index
schema change and a rebuild, so it is left out of this issue.
Closes #5427 from dev-donghwan/ZEPPELIN-6414.
Signed-off-by: ChanHo Lee <[email protected]>
---
.../org/apache/zeppelin/rest/NotebookRestApi.java | 14 +-
.../apache/zeppelin/search/EmbeddingSearch.java | 15 ++-
.../org/apache/zeppelin/search/LuceneSearch.java | 142 +++++++++++++--------
.../apache/zeppelin/search/NoSearchService.java | 3 +-
.../org/apache/zeppelin/search/SearchService.java | 17 ++-
.../zeppelin/search/EmbeddingSearchTest.java | 67 ++++++++--
.../apache/zeppelin/search/LuceneSearchTest.java | 70 ++++++++--
7 files changed, 234 insertions(+), 94 deletions(-)
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
index 184e01f080..fd7f87c5a6 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
@@ -1211,18 +1211,8 @@ public class NotebookRestApi extends AbstractRestApi {
HashSet<String> userAndRoles = new HashSet<>();
userAndRoles.add(principal);
userAndRoles.addAll(roles);
- List<Map<String, String>> notesFound = noteSearchService.query(queryTerm);
- for (int i = 0; i < notesFound.size(); i++) {
- String[] ids = notesFound.get(i).get("id").split("/", 2);
- String noteId = ids[0];
- if (!authorizationService.isOwner(noteId, userAndRoles) &&
- !authorizationService.isReader(noteId, userAndRoles) &&
- !authorizationService.isWriter(noteId, userAndRoles) &&
- !authorizationService.isRunner(noteId, userAndRoles)) {
- notesFound.remove(i);
- i--;
- }
- }
+ List<Map<String, String>> notesFound = noteSearchService.query(queryTerm,
+ noteId -> authorizationService.isReader(noteId, userAndRoles));
LOGGER.info("{} notes found", notesFound.size());
return new JsonResponse<>(Status.OK, notesFound).build();
}
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/search/EmbeddingSearch.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/search/EmbeddingSearch.java
index 2d60d3fc28..c95e6fbe70 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/search/EmbeddingSearch.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/search/EmbeddingSearch.java
@@ -42,6 +42,7 @@ import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.function.Predicate;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executors;
@@ -509,15 +510,11 @@ public class EmbeddingSearch extends SearchService {
// ---- SearchService implementation ----
@Override
- // TODO(ZEPPELIN-6414): Accept user/roles (or a readability Predicate) and
apply the auth
- // filter before Phase-1 table collection and before the top-K cutoff.
Currently the REST
- // layer filters after truncation, which can hide results the caller is
authorized for and
- // lets inaccessible notes contaminate the table-boost ranking. Requires a
SearchService
- // interface change that also affects LuceneSearch.
- public List<Map<String, String>> query(String queryStr) {
+ public List<Map<String, String>> query(String queryStr, Predicate<String>
readable) {
if (StringUtils.isBlank(queryStr) || index.isEmpty()) {
return Collections.emptyList();
}
+ Map<String, Boolean> readableNotes = new HashMap<>();
float[] queryEmbedding = embed(queryStr);
String queryLower = queryStr.toLowerCase(Locale.ROOT);
@@ -527,6 +524,12 @@ public class EmbeddingSearch extends SearchService {
indexLock.readLock().lock();
try {
for (Map.Entry<String, IndexEntry> entry : index.entrySet()) {
+ // Dropping the entries here keeps them out of the table weights below
and out of
+ // the cutoff, so the caller is served its own top results and not
what is left of
+ // everyone's top results.
+ if (!readableNotes.computeIfAbsent(noteIdOf(entry.getKey()),
readable::test)) {
+ continue;
+ }
float sim = cosineSimilarity(queryEmbedding,
entry.getValue().embedding);
IndexEntry ie = entry.getValue();
if (ie.text != null &&
ie.text.toLowerCase(Locale.ROOT).contains(queryLower)) {
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/search/LuceneSearch.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/search/LuceneSearch.java
index 904069fb33..8d1a081cf3 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/search/LuceneSearch.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/search/LuceneSearch.java
@@ -21,10 +21,13 @@ import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
import javax.annotation.PreDestroy;
import jakarta.inject.Inject;
@@ -76,6 +79,18 @@ public class LuceneSearch extends SearchService {
private static final String SEARCH_FIELD_TITLE = "header";
private static final String PARAGRAPH = "paragraph";
private static final String ID_FIELD = "id";
+ /** Number of results a query returns at most. */
+ private static final int MAX_RESULTS = 20;
+ /** Number of hits pulled from the index on the first round while looking
for readable ones. */
+ private static final int HIT_BATCH_SIZE = 20;
+ /**
+ * Upper bound for the growing batch. Each searchAfter round re-executes the
query, so the
+ * batch doubles per round to keep the round count logarithmic in the number
of hits walked;
+ * the cap bounds the per-round allocation.
+ */
+ private static final int MAX_HIT_BATCH_SIZE = 1024;
+ /** Only the id is needed to tell whether the caller may read a hit. */
+ private static final Set<String> ID_FIELD_ONLY =
Collections.singleton(ID_FIELD);
private final Directory indexDirectory;
private final IndexWriter indexWriter;
@@ -113,7 +128,7 @@ public class LuceneSearch extends SearchService {
* @see org.apache.zeppelin.search.Search#query(java.lang.String)
*/
@Override
- public List<Map<String, String>> query(String queryStr) {
+ public List<Map<String, String>> query(String queryStr, Predicate<String>
readable) {
if (null == indexDirectory) {
throw new IllegalStateException(
"Something went wrong on instance creation time, index dir is null");
@@ -134,7 +149,7 @@ public class LuceneSearch extends SearchService {
SimpleHTMLFormatter htmlFormatter = new SimpleHTMLFormatter();
Highlighter highlighter = new Highlighter(htmlFormatter, new
QueryScorer(query));
- result = doSearch(indexSearcher, query, analyzer, highlighter);
+ result = doSearch(indexSearcher, query, analyzer, highlighter, readable);
} catch (IOException e) {
LOGGER.error("Failed to open index dir {}, make sure indexing finished
OK", indexDirectory, e);
} catch (ParseException e) {
@@ -144,65 +159,45 @@ public class LuceneSearch extends SearchService {
}
private List<Map<String, String>> doSearch(
- IndexSearcher searcher, Query query, Analyzer analyzer, Highlighter
highlighter) {
+ IndexSearcher searcher, Query query, Analyzer analyzer, Highlighter
highlighter,
+ Predicate<String> readable) {
List<Map<String, String>> matchingParagraphs = new ArrayList<>();
- ScoreDoc[] hits;
+ Map<String, Boolean> readableNotes = new HashMap<>();
try {
- hits = searcher.search(query, 20).scoreDocs;
- for (int i = 0; i < hits.length; i++) {
- LOGGER.debug("doc={} score={}", hits[i].doc, hits[i].score);
-
- int id = hits[i].doc;
- Document doc = searcher.doc(id);
- String path = doc.get(ID_FIELD);
- if (path != null) {
- LOGGER.debug( "{}. {}", (i + 1), path);
- String title = doc.get("title");
- if (title != null) {
- LOGGER.debug(" Title: {}", doc.get("title"));
+ // Walk the hits in score order and keep the ones the caller may read
until the result
+ // set is full or the hits run out. Reading is checked here and not on
the result set,
+ // because a cut that runs first would hide results the caller is
allowed to see.
+ ScoreDoc lastHit = null;
+ int batchSize = HIT_BATCH_SIZE;
+ while (matchingParagraphs.size() < MAX_RESULTS) {
+ ScoreDoc[] hits = (lastHit == null
+ ? searcher.search(query, batchSize)
+ : searcher.searchAfter(lastHit, query, batchSize)).scoreDocs;
+ for (ScoreDoc hit : hits) {
+ if (matchingParagraphs.size() >= MAX_RESULTS) {
+ break;
}
-
- String text = doc.get(SEARCH_FIELD_TEXT);
- String header = doc.get(SEARCH_FIELD_TITLE);
- String fragment = "";
-
- if (text != null) {
- TokenStream tokenStream =
- TokenSources.getTokenStream(
- searcher.getIndexReader(), id, SEARCH_FIELD_TEXT,
analyzer);
- TextFragment[] frags =
highlighter.getBestTextFragments(tokenStream, text, true, 3);
- LOGGER.debug(" {} fragments found for query '{}'",
frags.length, query);
- for (TextFragment frag : frags) {
- if ((frag != null) && (frag.getScore() > 0)) {
- LOGGER.debug(" Fragment: {}", frag);
- }
- }
- fragment = (frags != null && frags.length > 0) ?
frags[0].toString() : "";
+ LOGGER.debug("doc={} score={}", hit.doc, hit.score);
+ // Read the id alone to decide on a hit. The rest of the document is
only worth
+ // loading for the hits that end up in the result set.
+ String path = searcher.doc(hit.doc, ID_FIELD_ONLY).get(ID_FIELD);
+ if (path == null) {
+ LOGGER.info("No {} for this document", ID_FIELD);
+ continue;
}
-
- if (header != null) {
- TokenStream tokenTitle =
- TokenSources.getTokenStream(
- searcher.getIndexReader(), id, SEARCH_FIELD_TITLE,
analyzer);
- TextFragment[] frgTitle =
highlighter.getBestTextFragments(tokenTitle, header, true, 3);
- header = (frgTitle != null && frgTitle.length > 0) ?
frgTitle[0].toString() : "";
- } else {
- header = "";
+ if (!readableNotes.computeIfAbsent(noteIdOf(path), readable::test)) {
+ continue;
}
matchingParagraphs.add(
- ImmutableMap.<String, String>builder()
- .put("id", path)
- .put("name", title)
- .put("snippet", fragment)
- .put("text", text)
- .put("header", header)
- .put("title", header)
- .put("tables", "")
- .put("output", "")
- .build());
- } else {
- LOGGER.info("{}. No {} for this document", i + 1, ID_FIELD);
+ toMatch(searcher, query, analyzer, highlighter, hit.doc,
searcher.doc(hit.doc)));
+ }
+ // Fewer hits than asked for means the index has no more; asking again
would only
+ // return an empty batch.
+ if (hits.length < batchSize) {
+ break;
}
+ lastHit = hits[hits.length - 1];
+ batchSize = Math.min(batchSize * 2, MAX_HIT_BATCH_SIZE);
}
} catch (IOException | InvalidTokenOffsetsException e) {
LOGGER.error("Exception on searching for {}", query, e);
@@ -210,6 +205,45 @@ public class LuceneSearch extends SearchService {
return matchingParagraphs;
}
+ private Map<String, String> toMatch(IndexSearcher searcher, Query query,
Analyzer analyzer,
+ Highlighter highlighter, int docId, Document doc)
+ throws IOException, InvalidTokenOffsetsException {
+ String path = doc.get(ID_FIELD);
+ String title = doc.get("title");
+ String text = doc.get(SEARCH_FIELD_TEXT);
+ String header = doc.get(SEARCH_FIELD_TITLE);
+ String fragment = "";
+
+ if (text != null) {
+ TokenStream tokenStream =
+ TokenSources.getTokenStream(
+ searcher.getIndexReader(), docId, SEARCH_FIELD_TEXT, analyzer);
+ TextFragment[] frags = highlighter.getBestTextFragments(tokenStream,
text, true, 3);
+ LOGGER.debug(" {} fragments found for query '{}'", frags.length,
query);
+ fragment = (frags != null && frags.length > 0) ? frags[0].toString() :
"";
+ }
+
+ if (header != null) {
+ TokenStream tokenTitle =
+ TokenSources.getTokenStream(
+ searcher.getIndexReader(), docId, SEARCH_FIELD_TITLE, analyzer);
+ TextFragment[] frgTitle = highlighter.getBestTextFragments(tokenTitle,
header, true, 3);
+ header = (frgTitle != null && frgTitle.length > 0) ?
frgTitle[0].toString() : "";
+ } else {
+ header = "";
+ }
+ return ImmutableMap.<String, String>builder()
+ .put("id", path)
+ .put("name", title)
+ .put("snippet", fragment)
+ .put("text", text)
+ .put("header", header)
+ .put("title", header)
+ .put("tables", "")
+ .put("output", "")
+ .build();
+ }
+
/* (non-Javadoc)
* @see
org.apache.zeppelin.search.Search#updateIndexDoc(org.apache.zeppelin.notebook.Note)
*/
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/search/NoSearchService.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/search/NoSearchService.java
index 1d0d33d304..10a0493236 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/search/NoSearchService.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/search/NoSearchService.java
@@ -21,6 +21,7 @@ import jakarta.inject.Inject;
import java.util.Collections;
import java.util.List;
import java.util.Map;
+import java.util.function.Predicate;
public class NoSearchService extends SearchService {
@@ -30,7 +31,7 @@ public class NoSearchService extends SearchService {
}
@Override
- public List<Map<String, String>> query(String queryStr) {
+ public List<Map<String, String>> query(String queryStr, Predicate<String>
readable) {
return Collections.emptyList();
}
diff --git
a/zeppelin-server/src/main/java/org/apache/zeppelin/search/SearchService.java
b/zeppelin-server/src/main/java/org/apache/zeppelin/search/SearchService.java
index d36e4b1693..b37e452aa1 100644
---
a/zeppelin-server/src/main/java/org/apache/zeppelin/search/SearchService.java
+++
b/zeppelin-server/src/main/java/org/apache/zeppelin/search/SearchService.java
@@ -19,6 +19,7 @@ package org.apache.zeppelin.search;
import java.io.IOException;
import java.util.List;
import java.util.Map;
+import java.util.function.Predicate;
import org.apache.zeppelin.notebook.NoteEventAsyncListener;
import javax.annotation.PreDestroy;
@@ -39,9 +40,23 @@ public abstract class SearchService extends
NoteEventAsyncListener {
* Full-text search in all the notes
*
* @param queryStr a query
+ * @param readable tells for a noteId whether the caller may read it.
Entries the caller
+ * cannot read are dropped before the result set is cut
down, so that the
+ * caller is not served fewer results than it is allowed to
see.
* @return A list of matching paragraphs (id, text, snippet w/ highlight)
*/
- public abstract List<Map<String, String>> query(String queryStr);
+ public abstract List<Map<String, String>> query(String queryStr,
Predicate<String> readable);
+
+ /**
+ * The id of an indexed document is either a noteId or a noteId followed by
the paragraph.
+ *
+ * @see LuceneSearch#formatId(String, org.apache.zeppelin.notebook.Paragraph)
+ * @see EmbeddingSearch#formatId(String,
org.apache.zeppelin.notebook.Paragraph)
+ */
+ static String noteIdOf(String documentId) {
+ int separator = documentId.indexOf('/');
+ return separator < 0 ? documentId : documentId.substring(0, separator);
+ }
/**
* Updates note index for the given note, only update index of note meta
info,
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/search/EmbeddingSearchTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/search/EmbeddingSearchTest.java
index 902925eb7c..ae2031aab8 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/search/EmbeddingSearchTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/search/EmbeddingSearchTest.java
@@ -17,6 +17,7 @@
package org.apache.zeppelin.search;
import static org.apache.zeppelin.search.EmbeddingSearch.formatId;
+import static org.apache.zeppelin.search.SearchService.noteIdOf;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -26,6 +27,9 @@ import static org.mockito.Mockito.when;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Predicate;
import java.util.List;
import java.util.Map;
@@ -127,7 +131,7 @@ class EmbeddingSearchTest {
drainSearchEvents();
// when — semantic search for a meaningful phrase
- List<Map<String, String>> results = searchService.query("testing
something");
+ List<Map<String, String>> results = searchService.query("testing
something", id -> true);
// then
assertFalse(results.isEmpty());
@@ -144,7 +148,7 @@ class EmbeddingSearchTest {
drainSearchEvents();
// when
- List<Map<String, String>> results = searchService.query("Notebook1");
+ List<Map<String, String>> results = searchService.query("Notebook1", id ->
true);
// then
assertFalse(results.isEmpty());
@@ -159,7 +163,7 @@ class EmbeddingSearchTest {
drainSearchEvents();
// when
- List<Map<String, String>> results =
searchService.query("testingTitleSearch");
+ List<Map<String, String>> results =
searchService.query("testingTitleSearch", id -> true);
// then
assertFalse(results.isEmpty());
@@ -178,7 +182,7 @@ class EmbeddingSearchTest {
drainSearchEvents();
// when — natural language query, no exact keyword match
- List<Map<String, String>> results = searchService.query("yesterday's
spending");
+ List<Map<String, String>> results = searchService.query("yesterday's
spending", id -> true);
// then — should rank the spend query higher than the user count query
assertFalse(results.isEmpty());
@@ -193,7 +197,7 @@ class EmbeddingSearchTest {
drainSearchEvents();
// when
- List<Map<String, String>> results = searchService.query("test");
+ List<Map<String, String>> results = searchService.query("test", id ->
true);
assertFalse(results.isEmpty());
// then — find the paragraph result (not the note-name result)
@@ -214,7 +218,7 @@ class EmbeddingSearchTest {
void canNotSearchBeforeIndexing() {
// given NO indexing was done
// when
- List<Map<String, String>> result = searchService.query("anything");
+ List<Map<String, String>> result = searchService.query("anything", id ->
true);
// then
assertTrue(result.isEmpty());
}
@@ -235,7 +239,8 @@ class EmbeddingSearchTest {
});
// then — updated content should now be findable
- List<Map<String, String>> results = searchService.query("reindexing
updated content");
+ List<Map<String, String>> results =
+ searchService.query("reindexing updated content", id -> true);
assertFalse(results.isEmpty());
}
@@ -252,16 +257,16 @@ class EmbeddingSearchTest {
String note2Id = newNoteWithParagraphs("Notebook2", "not test", "not test
at all");
drainSearchEvents();
- assertFalse(searchService.query("Notebook2").isEmpty());
+ assertFalse(searchService.query("Notebook2", id -> true).isEmpty());
// when
searchService.deleteNoteIndex(note2Id);
// then — no results should reference the deleted note's ID
- boolean foundNote2After = searchService.query("not test at all").stream()
+ boolean foundNote2After = searchService.query("not test at all", id ->
true).stream()
.anyMatch(r -> r.get("id").startsWith(note2Id));
assertFalse(foundNote2After, "Note2 should be removed from index after
deletion");
- assertFalse(searchService.query("Notebook1").isEmpty());
+ assertFalse(searchService.query("Notebook1", id -> true).isEmpty());
}
@Test
@@ -282,7 +287,7 @@ class EmbeddingSearchTest {
drainSearchEvents();
// then — "Notebook1" note name should still be findable
- assertFalse(searchService.query("Notebook1").isEmpty());
+ assertFalse(searchService.query("Notebook1", id -> true).isEmpty());
}
@Test
@@ -302,7 +307,7 @@ class EmbeddingSearchTest {
drainSearchEvents();
// then — the new paragraph should be findable by semantic query
- List<Map<String, String>> results = searchService.query("lifetime value");
+ List<Map<String, String>> results = searchService.query("lifetime value",
id -> true);
assertFalse(results.isEmpty(), "Newly added paragraph should be
searchable");
boolean found = results.stream()
.anyMatch(r -> r.get("text").contains("lifetime_value"));
@@ -311,6 +316,44 @@ class EmbeddingSearchTest {
// ---- Helper methods (same as LuceneSearchTest) ----
+ @Test
+ void keepsReadableResultsThatTheCutWouldHide() throws IOException,
InterruptedException {
+ // given: more notes than one result set holds. The notes the caller may
not read match
+ // the query exactly, the ones it may read carry the same words but say
more, so they
+ // score lower and fall outside the cut.
+ String queryStr = "quarterly revenue report";
+ Set<String> readableNoteIds = new HashSet<>();
+ for (int i = 0; i < 25; i++) {
+ newNoteWithParagraph("Hidden" + i, queryStr);
+ }
+ for (int i = 0; i < 3; i++) {
+ readableNoteIds.add(newNoteWithParagraph("Mine" + i, queryStr
+ + " which also walks through unrelated kitchen recipes, holiday
photographs and"
+ + " a long list of gardening tips that have nothing to do with the
numbers"));
+ }
+ drainSearchEvents();
+ Predicate<String> readable = readableNoteIds::contains;
+
+ // the fixture has to be one where cutting first actually loses results,
otherwise this
+ // test would pass on any implementation
+ List<Map<String, String>> unfiltered = searchService.query(queryStr, id ->
true);
+ long readableWithinCut = unfiltered.stream()
+ .filter(result -> readable.test(noteIdOf(result.get("id"))))
+ .count();
+ assertTrue(readableWithinCut < readableNoteIds.size(),
+ "the readable notes have to fall outside the cut for this test to mean
anything");
+
+ // when
+ List<Map<String, String>> results = searchService.query(queryStr,
readable);
+
+ // then: every readable note comes back, and nothing else does
+ assertEquals(readableNoteIds.size(), results.size());
+ for (Map<String, String> result : results) {
+ assertTrue(readable.test(noteIdOf(result.get("id"))),
+ "a result the caller may not read: " + result.get("id"));
+ }
+ }
+
private String newNoteWithParagraph(String noteName, String parText) throws
IOException {
String noteId = newNote(noteName);
notebook.processNote(noteId, note -> {
diff --git
a/zeppelin-server/src/test/java/org/apache/zeppelin/search/LuceneSearchTest.java
b/zeppelin-server/src/test/java/org/apache/zeppelin/search/LuceneSearchTest.java
index a27c2df358..71be92efd7 100644
---
a/zeppelin-server/src/test/java/org/apache/zeppelin/search/LuceneSearchTest.java
+++
b/zeppelin-server/src/test/java/org/apache/zeppelin/search/LuceneSearchTest.java
@@ -16,6 +16,7 @@
*/
package org.apache.zeppelin.search;
import static org.apache.zeppelin.search.LuceneSearch.formatId;
+import static org.apache.zeppelin.search.SearchService.noteIdOf;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -28,6 +29,9 @@ import static org.mockito.Mockito.when;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Predicate;
import java.util.List;
import java.util.Map;
@@ -98,7 +102,7 @@ class LuceneSearchTest {
drainSearchEvents();
// when
- List<Map<String, String>> results = noteSearchService.query("all");
+ List<Map<String, String>> results = noteSearchService.query("all", id ->
true);
// then
assertFalse(results.isEmpty());
@@ -118,7 +122,7 @@ class LuceneSearchTest {
drainSearchEvents();
// when
- List<Map<String, String>> results = noteSearchService.query("Notebook1");
+ List<Map<String, String>> results = noteSearchService.query("Notebook1",
id -> true);
// then
assertFalse(results.isEmpty());
@@ -134,7 +138,7 @@ class LuceneSearchTest {
drainSearchEvents();
// when
- List<Map<String, String>> results =
noteSearchService.query("testingTitleSearch");
+ List<Map<String, String>> results =
noteSearchService.query("testingTitleSearch", id -> true);
// then
assertFalse(results.isEmpty());
@@ -168,7 +172,7 @@ class LuceneSearchTest {
void canNotSearchBeforeIndexing() {
// given NO noteSearchService.index() was called
// when
- List<Map<String, String>> result = noteSearchService.query("anything");
+ List<Map<String, String>> result = noteSearchService.query("anything", id
-> true);
// then
assertTrue(result.isEmpty());
// assert logs were printed
@@ -193,10 +197,10 @@ class LuceneSearchTest {
});
// then
- List<Map<String, String>> results = noteSearchService.query("all");
+ List<Map<String, String>> results = noteSearchService.query("all", id ->
true);
assertTrue(results.isEmpty());
- results = noteSearchService.query("indeed");
+ results = noteSearchService.query("indeed", id -> true);
assertFalse(results.isEmpty());
}
@@ -221,7 +225,7 @@ class LuceneSearchTest {
noteSearchService.deleteNoteIndex(note2Id);
// then
- assertTrue(noteSearchService.query("all").isEmpty());
+ assertTrue(noteSearchService.query("all", id -> true).isEmpty());
assertTrue(resultForQuery("Notebook2").isEmpty());
List<Map<String, String>> results = resultForQuery("test");
@@ -287,7 +291,7 @@ class LuceneSearchTest {
}
private List<Map<String, String>> resultForQuery(String q) {
- return noteSearchService.query(q);
+ return noteSearchService.query(q, id -> true);
}
/**
@@ -297,6 +301,56 @@ class LuceneSearchTest {
* @param parText text of the paragraph
* @return Note
*/
+ @Test
+ void keepsReadableResultsThatTheCutWouldHide() throws IOException,
InterruptedException {
+ // given: more notes than one result set holds, and only a few of them
readable
+ Set<String> readableNoteIds = new HashSet<>();
+ for (int i = 0; i < 25; i++) {
+ newNoteWithParagraph("Hidden" + i, "shared search term");
+ }
+ for (int i = 0; i < 3; i++) {
+ readableNoteIds.add(newNoteWithParagraph("Mine" + i, "shared search
term"));
+ }
+ drainSearchEvents();
+ Predicate<String> readable = readableNoteIds::contains;
+
+ // the fixture has to be one where cutting first actually loses results,
otherwise this
+ // test would pass on any implementation
+ List<Map<String, String>> unfiltered = noteSearchService.query("shared
search term",
+ id -> true);
+ long readableWithinCut = unfiltered.stream()
+ .filter(result -> readable.test(noteIdOf(result.get("id"))))
+ .count();
+ assertTrue(readableWithinCut < readableNoteIds.size(),
+ "the readable notes have to fall outside the cut for this test to mean
anything");
+
+ // when
+ List<Map<String, String>> results = noteSearchService.query("shared search
term", readable);
+
+ // then: every readable note comes back, and nothing else does
+ assertEquals(readableNoteIds.size(), results.size());
+ for (Map<String, String> result : results) {
+ assertTrue(readable.test(noteIdOf(result.get("id"))),
+ "a result the caller may not read: " + result.get("id"));
+ }
+ }
+
+ @Test
+ void returnsNothingWhenTheCallerMayReadNothing() throws IOException,
InterruptedException {
+ // given
+ for (int i = 0; i < 25; i++) {
+ newNoteWithParagraph("Hidden" + i, "shared search term");
+ }
+ drainSearchEvents();
+
+ // when: the walk has to end on its own once the hits run out
+ List<Map<String, String>> results = noteSearchService.query("shared search
term",
+ id -> false);
+
+ // then
+ assertTrue(results.isEmpty(), () -> "unreadable results were returned: " +
results);
+ }
+
private String newNoteWithParagraph(String noteName, String parText) throws
IOException {
String note1Id = newNote(noteName);
notebook.processNote(note1Id,
