Add the cluster version to enrich policies

[jbaiera]

This is a reimplementation of #44595 which just adds the Elasticsearch version as a field on the EnrichPolicy object directly instead of creating a separate metadata object to hold it. This change has the advantage of not intrusively modifying the get policy response, and having a smaller ripple through the code base, while avoiding having a separate map for policy metadata keyed by policy name in the EnrichMetadata class.

[elasticmachine]

Pinging @elastic/es-core-features

[martijnvg]

Looks good. Left a few comments.

[martijnvg]

make final?

[martijnvg]

and then:

if (in.readBoolean()) {
   version = Version.readVersion(in);
} else {
   version = null;
}

?

[jbaiera]

Is the idea here then to just make the policy object fully immutable, and when storing it make a copy with the version set before saving it to the cluster state? I think I can manage that.

[martijnvg]

Yes, that works.
Or we can set version to Version.CURRENT if version is null in the 2nd constructor?

[jakelandis]

I think this is what Version.V_EMPTY is for.

+1 to remove boolean in favor of V_EMPTY if null.

[jbaiera]

I'm not against setting it to Version.CURRENT if it would be null otherwise. Does it make sense that a user could create a policy with version_created already set to a version? Do we care to limit that?

[jakelandis]

+1 to always set to Version.CURRENT if null.

Does it make sense that a user could create a policy with version_created already set to a version?

nope

Do we care to limit that?

Doesn't your validation already do that ?

[jbaiera]

The validation will fail if we set the version to Version.CURRENT in the event that it's null - If the user ends up not providing the version in the request, when the object is deserialized from XContent, the version will be null, which will mean it would get set to CURRENT, thus removing any way to tell if a user actually set the version or not.

[martijnvg]

What if do this:

Add an argument to fromXContent:

public static EnrichPolicy fromXContent(XContentParser parser, boolean fromAPI) throws IOException {
    return PARSER.parse(parser, fromAPI);
}

Change parsers' context type from void to Boolean:

private static final ConstructingObjectParser<EnrichPolicy, Boolean> PARSER = new ConstructingObjectParser<>("policy",

Change parser:

parser.declareField(ConstructingObjectParser.optionalConstructorArg(),
            (p, fromApi) -> {
            if (fromApi) {
                // fail    
            }
            return Version.fromString(p.text());
            },
            VERSION_CREATED,
            ObjectParser.ValueType.STRING
        );

This way we fail if a version is provided via api and otherwise we're ok with a version being provided?

[martijnvg]

btw I'm happy with the PR as is. I don't feel strong about this (^) any more.

[martijnvg]

Perhaps we should only serialize versionCreated if params.include_version is set to true? It should default to true, and in get and list policy api we would then set it to false. This way when the user adds a policy and then returns it via get policy api then he sees the exact content that was submitted.

[jbaiera]

I've made this change, but it looks like Strings.toString(this) does not use include_version and can lead to some strange assert messages when checking equals on two policy objects where one has a version set and the other does not:

    java.lang.AssertionError: 
    Expected: <{"type":"exact_match","indices":["index"],"enrich_key":"key","enrich_values":["val1"]}>
         but: was <{"type":"exact_match","indices":["index"],"enrich_key":"key","enrich_values":["val1"]}>

I don't think this is necessarily bad, but it can be confusing.

[martijnvg]

Agreed this can be confusing, but I think it is ok. This things are easy to spot in debug mode.

[jbaiera]

@elasticmachine update branch

[jbaiera]

@elasticmachine update branch

[jbaiera]

@elasticmachine run elasticsearch-ci/1

[martijnvg]

LGTM

[hub-cap]

Im not sure i like currentVersion. We could rename this to elasticsearchVersion to make it easier to read. I would hate to see us eventually having versioned policies, like we briefly discussed in regard to immutable policies. Then we would possibly end up with a createdVersion and a version or something like that and the former does not really tell us that its actually the es version.

[jbaiera]

@elasticmachine update branch