From ad6b374fd3293edb0992f4660d57efb233f1b7b1 Mon Sep 17 00:00:00 2001 From: Megha Goyal <goyamegh@amazon.com> Date: Thu, 30 Nov 2023 09:29:07 -0800 Subject: [PATCH 1/3] #709 Return empty response for empty mappings and no applied aliases (#724) * #709 Return empty response for empty mappings and no applied aliases Signed-off-by: Megha Goyal <goyamegh@amazon.com> * Adding integ tests for empty mappings/aliases use-cases Signed-off-by: Megha Goyal <goyamegh@amazon.com> * Fix unit tests for MappingsTraverser Signed-off-by: Megha Goyal <goyamegh@amazon.com> --------- Signed-off-by: Megha Goyal <goyamegh@amazon.com> (cherry picked from commit c0f7bd934b79f7c3a911229fa63e157116ec5ca9) --- .../mapper/MapperService.java | 19 ++++++--- .../mapper/MappingsTraverser.java | 17 ++++++-- .../mapper/MapperRestApiIT.java | 41 ++++++++++++++----- .../mapper/MappingsTraverserTests.java | 37 +++++++++++++---- 4 files changed, 85 insertions(+), 29 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java index 0f55daecd..6d42cb5db 100644 --- a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java +++ b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java @@ -290,13 +290,20 @@ public void onResponse(GetMappingsResponse getMappingsResponse) { } } } - } - if (appliedAliases.size() == 0) { - actionListener.onFailure(SecurityAnalyticsException.wrap( - new OpenSearchStatusException("No applied aliases not found", RestStatus.NOT_FOUND)) - ); - return; + // Traverse mappings and do copy with excluded type=alias properties + MappingsTraverser mappingsTraverser = new MappingsTraverser(mappingMetadata); + // Resulting mapping after filtering + Map<String, Object> filteredMapping = mappingsTraverser.traverseAndCopyWithFilter(appliedAliases); + + + // Construct filtered mappings and return them as result + Map<String, MappingMetadata> outIndexMappings = new HashMap<>(); + Map<String, Object> root = Map.of(org.opensearch.index.mapper.MapperService.SINGLE_MAPPING_NAME, filteredMapping); + MappingMetadata outMappingMetadata = new MappingMetadata(org.opensearch.index.mapper.MapperService.SINGLE_MAPPING_NAME, root); + outIndexMappings.put(indexName, outMappingMetadata); + + actionListener.onResponse(new GetIndexMappingsResponse(outIndexMappings)); } // Traverse mappings and do copy with excluded type=alias properties diff --git a/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java b/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java index 16335894b..e6f82eb47 100644 --- a/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java +++ b/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java @@ -5,23 +5,26 @@ package org.opensearch.securityanalytics.mapper; -import java.util.Collections; import java.util.LinkedHashMap; import java.util.ListIterator; import org.apache.commons.lang3.tuple.Pair; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.opensearch.cluster.metadata.MappingMetadata; import org.opensearch.common.xcontent.DeprecationHandler; import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; -import org.opensearch.index.mapper.MapperService; -import org.opensearch.securityanalytics.rules.condition.ConditionListener; +import org.opensearch.core.xcontent.DeprecationHandler; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.XContentParser; import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.HashSet; import java.util.HashMap; @@ -39,6 +42,8 @@ */ public class MappingsTraverser { + private static final Logger log = LogManager.getLogger(MappingsTraverser.class); + /** * Traverser listener used to process leaves */ @@ -157,7 +162,10 @@ public void traverse() { try { Map<String, Object> rootProperties = (Map<String, Object>) this.mappingsMap.get(PROPERTIES); - rootProperties.forEach((k, v) -> nodeStack.push(new Node(Map.of(k, v), null, rootProperties, "", ""))); + + if (Objects.nonNull(rootProperties)) { + rootProperties.forEach((k, v) -> nodeStack.push(new Node(Map.of(k, v), null, rootProperties, "", ""))); + } while (nodeStack.size() > 0) { Node node = nodeStack.pop(); @@ -193,6 +201,7 @@ public void traverse() { // This is coming from listeners. throw e; } catch (Exception e) { + log.error("Error traversing mappings tree", e); notifyError("Error traversing mappings tree"); } } diff --git a/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java b/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java index 9e7337a92..992166bb2 100644 --- a/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java +++ b/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java @@ -60,6 +60,27 @@ public void testGetMappingSuccess() throws IOException { assertTrue(respMap.containsKey(testIndexPattern)); } + // Tests the case when the mappings map is empty + public void testGetMappings_emptyIndex_Success() throws IOException { + String testIndexName1 = "my_index_1"; + String testIndexName2 = "my_index_2"; + String testIndexPattern = "my_index*"; + + createSampleIndex(testIndexName1); + createSampleIndex(testIndexName2); + + Request request = new Request("GET", MAPPER_BASE_URI + "?index_name=" + testIndexPattern); + Response response = client().performRequest(request); + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + Map<String, Object> respMap = (Map<String, Object>) responseAsMap(response); + Map<String, Object> props = (Map<String, Object>)((Map<String, Object>) respMap.get(testIndexPattern)).get("mappings"); + + // Assert that indexName returned is one passed by user + assertTrue(respMap.containsKey(testIndexPattern)); + //Assert that mappings map is also present in the output + assertTrue(props.containsKey("properties")); + } + public void testGetMappingSuccess_1() throws IOException { String testIndexName1 = "my_index_1"; String testIndexPattern = "my_index*"; @@ -227,13 +248,14 @@ public void testUpdateAndGetMappingSuccess() throws IOException { assertEquals(1L, searchResponse.getHits().getTotalHits().value); } + // Tests the case when alias mappings are not present on the index public void testUpdateAndGetMapping_notFound_Success() throws IOException { String testIndexName = "my_index"; createSampleIndex(testIndexName); - // Execute UpdateMappingsAction to add alias mapping for index + // Execute UpdateMappingsAction to add other settings except alias mapping for index Request updateRequest = new Request("PUT", SecurityAnalyticsPlugin.MAPPER_BASE_URI); // both req params and req body are supported updateRequest.setJsonEntity( @@ -241,21 +263,20 @@ public void testUpdateAndGetMapping_notFound_Success() throws IOException { " \"field\":\"netflow.source_transport_port\","+ " \"alias\":\"\\u0000\" }" ); - // request.addParameter("indexName", testIndexName); - // request.addParameter("ruleTopic", "netflow"); + Response response = client().performRequest(updateRequest); assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); // Execute GetIndexMappingsAction and verify mappings Request getRequest = new Request("GET", SecurityAnalyticsPlugin.MAPPER_BASE_URI); getRequest.addParameter("index_name", testIndexName); - try { - client().performRequest(getRequest); - fail(); - } catch (ResponseException e) { - assertEquals(HttpStatus.SC_NOT_FOUND, e.getResponse().getStatusLine().getStatusCode()); - assertTrue(e.getMessage().contains("No applied aliases not found")); - } + response = client().performRequest(getRequest); + XContentParser parser = createParser(JsonXContent.jsonXContent, new String(response.getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8)); + assertTrue( + (((Map)((Map)parser.map() + .get(testIndexName)) + .get("mappings")) + .containsKey("properties"))); } public void testExistingMappingsAreUntouched() throws IOException { diff --git a/src/test/java/org/opensearch/securityanalytics/mapper/MappingsTraverserTests.java b/src/test/java/org/opensearch/securityanalytics/mapper/MappingsTraverserTests.java index 1c71df93c..c5d7d077c 100644 --- a/src/test/java/org/opensearch/securityanalytics/mapper/MappingsTraverserTests.java +++ b/src/test/java/org/opensearch/securityanalytics/mapper/MappingsTraverserTests.java @@ -19,10 +19,6 @@ public class MappingsTraverserTests extends OpenSearchTestCase { - - - - public void testTraverseValidMappings() { // 1. Parse mappings from MappingMetadata ImmutableOpenMap.Builder<String, MappingMetadata> mappings = ImmutableOpenMap.builder(); @@ -136,30 +132,53 @@ public void onError(String error) { public void testTraverseInvalidMappings() { // 1. Parse mappings from MappingMetadata - ImmutableOpenMap.Builder<String, MappingMetadata> mappings = ImmutableOpenMap.builder(); Map<String, Object> m = new HashMap<>(); m.put("netflow.event_data.SourceAddress", Map.of("type", "ip")); m.put("netflow.event_data.SourcePort", Map.of("type", "integer")); Map<String, Object> properties = Map.of("incorrect_properties", m); Map<String, Object> root = Map.of(MapperService.SINGLE_MAPPING_NAME, properties); MappingMetadata mappingMetadata = new MappingMetadata(MapperService.SINGLE_MAPPING_NAME, root); - mappings.put("my_index", mappingMetadata); MappingsTraverser mappingsTraverser = new MappingsTraverser(mappingMetadata); - final boolean[] errorHappend = new boolean[1]; + List<String> paths = new ArrayList<>(); mappingsTraverser.addListener(new MappingsTraverser.MappingsTraverserListener() { @Override public void onLeafVisited(MappingsTraverser.Node node) { assertNotNull(node); + paths.add(node.currentPath); + } + + @Override + public void onError(String error) { + fail("Failed traversing invalid mappings"); + } + }); + mappingsTraverser.traverse(); + assertEquals(0, paths.size()); + } + + public void testTraverseEmptyMappings() { + Map<String, Object> root = Map.of(MapperService.SINGLE_MAPPING_NAME, new HashMap<>()); + MappingMetadata mappingMetadata = new MappingMetadata(MapperService.SINGLE_MAPPING_NAME, root); + + MappingsTraverser mappingsTraverser = new MappingsTraverser(mappingMetadata); + final boolean[] errorHappend = new boolean[1]; + List<String> paths = new ArrayList<>(); + + mappingsTraverser.addListener(new MappingsTraverser.MappingsTraverserListener() { + @Override + public void onLeafVisited(MappingsTraverser.Node node) { + assertNotNull(node); + paths.add(node.currentPath); } @Override public void onError(String error) { - errorHappend[0] = true; + fail("Failed traversing empty mappings"); } }); mappingsTraverser.traverse(); - assertTrue(errorHappend[0]); + assertEquals(0, paths.size()); } public void testTraverseValidMappingsWithTypeFilter() { From db2ee003ccb832e9d91eba5c8596a805b6fae9b9 Mon Sep 17 00:00:00 2001 From: Megha Goyal <56077967+goyamegh@users.noreply.github.com> Date: Wed, 29 Nov 2023 17:27:23 -0800 Subject: [PATCH 2/3] #709 Fix merge conflicts while backporting Signed-off-by: Megha Goyal <goyamegh@amazon.com> --- .../securityanalytics/mapper/MapperService.java | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java index 6d42cb5db..ff7479372 100644 --- a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java +++ b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java @@ -290,20 +290,6 @@ public void onResponse(GetMappingsResponse getMappingsResponse) { } } } - - // Traverse mappings and do copy with excluded type=alias properties - MappingsTraverser mappingsTraverser = new MappingsTraverser(mappingMetadata); - // Resulting mapping after filtering - Map<String, Object> filteredMapping = mappingsTraverser.traverseAndCopyWithFilter(appliedAliases); - - - // Construct filtered mappings and return them as result - Map<String, MappingMetadata> outIndexMappings = new HashMap<>(); - Map<String, Object> root = Map.of(org.opensearch.index.mapper.MapperService.SINGLE_MAPPING_NAME, filteredMapping); - MappingMetadata outMappingMetadata = new MappingMetadata(org.opensearch.index.mapper.MapperService.SINGLE_MAPPING_NAME, root); - outIndexMappings.put(indexName, outMappingMetadata); - - actionListener.onResponse(new GetIndexMappingsResponse(outIndexMappings)); } // Traverse mappings and do copy with excluded type=alias properties From 4201f35c32d3004d097a254ad8c5e74dd810fcde Mon Sep 17 00:00:00 2001 From: Megha Goyal <56077967+goyamegh@users.noreply.github.com> Date: Wed, 29 Nov 2023 17:27:23 -0800 Subject: [PATCH 3/3] #709 Fix merge conflicts while backporting Signed-off-by: Megha Goyal <goyamegh@amazon.com> --- .../opensearch/securityanalytics/mapper/MappingsTraverser.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java b/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java index e6f82eb47..010e21e1b 100644 --- a/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java +++ b/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java @@ -15,9 +15,6 @@ import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; -import org.opensearch.core.xcontent.DeprecationHandler; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; import java.io.IOException;