Allow for field declaration for future compatible versions #69774

pgomulka · 2021-03-02T09:29:46Z

When renaming/removing a field, a new field might be declared which
should be parseable starting with the current version.
This commit changes the way ParseField is declared for compatible
Version. Instead of concrete version a boolean function has to be used
to indicate for what version a field is parseable. The onOrAfter and
equalTo functions are declared on RestApiVersion to allow for
this.
#51816

When renaming/removing a field, a new field might be declared which should be parseable starting with the current version. This commit changes the way ParseField is declared for compatible Version. Instead of concrete version a boolean function has to be used to indicate for what version a field is parseable. The onOrAfter and equalTo functions are declared on RestApiCompatibleVersion to allow for this.

elasticmachine · 2021-03-02T09:29:49Z

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

pgomulka · 2021-03-02T09:31:42Z

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

        }
    }
+
+//    protected boolean enableWarningsCheck() {


how do we test for future version? A class instead of enum would allow for Spying, but would not look too great..

I don't think you need to test the future version. Since it does not exist (yet) it can not be set. Once current == 9 it will get tested via the current(). However, i realize that is not the spirit of what you are trying to test here ... you are trying to test the "after" part of onOrAfter right ?

I think to test the spirit of what you want here is to declare a Parse field with .onOrAfter(RestApiCompatibleVersion.minimumSupported()))); and a createParserWithCompatibilityFor(..., currentVersion()) and ensure it still parses correctly.

good idea, I created a test that uses minimumSupported ('pretending this being the version when a new field is introduced) and then I test that is being parsed for current version (which is the "future" version)

…re_version

pgomulka · 2021-03-03T14:15:24Z

@elasticmachine run elasticsearch-ci/rest-compat

…re_version

jakelandis

Looking good. One comment around the use of collection or not and possible way to test a "future" version.

Also, sorry you got the short straw w.r.t. to the massive rename.

jakelandis · 2021-03-03T19:35:26Z

libs/core/src/main/java/org/elasticsearch/common/compatibility/RestApiCompatibleVersion.java

        return fromMajorVersion(major - 1);
    }

+    public boolean matches(Collection<Function<RestApiCompatibleVersion, Boolean>> restApiCompatibleVersionFunctions){


Is the Collection here necessary ? with only two versions and only two functions, it seems redundant to send both equalTo and onOrAfter (note this applies through all the interfaces ... i think singular is sufficient. )

good point, onOrAfter can replace the only usage of two versions at once (the regular field without version specified which is matching V7 and V8)

jakelandis · 2021-03-03T20:26:04Z

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

        }
    }
+
+//    protected boolean enableWarningsCheck() {


I don't think you need to test the future version. Since it does not exist (yet) it can not be set. Once current == 9 it will get tested via the current(). However, i realize that is not the spirit of what you are trying to test here ... you are trying to test the "after" part of onOrAfter right ?

I think to test the spirit of what you want here is to declare a Parse field with .onOrAfter(RestApiCompatibleVersion.minimumSupported()))); and a createParserWithCompatibilityFor(..., currentVersion()) and ensure it still parses correctly.

…re_version

pgomulka · 2021-03-04T09:29:28Z

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

    private final String name;
    private final String[] deprecatedNames;
-    private final Set<RestApiVersion> restApiVersions = new HashSet<>(2);
+    private final Function<RestApiVersion, Boolean> restApiVersionMatcher;


any suggestions for a better name? restApiVersionMatcher? restApiVersionFunction? restApiVersionRange ?

how about forRestApiVersion see comment below.

pgomulka · 2021-03-04T15:01:32Z

@elasticmachine update branch

jakelandis

a couple naming suggestions

jakelandis · 2021-03-04T18:45:57Z

libs/core/src/main/java/org/elasticsearch/common/RestApiVersion.java

        return fromMajorVersion(major - 1);
    }

