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

Jump to bottom

Allow ObjectParsers to specify required sets of fields #49661

Merged
merged 10 commits into from
Feb 11, 2020
Merged

Allow ObjectParsers to specify required sets of fields #49661

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
461db92
Allow ObjectParsers to specified required field sets
polyfractal Nov 27, 2019
a5274d7
Fix broken test, remove outdated docs
polyfractal Nov 27, 2019
fc55850
Remove agg-related changes, fix static access, better javadocs
polyfractal Dec 13, 2019
b52c2df
Merge remote-tracking branch 'origin/master' into agg_parser_script_f…
polyfractal Dec 13, 2019
e6da525
Checkstyle
polyfractal Dec 13, 2019
046fee1
Review cleanup
polyfractal Dec 16, 2019
971f014
Merge remote-tracking branch 'origin/master' into agg_parser_script_f…
polyfractal Feb 6, 2020
02d63eb
Review comments, fix build via content tags
polyfractal Feb 6, 2020
d176f2c
Add back test lost in merge conflict
polyfractal Feb 6, 2020
ef53515
Merge branch 'master' into agg_parser_script_field
elasticmachine Feb 10, 2020
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
56 changes: 56 additions & 0 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
public abstract class AbstractObjectParser<Value, Context>
implements BiFunction<XContentParser, Context, Value>, ContextParser<Context, Value> {

final List<String[]> requiredFieldSets = new ArrayList<>();

/**
* Declare some field. Usually it is easier to use {@link #declareString(BiConsumer, ParseField)} or
* {@link #declareObject(BiConsumer, ContextParser, ParseField)} rather than call this directly.
Expand Down Expand Up @@ -211,6 +213,60 @@ public <T> void declareFieldArray(BiConsumer<Value, List<T>> consumer, ContextPa
declareField(consumer, (p, c) -> parseArray(p, () -> itemParser.parse(p, c)), field, type);
}

/**
* Declares a set of fields that are required for parsing to succeed. Only one of the values
* provided per String[] must be matched.
*
* E.g. <code>declareRequiredFieldSet("foo", "bar");</code> means at least one of "foo" or
* "bar" fields must be present. If neither of those fields are present, an exception will be thrown.
*
* Multiple required sets can be configured:
*
* <pre><code>
* parser.declareRequiredFieldSet("foo", "bar");
* parser.declareRequiredFieldSet("bizz", "buzz");
* </code></pre>
*
* requires that one of "foo" or "bar" fields are present, and also that one of "bizz" or
* "buzz" fields are present.
*
* In JSON, it means any of these combinations are acceptable:
*
* <ul>
* <li><code>{"foo":"...", "bizz": "..."}</code></li>
* <li><code>{"bar":"...", "bizz": "..."}</code></li>
* <li><code>{"foo":"...", "buzz": "..."}</code></li>
* <li><code>{"bar":"...", "buzz": "..."}</code></li>
* <li><code>{"foo":"...", "bar":"...", "bizz": "..."}</code></li>
* <li><code>{"foo":"...", "bar":"...", "buzz": "..."}</code></li>
* <li><code>{"foo":"...", "bizz":"...", "buzz": "..."}</code></li>
* <li><code>{"bar":"...", "bizz":"...", "buzz": "..."}</code></li>
* <li><code>{"foo":"...", "bar":"...", "bizz": "...", "buzz": "..."}</code></li>
* </ul>
*
* The following would however be rejected:
*
* <table summary="failure cases">
* <tr><th>Provided JSON</th><th>Reason for failure</th></tr>
* <tr><td><code>{"foo":"..."}</code></td><td>Missing "bizz" or "buzz" field</td></tr>
* <tr><td><code>{"bar":"..."}</code></td><td>Missing "bizz" or "buzz" field</td></tr>
* <tr><td><code>{"bizz": "..."}</code></td><td>Missing "foo" or "bar" field</td></tr>
* <tr><td><code>{"buzz": "..."}</code></td><td>Missing "foo" or "bar" field</td></tr>
* <tr><td><code>{"foo":"...", "bar": "..."}</code></td><td>Missing "bizz" or "buzz" field</td></tr>
* <tr><td><code>{"bizz":"...", "buzz": "..."}</code></td><td>Missing "foo" or "bar" field</td></tr>
* <tr><td><code>{"unrelated":"..."}</code></td> <td>Missing "foo" or "bar" field, and missing "bizz" or "buzz" field</td></tr>
* </table>
*
* @param requriedSet
polyfractal marked this conversation as resolved.
Show resolved Hide resolved
* A set of required fields, where at least one of the fields in the array _must_ be present
*/
public void declareRequiredFieldSet(String... requriedSet) {
polyfractal marked this conversation as resolved.
Show resolved Hide resolved
if (requriedSet.length == 0) {
return;
}
this.requiredFieldSets.add(requriedSet);
}

private interface IOSupplier<T> {
T get() throws IOException;
}
Expand Down
25 changes: 25 additions & 0 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.Arrays;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.function.BiConsumer;
Expand Down Expand Up @@ -222,6 +223,8 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
FieldParser fieldParser = null;
String currentFieldName = null;
XContentLocation currentPosition = null;
List<String[]> requiredFields = new ArrayList<>(this.requiredFieldSets);

while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
Expand All @@ -235,11 +238,32 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
unknownFieldParser.acceptUnknownField(name, currentFieldName, currentPosition, parser, value, context);
} else {
fieldParser.assertSupports(name, parser, currentFieldName);

// Check to see if this field is a required field, if it is we can
// remove the entry as the requirement is satisfied
Iterator<String[]> iter = requiredFields.iterator();
while (iter.hasNext()) {
String[] requriedFields = iter.next();
for (String field : requriedFields) {
if (field.equals(currentFieldName)) {
iter.remove();
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this removing the value from the ObjectParser's instance, which is static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof, you're right. I was just looking at the class itself (which is not static) but everyone declares ObjectParser as static. I'll fiddle to get rid of the mutability

break;
}
}
}

parseSub(parser, fieldParser, currentFieldName, value, context);
}
fieldParser = null;
}
}
if (requiredFields.isEmpty() == false) {
StringBuilder message = new StringBuilder();
for (String[] fields : requiredFields) {
message.append("Required one of fields ").append(Arrays.toString(fields)).append(", but none were specified. ");
}
throw new IllegalArgumentException(message.toString());
}
return value;
}

