Skip to content

Commit

Permalink
25 npe when featureset not exists (opensearch-project#49)
Browse files Browse the repository at this point in the history 
Signed-off-by: JohannesDaniel <[email protected]>
Signed-off-by: Scott Stults <[email protected]>
  • Loading branch information
@JohannesDaniel @sstults
JohannesDaniel authored and sstults committed Oct 31, 2024
1 parent aa5d228 commit 0f952be
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 17 deletions.
2 changes: 1 addition & 1 deletion src/javaRestTest/java/com/o19s/es/ltr/action/BaseIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public <E extends StorableElement> E getElement(Class<E> clazz, String type, Str
}

public <E extends StorableElement> E getElement(Class<E> clazz, String type, String name, String store) throws IOException {
return new IndexFeatureStore(store, this::client, parserFactory()).getAndParse(name, clazz, type);
return new IndexFeatureStore(store, this::client, parserFactory()).getAndParse(name, clazz, type).get();
}

protected LtrRankerParserFactory parserFactory() {
Expand Down
34 changes: 23 additions & 11 deletions src/main/java/com/o19s/es/ltr/feature/store/index/IndexFeatureStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.lucene.util.BytesRef;
import org.opensearch.ResourceNotFoundException;
import org.opensearch.action.admin.indices.create.CreateIndexRequest;
import org.opensearch.action.get.GetResponse;
import org.opensearch.client.Client;
Expand All @@ -53,6 +54,7 @@
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Supplier;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -108,13 +110,21 @@ public String getStoreName() {
}

@Override
public Feature load(String name) throws IOException {
return getAndParse(name, StoredFeature.class, StoredFeature.TYPE).optimize();
public Feature load(final String name) throws IOException {
return getAndParse(name, StoredFeature.class, StoredFeature.TYPE)
.orElseThrow(
() -> new ResourceNotFoundException("Unknown feature [" + name + "]")
)
.optimize();
}

@Override
public FeatureSet loadSet(String name) throws IOException {
return getAndParse(name, StoredFeatureSet.class, StoredFeatureSet.TYPE).optimize();
public FeatureSet loadSet(final String name) throws IOException {
return getAndParse(name, StoredFeatureSet.class, StoredFeatureSet.TYPE)
.orElseThrow(
() -> new ResourceNotFoundException("Unknown featureset [" + name + "]")
)
.optimize();
}

/**
Expand Down Expand Up @@ -165,19 +175,21 @@ public static boolean isIndexStore(String indexName) {

@Override
public CompiledLtrModel loadModel(String name) throws IOException {
StoredLtrModel model = getAndParse(name, StoredLtrModel.class, StoredLtrModel.TYPE);
if (model == null) {
throw new IllegalArgumentException("Unknown model [" + name + "]");
}
StoredLtrModel model = getAndParse(name, StoredLtrModel.class, StoredLtrModel.TYPE)
.orElseThrow(
() -> new ResourceNotFoundException("Unknown model [" + name + "]")
);

return model.compile(parserFactory);
}

public <E extends StorableElement> E getAndParse(String name, Class<E> eltClass, String type) throws IOException {
public <E extends StorableElement> Optional<E> getAndParse(String name, Class<E> eltClass, String type) throws IOException {
GetResponse response = internalGet(generateId(type, name)).get();
if (response.isExists()) {
return parse(eltClass, type, response.getSourceAsBytes());
return Optional.of(parse(eltClass, type, response.getSourceAsBytes()));

} else {
return null;
return Optional.empty();
}
}

Expand Down
81 changes: 76 additions & 5 deletions src/test/java/com/o19s/es/ltr/feature/store/index/IndexFeatureStoreTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,26 @@
import com.o19s.es.ltr.feature.store.StoredFeatureNormalizers;
import com.o19s.es.ltr.feature.store.StoredFeatureSet;
import com.o19s.es.ltr.feature.store.StoredLtrModel;
import com.o19s.es.ltr.ranker.parser.LtrRankerParserFactory;
import org.apache.lucene.util.BytesRef;
import org.apache.lucene.tests.util.LuceneTestCase;
import org.apache.lucene.tests.util.TestUtil;
import org.hamcrest.MatcherAssert;
import org.opensearch.ResourceNotFoundException;
import org.opensearch.action.get.GetRequestBuilder;
import org.opensearch.action.get.GetResponse;
import org.opensearch.client.Client;
import org.opensearch.client.Requests;
import org.opensearch.core.common.bytes.BytesReference;
import org.opensearch.common.xcontent.LoggingDeprecationHandler;
import org.opensearch.core.xcontent.NamedXContentRegistry;
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.core.xcontent.XContentParser;

import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Supplier;

import static com.o19s.es.ltr.feature.store.index.IndexFeatureStore.STORE_PREFIX;
import static com.o19s.es.ltr.feature.store.index.IndexFeatureStore.indexName;
Expand All @@ -44,9 +50,68 @@
import static org.apache.lucene.tests.util.TestUtil.randomRealisticUnicodeString;
import static org.apache.lucene.tests.util.TestUtil.randomSimpleString;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class IndexFeatureStoreTests extends LuceneTestCase {

private Client clientMock;
private GetRequestBuilder getRequestBuilder;
private GetResponse getResponseMock;

private Supplier<Client> clientSupplier = () -> clientMock;

private void setupMocks() {
clientMock = mock(Client.class);
getRequestBuilder = mock(GetRequestBuilder.class);
getResponseMock = mock(GetResponse.class);
when(clientMock.prepareGet(anyString(), anyString())).thenReturn(getRequestBuilder);
when(getRequestBuilder.get()).thenReturn(getResponseMock);
}

public void testThat_exceptionIsThrown_forNonExistingFeature() {
setupMocks();
when(getResponseMock.isExists()).thenReturn(false);
IndexFeatureStore store = new IndexFeatureStore("index", clientSupplier, mock(LtrRankerParserFactory.class));

MatcherAssert.assertThat(
expectThrows(
ResourceNotFoundException.class,
() -> store.load("my_feature")
).getMessage(),
equalTo("Unknown feature [my_feature]")
);
}

public void testThat_exceptionIsThrown_forNonExistingFeatureSet() {
setupMocks();
when(getResponseMock.isExists()).thenReturn(false);
IndexFeatureStore store = new IndexFeatureStore("index", clientSupplier, mock(LtrRankerParserFactory.class));

MatcherAssert.assertThat(
expectThrows(
ResourceNotFoundException.class,
() -> store.loadSet("my_feature_set")
).getMessage(),
equalTo("Unknown featureset [my_feature_set]")
);
}

public void testThat_exceptionIsThrown_forNonExistingModel() {
setupMocks();
when(getResponseMock.isExists()).thenReturn(false);
IndexFeatureStore store = new IndexFeatureStore("index", clientSupplier, mock(LtrRankerParserFactory.class));

MatcherAssert.assertThat(
expectThrows(
ResourceNotFoundException.class,
() -> store.loadModel("my_model")
).getMessage(),
equalTo("Unknown model [my_model]")
);
}

public void testParse() throws Exception {
parseAssertions(LtrTestUtils.randomFeature());
parseAssertions(LtrTestUtils.randomFeatureSet());
Expand Down Expand Up @@ -102,10 +167,16 @@ public void testBadValues() throws IOException {
builder = XContentBuilder.builder(Requests.INDEX_CONTENT_TYPE.xContent());
map.put("featureset", LtrTestUtils.randomFeatureSet());
BytesReference bytes2 = BytesReference.bytes(builder.map(map));
assertThat(expectThrows(IllegalArgumentException.class,
() -> IndexFeatureStore.parse(StoredFeature.class, StoredFeature.TYPE, bytes2))
.getMessage(), equalTo("Expected an element of type [" + StoredFeature.TYPE + "] but" +
" got [" + StoredFeatureSet.TYPE + "]."));
assertThat(
expectThrows(
IllegalArgumentException.class,
() -> IndexFeatureStore.parse(StoredFeature.class, StoredFeature.TYPE, bytes2)
).getMessage(),
equalTo(
"Expected an element of type [" + StoredFeature.TYPE + "] but" +
" got [" + StoredFeatureSet.TYPE + "]."
)
);
}

private void parseAssertions(StorableElement elt) throws IOException {
Expand Down

0 comments on commit 0f952be

Please sign in to comment.