Use optionalField for creation in ExplainSMPolicy serialization (#1507)

Co-authored-by: Tarun-kishore <[email protected]>
Co-authored-by: bowenlan-amzn <[email protected]>

Author: Tarun-kishore

src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt

@@ -5,6 +5,7 @@
 
 package org.opensearch.indexmanagement.snapshotmanagement.model
 
+import org.opensearch.Version
 import org.opensearch.core.common.io.stream.StreamInput
 import org.opensearch.core.common.io.stream.StreamOutput
 import org.opensearch.core.common.io.stream.Writeable
@@ -36,10 +37,15 @@ data class ExplainSMPolicy(
     override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
         metadata?.let {
             builder
-                .field(SMMetadata.CREATION_FIELD, it.creation)
                 .optionalField(SMMetadata.DELETION_FIELD, it.deletion)
                 .field(SMMetadata.POLICY_SEQ_NO_FIELD, it.policySeqNo)
                 .field(SMMetadata.POLICY_PRIMARY_TERM_FIELD, it.policyPrimaryTerm)
+
+            if (Version.CURRENT.onOrAfter(Version.V_3_3_0)) {
+                builder.optionalField(SMMetadata.CREATION_FIELD, it.creation)
+            } else {
+                builder.field(SMMetadata.CREATION_FIELD, it.creation)
+            }
         }
         return builder.field(SMPolicy.ENABLED_FIELD, enabled)
     }

src/test/kotlin/org/opensearch/indexmanagement/bwc/SMBackwardsCompatibilityIT.kt

@@ -84,6 +84,11 @@ class SMBackwardsCompatibilityIT : SnapshotManagementRestTestCase() {
                     val traditionalPolicy = getSMPolicy(traditionalPolicyName)
                     assertNotNull("Traditional policy creation should exist", traditionalPolicy.creation)
                     assertNotNull("Traditional policy deletion should exist", traditionalPolicy.deletion)
+
+                    // Verify explain response works in old version (covers old version serialization path)
+                    val explainResponse = explainSMPolicy(traditionalPolicyName)
+                    val explainMetadata = parseExplainResponse(explainResponse.entity.content)
+                    assertTrue("Explain should return results", explainMetadata.isNotEmpty())
                 }
                 ClusterType.MIXED -> {
                     assertTrue(pluginNames.contains("opensearch-index-management"))
@@ -92,6 +97,11 @@ class SMBackwardsCompatibilityIT : SnapshotManagementRestTestCase() {
                     val traditionalPolicy = getSMPolicy(traditionalPolicyName)
                     assertNotNull("Traditional policy creation should exist", traditionalPolicy.creation)
                     assertNotNull("Traditional policy deletion should exist", traditionalPolicy.deletion)
+
+                    // Verify explain response works during rolling upgrade (mixed old/new version serialization)
+                    val explainResponse = explainSMPolicy(traditionalPolicyName)
+                    val explainMetadata = parseExplainResponse(explainResponse.entity.content)
+                    assertTrue("Explain should return results", explainMetadata.isNotEmpty())
                 }
                 ClusterType.UPGRADED -> {
                     assertTrue(pluginNames.contains("opensearch-index-management"))
@@ -101,6 +111,11 @@ class SMBackwardsCompatibilityIT : SnapshotManagementRestTestCase() {
                     assertNotNull("Traditional policy creation should exist", traditionalPolicy.creation)
                     assertNotNull("Traditional policy deletion should exist", traditionalPolicy.deletion)
 
+                    // Verify explain response for traditional policy on upgraded cluster (new version serialization)
+                    val traditionalExplainResponse = explainSMPolicy(traditionalPolicyName)
+                    val traditionalExplainMetadata = parseExplainResponse(traditionalExplainResponse.entity.content)
+                    assertTrue("Explain should return results for traditional policy", traditionalExplainMetadata.isNotEmpty())
+
                     // Now test new features on upgraded cluster
 
                     // Create deletion-only policy (3.2.0+ feature)
@@ -109,12 +124,22 @@ class SMBackwardsCompatibilityIT : SnapshotManagementRestTestCase() {
                     assertNull("Deletion-only policy creation should be null", deletionOnlyPolicy.creation)
                     assertNotNull("Deletion-only policy deletion should exist", deletionOnlyPolicy.deletion)
 
+                    // Verify explain response for deletion-only policy (tests null creation serialization with optionalField)
+                    val deletionOnlyExplainResponse = explainSMPolicy(deletionOnlyPolicyName)
+                    val deletionOnlyExplainMetadata = parseExplainResponse(deletionOnlyExplainResponse.entity.content)
+                    assertTrue("Explain should return results for deletion-only policy", deletionOnlyExplainMetadata.isNotEmpty())
+
                     // Create policy with snapshot pattern (3.2.0+ feature)
                     createPatternPolicy(patternPolicyName)
                     val patternPolicy = getSMPolicy(patternPolicyName)
                     assertNotNull("Pattern policy creation should exist", patternPolicy.creation)
                     assertNotNull("Pattern policy deletion should exist", patternPolicy.deletion)
                     assertEquals("Pattern policy should have snapshot pattern", "external-backup-*", patternPolicy.deletion?.snapshotPattern)
+
+                    // Verify explain response for pattern policy
+                    val patternExplainResponse = explainSMPolicy(patternPolicyName)
+                    val patternExplainMetadata = parseExplainResponse(patternExplainResponse.entity.content)
+                    assertTrue("Explain should return results for pattern policy", patternExplainMetadata.isNotEmpty())
                 }
             }
             break

src/test/kotlin/org/opensearch/indexmanagement/snapshotmanagement/action/ResponseTests.kt

@@ -6,9 +6,12 @@
 package org.opensearch.indexmanagement.snapshotmanagement.action
 
 import org.opensearch.common.io.stream.BytesStreamOutput
+import org.opensearch.common.xcontent.XContentFactory
 import org.opensearch.core.common.io.stream.StreamInput
 import org.opensearch.core.rest.RestStatus
+import org.opensearch.core.xcontent.ToXContent
 import org.opensearch.indexmanagement.indexstatemanagement.util.XCONTENT_WITHOUT_TYPE_AND_USER
+import org.opensearch.indexmanagement.opensearchapi.toMap
 import org.opensearch.indexmanagement.snapshotmanagement.api.transport.explain.ExplainSMPolicyResponse
 import org.opensearch.indexmanagement.snapshotmanagement.api.transport.get.GetSMPoliciesResponse
 import org.opensearch.indexmanagement.snapshotmanagement.api.transport.get.GetSMPolicyResponse
@@ -17,8 +20,8 @@ import org.opensearch.indexmanagement.snapshotmanagement.model.ExplainSMPolicy
 import org.opensearch.indexmanagement.snapshotmanagement.randomSMMetadata
 import org.opensearch.indexmanagement.snapshotmanagement.randomSMPolicy
 import org.opensearch.indexmanagement.snapshotmanagement.smDocIdToPolicyName
-import org.opensearch.indexmanagement.snapshotmanagement.toMap
 import org.opensearch.test.OpenSearchTestCase
+import org.opensearch.indexmanagement.snapshotmanagement.toMap as toMapHelper
 
 class ResponseTests : OpenSearchTestCase() {
     fun `test index sm policy response`() {
@@ -38,8 +41,8 @@ class ResponseTests : OpenSearchTestCase() {
     fun `test index sm policy toXContent`() {
         val smPolicy = randomSMPolicy()
         val res = IndexSMPolicyResponse("someid", 1L, 2L, 3L, smPolicy, RestStatus.OK)
-        val resMap = res.toMap()
-        assertEquals(resMap["sm_policy"], smPolicy.toMap(XCONTENT_WITHOUT_TYPE_AND_USER))
+        val resMap = res.toMapHelper()
+        assertEquals(resMap["sm_policy"], smPolicy.toMapHelper(XCONTENT_WITHOUT_TYPE_AND_USER))
     }
 
     fun `test get sm policy response`() {
@@ -81,7 +84,32 @@ class ResponseTests : OpenSearchTestCase() {
     fun `test get sm policy toXContent`() {
         val smPolicy = randomSMPolicy()
         val res = GetSMPolicyResponse("someid", 1L, 2L, 3L, smPolicy)
-        val resMap = res.toMap()
-        assertEquals(resMap["sm_policy"], smPolicy.toMap(XCONTENT_WITHOUT_TYPE_AND_USER))
+        val resMap = res.toMapHelper()
+        assertEquals(resMap["sm_policy"], smPolicy.toMapHelper(XCONTENT_WITHOUT_TYPE_AND_USER))
+    }
+
+    fun `test explain sm policy toXContent with null creation`() {
+        // Test that ExplainSMPolicy.toXContent() handles null creation correctly (for V_3_3_0+)
+        val smMetadata = randomSMMetadata()
+        val smMetadataWithNullCreation = smMetadata.copy(creation = null)
+        val explainSMPolicy = ExplainSMPolicy(smMetadataWithNullCreation, true)
+
+        // Properly convert ToXContentFragment to map by wrapping in an object
+        val builder = XContentFactory.jsonBuilder().startObject()
+        explainSMPolicy.toXContent(builder, ToXContent.EMPTY_PARAMS)
+        builder.endObject()
+        val explainMap = builder.toMap()
+
+        // Verify that creation field is NOT present when null (optionalField behavior for V_3_3_0+)
+        assertFalse("Creation field should not be present when null", explainMap.containsKey("creation"))
+
+        // Verify deletion field IS present
+        assertTrue("Deletion field should be present", explainMap.containsKey("deletion"))
+
+        // Verify other mandatory fields are present
+        assertTrue("Policy seq_no should be present", explainMap.containsKey("policy_seq_no"))
+        assertTrue("Policy primary_term should be present", explainMap.containsKey("policy_primary_term"))
+        assertTrue("Enabled field should be present", explainMap.containsKey("enabled"))
+        assertEquals(true, explainMap["enabled"])
     }
 }