Allow ObjectParsers to specify required sets of fields #49661

polyfractal · 2019-11-27T18:42:44Z

ConstructingObjectParser can be used to specify required fields, but it is still difficult to configure "sets" of fields where only one of the set is required ((foo OR bar) AND (bizz OR buzz)). It requires hand-rolled logic in each ConstructingObjectParser, or special validation methods that are called during/after object construction.

This commit adds a new method on ObjectParser which allows the parsers to register required sets. E.g. ["foo", "bar"] can be registered, which means "foo", "bar" or both must be configured by the user otherwise an exception is thrown.

This pattern crops up in many places in our parsers, but as a demonstration this PR adds validation for aggregation "field" and "script" fields. One or both must be configured on all aggregations, and omitting both should result in an exception. This was previously handled far downstream resulting in an aggregation exception, when it should be a parse exception.

This might be a controversial change given it's partial overlap with ConstructingObjectParser. Open to changes/suggestions, as well as better naming :)

Closes #48824

ConstructingObjectParser can be used to specify required fields, but it is still difficult to configure "sets" of fields where only one of the set is required (requiring hand-rolled logic in each ConstructingObjectParser, or adding special validation methods to objects that are called after building the object). This commit adds a new method on ObjectParser which allows the parsers to register required sets. E.g. ["foo", "bar"] can be registered, which means "foo", "bar" or both must be configured by the user otherwise an exception is thrown. This pattern crops up in many places in our parsers, but as a demonstration this PR adds validation for aggregation "field" and "script" fields. One or both must be configured on all aggregations, omitting both should result in an exception. This was previously handled far downstream resulting in an aggregation exception, when it should be a parse exception.

elasticmachine · 2019-11-27T18:42:46Z

Pinging @elastic/es-core-infra (:Core/Infra/Core)

rjernst

Overall having better control over what combinations of fields are allowed or must be present, but I wonder if this is sufficient. The examples given in javadoc are pretty simplistic, and shown like a query, while we are talking about json objects. Would it be possible to give closer to a real world example?

rjernst · 2019-12-04T19:51:24Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

