Fix ClassCastException from immutable map returned by StreamInput.readMap()

[thecodingshrimp]

Summary

Fixes #967

StreamInput.readMap() documents that for zero-size maps it might return an immutable map (Collections.emptyMap()). Three deserialization constructors in this repo perform an unchecked cast (suppressWarning) that assumes a MutableMap, causing a ClassCastException at runtime when a monitor with empty uiMetadata, lastRunContext, or queryResults is deserialized over transport.

Exception:

kotlin.collections.EmptyMap cannot be cast to kotlin.collections.MutableMap

Root cause

StreamOutput.writeMap / StreamInput.readMap guarantee only that the result implements java.util.Map. The Javadoc states:

If the returned map contains any entries it will be mutable. If it is empty it might be immutable.

The suppressWarning() helper performs map as MutableMap<String, Any> without a defensive copy, which fails when the deserialized map is Collections.emptyMap().

Identified via opensearch-project/security-analytics#1722 — TransportIndexDetectorAction passes Map.of() as uiMetadata when constructing a Monitor, which serializes as a zero-size map, readMap() returns Collections.emptyMap(), and the cast fails.

Changes

Replace suppressWarning(sin.readMap()) with sin.readMap()?.toMutableMap() ?: mutableMapOf() at all three callsites and remove the suppressWarning() helper function entirely.

File	Change
Monitor.kt	uiMetadata deserialization — safe copy
MonitorMetadata.kt	lastRunContext deserialization — safe copy
Alert.kt	queryResults deserialization — safe copy; remove suppressWarning import

Testing

All existing Monitor-related tests pass (./gradlew test --tests "*Monitor*" → BUILD SUCCESSFUL).

Check List

• Existing tests pass
• Commits signed off (DCO)
• New tests (not added — fix is a defensive copy at deserialization; behaviour is identical for non-empty maps)

Related

