Skip to content

fix: guard against EmptyMap cast in doc-level monitor recreateRunContext and mapping traversal - #2221

Open
thecodingshrimp wants to merge 3 commits into
opensearch-project:mainfrom
thecodingshrimp:fix/doc-level-monitor-mutablemap-guards
Open

thecodingshrimp wants to merge 3 commits into
opensearch-project:mainfrom
thecodingshrimp:fix/doc-level-monitor-mutablemap-guards

Conversation

@thecodingshrimp

Copy link
Copy Markdown

Summary

This PR adds defence-in-depth guards against ClassCastException / UnsupportedOperationException
caused by immutable empty maps being cast to MutableMap in the doc-level monitor subsystem.

Closes: #2220
Related: opensearch-project/common-utils#967 (root-cause fix belongs there)


Root cause

MonitorMetadata.lastRunContext defaults to kotlin.collections.EmptyMap (the immutable
singleton returned by mapOf()) when deserialized from .opensearch-alerting-config and
the stored document has no last_run_context field (monitor never ran, or created under
an older plugin version).

MonitorMetadataService.recreateRunContext() line 210 casts this directly to
MutableMap<String, MutableMap<String, Any>>. EmptyMap is not a MutableMap, so the
JVM throws ClassCastException on any PUT update to such a monitor.

The correct fix — changing mapOf() to mutableMapOf() in MonitorMetadata.kt — is
tracked in opensearch-project/common-utils#967. This PR adds a guard at the cast site so
that the crash cannot occur regardless of which common-utils version is deployed.


Changes

Fix A — MonitorMetadataService.kt (critical)

Guard the cast at line 210: if lastRunContext.isEmpty(), pass null to
createFullRunContext() (which accepts null and builds a fresh context).

Fix B — DocLevelMonitorQueries.kt (plausible crash path)

traverseMappingsAndUpdate() casts values from indexMetadata.mapping()?.sourceAsMap
directly to MutableMap. Java's Collections.emptyMap() (returned for empty nested
objects {}) is not mutable; .put() / .remove() would throw
UnsupportedOperationException. Replace bare casts with .toMutableMap() defensive
copies at the three cast sites inside traverseMappingsAndUpdate().

Fix C — MonitorFanOutUtils.kt / DocumentLevelMonitorRunner.kt (contract)

initializeNewLastRunContext() always returns a MutableMap but its return type is
declared as Map. Update the declaration and remove the now-unnecessary as MutableMap
cast at the call site in DocumentLevelMonitorRunner.kt and
RemoteDocumentLevelMonitorRunner.kt.


Files changed

  • alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt
  • alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt
  • alerting/src/main/kotlin/org/opensearch/alerting/MonitorFanOutUtils.kt
  • alerting/src/main/kotlin/org/opensearch/alerting/DocumentLevelMonitorRunner.kt
  • alerting/src/main/kotlin/org/opensearch/alerting/remote/monitors/RemoteDocumentLevelMonitorRunner.kt

Testing

  • All existing unit and integration tests pass (./gradlew test)
  • Manual verification: create a doc-level monitor, delete its metadata entry from
    .opensearch-alerting-config, then PUT an update to the monitor — no
    ClassCastException thrown
  • Doc-level monitor mapping traversal with an index containing empty nested objects
    ({}) does not throw UnsupportedOperationException

@github-actions

github-actions Bot commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit f7425d9)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Guard DocLevelMonitorQueries mapping traversal against immutable maps

Relevant files:

  • alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt
  • alerting/src/test/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueriesTraversalTests.kt

Sub-PR theme: Guard lastRunContext casts in doc-level monitor runners and metadata service

Relevant files:

  • alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt
  • alerting/src/main/kotlin/org/opensearch/alerting/DocumentLevelMonitorRunner.kt
  • alerting/src/main/kotlin/org/opensearch/alerting/remote/monitors/RemoteDocumentLevelMonitorRunner.kt

Sub-PR theme: Tighten initializeNewLastRunContext return type to MutableMap

Relevant files:

  • alerting/src/main/kotlin/org/opensearch/alerting/MonitorFanOutUtils.kt
  • alerting/src/test/kotlin/org/opensearch/alerting/MonitorFanOutUtilsTests.kt

⚡ Recommended focus areas for review

Behavior Change

