-
Notifications
You must be signed in to change notification settings - Fork 282
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix NPE and add additional graceful error handling (#2691)
Signed-off-by: Craig Perkins <[email protected]>
- Loading branch information
Showing
3 changed files
with
257 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
85 changes: 85 additions & 0 deletions
85
src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| * | ||
| * Modifications Copyright OpenSearch Contributors. See | ||
| * GitHub history for details. | ||
| */ | ||
|
|
||
| package org.opensearch.security.support; | ||
|
|
||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| import org.opensearch.index.shard.SearchOperationListener; | ||
| import org.opensearch.search.internal.ReaderContext; | ||
| import org.opensearch.search.internal.SearchContext; | ||
| import org.opensearch.transport.TransportRequest; | ||
|
|
||
| /** | ||
| * Guarded version of Search Operation Listener to ensure critical request paths succeed | ||
| */ | ||
| public interface GuardedSearchOperationWrapper { | ||
|
|
||
| static final Logger log = LogManager.getLogger(GuardedSearchOperationWrapper.class); | ||
|
|
||
| void onPreQueryPhase(final SearchContext context); | ||
|
|
||
| void onNewReaderContext(final ReaderContext readerContext); | ||
|
|
||
| void onNewScrollContext(final ReaderContext readerContext); | ||
|
|
||
| void validateReaderContext(final ReaderContext readerContext, final TransportRequest transportRequest); | ||
|
|
||
| void onQueryPhase(final SearchContext searchContext, final long tookInNanos); | ||
|
|
||
| default SearchOperationListener toListener() { | ||
| return new InnerSearchOperationListener(this); | ||
| } | ||
|
|
||
| static class InnerSearchOperationListener implements SearchOperationListener { | ||
|
|
||
| private GuardedSearchOperationWrapper that; | ||
| InnerSearchOperationListener(GuardedSearchOperationWrapper that) { | ||
| this.that = that; | ||
| } | ||
|
|
||
| @Override | ||
| public void onPreQueryPhase(final SearchContext searchContext) { | ||
| try { | ||
| that.onPreQueryPhase(searchContext); | ||
| } catch (final Exception e) { | ||
| searchContext.setTask(null); | ||
| log.error("Cancelled request due to internal error", e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onNewReaderContext(final ReaderContext readerContext) { | ||
| that.onNewReaderContext(readerContext); | ||
| } | ||
|
|
||
| @Override | ||
| public void onNewScrollContext(final ReaderContext readerContext) { | ||
| that.onNewScrollContext(readerContext); | ||
| } | ||
|
|
||
| @Override | ||
| public void validateReaderContext(final ReaderContext readerContext, final TransportRequest transportRequest) { | ||
| that.validateReaderContext(readerContext, transportRequest); | ||
| } | ||
|
|
||
| @Override | ||
| public void onQueryPhase(final SearchContext searchContext, final long tookInNanos) { | ||
| try { | ||
| that.onQueryPhase(searchContext, tookInNanos); | ||
| } catch (final Exception e) { | ||
| searchContext.setTask(null); | ||
| log.error("Cancelled request due to internal error", e); | ||
| } | ||
| } | ||
| } | ||
| } |
146 changes: 146 additions & 0 deletions
146
src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| * | ||
| * Modifications Copyright OpenSearch Contributors. See | ||
| * GitHub history for details. | ||
| */ | ||
| package org.opensearch.security.support; | ||
|
|
||
| import java.util.concurrent.atomic.AtomicReference; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| import org.opensearch.index.shard.SearchOperationListener; | ||
| import org.opensearch.search.internal.ReaderContext; | ||
| import org.opensearch.search.internal.SearchContext; | ||
| import org.opensearch.transport.TransportRequest; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.hamcrest.Matchers.notNullValue; | ||
| import static org.junit.Assert.assertThrows; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.verify; | ||
|
|
||
|
|
||
| public class GuardedSearchOperationWrapperTest { | ||
|
|
||
| @Test | ||
| public void onNewReaderContextCanThrowException() { | ||
| final String expectedExceptionText = "abcd1234"; | ||
|
|
||
| DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { | ||
| @Override | ||
| public void onNewReaderContext(ReaderContext readerContext) { | ||
| throw new RuntimeException(expectedExceptionText); | ||
| } | ||
| }; | ||
|
|
||
| final RuntimeException expectedException = assertThrows(RuntimeException.class, testWrapper::exerciseAllMethods); | ||
|
|
||
| assertThat(expectedException.getMessage(), equalTo(expectedExceptionText)); | ||
| } | ||
|
|
||
| @Test | ||
| public void onNewScrollContextCanThrowException() { | ||
| final String expectedExceptionText = "qwerty978"; | ||
|
|
||
| DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { | ||
| @Override | ||
| public void onNewScrollContext(ReaderContext readerContext) { | ||
| throw new RuntimeException(expectedExceptionText); | ||
| } | ||
| }; | ||
|
|
||
| final RuntimeException expectedException = assertThrows(RuntimeException.class, testWrapper::exerciseAllMethods); | ||
|
|
||
| assertThat(expectedException.getMessage(), equalTo(expectedExceptionText)); | ||
| } | ||
|
|
||
| @Test | ||
| public void validateReaderContextCanThrowException() { | ||
| final String expectedExceptionText = "validationException"; | ||
|
|
||
| DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { | ||
| @Override | ||
| public void validateReaderContext(ReaderContext readerContext, TransportRequest transportRequest) { | ||
| throw new RuntimeException(expectedExceptionText); | ||
| } | ||
| }; | ||
|
|
||
| final RuntimeException expectedException = assertThrows(RuntimeException.class, testWrapper::exerciseAllMethods); | ||
|
|
||
| assertThat(expectedException.getMessage(), equalTo(expectedExceptionText)); | ||
| } | ||
|
|
||
| @Test | ||
| public void onPreQueryPhaseCannotThrow() { | ||
| AtomicReference<SearchContext> calledSearchContext = new AtomicReference<>(); | ||
| DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { | ||
| @Override | ||
| public void onPreQueryPhase(SearchContext context) { | ||
| calledSearchContext.set(context); | ||
| throw new RuntimeException("EXCEPTIONAL!"); | ||
| } | ||
| }; | ||
|
|
||
| testWrapper.exerciseAllMethods(); | ||
|
|
||
| assertThat(calledSearchContext.get(), notNullValue()); | ||
| verify(calledSearchContext.get()).setTask(null); | ||
| } | ||
|
|
||
| @Test | ||
| public void onQueryPhaseCannotThrow() { | ||
| AtomicReference<SearchContext> calledSearchContext = new AtomicReference<>(); | ||
| DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { | ||
| @Override | ||
| public void onQueryPhase(SearchContext context, long tookInNanos) { | ||
| calledSearchContext.set(context); | ||
| throw new RuntimeException("EXCEPTIONAL!"); | ||
| } | ||
| }; | ||
|
|
||
| testWrapper.exerciseAllMethods(); | ||
|
|
||
| assertThat(calledSearchContext.get(), notNullValue()); | ||
| verify(calledSearchContext.get()).setTask(null); | ||
| } | ||
|
|
||
| /** Only use to make testing easier */ | ||
| private static class DefaultingGuardedSearchOperationWrapper implements GuardedSearchOperationWrapper { | ||
|
|
||
| @Override | ||
| public void onNewReaderContext(ReaderContext readerContext) { | ||
| } | ||
|
|
||
| @Override | ||
| public void onNewScrollContext(ReaderContext readerContext) { | ||
| } | ||
|
|
||
| @Override | ||
| public void onPreQueryPhase(SearchContext context) { | ||
| } | ||
|
|
||
| @Override | ||
| public void onQueryPhase(SearchContext searchContext, long tookInNanos) { | ||
| } | ||
|
|
||
| @Override | ||
| public void validateReaderContext(ReaderContext readerContext, TransportRequest transportRequest) { | ||
| } | ||
|
|
||
| void exerciseAllMethods(){ | ||
| final SearchOperationListener sol = this.toListener(); | ||
| sol.onNewReaderContext(mock(ReaderContext.class)); | ||
| sol.onNewScrollContext(mock(ReaderContext.class)); | ||
| sol.onPreQueryPhase(mock(SearchContext.class)); | ||
| sol.onQueryPhase(mock(SearchContext.class), 12345L); | ||
| sol.validateReaderContext(mock(ReaderContext.class), mock(TransportRequest.class)); | ||
| } | ||
| } | ||
| } |