From 04b9181175b28290221de0086ea92c8555db9e0a 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] #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 * Adding integ tests for empty mappings/aliases use-cases Signed-off-by: Megha Goyal * Fix unit tests for MappingsTraverser Signed-off-by: Megha Goyal --------- Signed-off-by: Megha Goyal (cherry picked from commit c0f7bd934b79f7c3a911229fa63e157116ec5ca9) --- .../mapper/MapperService.java | 7 ---- .../mapper/MappingsTraverser.java | 14 +++++-- .../mapper/MapperRestApiIT.java | 41 ++++++++++++++----- .../mapper/MappingsTraverserTests.java | 37 +++++++++++++---- 4 files changed, 69 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java index bd45edb66..e50af189a 100644 --- a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java +++ b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java @@ -398,13 +398,6 @@ public void onResponse(GetMappingsResponse getMappingsResponse) { } } - if (appliedAliases.size() == 0) { - actionListener.onFailure(SecurityAnalyticsException.wrap( - new OpenSearchStatusException("No applied aliases found", RestStatus.NOT_FOUND)) - ); - return; - } - // Traverse mappings and do copy with excluded type=alias properties MappingsTraverser mappingsTraverser = new MappingsTraverser(mappingMetadata); // Resulting mapping after filtering diff --git a/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java b/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java index fbddaa31c..67a9f0e67 100644 --- a/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java +++ b/src/main/java/org/opensearch/securityanalytics/mapper/MappingsTraverser.java @@ -5,23 +5,23 @@ 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.json.JsonXContent; import org.opensearch.core.xcontent.DeprecationHandler; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.XContentParser; -import org.opensearch.index.mapper.MapperService; -import org.opensearch.securityanalytics.rules.condition.ConditionListener; 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 +39,8 @@ */ public class MappingsTraverser { + private static final Logger log = LogManager.getLogger(MappingsTraverser.class); + /** * Traverser listener used to process leaves */ @@ -157,7 +159,10 @@ public void traverse() { try { Map rootProperties = (Map) 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 +198,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 0f5d783c4..91057069f 100644 --- a/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java +++ b/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java @@ -67,6 +67,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 respMap = (Map) responseAsMap(response); + Map props = (Map)((Map) 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*"; @@ -233,13 +254,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( @@ -247,21 +269,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 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 746294a43..cb9bfd68a 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 Map mappings = new HashMap<>(); @@ -136,30 +132,53 @@ public void onError(String error) { public void testTraverseInvalidMappings() { // 1. Parse mappings from MappingMetadata - Map mappings = new HashMap<>(); Map m = new HashMap<>(); m.put("netflow.event_data.SourceAddress", Map.of("type", "ip")); m.put("netflow.event_data.SourcePort", Map.of("type", "integer")); Map properties = Map.of("incorrect_properties", m); Map 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 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 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 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() {