chore: Updates search_index resource with new SDK #1848

lantoli · 2024-01-17T11:39:51Z

Description

Updates search_index resource with new SDK.

Status added to documentation
Testing improved
Synonyms, Analyzers, Mapping_Fields can be updated to empty
Checked read-only fields: IndexD, Status

Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-222718

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

lantoli · 2024-01-17T17:23:41Z

internal/testutil/acc/attribute_checks.go

@@ -36,17 +36,21 @@ func IntGreatThan(value int) resource.CheckResourceAttrWithFunc {
 	}
 }

-func JSONEquals[T any](value T) resource.CheckResourceAttrWithFunc {
+func JSONEquals(expected string) resource.CheckResourceAttrWithFunc {


changed to make it easier to use

internal/service/searchindex/resource_search_index.go

AgustinBettati · 2024-01-18T07:49:04Z

internal/service/searchindex/resource_search_index.go

@@ -396,38 +391,39 @@ func flattenSearchIndexSynonyms(synonyms []admin.SearchSynonymMappingDefinition)

 func marshalSearchIndex(fields any) (string, error) {
 	bytes, err := json.Marshal(fields)
-	return string(bytes), err
+	str := string(bytes)
+	if str == "[]" || str == "{}" { // empty JSON array or object


q: why was this check added in these changes?

because now some fields can be empty arrays or objects instead of nil pointer

I see, seems a little hacky to include in this function. Maybe we can check when calling this function that the input parameter is non empty? That way it is clear when the terraform attribute will be left empty.

it's really the same idea as we're already doing in the unmarshal functions like:
if str == "" {
return fields, nil
}

not sure about checking this in each caller as we really want always this functionality, i.e. empty arrays to be serialized to empty strings

I've changed it to only check for empty arrays that it's the real need:

if str == "[]" { // empty JSON array serialized to empty string

there is no need to check for empty objects here as it's a local function, in a generic utiliy function it could make sense

Looking into the usages I struggle to understand why this is now needed vs the previous implementation, taking into account that the input of this function are using the .Get of the SDK to pass a slice (not pointer). Just want to make sure we have no change in behaviour.

in the prev. implementation empty arrays were nil but now they are len 0 arrays so they serialize to "[]" when we want to serialize to "".

if we don't do that we get this error:

Step 1/1 error: After applying this test step, the plan was not empty. ... # mongodbatlas_search_index.test will be updated in-place ~ resource "mongodbatlas_search_index" "test" { - analyzers = jsonencode([]) ... resource_search_index_test.go:231: Step 2/2 error: Check failed: Check 2/2 error: mongodbatlas_search_index.test: Attribute 'analyzers' expected "", got "[]" FAIL

marcosuma · 2024-01-18T07:51:41Z

internal/service/searchindex/resource_search_index.go

@@ -320,15 +319,39 @@ func resourceMongoDBAtlasSearchIndexRead(ctx context.Context, d *schema.Resource
 		return diag.Errorf("error setting `analyzer` for search index (%s): %s", d.Id(), err)
 	}

-	if analyzers := searchIndex.GetAnalyzers(); len(analyzers) > 0 {
+	if searchIndex.Type != nil && *searchIndex.Type == vectorSearch {


why are we changing the order and logic of the if statements here?

now we want to have empty arrays (instead of nil pointers) so for instance they're updated correctly. but depending on the vector type some arrays can't not be sent even empty. basically here we're doing the same logic as we do in Create.

the order is not important, at the end we need to set all fields in any order, the important part is to do different sets depending on the search type

at the end I've returned to the original order and logic following @AgustinBettati advice and using len > 0 again :-)

internal/service/searchindex/resource_search_index.go

marcosuma · 2024-01-18T07:54:52Z

internal/service/searchindex/resource_search_index.go

@@ -396,38 +391,39 @@ func flattenSearchIndexSynonyms(synonyms []admin.SearchSynonymMappingDefinition)

 func marshalSearchIndex(fields any) (string, error) {


not a strong opinion but don't know if they add big value here as in other cases because they're local functions that are simple wrappers to json functionality

marcosuma · 2024-01-18T07:57:31Z

internal/service/searchindex/resource_search_index_test.go

 }

-func testAccSearchIndexConfigSynonyms(projectIDStr, indexName, databaseName, clusterNameStr, clusterTerraformStr string) string {
+func configSynonyms(projectIDStr, indexName, databaseName, clusterNameStr, clusterTerraformStr string, has bool) string {


nit: maybe a better name for has? Unless I am missing something obvious here

if it's fine I'll rename to configWithSynonyms to be consistent with other functions

marcosuma · 2024-01-18T07:59:39Z

internal/service/searchindex/resource_search_index_test.go

@@ -159,7 +152,7 @@ func TestAccSearchIndexRS_withSynonyms(t *testing.T) {
 		CheckDestroy:             acc.CheckDestroySearchIndex,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccSearchIndexConfigSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr),
+				Config: configSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, true),


very nit: but for a good reading style we (not just you) should avoid passing bool values directtly to methods and instead use bool vars that has meaningful names.

Suggested change

Config: configSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, true),

Config: configSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, hasOrIsSomething),

i hear you. here being configWithSynonyms I thought it could be understood. WDYT about defining constansts:
withSynonyms = true
withoutSynonyms = true

and call like:

configWithSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, withSynonyms),

configWithSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, withoutSynonyms),

or being he function already with "synoyms" on it, what about constant to call:

configWithSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, with),

configWithSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, without),

i see it a bit redundant but maybe it's clearer

lantoli · 2024-01-18T09:13:10Z

internal/service/searchindex/resource_search_index_test.go

+				),
+			},
+			{
+				Config: configWithSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, without),