Expand All @@ -258,6 +282,7 @@ public Value apply(XContentParser parser, Context context) {
public interface Parser<Value, Context> {
void parse(XContentParser parser, Value value, Context context) throws IOException;
}

public void declareField(Parser<Value, Context> p, ParseField parseField, ValueType type) {
if (parseField == null) {
throw new IllegalArgumentException("[parseField] is required");
Expand Down
201 changes: 201 additions & 0 deletions libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@
import java.util.concurrent.atomic.AtomicReference;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.nullValue;

public class ObjectParserTests extends ESTestCase {

Expand Down Expand Up @@ -753,4 +755,203 @@ public void testConsumeUnknownFields() throws IOException {
assertEquals(List.of(1, 2, 3, 4), o.fields.get("test_array"));
assertEquals(Map.of("field", "value", "field2", List.of("list1", "list2")), o.fields.get("test_nested"));
}

public void testRequiredFieldSet() throws IOException {
class TestStruct {
private Long a;
private Long b;

private void setA(long value) {
this.a = value;
}

private void setB(long value) {
this.b = value;
}
}

XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\"}");
ObjectParser<TestStruct, Void> objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});

TestStruct obj = objectParser.apply(parser, null);
assertThat(obj.a, equalTo(123L));
assertThat(obj.b, nullValue());

parser = createParser(JsonXContent.jsonXContent, "{\"b\": \"123\"}");
objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});

