Fix ClassCastException from immutable map returned by StreamInput.readMap()#968
Conversation
…dMap() StreamInput.readMap() documents that empty maps 'might be immutable'. Replace all unsafe suppressWarning() unchecked casts and raw readMap() casts with safe toMutableMap() copies in deserialization constructors across all affected alerting model classes. Fixed files: - Monitor.kt: uiMetadata deserialization - Alert.kt: queryResults deserialization - MonitorMetadata.kt: lastRunContext and sourceToQueryIndexMapping - IndexExecutionContext.kt: lastRunContext, updatedLastRunContext - DocLevelMonitorFanOutResponse.kt: lastRunContexts; remove suppressWarning helper - WorkflowRunResult.kt: triggerResults; remove suppressWarning helper This prevents the kotlin.collections.EmptyMap cannot be cast to kotlin.collections.MutableMap exception when transport-deserializing monitors with empty map fields. Fixes opensearch-project/security-analytics#1715 Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
f3545a8 to
9511928
Compare
PR Reviewer Guide 🔍(Review updated until commit 02a9faf)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 02a9faf Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit e518795
Suggestions up to commit 504f10c
Suggestions up to commit 9e0e029
Suggestions up to commit 9511928
|
The constructor-level @Suppress("UNCHECKED_CAST") was missing even though the deserialization expression contains an explicit unchecked cast of a Map<String, TriggerRunResult> to Map<String, ChainedAlertTriggerRunResult>. Kotlin requires the annotation to acknowledge the cast is intentionally unsafe; omitting it produces a compiler warning. The cast is safe in practice because all values were written as ChainedAlertTriggerRunResult instances by writeTo(). Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
|
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`. |
|
Persistent review updated to latest commit 9e0e029 |
…tion StreamInput.readMap() may return Collections.emptyMap() (immutable) for zero-size maps. Before this fix, several deserialization constructors assumed the result was always mutable, causing ClassCastException or UnsupportedOperationException at runtime. Tests cover the empty-map (regression) path for all six fixed call sites: - Monitor.uiMetadata - MonitorMetadata.lastRunContext + sourceToQueryIndexMapping - IndexExecutionContext.lastRunContext + updatedLastRunContext - DocLevelMonitorFanOutResponse.lastRunContexts - WorkflowRunResult.triggerResults - Alert.queryResults (each element map) Each test serializes an object with an empty map via writeTo(), deserializes via the StreamInput constructor, and asserts the result is correct and the map is mutable. Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
|
Added regression tests in (commit 504f10c) covering all six fixed call sites. Each test exercises the empty-map path: serializes via |
|
Persistent review updated to latest commit 504f10c |
| executionId = sin.readString(), | ||
| monitorId = sin.readString(), | ||
| lastRunContexts = sin.readMap()!! as MutableMap<String, Any>, | ||
| lastRunContexts = sin.readMap()?.toMutableMap() ?: mutableMapOf(), |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
- Add StreamInput.readMapAsMutableMap() extension (cast-free, no @Suppress) - Replace suppressWarning(sin.readMap()) in MonitorRunResult.kt with extension - Narrow @Suppress("UNCHECKED_CAST") to specific cast line in DocLevelMonitorFanOutResponse.kt - Add regression tests for MonitorRunResult/InputRunResults empty-map paths
StreamInput.readMap(Reader, Reader) returns Collections.emptyMap() for zero-size maps, same as the untyped overload. Replace with an explicit empty-check that returns mutableMapOf(), and wrap non-empty results in HashMap to guarantee mutability. The @Suppress("UNCHECKED_CAST") is retained narrowly on the cast from TriggerRunResult to DocumentLevelTriggerRunResult, which is unavoidable because readFrom declares TriggerRunResult as its return type. Add regression test for the empty triggerResults path.
- Fix QueryLevelTriggerRunResult, ClusterMetricsTriggerRunResult, ChainedAlertTriggerRunResult, BucketLevelTriggerRunResult: replace sin.readMap() as MutableMap cast with sin.readMapAsMutableMap() - Fix ActionRunResult.output: replace suppressWarning(sin.readMap()) with sin.readMapAsMutableMap(); delete now-unused suppressWarning helper - Delete dead suppressWarning helpers in TriggerRunResult and Workflow - Add regression tests for all fixed deserialization paths
|
Persistent review updated to latest commit 02a9faf |
2 participants
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 aMutableMap, causing aClassCastExceptionat runtime when a monitor with emptyuiMetadata,lastRunContext, orqueryResultsis deserialized over transport.Exception:
Root cause
StreamOutput.writeMap/StreamInput.readMapguarantee only that the result implementsjava.util.Map. The Javadoc states:The
suppressWarning()helper performsmap as MutableMap<String, Any>without a defensive copy, which fails when the deserialized map isCollections.emptyMap().Identified via opensearch-project/security-analytics#1722 —
TransportIndexDetectorActionpassesMap.of()asuiMetadatawhen constructing aMonitor, which serializes as a zero-size map,readMap()returnsCollections.emptyMap(), and the cast fails.Changes
Replace
suppressWarning(sin.readMap())withsin.readMap()?.toMutableMap() ?: mutableMapOf()at all three callsites and remove thesuppressWarning()helper function entirely.Monitor.ktuiMetadatadeserialization — safe copyMonitorMetadata.ktlastRunContextdeserialization — safe copyAlert.ktqueryResultsdeserialization — safe copy; removesuppressWarningimportTesting
All existing Monitor-related tests pass (
./gradlew test --tests "*Monitor*"→ BUILD SUCCESSFUL).Check List
Related