+                        String[] requriedFields = iter.next();
+                        for (String field : requriedFields) {
+                            if (field.equals(currentFieldName)) {
+                                iter.remove();


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

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

server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java

docs/reference/aggregations/bucket/range-aggregation.asciidoc

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java

polyfractal · 2019-12-04T20:36:21Z

Thanks!

The examples given in javadoc are pretty simplistic, and shown like a query, while we are talking about json objects. Would it be possible to give closer to a real world example?

The motivating example for me was from aggregations: script or field (or both) must be specified, but if both are omitted it should be a parse exception. That's also why the PR has agg-related test fixes and documentation tweaks, since the PR included this as a demonstration example.I'll adjust the javadocs to show some json examples instead of boolean logic.

Regarding the agg-specific parts of the PR, I can remove them if we want it to be just infrastructure, and followup with a second PR for the agg stuff.

…ield

polyfractal · 2019-12-13T18:03:18Z

I backed out all the agg-related changes (making field/script required, the test tweaks and the documentation change) so this PR is now just related to the object parser.

Fixed access to the static object by making a copy of the list before parsing. This didn't feel especially clever, so open to alternatives :)

Also beefed up the javadocs a bit with json examples

rjernst

A few more comments.

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java

libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java

polyfractal · 2019-12-16T17:35:47Z

Review comments addressed! The extra javadoc markup made the json snippets significantly less readable though so not sure if they are still helpful or not...

I opted to ignore <p> since none of the other javadocs in this class use it either. I also used <code> instead of {@code} to avoid escaping all the braces

EDIT Looks like javadoc is unhappy, fixing...

EDIT 2 Hmm, so it seems that precommit wants a "summary" field on the table

> Task :libs:elasticsearch-x-content:compileJava FAILED
/home/polyfractal/projects/es/elasticsearch/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java:258: error: no summary or caption for table
     * </table>

But the build fails when a "summary" field is included

> Task :libs:elasticsearch-x-content:javadoc
13:37:07 /dev/shm/elastic+elasticsearch+pull-request-1/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java:249: error: attribute not supported in HTML5: summary
13:37:07      * <table summary="failure cases">

rjernst

LGTM

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java

…ield

polyfractal · 2020-02-06T19:07:09Z

Had a chance to revisit this, think I resolved the javadoc compilation build issue. Unsure why the "summary" field broke everything, but replacing it with a <content> tag instead satisfies the build. (kudos to @mark-vieira for the tip).

This got stale, merged in master and fixed conflicts, will monitor to make sure there aren't any new test issues before merging.

polyfractal · 2020-02-10T21:34:30Z

@elasticmachine update branch

ConstructingObjectParser can be used to specify required fields, but it is still difficult to configure "sets" of fields where only one of the set is required (requiring hand-rolled logic in each ConstructingObjectParser, or adding special validation methods to objects that are called after building the object). This commit adds a new method on ObjectParser which allows the parsers to register required sets. E.g. ["foo", "bar"] can be registered, which means "foo", "bar" or both must be configured by the user otherwise an exception is thrown. This pattern crops up in many places in our parsers; a good example are the aggregation "field" and "script" fields. One or both must be configured on all aggregations, omitting both should result in an exception. This was previously handled far downstream resulting in an aggregation exception, when it should be a parse exception.

$polyfractal$

$@polyfractal$ polyfractal added the :Core/Infra/Core Core issues without another label label Nov 27, 2019

$@polyfractal$

Fix broken test, remove outdated docs

a5274d7

rjernst reviewed Dec 4, 2019

View reviewed changes

polyfractal added 3 commits December 13, 2019 12:33

$@polyfractal$

Remove agg-related changes, fix static access, better javadocs

fc55850

$@polyfractal$

Merge remote-tracking branch 'origin/master' into agg_parser_script_f…

b52c2df

…ield

$@polyfractal$

Checkstyle

e6da525

rjernst reviewed Dec 13, 2019

View reviewed changes

$@polyfractal$

Review cleanup

046fee1

rjernst approved these changes Dec 17, 2019

View reviewed changes

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java Outdated Show resolved Hide resolved

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java Outdated Show resolved Hide resolved

polyfractal added 3 commits February 6, 2020 11:23

$@polyfractal$

Merge remote-tracking branch 'origin/master' into agg_parser_script_f…

971f014

…ield

$@polyfractal$

Review comments, fix build via content tags

02d63eb

$@polyfractal$

Add back test lost in merge conflict

d176f2c

Merge branch 'master' into agg_parser_script_field

ef53515

$@polyfractal$ polyfractal added v7.7.0 v8.0.0 >enhancement labels Feb 11, 2020

$@polyfractal$ polyfractal merged commit 208cbc2 into elastic:master Feb 11, 2020

$@polyfractal$ polyfractal mentioned this pull request Feb 11, 2020

Aggs must specify a field or script (or both) #52226

Merged

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ObjectParsers to specify required sets of fields #49661

Allow ObjectParsers to specify required sets of fields #49661

$@polyfractal$ polyfractal commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

rjernst left a comment

rjernst Dec 4, 2019

$@polyfractal$ polyfractal Dec 4, 2019

polyfractal commented Dec 4, 2019

polyfractal commented Dec 13, 2019

rjernst left a comment

polyfractal commented Dec 16, 2019 •

edited

Loading

rjernst left a comment

polyfractal commented Feb 6, 2020

polyfractal commented Feb 10, 2020

Allow ObjectParsers to specify required sets of fields #49661

Allow ObjectParsers to specify required sets of fields #49661

Conversation

polyfractal commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

rjernst left a comment

Choose a reason for hiding this comment

rjernst Dec 4, 2019

Choose a reason for hiding this comment

polyfractal Dec 4, 2019

Choose a reason for hiding this comment

polyfractal commented Dec 4, 2019

polyfractal commented Dec 13, 2019

rjernst left a comment

Choose a reason for hiding this comment

polyfractal commented Dec 16, 2019 • edited Loading

rjernst left a comment

Choose a reason for hiding this comment

polyfractal commented Feb 6, 2020

polyfractal commented Feb 10, 2020

$@polyfractal$ polyfractal commented Nov 27, 2019

$@polyfractal$ polyfractal Dec 4, 2019

polyfractal commented Dec 16, 2019 •

edited

Loading