Skip to content

Conversation

@jbaiera
Copy link
Member

@jbaiera jbaiera commented May 23, 2019

This PR ensures that fields retained in an enrich index from a source are not contained under a nested field. It additionally makes sure that key fields exist, and that value fields are checked if they are present. The policy runner test has also been expanded with some faulty mapping test cases.

@jbaiera jbaiera added >enhancement :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels May 23, 2019
@jbaiera jbaiera requested review from hub-cap and martijnvg May 23, 2019 16:04
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

This looks good. I left a few comments.


private ActionListener<PolicyExecutionResult> createTestListener(final CountDownLatch latch,
final Consumer<Exception> exceptionConsumer) {
return new ActionListener<>() {
Copy link
Member

Choose a reason for hiding this comment

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

maybe use LatchedActionListener and ActionListener#wrap(...) instead?

// Ensure that the current field is of object type only (not a nested type or a non compound field)
Object type = currentField.get("type");
if (type != null) {
throw new ElasticsearchException(
Copy link
Member

Choose a reason for hiding this comment

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

an object field can be specifically configured with: type: object, so we need to check for this here

.endObject()
.startObject("field3")
.field("type", "keyword")
.endObject()
Copy link
Member

Choose a reason for hiding this comment

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

maybe also add a test that has a mapping with two nested object fields (level1.level2.*)?

assertThat(shard.getSegments().size(), is(equalTo(1)));
Segment segment = shard.getSegments().iterator().next();
assertThat(segment.getNumDocs(), is(equalTo(3)));
assertThat(segment.getNumDocs(), is(equalTo(expectedDocs)));
Copy link
Member

Choose a reason for hiding this comment

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

maybe also add a thing test that configures object fields with a type (type: object)?

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

minor nits

}

private void validateField(Map<?, ?> properties, String fieldName, boolean fieldRequired) {
assert fieldName != null && !fieldName.isEmpty() : "Field name cannot be null or empty";
Copy link
Contributor

Choose a reason for hiding this comment

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

assert Strings.isEmpty(fieldName) == false: "..";

for (String fieldPart : fieldParts) {
// Ensure that the current field is of object type only (not a nested type or a non compound field)
Object type = currentField.get("type");
if (type != null && !"object".equals(type)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

"object".equals(type) == false


// Validate Mapping
Map<String, Object> mapping = enrichIndex.getMappings().get(createdEnrichIndex).get("_doc").sourceAsMap();
logger.info(mapping);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this meant to be here?

@jbaiera jbaiera merged commit 8a1173d into elastic:enrich Jun 3, 2019
@jbaiera jbaiera deleted the enrich-validate-nested-mappings branch June 3, 2019 18:58
jbaiera added a commit that referenced this pull request Jun 3, 2019
Ensures that fields retained in an enrich index from a source are not contained
under a nested field. It additionally makes sure that key fields exist, and that
value fields are checked if they are present. The policy runner test has also
been expanded with some faulty mapping test cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants