Skip to content

Commit 93b8ec7

Browse files
committed
XWIKI-21138: Improve Solr query filtering
1 parent 5f206f4 commit 93b8ec7

File tree

2 files changed

+83
-26
lines changed

2 files changed

+83
-26
lines changed

Diff for: xwiki-platform-core/xwiki-platform-search/xwiki-platform-search-solr/xwiki-platform-search-solr-query/src/main/java/org/xwiki/query/solr/internal/SolrQueryExecutor.java

+19-21
Original file line numberDiff line numberDiff line change
@@ -233,36 +233,34 @@ private String[] toStringArray(Iterable iterable)
233233
protected void filterResponse(QueryResponse response, List<DocumentReference> usersToCheck)
234234
{
235235
SolrDocumentList results = response.getResults();
236-
long numFound = results.getNumFound();
236+
long numResults = results.size();
237237

238-
// Since we are modifying the results collection, we need to iterate over its copy.
239-
for (SolrDocument result : new ArrayList<SolrDocument>(results)) {
238+
results.removeIf(result -> {
239+
boolean keep = false;
240240
try {
241241
DocumentReference resultDocumentReference = this.solrDocumentReferenceResolver.resolve(result);
242242

243-
if (!isAllowed(resultDocumentReference, usersToCheck)) {
243+
keep = isAllowed(resultDocumentReference, usersToCheck);
244+
} catch (Exception e) {
245+
// Don't take any risk of including a result for which we cannot determine the document reference and
246+
// thus cannot determine if the given users have access to it or not.
247+
this.logger.warn("Removing bad result: {}", result, e);
248+
}
244249

245-
// Remove the current incompatible result.
246-
results.remove(result);
250+
// FIXME: We should update maxScore as well when removing the top scored item. How do we do that?
251+
// Sorting based on score might be a not so expensive option.
247252

248-
// Decrement the number of results.
249-
numFound--;
253+
// FIXME: What about highlighting, facets and all the other data inside the QueryResponse?
250254

251-
// FIXME: We should update maxScore as well when removing the top scored item. How do we do that?
252-
// Sorting based on score might be a not so expensive option.
255+
return !keep;
256+
});
253257

254-
// FIXME: What about highlighting, facets and all the other data inside the QueryResponse?
255-
}
256-
} catch (Exception e) {
257-
this.logger.warn("Skipping bad result: {}", result, e);
258-
}
259-
}
258+
long numFilteredResults = numResults - results.size();
259+
260+
// Update the number of results, excluding the filtered ones.
261+
// Lower bound guard for the total number of results.
262+
long numFound = Math.max(0, response.getResults().getNumFound() - numFilteredResults);
260263

261-
// Update the new number of results, excluding the filtered ones.
262-
if (numFound < 0) {
263-
// Lower bound guard for the total number of results.
264-
numFound = 0;
265-
}
266264
results.setNumFound(numFound);
267265
}
268266

Diff for: xwiki-platform-core/xwiki-platform-search/xwiki-platform-search-solr/xwiki-platform-search-solr-query/src/test/java/org/xwiki/query/solr/SolrQueryExecutorTest.java

+64-5
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import java.lang.reflect.ParameterizedType;
2323
import java.util.Arrays;
24+
import java.util.Collections;
2425
import java.util.Locale;
2526