traverseMappingsAndUpdate now operates on .toMutableMap() defensive copies rather than the original backing maps. Previously, when the leaf processor returned the same MutableMap instance it received (or mutated it in place), those changes were visible in the parent node because the cast was by-reference. Now, since each call receives a copy, in-place mutations by processLeafFn on the passed props are lost unless the function returns the modified map, which newNodes.forEach then writes back via node[newName]. If any existing processLeafFn implementation relies on in-place mutation of the props map while returning a different map (or ignoring the returned props), the leaf's property updates will be silently dropped. Verify all callers of traverseMappingsAndUpdate return the mutated props as the third element of the Triple.

val nodeProps = (it.value as Map<String, Any>).toMutableMap()
// If it has type property and type is not "nested" then this is a leaf
if (nodeProps.containsKey(TYPE) && nodeProps[TYPE] != NESTED) {
    // At this point we know full path of node, so we add it to output array
    flattenPaths.put(fullPath, nodeProps)
    // Calls processLeafFn and gets old node name, new node name and new properties of node.
    // This is all information we need to update this node
    // Reuse nodeProps rather than creating a second independent copy from it.value.
    val (oldName, newName, props) = processLeafFn(it.key, fullPath, nodeProps)
    newNodes.add(Triple(oldName, newName, props))
} else if (nodeProps.containsKey(PROPERTIES) && nodeProps[PROPERTIES] != null) {
    // Internal(non-leaf) node - visit children.
    // Copy nodeProps[PROPERTIES] into childProps, recurse on childProps, then write
    // childProps back into nodeProps[PROPERTIES] so the parent node sees updated children.
    @Suppress("UNCHECKED_CAST")
    val childProps = (nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap()
    traverseMappingsAndUpdate(childProps, fullPath, processLeafFn, flattenPaths)
    // Write the mutated child properties back so the parent node sees the updates.
    nodeProps[PROPERTIES] = childProps
    newNodes.add(Triple(it.key, it.key, nodeProps as Any))
Silent Data Loss

The new filter .filter { it.value is Map<*, *> } silently drops any lastRunContext entries whose value is not a Map. If a legitimate per-index entry is temporarily shaped differently (e.g. due to an in-progress migration or an unexpected serialization), it will be omitted from lastRunContext without any log line, causing the monitor to re-process documents from scratch for that index. Consider logging a warning when filtering out such entries so the condition is observable in production.

// Safe reconstruction: filter non-Map values to avoid ClassCastException on stale or malformed entries.
@Suppress("UNCHECKED_CAST")
val lastRunContext: MutableMap<String, MutableMap<String, Any>> = if (monitorMetadata.lastRunContext.isNullOrEmpty()) mutableMapOf()
else monitorMetadata.lastRunContext
    .entries
    .filter { it.value is Map<*, *> }
    .associate { it.key to (it.value as Map<String, Any>).toMutableMap() }
    .toMutableMap()

@github-actions

github-actions Bot commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to f7425d9

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Verify internal-node update propagation semantics

For internal (non-leaf) nodes, the code now appends Triple(it.key, it.key,
nodeProps) to newNodes. Downstream, newNodes.forEach likely removes the old key and
re-adds under the (same) new key, which for internal nodes effectively replaces the
entry — but if the downstream loop only re-inserts leaf nodes, this change may now
overwrite internal nodes unnecessarily. Verify the downstream handling of newNodes
correctly distinguishes leaf renames from internal-node updates, or restrict adding
to newNodes only when children were actually modified.

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt [241-251]

 } else if (nodeProps.containsKey(PROPERTIES) && nodeProps[PROPERTIES] != null) {
-    // Internal(non-leaf) node - visit children.
-    // Copy nodeProps[PROPERTIES] into childProps, recurse on childProps, then write
-    // childProps back into nodeProps[PROPERTIES] so the parent node sees updated children.
     @Suppress("UNCHECKED_CAST")
     val childProps = (nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap()
     traverseMappingsAndUpdate(childProps, fullPath, processLeafFn, flattenPaths)
-    // Write the mutated child properties back so the parent node sees the updates.
     nodeProps[PROPERTIES] = childProps
-    newNodes.add(Triple(it.key, it.key, nodeProps as Any))
+    // Only propagate internal-node updates if the parent map was a copy (not the original).
+    node[it.key] = nodeProps
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a legitimate concern about whether adding internal nodes to newNodes changes downstream behavior, since previously only leaf renames were added. However, the proposed improved_code directly mutates node during iteration which could cause ConcurrentModificationException, so the fix is questionable.

Low
General
Log when malformed entries are filtered out

The previous code passed metadata.lastRunContext.toMutableMap() directly to
createFullRunContext, which treated an empty map as "no prior context". The new code
calls lastRunCtx.ifEmpty { null }, but this also converts a lastRunContext whose
entries were all filtered out (because none of them were Maps) into null, silently
discarding malformed state and potentially triggering a full re-scan. Consider
logging a warning when entries are filtered to aid diagnosis.

alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt [213-225]

 val runContext = if (monitor.monitorType.endsWith(Monitor.MonitorType.DOC_LEVEL_MONITOR.value)) {
-    // Safe reconstruction: filter out any entries whose value is not a Map (e.g. stale scalar entries
-    // from prior migrations or XContent parse paths that produce non-MutableMap values), then copy
-    // each surviving value into a fresh MutableMap. This avoids ClassCastException on non-empty but
-    // wrongly-shaped lastRunContext.
     @Suppress("UNCHECKED_CAST")
     val lastRunCtx: MutableMap<String, MutableMap<String, Any>> = metadata.lastRunContext
         .entries
         .filter { it.value is Map<*, *> }
         .associate { it.key to (it.value as Map<String, Any>).toMutableMap() }
         .toMutableMap()
+    if (lastRunCtx.size != metadata.lastRunContext.size) {
+        log.warn("Filtered ${metadata.lastRunContext.size - lastRunCtx.size} non-Map entries from lastRunContext for monitor ${monitor.id}")
+    }
     createFullRunContext(monitorIndex, lastRunCtx.ifEmpty { null })
 } else null
Suggestion importance[1-10]: 4

__

Why: Adding a warning log when entries are filtered is a reasonable observability improvement for diagnosing malformed state, but it is a minor enhancement, not addressing a correctness issue.

Low

Previous suggestions

Suggestions up to commit f7425d9
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid treating internal nodes as leaves

Adding internal (non-leaf) nodes to newNodes is a behavior change from the prior
code which only added leaf nodes. If the subsequent newNodes.forEach block (not
shown) performs rename/replace operations assuming leaves, it may now overwrite
internal nodes incorrectly or duplicate them. Confirm the downstream loop handles
the oldName == newName identity case for internal nodes safely, or avoid adding
internal nodes to newNodes entirely since nodeProps[PROPERTIES] = childProps already
mutates in place.

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt [241-250]

 } else if (nodeProps.containsKey(PROPERTIES) && nodeProps[PROPERTIES] != null) {
-    // Internal(non-leaf) node - visit children.
-    // Copy nodeProps[PROPERTIES] into childProps, recurse on childProps, then write
-    // childProps back into nodeProps[PROPERTIES] so the parent node sees updated children.
     @Suppress("UNCHECKED_CAST")
     val childProps = (nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap()
     traverseMappingsAndUpdate(childProps, fullPath, processLeafFn, flattenPaths)
-    // Write the mutated child properties back so the parent node sees the updates.
     nodeProps[PROPERTIES] = childProps
-    newNodes.add(Triple(it.key, it.key, nodeProps as Any))
+    // Replace the original (possibly unmodifiable) node value with the mutable copy
+    // so parent-level mutations are visible, but do not treat it as a renamed leaf.
+    node[it.key] = nodeProps
 }
Suggestion importance[1-10]: 7

__

Why: This is a legitimate concern - the original code did not add internal nodes to newNodes, but the PR now adds Triple(it.key, it.key, nodeProps as Any). This behavior change could affect how the downstream rename loop processes the tree, potentially introducing bugs.

Medium
General
Verify filtering preserves required context entries

Previously, when metadata.lastRunContext was non-empty, the full map (including
non-Map scalar entries like "index" or "shards_count") was passed through. The new
filter silently drops those keys, which may cause createFullRunContext to lose
per-index metadata that previously existed. Preserve non-Map entries in a way that
createFullRunContext expects, or confirm that only Map-valued entries are consumed
downstream.

alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt [213-225]

 val runContext = if (monitor.monitorType.endsWith(Monitor.MonitorType.DOC_LEVEL_MONITOR.value)) {
-    // Safe reconstruction: filter out any entries whose value is not a Map (e.g. stale scalar entries
-    // from prior migrations or XContent parse paths that produce non-MutableMap values), then copy
-    // each surviving value into a fresh MutableMap. This avoids ClassCastException on non-empty but
-    // wrongly-shaped lastRunContext.
     @Suppress("UNCHECKED_CAST")
     val lastRunCtx: MutableMap<String, MutableMap<String, Any>> = metadata.lastRunContext
         .entries
         .filter { it.value is Map<*, *> }
         .associate { it.key to (it.value as Map<String, Any>).toMutableMap() }
         .toMutableMap()
     createFullRunContext(monitorIndex, lastRunCtx.ifEmpty { null })
 } else null
Suggestion importance[1-10]: 5

__

Why: The concern is valid - filtering may silently drop non-Map entries that previously flowed through. However, the type signature MutableMap<String, MutableMap<String, Any>> implies only Map values are expected, so the risk may be limited. The suggestion is largely a verification request.

Low
Log when discarding malformed context entries

Silently filtering out non-Map entries from lastRunContext may hide corrupted or
legacy metadata that previously would have surfaced as an exception. Consider
logging a warning when entries are filtered so operators can detect and investigate
malformed metadata rather than running with silently truncated state (which could
re-process documents).

alerting/src/main/kotlin/org/opensearch/alerting/DocumentLevelMonitorRunner.kt [108-114]

 @Suppress("UNCHECKED_CAST")
 val lastRunContext: MutableMap<String, MutableMap<String, Any>> = if (monitorMetadata.lastRunContext.isNullOrEmpty()) mutableMapOf()
-else monitorMetadata.lastRunContext
-    .entries
-    .filter { it.value is Map<*, *> }
-    .associate { it.key to (it.value as Map<String, Any>).toMutableMap() }
-    .toMutableMap()
+else {
+    val (valid, invalid) = monitorMetadata.lastRunContext.entries.partition { it.value is Map<*, *> }
+    if (invalid.isNotEmpty()) {
+        logger.warn("Filtered ${invalid.size} non-Map entries from lastRunContext: ${invalid.map { it.key }}")
+    }
+    valid.associate { it.key to (it.value as Map<String, Any>).toMutableMap() }.toMutableMap()
+}
Suggestion importance[1-10]: 4

__

Why: Adding logging for filtered entries is a reasonable observability improvement to help detect malformed metadata, but it is a minor enhancement rather than fixing a critical issue.

Low
Suggestions up to commit f3a8aac
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid altering internal-node update semantics

The internal-node branch now adds a Triple(it.key, it.key, nodeProps) to newNodes,
which the subsequent loop treats as a rename/update and will likely replace the
original internal node entry with the copied nodeProps. Previously internal nodes
were not added to newNodes at all. This changes the semantics: the original backing
map reference for internal nodes is replaced by the copy, potentially discarding
mutations expected elsewhere. Consider either mutating node[it.key] in place or
ensuring newNodes processing handles internal nodes identically.

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt [241-251]

 } else if (nodeProps.containsKey(PROPERTIES) && nodeProps[PROPERTIES] != null) {
-    // Internal(non-leaf) node - visit children.
-    // Copy nodeProps[PROPERTIES] into childProps, recurse on childProps, then write
-    // childProps back into nodeProps[PROPERTIES] so the parent node sees updated children.
     @Suppress("UNCHECKED_CAST")
     val childProps = (nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap()
     traverseMappingsAndUpdate(childProps, fullPath, processLeafFn, flattenPaths)
-    // Write the mutated child properties back so the parent node sees the updates.
     nodeProps[PROPERTIES] = childProps
-    newNodes.add(Triple(it.key, it.key, nodeProps as Any))
+    // Replace the original (possibly unmodifiable) child map with our mutable copy
+    node[it.key] = nodeProps
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a behavioral change: internal nodes are now added to newNodes which could alter downstream processing semantics compared to the previous code where internal nodes were skipped. This is a legitimate concern worth reviewing.

Medium
General
Log when malformed context entries are dropped

Previously, when metadata.lastRunContext was non-empty but all entries were filtered
out (none were Maps), the code would have thrown a ClassCastException; now
lastRunCtx becomes empty and ifEmpty { null } passes null to createFullRunContext,
silently discarding the stale context. If silent discard is intended, this is fine,
but consider logging a warning when entries are filtered so operators can detect
corrupted metadata.

alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt [214-220]

 @Suppress("UNCHECKED_CAST")
-val lastRunCtx: MutableMap<String, MutableMap<String, Any>> = metadata.lastRunContext
-    .entries
-    .filter { it.value is Map<*, *> }
+val filtered = metadata.lastRunContext.entries.filter { it.value is Map<*, *> }
+if (filtered.size != metadata.lastRunContext.size) {
+    log.warn("Dropping ${metadata.lastRunContext.size - filtered.size} malformed lastRunContext entries for monitor ${monitor.id}")
+}
+val lastRunCtx: MutableMap<String, MutableMap<String, Any>> = filtered
     .associate { it.key to (it.value as Map<String, Any>).toMutableMap() }
     .toMutableMap()
 createFullRunContext(monitorIndex, lastRunCtx.ifEmpty { null })
Suggestion importance[1-10]: 4

__

Why: Adding a warning log for silently dropped malformed entries is a reasonable observability improvement, but it is a minor enhancement rather than a critical fix.

Low
Suggestions up to commit 276f481
CategorySuggestion                                                                                                                                    Impact
General
Avoid redundant map casts and copies

The it.value as Map<String, Any> cast is repeated three times, causing redundant
work and risking inconsistency if the value changes. Reuse the already-computed
nodeProps for both processLeafFn and the recursive traversal to avoid re-casting and
re-copying.

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt [231-242]

 val nodeProps = (it.value as Map<String, Any>).toMutableMap()
 // If it has type property and type is not "nested" then this is a leaf
 if (nodeProps.containsKey(TYPE) && nodeProps[TYPE] != NESTED) {
     // At this point we know full path of node, so we add it to output array
     flattenPaths.put(fullPath, nodeProps)
     // Calls processLeafFn and gets old node name, new node name and new properties of node.
     // This is all information we need to update this node
-    val (oldName, newName, props) = processLeafFn(it.key, fullPath, (it.value as Map<String, Any>).toMutableMap())
+    val (oldName, newName, props) = processLeafFn(it.key, fullPath, nodeProps)
     newNodes.add(Triple(oldName, newName, props))
 } else if (nodeProps.containsKey(PROPERTIES) && nodeProps[PROPERTIES] != null) {
     // Internal(non-leaf) node - visit children
     traverseMappingsAndUpdate((nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap(), fullPath, processLeafFn, flattenPaths)
 }
Suggestion importance[1-10]: 6

__

Why: Reusing the already computed nodeProps avoids redundant casts and copies, improving both efficiency and consistency. This is a valid minor improvement.

Low
Possible issue
Defensively convert map instead of casting

Casting metadata.lastRunContext directly to MutableMap<String, MutableMap<String,
Any>> will still fail with ClassCastException if the underlying map (or its inner
values) is an immutable/EmptyMap-typed instance even when non-empty. Convert to a
mutable map defensively via toMutableMap() with mutable inner maps to fully guard
against the cast issue.

alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt [209-214]

 val runContext = if (monitor.monitorType.endsWith(Monitor.MonitorType.DOC_LEVEL_MONITOR.value)) {
     @Suppress("UNCHECKED_CAST")
     val lastRunCtx = if (metadata.lastRunContext.isEmpty()) null
-    else (metadata.lastRunContext as MutableMap<String, MutableMap<String, Any>>)
+    else metadata.lastRunContext.mapValues {
+        (it.value as Map<String, Any>).toMutableMap()
+    }.toMutableMap()
     createFullRunContext(monitorIndex, lastRunCtx)
 } else null
Suggestion importance[1-10]: 5

__

Why: The suggestion addresses a real concern: the unchecked cast could still fail if the underlying map is immutable. However, whether this matters depends on how the map was originally constructed, so the impact is moderate.

Low
Suggestions up to commit ef25277
CategorySuggestion                                                                                                                                    Impact
Possible issue
Persist mutations made on copied child maps

Since traverseMappingsAndUpdate mutates the passed node map (via newNodes.forEach
updates later), creating a new mutable copy of nodeProps[PROPERTIES] means the
recursive updates will be applied to the copy and discarded, not persisted back into
the original mappings tree. Assign the copy back into nodeProps[PROPERTIES] (and
ensure nodeProps itself is written back) or defensively cast when the original is
already a MutableMap to preserve mutation semantics.

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt [240-242]

 } else if (nodeProps.containsKey(PROPERTIES) && nodeProps[PROPERTIES] != null) {
     // Internal(non-leaf) node - visit children
-    traverseMappingsAndUpdate((nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap(), fullPath, processLeafFn, flattenPaths)
+    val childProps = (nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap()
+    traverseMappingsAndUpdate(childProps, fullPath, processLeafFn, flattenPaths)
+    nodeProps[PROPERTIES] = childProps
+    @Suppress("UNCHECKED_CAST")
+    (node as MutableMap<String, Any>)[it.key] = nodeProps
 }
Suggestion importance[1-10]: 8

__

Why: Valid concern: converting nodeProps[PROPERTIES] to a new mutable map via toMutableMap() breaks the mutation semantics of the recursive traversal, since updates in newNodes.forEach on the copy won't propagate back into the original mappings tree. This could be a real correctness regression from the change.

Medium
General
Defensively copy lastRunContext to mutable maps

The cast to MutableMap<String, MutableMap<String, Any>> can still fail if
metadata.lastRunContext is a non-empty immutable map (e.g., a deserialized
Collections.unmodifiableMap or EmptyMap-like structure with entries). Consider
defensively copying into a mutable structure using toMutableMap() (and mapping inner
maps similarly) to guarantee safe mutation downstream.

alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt [209-214]

 val runContext = if (monitor.monitorType.endsWith(Monitor.MonitorType.DOC_LEVEL_MONITOR.value)) {
     @Suppress("UNCHECKED_CAST")
     val lastRunCtx = if (metadata.lastRunContext.isEmpty()) null
-    else (metadata.lastRunContext as MutableMap<String, MutableMap<String, Any>>)
+    else metadata.lastRunContext.mapValues { (it.value as Map<String, Any>).toMutableMap() }.toMutableMap()
+        as MutableMap<String, MutableMap<String, Any>>
     createFullRunContext(monitorIndex, lastRunCtx)
 } else null
Suggestion importance[1-10]: 5

__

Why: Reasonable defensive suggestion: using toMutableMap() on the outer and inner maps would avoid ClassCastException at cast time (though Kotlin's unchecked cast doesn't throw immediately). Moderate impact on robustness.

Low
Suggestions up to commit dec1828
CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve child mapping updates after copy

The recursive call now passes a copy of the children map (via toMutableMap())
instead of the original map. Any updates made by traverseMappingsAndUpdate to that
copy will no longer be reflected in the parent's nodeProps[PROPERTIES], potentially
breaking mapping updates. Consider casting via a safe unchecked cast to preserve the
same underlying map reference, or reassigning the updated copy back into
nodeProps[PROPERTIES].

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt [242]

 } else if (nodeProps.containsKey(PROPERTIES) && nodeProps[PROPERTIES] != null) {
     // Internal(non-leaf) node - visit children
-    traverseMappingsAndUpdate((nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap(), fullPath, processLeafFn, flattenPaths)
+    val childProps = (nodeProps[PROPERTIES] as Map<String, Any>).toMutableMap()
+    traverseMappingsAndUpdate(childProps, fullPath, processLeafFn, flattenPaths)
+    nodeProps[PROPERTIES] = childProps
 }
Suggestion importance[1-10]: 8

__

Why: Valid concern: by passing toMutableMap() copy, any updates made by the recursive call are lost from the parent's nodeProps[PROPERTIES], potentially breaking mapping traversal behavior that previously mutated in-place.

Medium
General
Ensure mutable copy of lastRunContext

Casting metadata.lastRunContext directly to MutableMap<String, MutableMap<String,
Any>> may still fail if the underlying map is an immutable Map (e.g., deserialized
as Collections$UnmodifiableMap or EmptyMap-like) even when non-empty. Consider
defensively copying to a MutableMap via toMutableMap() and mapping inner values
similarly to guarantee mutability.

alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataService.kt [209-213]

 val runContext = if (monitor.monitorType.endsWith(Monitor.MonitorType.DOC_LEVEL_MONITOR.value)) {
     val lastRunCtx = if (metadata.lastRunContext.isEmpty()) null
-        else @Suppress("UNCHECKED_CAST") (metadata.lastRunContext as MutableMap<String, MutableMap<String, Any>>)
+        else @Suppress("UNCHECKED_CAST")
+            metadata.lastRunContext.mapValues { (it.value as Map<String, Any>).toMutableMap() }.toMutableMap()
+                as MutableMap<String, MutableMap<String, Any>>
     createFullRunContext(monitorIndex, lastRunCtx)
 } else null
Suggestion importance[1-10]: 5

__

Why: Reasonable defensive suggestion—if the map is immutable, the unchecked cast could still lead to runtime errors when createFullRunContext mutates it. The impact depends on how lastRunContext is typically constructed.

Low

@thecodingshrimp
thecodingshrimp force-pushed the fix/doc-level-monitor-mutablemap-guards branch from 31032ab to dec1828 Compare August 21, 2026 13:53
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit dec1828

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit ef25277

@thecodingshrimp
thecodingshrimp force-pushed the fix/doc-level-monitor-mutablemap-guards branch from ef25277 to 276f481 Compare August 21, 2026 14:15
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 276f481

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f3a8aac

…mapping traversal

Fix A (MonitorMetadataService.kt): guard lastRunContext cast in recreateRunContext() —
if lastRunContext.isEmpty() pass null to createFullRunContext() (which accepts null and
builds a fresh context) instead of casting EmptyMap to MutableMap, which throws
ClassCastException on monitors that have no last_run_context in stored metadata.

Fix B (DocLevelMonitorQueries.kt): replace bare MutableMap casts in
traverseMappingsAndUpdate() with .toMutableMap() defensive copies so that
Collections.emptyMap() values from index mapping nested objects {} do not cause
UnsupportedOperationException when the traversal tries to mutate them.

Fix C (MonitorFanOutUtils.kt / DocumentLevelMonitorRunner.kt /
RemoteDocumentLevelMonitorRunner.kt): tighten initializeNewLastRunContext() return
type from Map to MutableMap (matching the actual runtime type) and remove the now-
unnecessary as MutableMap casts at both call sites.

Related: opensearch-project/common-utils#967
Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
@thecodingshrimp
thecodingshrimp force-pushed the fix/doc-level-monitor-mutablemap-guards branch from f3a8aac to 67be38d Compare October 28, 2026 17:03
…tern

- MonitorMetadataService: guard recreateRunContext against non-empty but
  wrongly-typed lastRunContext by filtering non-Map entries before casting
- DocLevelMonitorQueries: fix traverseMappingsAndUpdate to write mutated
  child subtree back into parent node (previously lost on recursive calls);
  remove redundant second toMutableMap() copy passed to processLeafFn
- DocumentLevelMonitorRunner: replace outer lastRunContext unchecked cast
  with safe filter-reconstruct; use ?: continue instead of cast on value access
- RemoteDocumentLevelMonitorRunner: same fixes as DocumentLevelMonitorRunner
- MonitorFanOutUtils: add @return KDoc ownership note to initializeNewLastRunContext
- Add unit tests: MonitorFanOutUtilsTests (3 tests for initializeNewLastRunContext)
  and DocLevelMonitorQueriesTraversalTests (5 tests for traverseMappingsAndUpdate
  covering unmodifiable/empty-map inputs, rename, and nested recursion)

Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
…uard

- MonitorFanOutUtils: remove stale @PARAM monitorCtx (no such parameter);
  add missing @PARAM shardCount
- DocLevelMonitorQueries: correct internal-node comment to describe the
  actual copy-mutate-writeback mechanism, not the old direct-mutation contract
- DocumentLevelMonitorRunner, RemoteDocumentLevelMonitorRunner: replace
  '?: continue' dead code (key always present when iterating map's own keys)
  with '!!' plus clarifying comment

Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
@thecodingshrimp
thecodingshrimp force-pushed the fix/doc-level-monitor-mutablemap-guards branch from 67be38d to f7425d9 Compare October 28, 2026 17:04
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f7425d9

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f7425d9

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.

fix: ClassCastException when updating doc_level_monitor with absent last_run_context in stored metadata

1 participant