Skip to content

Commit

Permalink
Address Review Comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Naveen Tatikonda <[email protected]>
  • Loading branch information
@naveentatikonda
naveentatikonda committed Dec 22, 2022
1 parent 6e50cf0 commit 34e4fec
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 23 deletions.
48 changes: 33 additions & 15 deletions src/main/java/org/opensearch/knn/indices/ModelGraveyard.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,15 @@ public static NamedDiff readDiffFrom(StreamInput streamInput) throws IOException
*/
public static ModelGraveyard fromXContent(XContentParser xContentParser) throws IOException {
// Added validation checks to validate all the different possible scenarios
// model_ids:"abcd"
// {}
// {["abcd", "1234"]}
// {"dummy_field_name":}
// {model_ids:"abcd"}
// {model_ids:null}
// {model_ids:[]}
// {model_ids: ["abcd", "1234"]}
// model_ids:"abcd" - Throws exception as the START_OBJECT token is missing
// {} - Returns an empty ModelGraveyard object (BackwardCompatibility)
// {["abcd", "1234"]} - Throws exception as the FIELD_NAME token is missing
// {"dummy_field_name":} - Throws exception as the FIELD_NAME is not matching with model_ids
// {model_ids:"abcd"} - Throws exception as the START_ARRAY token is missing after field name
// {model_ids:null} - Throws exception as the START_ARRAY token is missing
// {model_ids:[]} - Parses and returns an empty ModelGraveyard object as there are no model ids
// {model_ids: ["abcd", "1234"]} - Parses and returns a ModelGraveyard object which contains the model ids "abcd" and "1234"
// {model_ids:[],dummy_field:[]} - Throws exception as we have FIELD_NAME(dummy_field) instead of END_OBJECT token

ModelGraveyard modelGraveyard = new ModelGraveyard();

Expand All @@ -173,7 +174,8 @@ public static ModelGraveyard fromXContent(XContentParser xContentParser) throws
// Validate if the first token is START_OBJECT
if (xContentParser.currentToken() != XContentParser.Token.START_OBJECT) {
throw new OpenSearchParseException(
"Unable to parse ModelGraveyard. Expecting START_OBJECT but got " + xContentParser.currentToken()
"Unable to parse ModelGraveyard. Expecting token start of an object but got {}",
xContentParser.currentToken()
);
}

Expand All @@ -185,28 +187,44 @@ public static ModelGraveyard fromXContent(XContentParser xContentParser) throws
// Validate it starts with FIELD_NAME token after START_OBJECT
if (xContentParser.currentToken() != XContentParser.Token.FIELD_NAME) {
throw new OpenSearchParseException(
"Unable to parse ModelGraveyard. Expecting FIELD_NAME but got " + xContentParser.currentToken()
"Unable to parse ModelGraveyard. Expecting token field name but got {}",
xContentParser.currentToken()
);
}

// Validating that FIELD_NAME matches with "model_ids"
if (!MODEL_IDS.equals(xContentParser.currentName())) {
throw new OpenSearchParseException(
"Unable to parse ModelGraveyard. Expecting field model_ids but got " + xContentParser.currentName()
"Unable to parse ModelGraveyard. Expecting field {} but got {}",
MODEL_IDS,
xContentParser.currentName()
);
}

// Validate it starts with START_ARRAY token after FIELD_NAME
if (xContentParser.nextToken() != XContentParser.Token.START_ARRAY) {
throw new OpenSearchParseException(
"Unable to parse ModelGraveyard. Expecting START_ARRAY but got " + xContentParser.currentToken()
"Unable to parse ModelGraveyard. Expecting token start of an array but got {}",
xContentParser.currentToken()
);
}

while (xContentParser.nextToken() != XContentParser.Token.END_OBJECT) {
if (xContentParser.currentToken() == XContentParser.Token.VALUE_STRING) {
modelGraveyard.add(xContentParser.text());
while (xContentParser.nextToken() != XContentParser.Token.END_ARRAY) {
if (xContentParser.currentToken() != XContentParser.Token.VALUE_STRING) {
throw new OpenSearchParseException(
"Unable to parse ModelGraveyard. Expecting token value string but got {}",
xContentParser.currentToken()
);
}
modelGraveyard.add(xContentParser.text());
}

// Validate if the last token is END_OBJECT
if (xContentParser.nextToken() != XContentParser.Token.END_OBJECT) {
throw new OpenSearchParseException(
"Unable to parse ModelGraveyard. Expecting token end of an object but got {}",
xContentParser.currentToken()
);
}
return modelGraveyard;
}
Expand Down
42 changes: 34 additions & 8 deletions src/test/java/org/opensearch/knn/indices/ModelGraveyardTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.knn.indices;

import lombok.SneakyThrows;
import org.opensearch.OpenSearchParseException;
import org.opensearch.common.io.stream.BytesStreamOutput;
import org.opensearch.common.xcontent.ToXContent;
Expand Down Expand Up @@ -62,7 +63,8 @@ public void testStreams() throws IOException {
}

// Validating {model_ids: ["test-model-id1", "test-model-id2"]}
public void testXContentBuilder() throws IOException {
@SneakyThrows
public void testXContentBuilder_withModelIds_returnsModelGraveyardWithModelIds() {
Set<String> modelIds = new HashSet<>();
String testModelId1 = "test-model-id1";
String testModelId2 = "test-model-id2";
Expand All @@ -82,7 +84,8 @@ public void testXContentBuilder() throws IOException {
}

// Validating {model_ids:[]}
public void testXContentBuilder2() throws IOException {
@SneakyThrows
public void testXContentBuilder_withoutModelIds_returnsModelGraveyardWithoutModelIds() {
ModelGraveyard testModelGraveyard = new ModelGraveyard();
XContentBuilder xContentBuilder = XContentFactory.jsonBuilder();
xContentBuilder.startObject();
Expand All @@ -94,7 +97,8 @@ public void testXContentBuilder2() throws IOException {
}

// Validating {test-model:"abcd"}
public void testXContentBuilder3() throws IOException {
@SneakyThrows
public void testXContentBuilder_withWrongFieldName_throwsException() {
XContentBuilder xContentBuilder = XContentFactory.jsonBuilder();
xContentBuilder.startObject();
xContentBuilder.field("test-model");
Expand All @@ -109,7 +113,8 @@ public void testXContentBuilder3() throws IOException {
}

// Validating {}
public void testXContentBuilder4() throws IOException {
@SneakyThrows
public void testXContentBuilder_validateBackwardCompatibility_returnsEmptyModelGraveyardObject() {
XContentBuilder xContentBuilder = XContentFactory.jsonBuilder();
xContentBuilder.startObject();
xContentBuilder.endObject();
Expand All @@ -119,18 +124,20 @@ public void testXContentBuilder4() throws IOException {
}

// Validating null
public void testXContentBuilder5() throws IOException {
@SneakyThrows
public void testXContentBuilder_withNull_throwsExceptionExpectingStartObject() {
XContentBuilder xContentBuilder = XContentFactory.jsonBuilder();

OpenSearchParseException ex = expectThrows(
OpenSearchParseException.class,
() -> ModelGraveyard.fromXContent(createParser(xContentBuilder))
);
assertTrue(ex.getMessage().contains("Expecting START_OBJECT but got null"));
assertTrue(ex.getMessage().contains("Expecting token start of an object but got null"));
}

// Validating {model_ids:"abcd"}
public void testXContentBuilder6() throws IOException {
@SneakyThrows
public void testXContentBuilder_withMissingStartArray_throwsExceptionExpectingStartArray() {
XContentBuilder xContentBuilder = XContentFactory.jsonBuilder();
xContentBuilder.startObject();
xContentBuilder.field("model_ids");
Expand All @@ -141,7 +148,26 @@ public void testXContentBuilder6() throws IOException {
OpenSearchParseException.class,
() -> ModelGraveyard.fromXContent(createParser(xContentBuilder))
);
assertTrue(ex.getMessage().contains("Expecting START_ARRAY but got VALUE_STRING"));
assertTrue(ex.getMessage().contains("Expecting token start of an array but got VALUE_STRING"));
}

// Validating {model_ids:["abcd"],model_ids_2:[]}
@SneakyThrows
public void testXContentBuilder_validateEndObject_throwsExceptionGotFieldName() {
XContentBuilder xContentBuilder = XContentFactory.jsonBuilder();
xContentBuilder.startObject();
xContentBuilder.startArray("model_ids");
xContentBuilder.value("abcd");
xContentBuilder.endArray();
xContentBuilder.startArray("model_ids_2");
xContentBuilder.endArray();
xContentBuilder.endObject();

OpenSearchParseException ex = expectThrows(
OpenSearchParseException.class,
() -> ModelGraveyard.fromXContent(createParser(xContentBuilder))
);
assertTrue(ex.getMessage().contains("Expecting token end of an object but got FIELD_NAME"));
}

public void testDiffStreams() throws IOException {
Expand Down

0 comments on commit 34e4fec

Please sign in to comment.