obj = objectParser.apply(parser, null);
assertThat(obj.a, nullValue());
assertThat(obj.b, equalTo(123L));

parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\", \"b\": \"456\"}");
objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});

obj = objectParser.apply(parser, null);
assertThat(obj.a, equalTo(123L));
assertThat(obj.b, equalTo(456L));
}

public void testMultipleRequiredFieldSet() throws IOException {
class TestStruct {
private Long a;
private Long b;
private Long c;
private Long d;

private void setA(long value) {
this.a = value;
}

private void setB(long value) {
this.b = value;
}

private void setC(long value) {
this.c = value;
}

private void setD(long value) {
this.d = value;
}
}

XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\", \"c\": \"456\"}");
ObjectParser<TestStruct, Void> objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareLong(TestStruct::setC, new ParseField("c"));
objectParser.declareLong(TestStruct::setD, new ParseField("d"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});
objectParser.declareRequiredFieldSet(new String[]{"c", "d"});

TestStruct obj = objectParser.apply(parser, null);
assertThat(obj.a, equalTo(123L));
assertThat(obj.b, nullValue());
assertThat(obj.c, equalTo(456L));
assertThat(obj.d, nullValue());

parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\", \"d\": \"456\"}");
objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareLong(TestStruct::setC, new ParseField("c"));
objectParser.declareLong(TestStruct::setD, new ParseField("d"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});
objectParser.declareRequiredFieldSet(new String[]{"c", "d"});

obj = objectParser.apply(parser, null);
assertThat(obj.a, equalTo(123L));
assertThat(obj.b, nullValue());
assertThat(obj.c, nullValue());
assertThat(obj.d, equalTo(456L));
}

public void testMissingRequiredFieldSet() throws IOException {
class TestStruct {
private long a;
private long b;

private void setA(long value) {
this.a = value;
}

private void setB(long value) {
this.b = value;
}
}

XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}");

ObjectParser<TestStruct, Void> objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null));
assertThat(e.getMessage(), containsString("Required one of fields [a, b], but none were specified. "));
}

public void testMultipleRequiredFieldSetTwoMissing() throws IOException {
class TestStruct {
private Long a;
private Long b;
private Long c;
private Long d;

private void setA(long value) {
this.a = value;
}

private void setB(long value) {
this.b = value;
}

private void setC(long value) {
this.c = value;
}

private void setD(long value) {
this.d = value;
}
}

XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}");
ObjectParser<TestStruct, Void> objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareLong(TestStruct::setC, new ParseField("c"));
objectParser.declareLong(TestStruct::setD, new ParseField("d"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});
objectParser.declareRequiredFieldSet(new String[]{"c", "d"});

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null));
assertThat(e.getMessage(), equalTo("Required one of fields [a, b], but none were specified. " +
"Required one of fields [c, d], but none were specified. "));
}

public void testMultipleRequiredFieldSetOneMissing() throws IOException {
class TestStruct {
private Long a;
private Long b;
private Long c;
private Long d;

private void setA(long value) {
this.a = value;
}

private void setB(long value) {
this.b = value;
}

private void setC(long value) {
this.c = value;
}

private void setD(long value) {
this.d = value;
}
}
XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"a\": \"123\"}");
ObjectParser<TestStruct, Void> objectParser = new ObjectParser<>("foo", true, TestStruct::new);
objectParser.declareLong(TestStruct::setA, new ParseField("a"));
objectParser.declareLong(TestStruct::setB, new ParseField("b"));
objectParser.declareLong(TestStruct::setC, new ParseField("c"));
objectParser.declareLong(TestStruct::setD, new ParseField("d"));
objectParser.declareRequiredFieldSet(new String[]{"a", "b"});
objectParser.declareRequiredFieldSet(new String[]{"c", "d"});

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> objectParser.apply(parser, null));
assertThat(e.getMessage(), equalTo("Required one of fields [c, d], but none were specified. "));
}
}