Don't recreate when field is removed

CHANGELOG.md

@@ -3,6 +3,7 @@
 ### Fixed
 - Respect `ignore_unavailable` and `include_global_state` values when configuring SLM policies ([#224](https://github.com/elastic/terraform-provider-elasticstack/pull/224))
 - Refactor API client functions and return diagnostics ([#220](https://github.com/elastic/terraform-provider-elasticstack/pull/220))
+- Fix not to recreate index when field is removed from mapping ([#232](https://github.com/elastic/terraform-provider-elasticstack/pull/232))
 
 ## [0.5.0] - 2022-12-07
 

docs/resources/elasticsearch_index.md

@@ -88,7 +88,9 @@ resource "elasticstack_elasticsearch_index" "my_index" {
 - `load_fixed_bitset_filters_eagerly` (Boolean) Indicates whether cached filters are pre-loaded for nested queries. This can be set only on creation.
 - `mappings` (String) Mapping for fields in the index.
 If specified, this mapping can include: field names, [field data types](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html), [mapping parameters](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-params.html).
-**NOTE:** changing datatypes in the existing _mappings_ will force index to be re-created.
+**NOTE:** 
+- Changing datatypes in the existing _mappings_ will force index to be re-created.
+- Removing field will be ignored by default same as elasticsearch. You need to recreate the index to remove field completely.
 - `max_docvalue_fields_search` (Number) The maximum number of `docvalue_fields` that are allowed in a query.
 - `max_inner_result_window` (Number) The maximum value of `from + size` for inner hits definition and top hits aggregations to this index.
 - `max_ngram_diff` (Number) The maximum allowed difference between min_gram and max_gram for NGramTokenizer and NGramTokenFilter.

internal/elasticsearch/index/index.go

@@ -471,7 +471,10 @@ func ResourceIndex() *schema.Resource {
 		"mappings": {
 			Description: `Mapping for fields in the index.
 If specified, this mapping can include: field names, [field data types](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html), [mapping parameters](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-params.html).
-**NOTE:** changing datatypes in the existing _mappings_ will force index to be re-created.`,
+**NOTE:** 
+- Changing datatypes in the existing _mappings_ will force index to be re-created.
+- Removing field will be ignored by default same as elasticsearch. You need to recreate the index to remove field completely.
+`,
 			Type:             schema.TypeString,
 			Optional:         true,
 			DiffSuppressFunc: utils.DiffJsonSuppress,
@@ -603,7 +606,7 @@ If specified, this mapping can include: field names, [field data types](https://
 				if !ok {
 					return true
 				}
-				return IsMappingForceNewRequired(oldProps.(map[string]interface{}), newProps.(map[string]interface{}))
+				return IsMappingForceNewRequired(ctx, oldProps.(map[string]interface{}), newProps.(map[string]interface{}))
 			}
 
 			// if all check passed, we can update the map
@@ -900,37 +903,37 @@ func resourceIndexDelete(ctx context.Context, d *schema.ResourceData, meta inter
 	return diags
 }
 
-func IsMappingForceNewRequired(old map[string]interface{}, new map[string]interface{}) bool {
+func IsMappingForceNewRequired(ctx context.Context, old map[string]interface{}, new map[string]interface{}) bool {
 	for k, v := range old {
 		oldFieldSettings := v.(map[string]interface{})
-		if newFieldSettings, ok := new[k]; ok {
-			newSettings := newFieldSettings.(map[string]interface{})
-			// check if the "type" field exists and match with new one
-			if s, ok := oldFieldSettings["type"]; ok {
-				if ns, ok := newSettings["type"]; ok {
-					if !reflect.DeepEqual(s, ns) {
-						return true
-					}
-					continue
-				} else {
+		newFieldSettings, ok := new[k]
+		// When field is removed, it'll be ignored in elasticsearch
+		if !ok {
+			tflog.Warn(ctx, fmt.Sprintf("removing %s field in mappings is ignored. Re-index to remove the field completely.", k))
+			continue
+		}
+		newSettings := newFieldSettings.(map[string]interface{})
+		// check if the "type" field exists and match with new one
+		if s, ok := oldFieldSettings["type"]; ok {
+			if ns, ok := newSettings["type"]; ok {
+				if !reflect.DeepEqual(s, ns) {
 					return true
 				}
+				continue
+			} else {
+				return true
 			}
+		}
 
-			// if we have "mapping" field, let's call ourself to check again
-			if s, ok := oldFieldSettings["properties"]; ok {
-				if ns, ok := newSettings["properties"]; ok {
-					if IsMappingForceNewRequired(s.(map[string]interface{}), ns.(map[string]interface{})) {
-						return true
-					}
-					continue
-				} else {
+		// if we have "mapping" field, let's call ourself to check again
+		if s, ok := oldFieldSettings["properties"]; ok {
+			if ns, ok := newSettings["properties"]; ok {
+				if IsMappingForceNewRequired(context.Background(), s.(map[string]interface{}), ns.(map[string]interface{})) {
 					return true
 				}
+			} else {
+				tflog.Warn(ctx, fmt.Sprintf("removing %s propeties in mappings is ignored, if you neeed to remove it completely, please recreate the index", k))
 			}
-		} else {
-			// if the key not found in the new props, force new resource
-			return true
 		}
 	}
 	return false

internal/elasticsearch/index/index_test.go

@@ -1,6 +1,7 @@
 package index_test
 
 import (
+	"context"
 	"fmt"
 	"regexp"
 	"testing"
@@ -140,6 +141,22 @@ func TestAccResourceIndexSettingsConflict(t *testing.T) {
 	})
 }
 
+func TestAccResourceIndexRemovingField(t *testing.T) {
+	indexName := sdkacctest.RandStringFromCharSet(22, sdkacctest.CharSetAlphaNum)
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 func() { acctest.PreCheck(t) },
+		CheckDestroy:             checkResourceIndexDestroy,
+		ProtoV5ProviderFactories: acctest.Providers,
+		Steps: []resource.TestStep{
+			// Confirm removing field doesn't produce recreate by using prevent_destroy
+			{Config: testAccResourceIndexRemovingFieldCreate(indexName)},
+			{Config: testAccResourceIndexRemovingFieldUpdate(indexName), ExpectNonEmptyPlan: true},
+			{Config: testAccResourceIndexRemovingFieldPostUpdate(indexName), ExpectNonEmptyPlan: true},
+		},
+	})
+}
+
 func testAccResourceIndexCreate(name string) string {
 	return fmt.Sprintf(`
 provider "elasticstack" {
@@ -340,6 +357,67 @@ resource "elasticstack_elasticsearch_index" "test_settings_conflict" {
 	`, name)
 }
 
+func testAccResourceIndexRemovingFieldCreate(name string) string {
+	return fmt.Sprintf(`
+provider "elasticstack" {
+  elasticsearch {}
+}
+
+resource "elasticstack_elasticsearch_index" "test_settings_removing_field" {
+  name = "%s"
+
+  mappings = jsonencode({
+    properties = {
+      field1    = { type = "text" }
+      field2    = { type = "text" }
+    }
+  })
+  lifecycle {
+    prevent_destroy = true
+  }
+}
+	`, name)
+}
+
+func testAccResourceIndexRemovingFieldUpdate(name string) string {
+	return fmt.Sprintf(`
+provider "elasticstack" {
+  elasticsearch {}
+}
+
+resource "elasticstack_elasticsearch_index" "test_settings_removing_field" {
+  name = "%s"
+
+  mappings = jsonencode({
+    properties = {
+      field1    = { type = "text" }
+    }
+  })
+  lifecycle {
+    prevent_destroy = true
+  }
+}
+	`, name)
+}
+
+func testAccResourceIndexRemovingFieldPostUpdate(name string) string {
+	return fmt.Sprintf(`
+provider "elasticstack" {
+  elasticsearch {}
+}
+
+resource "elasticstack_elasticsearch_index" "test_settings_removing_field" {
+  name = "%s"
+
+  mappings = jsonencode({
+    properties = {
+      field1    = { type = "text" }
+    }
+  })
+}
+	`, name)
+}
+
 func checkResourceIndexDestroy(s *terraform.State) error {
 	client, err := clients.NewAcceptanceTestingClient()
 	if err != nil {
@@ -405,14 +483,32 @@ func Test_IsMappingForceNewRequired(t *testing.T) {
 			want: true,
 		},
 		{
-			name: "return true when field is removed",
+			name: "return false when field is removed",
 			old: map[string]interface{}{
 				"field1": map[string]interface{}{
 					"type": "text",
 				},
 			},
 			new:  map[string]interface{}{},
-			want: true,
+			want: false,
+		},
+		{
+			name: "return false when dynamically added child property is removed",
+			old: map[string]interface{}{
+				"parent": map[string]interface{}{
+					"properties": map[string]interface{}{
+						"child": map[string]interface{}{
+							"type": "keyword",
+						},
+					},
+				},
+			},
+			new: map[string]interface{}{
+				"parent": map[string]interface{}{
+					"type": "object",
+				},
+			},
+			want: false,
 		},
 		{
 			name: "return true when child property's type changes",
@@ -439,7 +535,7 @@ func Test_IsMappingForceNewRequired(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := index.IsMappingForceNewRequired(tt.old, tt.new); got != tt.want {
+			if got := index.IsMappingForceNewRequired(context.Background(), tt.old, tt.new); got != tt.want {
 				t.Errorf("IsMappingForceNewRequired() = %v, want %v", got, tt.want)
 			}
 		})