• Fixes Fix ClassCastException from immutable map returned by StreamInput.readMap() #967
• Fix mutable script params for detector trigger actions (#1715) security-analytics#1722
• [BUG] Detector creation/update fails with Kotlin casting error: Collections.EmptyMap cannot be cast to MutableMap security-analytics#1715
• [BUG] Attempt to disable an active rule under a threat detector causes an error security-analytics#1553

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a2aa8ae)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Wire Format Breaking Change writeTo now writes sourceToQueryIndexMapping (added after writeMap(lastRunContext)), but the previous implementation did not write this field (it was removed from the diff). Additionally, the deserialization constructor now reads lastRunContext followed by sourceToQueryIndexMapping. This changes the transport wire format and will break compatibility with any node still running the previous version (mixed-cluster upgrade scenario). Confirm this was intentional and coordinated with a version guard, or restore the original serialization order/fields. override fun writeTo(out: StreamOutput) { out.writeString(id) out.writeLong(seqNo) out.writeLong(primaryTerm) out.writeString(monitorId) out.writeCollection(lastActionExecutionTimes) out.writeMap(lastRunContext) out.writeMap(sourceToQueryIndexMapping as Map<String, Any>) Possible Issue sin.readMapAsMutableMap() returns MutableMap<String, Any>, but the field is Map<String, TriggerResult>. The generic readMap() reads values as arbitrary Any (not as TriggerRunResult writables), so the resulting values will not be actual TriggerRunResult instances if any were serialized as complex objects. The old code had the same latent issue, but this should be verified — if triggerResults is ever populated non-empty over the wire, downstream casts to TriggerResult will fail. sin.readMapAsMutableMap() as Map<String, TriggerResult> // triggerResults Import Ordering The added import org.opensearch.commons.alerting.util.readMapAsMutableMap is placed before org.opensearch.commons.alerting.model.* imports, violating alphabetical import ordering. This will likely fail ktlint/detekt formatting checks. Same pattern is repeated across Alert.kt, Monitor.kt, MonitorMetadata.kt, MonitorRunResult.kt, Workflow.kt, and several trigger run result files. import org.opensearch.commons.alerting.util.readMapAsMutableMap Unsafe Cast xcp.map()?.toMutableMap() ?: mutableMapOf()) as MutableMap<String, String> casts a MutableMap<String, Any> to MutableMap<String, String>. If any parsed value is non-string (e.g., a numeric or nested object due to malformed input), this cast succeeds at compile time but a ClassCastException occurs later when values are accessed as String. Consider validating or mapping values explicitly. SOURCE_TO_QUERY_INDEX_MAP_FIELD -> sourceToQueryIndexMapping = (xcp.map()?.toMutableMap() ?: mutableMapOf()) as MutableMap<String, String>

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a2aa8ae

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix return type to allow nullable values StreamInput.readMap() returns Map<String, Object> (nullable Any?). Declaring the return as MutableMap<String, Any> performs an implicit unchecked cast that could produce values with null at runtime, causing later NPEs. Consider returning MutableMap<String, Any?> to match the actual signature, or filter/assert non-null values. src/main/kotlin/org/opensearch/commons/alerting/util/StreamInputExtensions.kt [15-16] -fun StreamInput.readMapAsMutableMap(): MutableMap<String, Any> = +fun StreamInput.readMapAsMutableMap(): MutableMap<String, Any?> = readMap()?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 5 __ Why: Valid concern: readMap() returns Map<String, Object> where values may be null, so declaring MutableMap<String, Any> masks nullability. However, many call sites cast to specific mutable types, so changing this could ripple.	Low
	Always return a mutable copy to simplify logic The current code returns a plain mutableMapOf() on empty and a HashMap copy on non-empty. Since tests cast this to MutableMap, and readMap may return an immutable map for the empty case, simply always wrapping in a HashMap (or LinkedHashMap to preserve order) would be simpler and safer, avoiding the branching and ensuring consistent mutability behavior. src/main/kotlin/org/opensearch/commons/alerting/action/DocLevelMonitorFanOutResponse.kt [88-93] private fun readTriggerResults(sin: StreamInput): Map<String, DocumentLevelTriggerRunResult> { val raw = sin.readMap(StreamInput::readString, DocumentLevelTriggerRunResult::readFrom) - if (raw.isEmpty()) return mutableMapOf() @Suppress("UNCHECKED_CAST") - return HashMap(raw as Map<String, DocumentLevelTriggerRunResult>) + return LinkedHashMap(raw as Map<String, DocumentLevelTriggerRunResult>) } Suggestion importance[1-10]: 4 __ Why: Minor simplification; both approaches produce a mutable map. The improvement is stylistic rather than functional.	Low
	Safely cast map values to correct type The cast as Map<String, String> on a MutableMap<String, Any?> is unchecked and won't fail at cast time, but any later access assuming String values could still ClassCastException if the stream contains non-String values. Consider mapping values to Strings explicitly to preserve type safety, matching the field declaration. src/main/kotlin/org/opensearch/commons/alerting/aggregation/bucketselectorext/BucketSelectorExtAggregationBuilder.kt [45-46] -@Suppress("UNCHECKED_CAST") -bucketsPathsMap = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as Map<String, String> +bucketsPathsMap = sin.readMap()?.mapValuesTo(mutableMapOf()) { it.value as String } ?: mutableMapOf() Suggestion importance[1-10]: 3 __ Why: The original code has always used an unchecked cast; the suggestion is more defensive but changes semantics slightly and is not directly related to the PR's fix.	Low

---

Previous suggestions

Suggestions up to commit b41f66d

Category	Suggestion	Impact
Possible issue	Fix inconsistent map type cast The cast as Map<String, String> may cause a ClassCastException at runtime when values are actually Any, and assigning Map<String, String> to a MutableMap<String, String> field is inconsistent. Since the field type is MutableMap<String, String>, cast the mutable map directly and cast entries to String. src/main/kotlin/org/opensearch/commons/alerting/aggregation/bucketselectorext/BucketSelectorExtAggregationBuilder.kt [46] -bucketsPathsMap = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as Map<String, String> +@Suppress("UNCHECKED_CAST") +bucketsPathsMap = (sin.readMap()?.toMutableMap() ?: mutableMapOf<String, Any>()) as MutableMap<String, String> Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the cast as Map<String, String> is inconsistent with the field's declared MutableMap<String, String> type. The improved code makes the mutability contract explicit, though the runtime behavior is largely equivalent since the underlying map is already mutable.	Low
General	Return mutable map explicitly The raw as Map<String, DocumentLevelTriggerRunResult> cast is unnecessary since readMap with typed readers already returns Map<String, DocumentLevelTriggerRunResult>. More importantly, HashMap(raw) copies to a mutable map but the return type Map doesn't advertise mutability; consider returning MutableMap directly to make the mutability contract explicit for callers that cast to MutableMap. src/main/kotlin/org/opensearch/commons/alerting/action/DocLevelMonitorFanOutResponse.kt [88-93] -private fun readTriggerResults(sin: StreamInput): Map<String, DocumentLevelTriggerRunResult> { +private fun readTriggerResults(sin: StreamInput): MutableMap<String, DocumentLevelTriggerRunResult> { val raw = sin.readMap(StreamInput::readString, DocumentLevelTriggerRunResult::readFrom) if (raw.isEmpty()) return mutableMapOf() - @Suppress("UNCHECKED_CAST") - return HashMap(raw as Map<String, DocumentLevelTriggerRunResult>) + return HashMap(raw) } Suggestion importance[1-10]: 4 __ Why: Correctly notes the unnecessary cast since readMap with typed readers already returns the proper type. Returning MutableMap explicitly is a minor readability/API clarity improvement but doesn't change functionality significantly.	Low
	Handle nullable map values safely StreamInput.readMap() returns Map<String, Any> where values may be null (the underlying generic map allows nulls). Declaring the return as MutableMap<String, Any> while toMutableMap() on a Map<String, Any?> produces MutableMap<String, Any?> may silently hide nulls that violate the non-null contract. Consider making the value type nullable or filtering nulls to avoid downstream NPEs. src/main/kotlin/org/opensearch/commons/alerting/util/StreamInputExtensions.kt [15-16] fun StreamInput.readMapAsMutableMap(): MutableMap<String, Any> = - readMap()?.toMutableMap() ?: mutableMapOf() + readMap()?.filterValues { it != null }?.mapValues { it.value!! }?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 3 __ Why: The concern about null values is theoretical; the original code preserves existing behavior of sin.readMap(). Filtering nulls could actually change semantics and hide data loss, so the suggested fix may not be desirable. </result> </details></details></td><td align=center>Low </td></tr></tr></tbody></table> </details> <details><summary>Suggestions up to commit e518795</summary> <br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Use typed reader for trigger results map</summary> ___ **<code>sin.readMap()</code> returns <code>Map<String, Any></code>, but values in <code>triggerResults</code> must be <br><code>ChainedAlertTriggerRunResult</code> (a Writeable). The generic <code>readMap()</code> cannot deserialize <br>those complex objects — non-empty maps written by <code>writeTo</code> will not round-trip <br>correctly, and the unchecked cast masks this. Use the typed overload <br><code>sin.readMap(StreamInput::readString, ::ChainedAlertTriggerRunResult)</code> and defensively <br>copy into a mutable map.** [src/main/kotlin/org/opensearch/commons/alerting/model/WorkflowRunResult.kt [38]](https://github.com/opensearch-project/common-utils/pull/968/files#diff-e97cc9df46b267ae869138da97b3e8f1ced7943dd508862c29a5969403ab3258R38-R38) ```diff -triggerResults = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as Map<String, ChainedAlertTriggerRunResult> +triggerResults = run { + val raw = sin.readMap(StreamInput::readString, ::ChainedAlertTriggerRunResult) + if (raw.isEmpty()) mutableMapOf() else HashMap(raw) +} Suggestion importance[1-10]: 7 __ Why: Legitimate concern: the untyped readMap() cannot properly deserialize ChainedAlertTriggerRunResult Writeable values, so non-empty maps won't round-trip correctly. The test added in the PR even acknowledges this limitation, suggesting this is a real gap.	Medium
Deserialize trigger results with typed reader Using the untyped readMap() here cannot reconstruct concrete TriggerRunResult subclasses (values are read as generic Any), so the unchecked cast to Map<String, TriggerResult> will produce a map whose values are not the expected type, causing deferred ClassCastException on access. Deserialize with the appropriate typed reader (matching how writeTo serializes trigger results) instead of the generic map reader. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [40] -sin.readMapAsMutableMap() as Map<String, TriggerResult> // triggerResults +sin.readMap(StreamInput::readString) { s -> TriggerRunResult.readFrom(s) as TriggerResult } + .let { if (it.isEmpty()) mutableMapOf() else HashMap(it) } Suggestion importance[1-10]: 6 __ Why: Valid concern about untyped map deserialization being unable to reconstruct concrete TriggerRunResult subclasses. However, this pattern pre-existed and the PR only changed to defensive copying, so it's a legitimate but pre-existing issue.	Low
General	Avoid unchecked cast for string-value map Casting a MutableMap<String, Any> to MutableMap<String, String> is an unchecked cast that can produce heap pollution and defer ClassCastException until later reads. Prefer explicitly copying entries with a String value cast, or use readMap(StreamInput::readString, StreamInput::readString) if the wire format is known to contain strings. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorMetadata.kt [40] lastRunContext = sin.readMap()?.toMutableMap() ?: mutableMapOf(), -sourceToQueryIndexMapping = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as MutableMap<String, String> +sourceToQueryIndexMapping = sin.readMap() + ?.mapValuesTo(HashMap()) { it.value as String } + ?: mutableMapOf() Suggestion importance[1-10]: 5 __ Why: Valid concern about unchecked casts causing deferred ClassCastException. The improvement is reasonable but the pre-existing code also had this pattern, so it's a minor code-quality improvement rather than a critical bug fix.	Low
	Safely copy trigger results into a mutable map The non-empty branch returns a HashMap wrapping a cast of raw, but readMap(..., readFrom) returns a Map<String, TriggerRunResult> whose values are the base type. The unchecked cast to Map<String, DocumentLevelTriggerRunResult> is unsafe and may fail at value-access time in some JVMs; more importantly, mapping the values explicitly avoids relying on ClassCastException-deferred casts. Build the result by iterating and casting each entry, or return a defensive copy that is always mutable regardless of size. src/main/kotlin/org/opensearch/commons/alerting/action/DocLevelMonitorFanOutResponse.kt [58-64] -private fun readTriggerResults(sin: StreamInput): Map<String, DocumentLevelTriggerRunResult> { +private fun readTriggerResults(sin: StreamInput): MutableMap<String, DocumentLevelTriggerRunResult> { val raw = sin.readMap(StreamInput::readString, DocumentLevelTriggerRunResult::readFrom) - if (raw.isEmpty()) return mutableMapOf() - // readFrom returns TriggerRunResult (base type) but always constructs DocumentLevelTriggerRunResult - @Suppress("UNCHECKED_CAST") - return HashMap(raw as Map<String, DocumentLevelTriggerRunResult>) + val result = HashMap<String, DocumentLevelTriggerRunResult>(raw.size) + for ((k, v) in raw) { + result[k] = v as DocumentLevelTriggerRunResult + } + return result } Suggestion importance[1-10]: 4 __ Why: The suggestion is a minor style improvement; the original code's unchecked cast is functionally equivalent since readFrom already constructs DocumentLevelTriggerRunResult instances. The proposed change is slightly more defensive but not critical.	Low

Suggestions up to commit e518795

Category	Suggestion	Impact
Possible issue	Avoid unchecked cast on typed map values Casting a MutableMap<String, Any?> to MutableMap<String, String> is an unchecked cast that can cause ClassCastException at value-access time if any value is not a String. Since readMap() returns Any? values, use mapValuesTo (or explicit conversion) to safely produce a MutableMap<String, String> and fail early on unexpected values. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorMetadata.kt [40] -sourceToQueryIndexMapping = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as MutableMap<String, String> +sourceToQueryIndexMapping = sin.readMap()?.mapValuesTo(mutableMapOf()) { it.value as String } ?: mutableMapOf() Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that the unchecked cast to MutableMap<String, String> could defer ClassCastException to access time. Using mapValuesTo provides earlier failure but is a minor safety/style improvement, not a critical bug fix.	Low
General	Always wrap deserialized map for consistency The empty-map branch returns mutableMapOf() while the non-empty branch returns HashMap(...); both should consistently return a mutable map. More importantly, if readMap ever returns an immutable empty map on the non-empty path in some edge case, wrapping in HashMap is only done conditionally. Simplify by always wrapping in a HashMap to guarantee mutability and avoid the branch, and note the raw generic type also loses value-type checking. src/main/kotlin/org/opensearch/commons/alerting/action/DocLevelMonitorFanOutResponse.kt [58-64] private fun readTriggerResults(sin: StreamInput): Map<String, DocumentLevelTriggerRunResult> { val raw = sin.readMap(StreamInput::readString, DocumentLevelTriggerRunResult::readFrom) - if (raw.isEmpty()) return mutableMapOf() - // readFrom returns TriggerRunResult (base type) but always constructs DocumentLevelTriggerRunResult @Suppress("UNCHECKED_CAST") return HashMap(raw as Map<String, DocumentLevelTriggerRunResult>) } Suggestion importance[1-10]: 3 __ Why: The simplification is minor; HashMap of an empty map already yields a mutable empty map, so removing the branch is a stylistic cleanup with negligible functional impact. """ </details></details></td><td align=center>Low </td></tr></tr></tbody></table> </details> <details><summary>Suggestions up to commit 02a9faf</summary> <br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Use typed readMap for Writeable values</summary> ___ **Using the untyped <code>sin.readMap()</code> for a map whose values are <br><code>ChainedAlertTriggerRunResult</code> will not correctly deserialize the value objects — <br><code>readMap()</code> reads generic values, not Writeable objects. This means the cast will fail <br>at runtime the moment the map contains any entries. Use the typed overload <br><code>sin.readMap(StreamInput::readString, ::ChainedAlertTriggerRunResult)</code> to properly <br>deserialize entries.** [src/main/kotlin/org/opensearch/commons/alerting/model/WorkflowRunResult.kt [38]](https://github.com/opensearch-project/common-utils/pull/968/files#diff-e97cc9df46b267ae869138da97b3e8f1ced7943dd508862c29a5969403ab3258R38-R38) ```diff -triggerResults = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as Map<String, ChainedAlertTriggerRunResult> +triggerResults = sin.readMap(StreamInput::readString, ::ChainedAlertTriggerRunResult) + ?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 7 __ Why: Valid concern: the untyped readMap() cannot properly reconstruct ChainedAlertTriggerRunResult Writeable instances, and the cast will fail for non-empty maps. However, this issue predates this PR and the writer side uses the same untyped approach.	Medium
Deserialize trigger results with typed reader readMapAsMutableMap() uses the untyped readMap(), which does not know how to reconstruct TriggerResult subclass instances from the stream. Any non-empty map will produce values of the wrong type and the unchecked cast will defer the failure to first use. Consider deserializing trigger results using a typed reader or restructuring serialization to use writeMap/readMap with the proper Writeable readers. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [40] -sin.readMapAsMutableMap() as Map<String, TriggerResult> // triggerResults +sin.readMap(StreamInput::readString) { TriggerRunResult.readFrom(it) as TriggerResult } + ?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 7 __ Why: Correct observation that untyped readMap() cannot reconstruct TriggerResult subclasses, potentially causing runtime failures with non-empty maps. This existed prior to the PR but the suggestion highlights a real correctness gap.	Medium
General	Avoid unchecked cast for typed string map Casting MutableMap<String, Any> to MutableMap<String, String> via an unchecked cast is unsafe and can hide runtime issues if non-String values are ever present. Consider explicitly filtering/converting values to String, or reading with a typed reader such as sin.readMap(StreamInput::readString, StreamInput::readString) to ensure type safety. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorMetadata.kt [39-40] lastRunContext = sin.readMap()?.toMutableMap() ?: mutableMapOf(), -sourceToQueryIndexMapping = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as MutableMap<String, String> +sourceToQueryIndexMapping = sin.readMap(StreamInput::readString, StreamInput::readString) + ?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 6 __ Why: Using a typed readMap(readString, readString) is safer than the unchecked cast to MutableMap<String, String>, avoiding potential ClassCastException issues, though the original writer path likely writes strings.	Low
	Safely copy trigger results into mutable map The HashMap(raw as Map<...>) copy is only executed for non-empty maps; when raw is non-empty but is an immutable map returned by readMap, the cast succeeds but callers that mutate the result will still work because of the copy. However, if readMap returns a mutable non-HashMap, the extra copy is unnecessary. More importantly, the cast may fail at runtime if the erased generic type check is enforced; using mapValues { it.value as DocumentLevelTriggerRunResult } into a new HashMap is safer and avoids relying on erasure. src/main/kotlin/org/opensearch/commons/alerting/action/DocLevelMonitorFanOutResponse.kt [58-64] -private fun readTriggerResults(sin: StreamInput): Map<String, DocumentLevelTriggerRunResult> { +private fun readTriggerResults(sin: StreamInput): MutableMap<String, DocumentLevelTriggerRunResult> { val raw = sin.readMap(StreamInput::readString, DocumentLevelTriggerRunResult::readFrom) - if (raw.isEmpty()) return mutableMapOf() - // readFrom returns TriggerRunResult (base type) but always constructs DocumentLevelTriggerRunResult - @Suppress("UNCHECKED_CAST") - return HashMap(raw as Map<String, DocumentLevelTriggerRunResult>) + val result = HashMap<String, DocumentLevelTriggerRunResult>(raw.size) + raw.forEach { (k, v) -> result[k] = v as DocumentLevelTriggerRunResult } + return result } Suggestion importance[1-10]: 3 __ Why: The existing code already handles the empty-map case and copies to a HashMap. The suggested change is a minor stylistic improvement with negligible practical impact since readFrom already constructs DocumentLevelTriggerRunResult.	Low

Suggestions up to commit e518795

Category	Suggestion	Impact
Possible issue	Unsafe cast to typed trigger results The cast to Map<String, ChainedAlertTriggerRunResult> is unsafe and will fail at runtime since readMap() returns a map with generic values, not typed ChainedAlertTriggerRunResult instances. Use a typed read method or validate entries before casting. src/main/kotlin/org/opensearch/commons/alerting/model/WorkflowRunResult.kt [38] -triggerResults = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as Map<String, ChainedAlertTriggerRunResult> +triggerResults = sin.readMap(StreamInput::readString, ChainedAlertTriggerRunResult::readFrom)?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 9 __ Why: The cast to Map<String, ChainedAlertTriggerRunResult> is unsafe because readMap() returns untyped values. This will cause ClassCastException when accessing the map entries, making it a critical bug that needs fixing.	High
	Unsafe cast to nested typed map The cast to nested MutableMap<String, MutableMap<String, ActionRunResult>> is unsafe. The readMapAsMutableMap() returns MutableMap<String, Any>, and casting to a nested typed structure will fail at runtime when accessing nested values. src/main/kotlin/org/opensearch/commons/alerting/model/BucketLevelTriggerRunResult.kt [28] -sin.readMapAsMutableMap() as MutableMap<String, MutableMap<String, ActionRunResult>> +sin.readMap(StreamInput::readString) { s -> + s.readMap(StreamInput::readString, ActionRunResult::readFrom) +}?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 9 __ Why: The cast to nested MutableMap<String, MutableMap<String, ActionRunResult>> is unsafe. The readMapAsMutableMap() returns MutableMap<String, Any>, and this cast will fail when accessing nested values, causing runtime errors.	High
	Unsafe cast to typed trigger result The cast to Map<String, TriggerResult> is unsafe since readMapAsMutableMap() returns MutableMap<String, Any>. The values are not typed as TriggerResult instances and will cause ClassCastException when accessed. Use a typed read method instead. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [40] -sin.readMapAsMutableMap() as Map<String, TriggerResult> +sin.readMap(StreamInput::readString) { s -> + TriggerRunResult.readFrom(s) as TriggerResult +} ?: mutableMapOf() Suggestion importance[1-10]: 9 __ Why: The cast to Map<String, TriggerResult> is unsafe since readMapAsMutableMap() returns MutableMap<String, Any>. This will cause ClassCastException when accessing map values, representing a critical type safety issue.	High
	Unsafe cast to MutableMap The cast to MutableMap<String, String> is unsafe and can fail at runtime if the deserialized map contains non-String values. Consider validating the map entries before casting or using a safer conversion method that filters entries by type. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorMetadata.kt [40] -sourceToQueryIndexMapping = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as MutableMap<String, String> +sourceToQueryIndexMapping = sin.readMap()?.mapNotNull { (k, v) -> + if (k is String && v is String) k to v else null +}?.toMap()?.toMutableMap() ?: mutableMapOf() Suggestion importance[1-10]: 8 __ Why: The unsafe cast to MutableMap<String, String> can fail at runtime if the deserialized map contains non-String values. This is a critical type safety issue that could cause ClassCastException in production.	Medium

[thecodingshrimp]

Thanks for the automated review. Addressing each flagged item (via claude):

Unsafe cast: `WorkflowRunResult.kt` → `Map<String, ChainedAlertTriggerRunResult>`

The bot suggested switching to the typed `readMap(StreamInput::readString, ChainedAlertTriggerRunResult::readFrom)` overload to avoid the unchecked cast.

This is not viable without also changing the write side — the existing `out.writeMap(triggerResults)` uses the generic (type-tagged) wire format, while the typed `readMap` overload expects the type-free format written by `out.writeMap(map, keyWriter, valueWriter)`. Changing both sides in the same PR would introduce a wire-format break for rolling upgrades. The wire format for this map was established in its original introduction; preserving it is necessary.

What I've done instead (commit 9e0e029): added the missing `@Suppress("UNCHECKED_CAST")` on the constructor, which was inadvertently omitted. The cast is safe because the values were serialized as `ChainedAlertTriggerRunResult` instances by `writeTo()`.

Unsafe cast: `MonitorMetadata.kt` → `MutableMap<String, String>`

The suggestion to add per-entry type validation via `as? String ?: throw IllegalStateException` is over-defensive. The data at this position was always written by `MonitorMetadata.writeTo()` which casts `sourceToQueryIndexMapping: MutableMap<String, String>` down to `MutableMap<String, Any>` for the generic writer. The types are guaranteed by the write side. Adding entry-level type assertions would obscure a straightforward deserialization with complexity that adds no real value.

Unsafe cast: `DocLevelMonitorFanOutResponse.kt` → `Map<String, DocumentLevelTriggerRunResult>`

The suggestion to replace the cast with `.mapValues { it.value as? DocumentLevelTriggerRunResult ?: throw ... }` is redundant: `readMap(StreamInput::readString, DocumentLevelTriggerRunResult::readFrom)` already guarantees each value was deserialized via `DocumentLevelTriggerRunResult::readFrom`. A subsequent runtime check on what the typed deserializer just produced adds only overhead.

In summary: the flagged casts are all inherently unchecked due to JVM type erasure, but each is provably safe given the matching write side. The only actionable item was the missing `@Suppress` annotation in `WorkflowRunResult`.

[github-actions]

Persistent review updated to latest commit 9e0e029

[thecodingshrimp]

Added regression tests in (commit 504f10c) covering all six fixed call sites. Each test exercises the empty-map path: serializes via writeTo(), deserializes via the StreamInput constructor, and asserts the round-trip succeeds and the result map is mutable.

[github-actions]

Persistent review updated to latest commit 504f10c

[andrross]

@thecodingshrimp What do you think about using kotlin extensions here? Something like:

package org.opensearch.commons.alerting.util

import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.Writeable

/**
 * Reads a map written by `StreamOutput.writeMap` and always returns a mutable map.
 *
 * Why this exists:
 * `StreamInput.readMap()` only guarantees the result implements `java.util.Map`. Its
 * Javadoc states that a non-empty result will be mutable, but an empty result *might*
 * be immutable (`Collections.emptyMap()`). This extension guarantees the returned
 * map is mutable.
 */
@Suppress("UNCHECKED_CAST")
fun StreamInput.readMapAsMutableMap(): MutableMap<String, Any> {
    val map = this.readMap() ?: return mutableMapOf()
    return if (map is MutableMap<*, *>) {
        map as MutableMap<String, Any>
    } else {
        // Immutable (e.g. Collections.emptyMap()) or unknown type: copy every entry
        // into a fresh mutable map.
        LinkedHashMap(map) as MutableMap<String, Any>
    }
}

/**
 * Typed variant for maps written with explicit key/value readers
 * (`StreamOutput.writeMap(map, keyWriter, valueWriter)`).
 */
fun <K, V> StreamInput.readMapAsMutableMap(
    keyReader: Writeable.Reader<K>,
    valueReader: Writeable.Reader<V>
): MutableMap<K, V> {
    val map = this.readMap(keyReader, valueReader) ?: return mutableMapOf()
    return if (map is MutableMap<K, V>) {
        map
    } else {
        LinkedHashMap(map)
    }
}

Then all the call sites that need a mutable map from StreamInput can use one of these methods.

[thecodingshrimp]

Thanks for the suggestion — agreed that centralizing this in an extension is the right call. I went with a cast-free version to avoid introducing a new @Suppress:

fun StreamInput.readMapAsMutableMap(): MutableMap<String, Any> =
    readMap()?.toMutableMap() ?: mutableMapOf()

This is placed in util/StreamInputExtensions.kt and used across all the fixed call sites. The is MutableMap<*, *> branch check in your proposal avoids an unnecessary copy for non-empty maps, but toMutableMap() is safe and keeps the helper annotation-free. I also extended the fix to cover the remaining suppressWarning(sin.readMap()) calls across the TriggerRunResult family and deleted the now-dead suppressWarning helpers.

[github-actions]

Persistent review updated to latest commit 02a9faf

[github-actions]

Persistent review updated to latest commit e518795

[github-actions]

Persistent review updated to latest commit e518795

[github-actions]

Persistent review updated to latest commit b41f66d

[github-actions]

Persistent review updated to latest commit a2aa8ae