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.

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

Allow specifying an exclusive set of fields on ObjectParser #52893

Merged
merged 3 commits into from Mar 3, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -38,6 +38,7 @@ public abstract class AbstractObjectParser<Value, Context>
implements BiFunction<XContentParser, Context, Value>, ContextParser<Context, Value> {

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

/**
* Declare some field. Usually it is easier to use {@link #declareString(BiConsumer, ParseField)} or
Expand Down Expand Up @@ -294,6 +295,23 @@ public void declareRequiredFieldSet(String... requiredSet) {
this.requiredFieldSets.add(requiredSet);
}

/**
* Declares a set of fields of which at most one must appear for parsing to succeed
*
* E.g. <code>declareExclusiveFieldSet("foo", "bar");</code> means that only one of 'foo'
* or 'bar' must be present, and if both appear then an exception will be thrown.
*
* Multiple exclusive sets may be declared
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be worth mentioning that it doesn't make foo or bar required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

*
* @param exclusiveSet a set of field names, at most one of which must appear
*/
public void declareExclusiveFieldSet(String... exclusiveSet) {
if (exclusiveSet.length == 0) {
return;
}
this.exclusiveFieldSets.add(exclusiveSet);
}

private interface IOSupplier<T> {
T get() throws IOException;
}
Expand Down
Expand Up @@ -299,6 +299,16 @@ public String getName() {
return objectParser.getName();
}

@Override
public void declareRequiredFieldSet(String... requiredSet) {
objectParser.declareRequiredFieldSet(requiredSet);
}

@Override
public void declareExclusiveFieldSet(String... exclusiveSet) {
objectParser.declareExclusiveFieldSet(exclusiveSet);
}

private Consumer<Target> wrapOrderedModeCallBack(Consumer<Value> callback) {
return (target) -> {
if (target.targetObject != null) {
Expand Down
Expand Up @@ -274,6 +274,10 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
String currentFieldName = null;
XContentLocation currentPosition = null;
List<String[]> requiredFields = new ArrayList<>(this.requiredFieldSets);
List<List<String>> exclusiveFields = new ArrayList<>();
for (int i = 0; i < this.exclusiveFieldSets.size(); i++) {
exclusiveFields.add(new ArrayList<>());
}

while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
Expand Down Expand Up @@ -302,18 +306,40 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
}
}

// Check if this field is in an exclusive set, if it is then mark
// it as seen. If the set is already marked, then we have a duplicate
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't throw an exception though!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. It does, but later. Got it. Maybe update the comment. Not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote the comments a bit to make things clearer

// so throw an exception
for (int i = 0; i < this.exclusiveFieldSets.size(); i++) {
for (String field : this.exclusiveFieldSets.get(i)) {
if (field.equals(currentFieldName)) {
exclusiveFields.get(i).add(currentFieldName);
}
}
}

parseSub(parser, fieldParser, currentFieldName, value, context);
}
fieldParser = null;
}
}

StringBuilder message = new StringBuilder();
for (List<String> fieldset : exclusiveFields) {
if (fieldset.size() > 1) {
message.append("The following fields are not allowed together: ").append(fieldset.toString()).append(" ");
}
}
if (message.length() > 0) {
throw new IllegalArgumentException(message.toString());
}

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 Down
Expand Up @@ -528,4 +528,45 @@ public void keepNamedInOrder() {
namedSuppliedInOrder = true;
}
}

public void testRequiredAndExclusiveFields() throws IOException {

class TestStruct {
final String a;
final long b;
TestStruct(String a) {
this.a = a;
this.b = 0;
}
TestStruct(long b) {
this.a = null;
this.b = b;
}
}

XContentParser ok = createParser(JsonXContent.jsonXContent, "{ \"a\" : \"a\" }");
XContentParser toomany = createParser(JsonXContent.jsonXContent, "{ \"a\" : \"a\", \"b\" : 1 }");
XContentParser notenough = createParser(JsonXContent.jsonXContent, "{ }");

ConstructingObjectParser<TestStruct, Void> parser = new ConstructingObjectParser<>("teststruct", args -> {
if (args[0] != null) {
return new TestStruct((String) args[0]);
}
return new TestStruct((Long) args[1]);
});
parser.declareString(optionalConstructorArg(), new ParseField("a"));
parser.declareLong(optionalConstructorArg(), new ParseField("b"));
parser.declareExclusiveFieldSet("a", "b");
parser.declareRequiredFieldSet("a", "b");

TestStruct actual = parser.parse(ok, null);
assertThat(actual.a, equalTo("a"));
assertThat(actual.b, equalTo(0L));

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> parser.parse(toomany, null));
assertThat(e.getMessage(), containsString("allowed together: [a, b]"));

e = expectThrows(IllegalArgumentException.class, () -> parser.parse(notenough, null));
assertThat(e.getMessage(), containsString("Required one of fields [a, b], but none were specified."));
}
}
Expand Up @@ -850,29 +850,30 @@ private void setB(long value) {
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 static class TestStruct {
private Long a;
private Long b;
private Long c;
private Long d;

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

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

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

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

public void testMultipleRequiredFieldSet() throws IOException {

XContentParser parser = createParser(JsonXContent.jsonXContent, "{\"unrelated\": \"123\"}");
ObjectParser<TestStruct, Void> objectParser = new ObjectParser<>("foo", true, TestStruct::new);
Expand All @@ -888,6 +889,32 @@ private void setD(long value) {
"Required one of fields [c, d], but none were specified. "));
}

public void testExclusiveFieldSet() throws IOException {

XContentParser goodA = createParser(JsonXContent.jsonXContent, "{\"a\" : 1, \"c\" : 4}");
XContentParser bad = createParser(JsonXContent.jsonXContent, "{\"a\" : 1, \"b\" : 2}");
XContentParser badmulti = createParser(JsonXContent.jsonXContent, "{\"a\" : 1, \"b\" : 2, \"c\" : 3, \"d\" : 4 }");

ObjectParser<TestStruct, Void> parser = new ObjectParser<>("foo", TestStruct::new);
parser.declareLong(TestStruct::setA, new ParseField("a"));
parser.declareLong(TestStruct::setB, new ParseField("b"));
parser.declareLong(TestStruct::setC, new ParseField("c"));
parser.declareLong(TestStruct::setD, new ParseField("d"));
parser.declareExclusiveFieldSet("a", "b");
parser.declareExclusiveFieldSet("c", "d");

TestStruct actualA = parser.parse(goodA, null);
assertThat(actualA.a, equalTo(1L));
assertThat(actualA.c, equalTo(4L));

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> parser.parse(bad, null));
assertThat(e.getMessage(), containsString("The following fields are not allowed together: [a, b]"));

e = expectThrows(IllegalArgumentException.class, () -> parser.parse(badmulti, null));
assertThat(e.getMessage(),
containsString("allowed together: [a, b] The following fields are not allowed together: [c, d]"));
}

@Override
protected NamedXContentRegistry xContentRegistry() {
return new NamedXContentRegistry(Arrays.asList(
Expand Down