2627
import org.apache.solr.client.solrj.SolrQuery;
@@ -72,19 +73,19 @@ public class SolrQueryExecutorTest
7273
{
7374
private static final String ITERABLE_PARAM_NAME = "multiParam";
7475

75-
private static final String[] ITERABLE_PARAM_EXPECTED = {"value1", "value2"};
76+
private static final String[] ITERABLE_PARAM_EXPECTED = { "value1", "value2" };
7677

7778
private static final Iterable<String> ITERABLE_PARAM_VALUE = Arrays.asList(ITERABLE_PARAM_EXPECTED);
7879

7980
private static final String INT_ARR_PARAM_NAME = "intArrayParam";
8081

81-
private static final String[] INT_ARR_PARAM_EXPECTED = {"-42", "4711"};
82+
private static final String[] INT_ARR_PARAM_EXPECTED = { "-42", "4711" };
8283

83-
private static final int[] INT_ARR_PARAM_VALUE = {-42, 4711};
84+
private static final int[] INT_ARR_PARAM_VALUE = { -42, 4711 };
8485

8586
private static final String STR_ARR_PARAM_NAME = "stringArrayParam";
8687

87-
private static final String[] STR_ARR_PARAM_EXPECTED = {"valueA", "valueB"};
88+
private static final String[] STR_ARR_PARAM_EXPECTED = { "valueA", "valueB" };
8889

8990
private static final String[] STR_ARR_PARAM_VALUE = STR_ARR_PARAM_EXPECTED;
9091

@@ -228,4 +229,62 @@ public void filterResponse() throws Exception
228229
results = ((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
229230
assertEquals(Arrays.asList(alice, bob), results);
230231
}
231-
}
232+
233+
@Test
234+
public void filterResponseWithException() throws Exception
235+
{
236+
ParameterizedType resolverType =
237+
new DefaultParameterizedType(null, DocumentReferenceResolver.class, SolrDocument.class);
238+
DocumentReferenceResolver<SolrDocument> resolver = this.componentManager.getInstance(resolverType);
239+
240+
AuthorizationManager authorizationManager = this.componentManager.getInstance(AuthorizationManager.class);
241+
242+
DocumentReference currentUserReference = new DocumentReference("xwiki", "XWiki", "currentuser");
243+
this.oldCore.getXWikiContext().setUserReference(currentUserReference);
244+
245+
DocumentReference aliceReference = new DocumentReference("wiki", "Users", "Alice");
246+
SolrDocument alice = new SolrDocument();
247+
when(resolver.resolve(alice)).thenReturn(aliceReference);
248+
249+
DocumentReference bobReference = new DocumentReference("wiki", "Users", "Bob");
250+
when(authorizationManager.hasAccess(Right.VIEW, currentUserReference, bobReference)).thenReturn(true);
251+
SolrDocument bob = new SolrDocument();
252+
when(resolver.resolve(bob)).thenReturn(bobReference);
253+
254+
SolrDocumentList sourceResults = new SolrDocumentList();
255+
sourceResults.addAll(Arrays.asList(alice, bob));
256+
sourceResults.setNumFound(2);
257+
258+
QueryResponse response = mock(QueryResponse.class);
259+
when(this.solr.query(any(SolrParams.class))).thenReturn(response);
260+
261+
DefaultQuery query = new DefaultQuery("", null);
262+
263+
// No right check, verify that the setup works.
264+
when(response.getResults()).thenReturn((SolrDocumentList) sourceResults.clone());
265+
SolrDocumentList results =
266+
((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
267+
assertEquals(Arrays.asList(alice, bob), results);
268+
assertEquals(2, results.getNumFound());
269+
270+
// Check current user right
271+
query.checkCurrentUser(true);
272+
273+
// Throw an exception when resolving Alice
274+
when(resolver.resolve(alice)).thenThrow(new RuntimeException("Alice"));
275+
when(response.getResults()).thenReturn((SolrDocumentList) sourceResults.clone());
276+
277+
results = ((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
278+
assertEquals(Collections.singletonList(bob), results);
279+
assertEquals(1, results.getNumFound());
280+
281+
// Throw also an exception when resolving Bob
282+
when(resolver.resolve(bob)).thenThrow(new RuntimeException("Bob"));
283+
when(response.getResults()).thenReturn((SolrDocumentList) sourceResults.clone());
284+
285+
// Assert that the results are empty when both throw an exception
286+
results = ((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
287+
assertEquals(Collections.emptyList(), results);
288+
assertEquals(0, results.getNumFound());
289+
}
290+
}

0 commit comments

Comments
 (0)