Skip to content

Conversation

@Tarun-kishore
Copy link
Contributor

Description

Fixed incorrect serialization of the creation field in ExplainSMPolicy.toXContent(). The method was using .field() instead of .optionalField() for the nullable creation field, which is inconsistent with how the deletion field is handled and with the reference implementation in SMMetadata.toXContent().

Changed line 39 in ExplainSMPolicy.kt:

  • Before: .field(SMMetadata.CREATION_FIELD, it.creation)
  • After: .optionalField(SMMetadata.CREATION_FIELD, it.creation)

This ensures proper handling of null creation workflow metadata in Explain API responses and aligns with the serialization behavior in SMMetadata.

Related Issues

Resolves #1506

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

metadata?.let {
builder
.field(SMMetadata.CREATION_FIELD, it.creation)
.optionalField(SMMetadata.CREATION_FIELD, it.creation)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this have bwc issue when doing rolling upgrade? guess we need to add version check here

Copy link
Contributor Author

@Tarun-kishore Tarun-kishore Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Good catch. Added a version check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hailong-am I added a version check for greater than 3.3 (which is current version). The coverage is failing though, version 3.4 needs to be available for testing it. Rest all CI workflows are passing.

Signed-off-by: Tarun-kishore <[email protected]>
@Tarun-kishore Tarun-kishore force-pushed the bug-fix/explain-serialization branch from b07015e to 5079606 Compare October 13, 2025 10:34
@codecov
Copy link

codecov bot commented Oct 13, 2025

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.05%. Comparing base (a8e15da) to head (692fa59).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...gement/snapshotmanagement/model/ExplainSMPolicy.kt 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1507      +/-   ##
==========================================
- Coverage   76.11%   76.05%   -0.06%     
==========================================
  Files         375      375              
  Lines       17565    17567       +2     
  Branches     2409     2410       +1     
==========================================
- Hits        13370    13361       -9     
- Misses       2958     2963       +5     
- Partials     1237     1243       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bowenlan-amzn
Copy link
Member

Could you add a test covering this?

@Tarun-kishore
Copy link
Contributor Author

Tarun-kishore commented Oct 17, 2025

I've added unit test covering the parsing, but unit test cannot change Version.Current value. I can add a method like getCurrentVersion and mock that but that doesn't seem like a good way. I've also updated bwc tests to get explain response, Seems like it's coverage isn't included in coverage check.
I've update version check to onOrAfter V.3_3_0 to test bwc coverage, once V.3_4_0 is available I'll update it to that.

@bowenlan-amzn bowenlan-amzn merged commit 39b856d into opensearch-project:main Nov 17, 2025
22 of 23 checks passed
@Tarun-kishore Tarun-kishore deleted the bug-fix/explain-serialization branch November 17, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] ExplainSMPolicy incorrectly uses .field() for nullable creation field

3 participants