From d0d9f83a83467c2fbf75e655c52d19b4eeb7ff54 Mon Sep 17 00:00:00 2001 From: Tarun-kishore Date: Mon, 13 Oct 2025 15:48:49 +0530 Subject: [PATCH 1/6] Use optionalField for creation in ExplainSMPolicy serialization Signed-off-by: Tarun-kishore --- .../indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt index 28158bf92..2bb8ccd07 100644 --- a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt +++ b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt @@ -36,7 +36,7 @@ data class ExplainSMPolicy( override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder { metadata?.let { builder - .field(SMMetadata.CREATION_FIELD, it.creation) + .optionalField(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) From 5079606b97beb05abf56b2edbfc9b0adede34942 Mon Sep 17 00:00:00 2001 From: Tarun-kishore Date: Mon, 13 Oct 2025 16:03:43 +0530 Subject: [PATCH 2/6] Adding Version check Signed-off-by: Tarun-kishore --- .../snapshotmanagement/model/ExplainSMPolicy.kt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt index 2bb8ccd07..cf0ceaef3 100644 --- a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt +++ b/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 - .optionalField(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 > 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) } From e2b98b52824b38d08b70e03c6f576385899a6a66 Mon Sep 17 00:00:00 2001 From: Tarun-kishore Date: Fri, 17 Oct 2025 09:37:38 +0530 Subject: [PATCH 3/6] Adding unit tests Signed-off-by: Tarun-kishore --- .../model/ExplainSMPolicy.kt | 2 +- .../action/ResponseTests.kt | 38 ++++++++++++++++--- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt index cf0ceaef3..46198bfce 100644 --- a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt +++ b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt @@ -41,7 +41,7 @@ data class ExplainSMPolicy( .field(SMMetadata.POLICY_SEQ_NO_FIELD, it.policySeqNo) .field(SMMetadata.POLICY_PRIMARY_TERM_FIELD, it.policyPrimaryTerm) - if (Version.CURRENT > Version.V_3_3_0) { + if (Version.CURRENT.onOrAfter(Version.V_3_3_0)) { builder.optionalField(SMMetadata.CREATION_FIELD, it.creation) } else { builder.field(SMMetadata.CREATION_FIELD, it.creation) diff --git a/src/test/kotlin/org/opensearch/indexmanagement/snapshotmanagement/action/ResponseTests.kt b/src/test/kotlin/org/opensearch/indexmanagement/snapshotmanagement/action/ResponseTests.kt index 409f3b29a..d519ae4ba 100644 --- a/src/test/kotlin/org/opensearch/indexmanagement/snapshotmanagement/action/ResponseTests.kt +++ b/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"]) } } From ae99a59c2006978c81eb135e893db5be7c8c1bce Mon Sep 17 00:00:00 2001 From: Tarun-kishore Date: Fri, 17 Oct 2025 11:46:10 +0530 Subject: [PATCH 4/6] Adding explain check in backward compatibility Signed-off-by: Tarun-kishore --- .../bwc/SMBackwardsCompatibilityIT.kt | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/test/kotlin/org/opensearch/indexmanagement/bwc/SMBackwardsCompatibilityIT.kt b/src/test/kotlin/org/opensearch/indexmanagement/bwc/SMBackwardsCompatibilityIT.kt index 141c57764..1be4695ed 100644 --- a/src/test/kotlin/org/opensearch/indexmanagement/bwc/SMBackwardsCompatibilityIT.kt +++ b/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 From cae269371cb04aa7eeeea6bf520bbbe591d4e03b Mon Sep 17 00:00:00 2001 From: Tarun-kishore Date: Thu, 23 Oct 2025 14:24:25 +0530 Subject: [PATCH 5/6] Updating Version Check to after V3.3 Signed-off-by: Tarun-kishore --- .../indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt index 46198bfce..2bbba9bef 100644 --- a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt +++ b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt @@ -41,7 +41,7 @@ data class ExplainSMPolicy( .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)) { + if (Version.CURRENT.after(Version.V_3_3_0)) { builder.optionalField(SMMetadata.CREATION_FIELD, it.creation) } else { builder.field(SMMetadata.CREATION_FIELD, it.creation) From 145b9c2a79c74d49e10dcb65fda5e51be6f87710 Mon Sep 17 00:00:00 2001 From: Tarun-kishore Date: Thu, 23 Oct 2025 14:42:20 +0530 Subject: [PATCH 6/6] Revert "Updating Version Check to after V3.3" This reverts commit cae269371cb04aa7eeeea6bf520bbbe591d4e03b. Signed-off-by: Tarun-kishore --- .../indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt index 2bbba9bef..46198bfce 100644 --- a/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt +++ b/src/main/kotlin/org/opensearch/indexmanagement/snapshotmanagement/model/ExplainSMPolicy.kt @@ -41,7 +41,7 @@ data class ExplainSMPolicy( .field(SMMetadata.POLICY_SEQ_NO_FIELD, it.policySeqNo) .field(SMMetadata.POLICY_PRIMARY_TERM_FIELD, it.policyPrimaryTerm) - if (Version.CURRENT.after(Version.V_3_3_0)) { + if (Version.CURRENT.onOrAfter(Version.V_3_3_0)) { builder.optionalField(SMMetadata.CREATION_FIELD, it.creation) } else { builder.field(SMMetadata.CREATION_FIELD, it.creation)