Skip to content

Change how type is stored in an enrich policy. #45789

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

Merged
merged 4 commits into from
Aug 23, 2019
Merged

Change how type is stored in an enrich policy. #45789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86c6597
Change how type is stored in an enrich policy.
martijnvg Aug 21, 2019
9df41db
Merge remote-tracking branch 'es/enrich' into enrich_change_policy_st…
martijnvg Aug 22, 2019
b9505ca
Merge remote-tracking branch 'es/enrich' into enrich_change_policy_st…
martijnvg Aug 23, 2019
8c8a9ee
iter
martijnvg Aug 23, 2019
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
18 changes: 11 additions & 7 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/enrich/PutPolicyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@

public class PutPolicyRequest implements Validatable, ToXContentObject {

static final ParseField TYPE_FIELD = new ParseField("type");
static final ParseField QUERY_FIELD = new ParseField("query");
static final ParseField INDICES_FIELD = new ParseField("indices");
static final ParseField MATCH_FIELD_FIELD = new ParseField("match_field");
Expand Down Expand Up @@ -108,13 +107,18 @@ public List<String> getEnrichFields() {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.field(TYPE_FIELD.getPreferredName(), type);
builder.field(INDICES_FIELD.getPreferredName(), indices);
if (query != null) {
builder.field(QUERY_FIELD.getPreferredName(), asMap(query, builder.contentType()));
{
builder.startObject(type);
{
builder.field(INDICES_FIELD.getPreferredName(), indices);
if (query != null) {
builder.field(QUERY_FIELD.getPreferredName(), asMap(query, builder.contentType()));
}
builder.field(MATCH_FIELD_FIELD.getPreferredName(), matchField);
builder.field(ENRICH_FIELDS_FIELD.getPreferredName(), enrichFields);
}
builder.endObject();
}
builder.field(MATCH_FIELD_FIELD.getPreferredName(), matchField);
builder.field(ENRICH_FIELDS_FIELD.getPreferredName(), enrichFields);
builder.endObject();
return builder;
}
Expand Down
11 changes: 7 additions & 4 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/EnrichIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.elasticsearch.client.enrich.PutPolicyRequest;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.common.xcontent.support.XContentMapValues;

import java.io.IOException;
import java.util.List;
Expand All @@ -48,10 +49,12 @@ public void testCRUD() throws Exception {
@SuppressWarnings("unchecked")
List<Map<String, Object>> responsePolicies = (List<Map<String, Object>>) responseBody.get("policies");
assertThat(responsePolicies.size(), equalTo(1));
assertThat(responsePolicies.get(0).get("type"), equalTo(putPolicyRequest.getType()));
assertThat(responsePolicies.get(0).get("indices"), equalTo(putPolicyRequest.getIndices()));
assertThat(responsePolicies.get(0).get("match_field"), equalTo(putPolicyRequest.getMatchField()));
assertThat(responsePolicies.get(0).get("enrich_fields"), equalTo(putPolicyRequest.getEnrichFields()));
assertThat(XContentMapValues.extractValue("exact_match.indices", responsePolicies.get(0)),
equalTo(putPolicyRequest.getIndices()));
assertThat(XContentMapValues.extractValue("exact_match.match_field", responsePolicies.get(0)),
equalTo(putPolicyRequest.getMatchField()));
assertThat(XContentMapValues.extractValue("exact_match.enrich_fields", responsePolicies.get(0)),
equalTo(putPolicyRequest.getEnrichFields()));
}

private static Map<String, Object> toMap(Response response) throws IOException {
Expand Down
68 changes: 36 additions & 32 deletions docs/reference/ingest/ingest-node.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,10 +858,11 @@ Create an enrich policy:
--------------------------------------------------
PUT /_enrich/policy/users-policy
{
"type": "exact_match",
"indices": "users",
"match_field": "email",
"enrich_fields": ["first_name", "last_name", "address", "city", "zip", "state"]
"exact_match": {
"indices": "users",
"match_field": "email",
"enrich_fields": ["first_name", "last_name", "address", "city", "zip", "state"]
}
}
--------------------------------------------------
// CONSOLE
Expand Down Expand Up @@ -990,10 +991,11 @@ Request:
--------------------------------------------------
PUT /_enrich/policy/my-policy
{
"type": "exact_match",
"indices": "users",
"match_field": "email",
"enrich_fields": ["first_name", "last_name", "address", "city", "zip", "state"]
"exact_match": {
"indices": "users",
"match_field": "email",
"enrich_fields": ["first_name", "last_name", "address", "city", "zip", "state"]
}
Copy link
Contributor

@jakelandis jakelandis Aug 22, 2019

Choose a reason for hiding this comment

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

Do we have an example to includes the query ? (its fine if it is not part of the PR, but probably want it in there somewhere)

Copy link
Member Author

@martijnvg martijnvg Aug 23, 2019

Choose a reason for hiding this comment

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

No, there is not. It makes sense to emphasise that in a special section with the fact that reference data can be read from multiple indices.

}
--------------------------------------------------
// CONSOLE
Expand Down Expand Up @@ -1029,18 +1031,19 @@ Response:
{
"policies": [
{
"name" : "my-policy",
"type" : "exact_match",
"indices" : ["users"],
"match_field" : "email",
"enrich_fields" : [
"first_name",
"last_name",
"address",
"city",
"zip",
"state"
]
"exact_match": {
"name" : "my-policy",
"indices" : ["users"],
"match_field" : "email",
"enrich_fields" : [
"first_name",
"last_name",
"address",
"city",
"zip",
"state"
]
}
}
]
}
Expand All @@ -1065,18 +1068,19 @@ Response:
{
"policies": [
{
"name" : "my-policy",
"type" : "exact_match",
"indices" : ["users"],
"match_field" : "email",
"enrich_fields" : [
"first_name",
"last_name",
"address",
"city",
"zip",
"state"
]
"exact_match": {
"name" : "my-policy",
"indices" : ["users"],
"match_field" : "email",
"enrich_fields" : [
"first_name",
"last_name",
"address",
"city",
"zip",
"state"
]
}
}
]
}
Expand Down
93 changes: 72 additions & 21 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/enrich/EnrichPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.core.enrich;

import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.StreamInput;
Expand All @@ -17,6 +18,7 @@
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentParser.Token;
import org.elasticsearch.common.xcontent.XContentType;

import java.io.IOException;
Expand All @@ -34,20 +36,21 @@ public final class EnrichPolicy implements Writeable, ToXContentFragment {
public static final String EXACT_MATCH_TYPE = "exact_match";
public static final String[] SUPPORTED_POLICY_TYPES = new String[]{EXACT_MATCH_TYPE};

private static final ParseField TYPE = new ParseField("type");
private static final ParseField QUERY = new ParseField("query");
private static final ParseField INDICES = new ParseField("indices");
private static final ParseField MATCH_FIELD = new ParseField("match_field");
private static final ParseField ENRICH_FIELDS = new ParseField("enrich_fields");

@SuppressWarnings("unchecked")
private static final ConstructingObjectParser<EnrichPolicy, Void> PARSER = new ConstructingObjectParser<>("policy",
args -> new EnrichPolicy(
(String) args[0],
(QuerySource) args[1],
(List<String>) args[2],
(String) args[3],
(List<String>) args[4]
private static final ConstructingObjectParser<EnrichPolicy, String> PARSER = new ConstructingObjectParser<>(
"policy",
false,
(args, policyType) -> new EnrichPolicy(
policyType,
(QuerySource) args[0],
(List<String>) args[1],
(String) args[2],
(List<String>) args[3]
)
);

Expand All @@ -56,7 +59,6 @@ public final class EnrichPolicy implements Writeable, ToXContentFragment {
}

private static void declareParserOptions(ConstructingObjectParser<?, ?> parser) {
parser.declareString(ConstructingObjectParser.constructorArg(), TYPE);
parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> {
XContentBuilder contentBuilder = XContentBuilder.builder(p.contentType().xContent());
contentBuilder.generator().copyCurrentStructure(p);
Expand All @@ -68,7 +70,24 @@ private static void declareParserOptions(ConstructingObjectParser<?, ?> parser)
}

public static EnrichPolicy fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
Token token = parser.currentToken();
Copy link
Member Author

@martijnvg martijnvg Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not super happy with this parsing code, but in ordere to use object parser, another class would need to be introduced for policy types and since there is currently only policy type, this seems overkill to me. I think the important thing here is that the policy format is future proof now.

if (token != Token.START_OBJECT) {
token = parser.nextToken();
}
if (token != Token.START_OBJECT) {
throw new ParsingException(parser.getTokenLocation(), "unexpected token");
}
token = parser.nextToken();
if (token != Token.FIELD_NAME) {
throw new ParsingException(parser.getTokenLocation(), "unexpected token");
}
String policyType = parser.currentName();
EnrichPolicy policy = PARSER.parse(parser, policyType);
token = parser.nextToken();
if (token != Token.END_OBJECT) {
throw new ParsingException(parser.getTokenLocation(), "unexpected token");
}
return policy;
}

private final String type;
Expand Down Expand Up @@ -134,14 +153,21 @@ public void writeTo(StreamOutput out) throws IOException {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field(TYPE.getPreferredName(), type);
builder.startObject(type);
{
toInnerXContent(builder);
}
builder.endObject();
return builder;
}

private void toInnerXContent(XContentBuilder builder) throws IOException {
if (query != null) {
builder.field(QUERY.getPreferredName(), query.getQueryAsMap());
}
builder.array(INDICES.getPreferredName(), indices.toArray(new String[0]));
builder.field(MATCH_FIELD.getPreferredName(), matchField);
builder.array(ENRICH_FIELDS.getPreferredName(), enrichFields.toArray(new String[0]));
return builder;
}

@Override
Expand Down Expand Up @@ -222,14 +248,16 @@ public static class NamedPolicy implements Writeable, ToXContent {

static final ParseField NAME = new ParseField("name");
@SuppressWarnings("unchecked")
static final ConstructingObjectParser<NamedPolicy, Void> PARSER = new ConstructingObjectParser<>("named_policy",
args -> new NamedPolicy(
static final ConstructingObjectParser<NamedPolicy, String> PARSER = new ConstructingObjectParser<>(
"named_policy",
false,
(args, policyType) -> new NamedPolicy(
(String) args[0],
new EnrichPolicy((String) args[1],
(QuerySource) args[2],
(List<String>) args[3],
(String) args[4],
(List<String>) args[5])
new EnrichPolicy(policyType,
(QuerySource) args[1],
(List<String>) args[2],
(String) args[3],
(List<String>) args[4])
)
);

Expand Down Expand Up @@ -268,16 +296,39 @@ public void writeTo(StreamOutput out) throws IOException {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.startObject(policy.type);
{
builder.field(NAME.getPreferredName(), name);
Copy link
Member Author

@martijnvg martijnvg Aug 21, 2019

Choose a reason for hiding this comment

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

should name appear outside the policy type object? I assumed not, because otherwise we have the name and policy type at the same level.

Copy link
Member

@jbaiera jbaiera Aug 21, 2019

Choose a reason for hiding this comment

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

I feel like name should stay within the policy's definition. The object above this will only have a few fields that are considered valid: the existing policy types. On top of that, I think the previously decided direction is to keep other metadata about the policy (like the es version it was created under) contained in the definition part.

policy.toXContent(builder, params);
policy.toInnerXContent(builder);
}
builder.endObject();
builder.endObject();
return builder;
}

public static NamedPolicy fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
Token token = parser.currentToken();
if (token != Token.START_OBJECT) {
token = parser.nextToken();
}
if (token != Token.START_OBJECT) {
throw new ParsingException(parser.getTokenLocation(), "unexpected token");
}
token = parser.nextToken();
if (token != Token.FIELD_NAME) {
throw new ParsingException(parser.getTokenLocation(), "unexpected token");
}
String policyType = parser.currentName();
token = parser.nextToken();
if (token != Token.START_OBJECT) {
throw new ParsingException(parser.getTokenLocation(), "unexpected token");
}
NamedPolicy policy = PARSER.parse(parser, policyType);
token = parser.nextToken();
if (token != Token.END_OBJECT) {
throw new ParsingException(parser.getTokenLocation(), "unexpected token");
}
return policy;
}

@Override
Expand Down
Loading