@marcosuma using with / without instead of true / false

website/docs/r/search_index.html.markdown

AgustinBettati · 2024-01-18T12:23:58Z

internal/service/searchindex/resource_search_index.go

@@ -396,38 +391,39 @@ func flattenSearchIndexSynonyms(synonyms []admin.SearchSynonymMappingDefinition)

 func marshalSearchIndex(fields any) (string, error) {
 	bytes, err := json.Marshal(fields)
-	return string(bytes), err
+	str := string(bytes)
+	if str == "[]" || str == "{}" { // empty JSON array or object


I see, seems a little hacky to include in this function. Maybe we can check when calling this function that the input parameter is non empty? That way it is clear when the terraform attribute will be left empty.

internal/service/searchindex/resource_search_index.go

AgustinBettati

LGTM

lantoli added 6 commits January 17, 2024 12:38

remove NonEmptyToPtr

5fc273a

Status is read only

db0aacb

rename resource operations

a8da9a2

updatedToEmptySynonyms

cae0b3d

updatedToEmptyAnalyzers

d4b177c

updatedToEmptyMappingsFields

12dacd4

lantoli marked this pull request as ready for review January 17, 2024 16:05

lantoli requested a review from a team as a code owner January 17, 2024 16:05

refactor fields in tests

9b7b253

lantoli commented Jan 17, 2024

View reviewed changes

AgustinBettati reviewed Jan 18, 2024

View reviewed changes

marcosuma reviewed Jan 18, 2024

View reviewed changes

internal/service/searchindex/resource_search_index.go Show resolved Hide resolved

marcosuma reviewed Jan 18, 2024

View reviewed changes

apply feedback forconfigWithSynonyms

26caf6f

lantoli commented Jan 18, 2024

View reviewed changes

lantoli added 2 commits January 18, 2024 10:17

rename test helper functions

ed8c1ae

status doc

baaece8

lantoli requested review from marcosuma and AgustinBettati January 18, 2024 11:24

AgustinBettati reviewed Jan 18, 2024

View reviewed changes

lantoli added 2 commits January 18, 2024 13:50

apply feedback for doc

db9614a

marshalSearchIndex only special case for empty arrays

36f3e16

AgustinBettati reviewed Jan 18, 2024

View reviewed changes

internal/service/searchindex/resource_search_index.go Outdated Show resolved Hide resolved

lantoli added 2 commits January 18, 2024 16:56

remove special case in marshalSearchIndex

0ff439a

use len as per Agustin suggestion to simplify Read

a36e763

lantoli requested a review from AgustinBettati January 18, 2024 16:08

AgustinBettati approved these changes Jan 18, 2024

View reviewed changes

marcosuma approved these changes Jan 18, 2024

View reviewed changes

lantoli merged commit f558d61 into master Jan 18, 2024
33 checks passed

lantoli deleted the CLOUDP-222718_search_index branch January 18, 2024 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Updates search_index resource with new SDK #1848

chore: Updates search_index resource with new SDK #1848

lantoli commented Jan 17, 2024 •

edited

Loading

lantoli Jan 17, 2024

AgustinBettati Jan 18, 2024

lantoli Jan 18, 2024

AgustinBettati Jan 18, 2024

lantoli Jan 18, 2024

lantoli Jan 18, 2024

AgustinBettati Jan 18, 2024

lantoli Jan 18, 2024 •

edited

Loading

marcosuma Jan 18, 2024

lantoli Jan 18, 2024 •

edited

Loading

lantoli Jan 18, 2024 •

edited

Loading

marcosuma Jan 18, 2024

lantoli Jan 18, 2024 •

edited

Loading

marcosuma Jan 18, 2024

lantoli Jan 18, 2024

marcosuma Jan 18, 2024

lantoli Jan 18, 2024

lantoli Jan 18, 2024

AgustinBettati Jan 18, 2024

AgustinBettati left a comment

		@@ -396,38 +391,39 @@ func flattenSearchIndexSynonyms(synonyms []admin.SearchSynonymMappingDefinition)

		func marshalSearchIndex(fields any) (string, error) {

	Config: configSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, true),
	Config: configSynonyms(clusterInfo.ProjectIDStr, indexName, databaseName, clusterInfo.ClusterNameStr, clusterInfo.ClusterTerraformStr, hasOrIsSomething),

chore: Updates search_index resource with new SDK #1848

chore: Updates search_index resource with new SDK #1848

Conversation

lantoli commented Jan 17, 2024 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

lantoli Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgustinBettati left a comment

Choose a reason for hiding this comment

lantoli commented Jan 17, 2024 •

edited

Loading

lantoli Jan 18, 2024 •

edited

Loading

lantoli Jan 18, 2024 •

edited

Loading

lantoli Jan 18, 2024 •

edited

Loading

lantoli Jan 18, 2024 •

edited

Loading