Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

25 npe when featureset not exists #49

Merged
merged 4 commits into from
Oct 15, 2024
Merged

25 npe when featureset not exists #49

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86e602e
fix: enhanced .gitignore
JohannesDaniel Oct 14, 2024
21d43f0
fix:
JohannesDaniel Oct 14, 2024
cc01f1a
fix: Removed unused imports
JohannesDaniel Oct 14, 2024
d19cc63
fix: Reverted change in .gitignore
JohannesDaniel Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,4 @@ eclipse-build
!/.settings/org.eclipse.jdt.ui.prefs

bin/*
.DS_Store
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we confirm that other OpenSearch projects have these? I know some projects instead say "put this in your global git ignore file!"

2 changes: 1 addition & 1 deletion src/javaRestTest/java/com/o19s/es/ltr/action/BaseIntegrationTest.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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