+    public boolean matches(Function<RestApiVersion, Boolean> restApiVersionFunctions){


nit: s/Functions/Function

jakelandis · 2021-03-04T18:56:19Z

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

     */
-    public ParseField withRestApiVersions(RestApiVersion... restApiVersions) {
-        return new ParseField(this.name, Arrays.asList(restApiVersions), this.deprecatedNames);
+    public ParseField withRestApiVersionMacher(Function<RestApiVersion, Boolean> restApiVersionMatcher) {


how about forRestApiVersion( Function<RestApiVersion, Boolean> forRestApiVersion)

so

PARSER.declareInt(StructWithCompatibleFields::setIntField, new ParseField("new_name").forRestApiVersion(RestApiVersion.onOrAfter(RestApiVersion.V_8)));

that looks really good - will rename in all the places accordingly

jakelandis · 2021-03-04T18:56:35Z

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

    private final String name;
    private final String[] deprecatedNames;
-    private final Set<RestApiVersion> restApiVersions = new HashSet<>(2);
+    private final Function<RestApiVersion, Boolean> restApiVersionMatcher;


how about forRestApiVersion see comment below.

…sticsearch into compat/field_future_version

jakelandis · 2021-03-04T21:08:48Z

server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java

            .field("format", "epoch_second")));

-        ParsedDocument doc = mapper.parse(source(b -> b.field("field", 1457654400)));
+        ParsedDocument doc = mapper.parse(source(b -> b.field("field", 1614774616.0585806)));


is this intentional ?

I assume not and changed it back for you since it was failing a test and is unrelated to this change: 60ea56f (#69774)

Omg yes. It was totally unrelated and slipped by accident. Thanks for a fix

jakelandis

LGTM.

However, there may be a rouge change to a test. (see comnent)

server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java

…apperTests.java

jakelandis · 2021-03-04T22:42:58Z

@elasticmachine run elasticsearch-ci/2

pgomulka added :Core/Infra/REST API REST infrastructure and utilities v8.0.0 labels Mar 2, 2021

pgomulka self-assigned this Mar 2, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Mar 2, 2021

pgomulka commented Mar 2, 2021

View reviewed changes

pgomulka added 3 commits March 2, 2021 10:37

remove warning from comment

4f46099

Merge remote-tracking branch 'upstream/master' into compat/field_futu…

9fdfeb5

…re_version

fix suggestion test

133ab7f

Merge remote-tracking branch 'upstream/master' into compat/field_futu…

e2a598e

…re_version

pgomulka requested review from jakelandis and joegallo March 3, 2021 15:53

jakelandis reviewed Mar 3, 2021

View reviewed changes

pgomulka added 3 commits March 4, 2021 08:43

Merge remote-tracking branch 'upstream/master' into compat/field_futu…

b7aae66

…re_version

renames

24d1714

unused import

3ab5594

pgomulka commented Mar 4, 2021

View reviewed changes

test for future version

6a4fc6e

elasticmachine added 2 commits March 4, 2021 10:02

Merge branch 'master' into compat/field_future_version

e536127

Merge branch 'master' into compat/field_future_version

d305a78

jakelandis reviewed Mar 4, 2021

View reviewed changes

pgomulka added 2 commits March 4, 2021 20:31

rename

273a1ac

Merge branch 'compat/field_future_version' of github.com:pgomulka/ela…

180ab58

…sticsearch into compat/field_future_version

pgomulka requested a review from jakelandis March 4, 2021 19:48

jakelandis reviewed Mar 4, 2021

View reviewed changes

jakelandis approved these changes Mar 4, 2021

View reviewed changes

jakelandis reviewed Mar 4, 2021

View reviewed changes

server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java Outdated Show resolved Hide resolved

Update server/src/test/java/org/elasticsearch/index/mapper/DateFieldM…

60ea56f

…apperTests.java

pgomulka merged commit 8d09fbf into elastic:master Mar 5, 2021

pugnascotia added the >enhancement label Mar 31, 2021

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

ChrisHegarty unassigned pgomulka Oct 29, 2024

Allow for field declaration for future compatible versions #69774

Allow for field declaration for future compatible versions #69774

Uh oh!

Conversation

pgomulka commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 2, 2021

Uh oh!

pgomulka Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Mar 3, 2021

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakelandis commented Mar 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pgomulka commented Mar 2, 2021 •

edited

Loading

pgomulka Mar 2, 2021 •

edited

Loading

pgomulka commented Mar 4, 2021 •

edited

Loading