Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ class DocLevelMonitorFanOutResponse : ActionResponse, ToXContentObject {
nodeId = sin.readString(),
executionId = sin.readString(),
monitorId = sin.readString(),
lastRunContexts = sin.readMap()!! as MutableMap<String, Any>,
lastRunContexts = sin.readMap()?.toMutableMap() ?: mutableMapOf(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

inputResults = InputRunResults.readFrom(sin),
triggerResults = suppressWarning(sin.readMap(StreamInput::readString, DocumentLevelTriggerRunResult::readFrom)),
triggerResults = readTriggerResults(sin),
exception = sin.readException()
)

Expand All @@ -54,6 +54,16 @@ class DocLevelMonitorFanOutResponse : ActionResponse, ToXContentObject {
this.exception = exception
}

companion object {
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>)
}
}

@Throws(IOException::class)
override fun writeTo(out: StreamOutput) {
out.writeString(nodeId)
Expand Down Expand Up @@ -82,11 +92,4 @@ class DocLevelMonitorFanOutResponse : ActionResponse, ToXContentObject {
.endObject()
return builder
}

companion object {
@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): Map<String, DocumentLevelTriggerRunResult> {
return map as Map<String, DocumentLevelTriggerRunResult>
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package org.opensearch.commons.alerting.model
import org.opensearch.Version
import org.opensearch.common.lucene.uid.Versions
import org.opensearch.commons.alerting.alerts.AlertError
import org.opensearch.commons.alerting.model.Monitor.Companion.suppressWarning
import org.opensearch.commons.alerting.util.IndexUtils.Companion.NO_SCHEMA_VERSION
import org.opensearch.commons.alerting.util.instant
import org.opensearch.commons.alerting.util.optionalTimeField
Expand Down Expand Up @@ -423,7 +422,7 @@ data class Alert(
null
},
queryResults = if (sin.version.onOrAfter(Version.V_3_7_0)) {
sin.readList { input -> suppressWarning(input.readMap()) }
sin.readList { input -> input.readMap()?.toMutableMap() ?: mutableMapOf() }
} else {
listOf()
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.commons.alerting.model

import org.opensearch.commons.alerting.util.readMapAsMutableMap
import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.StreamOutput
import org.opensearch.core.xcontent.ToXContent
Expand All @@ -24,7 +25,7 @@ data class BucketLevelTriggerRunResult(
sin.readString(),
sin.readException() as Exception?, // error
sin.readMap(StreamInput::readString, ::AggregationResultBucket),
sin.readMap() as MutableMap<String, MutableMap<String, ActionRunResult>>
sin.readMapAsMutableMap() as MutableMap<String, MutableMap<String, ActionRunResult>>
)

override fun internalXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.opensearch.commons.alerting.model

import org.opensearch.commons.alerting.alerts.AlertError
import org.opensearch.commons.alerting.util.readMapAsMutableMap
import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.StreamOutput
import org.opensearch.core.xcontent.ToXContent
Expand All @@ -28,7 +29,7 @@ data class ChainedAlertTriggerRunResult(
triggerName = sin.readString(),
error = sin.readException(),
triggered = sin.readBoolean(),
actionResults = sin.readMap() as MutableMap<String, ActionRunResult>,
actionResults = sin.readMapAsMutableMap() as MutableMap<String, ActionRunResult>,
associatedAlertIds = sin.readStringList().toSet()
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.opensearch.commons.alerting.model

import org.opensearch.commons.alerting.alerts.AlertError
import org.opensearch.commons.alerting.util.readMapAsMutableMap
import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.StreamOutput
import org.opensearch.core.common.io.stream.Writeable
Expand Down Expand Up @@ -35,7 +36,7 @@ data class ClusterMetricsTriggerRunResult(
triggerName = sin.readString(),
error = sin.readException(),
triggered = sin.readBoolean(),
actionResults = sin.readMap() as MutableMap<String, ActionRunResult>,
actionResults = sin.readMapAsMutableMap() as MutableMap<String, ActionRunResult>,
clusterTriggerResults = sin.readList((ClusterTriggerResult)::readFrom)
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ data class IndexExecutionContext(
@Throws(IOException::class)
constructor(sin: StreamInput) : this(
queries = sin.readList { DocLevelQuery(sin) },
lastRunContext = sin.readMap() as MutableMap<String, Any>,
updatedLastRunContext = sin.readMap() as MutableMap<String, Any>,
lastRunContext = sin.readMap()?.toMutableMap() ?: mutableMapOf(),
updatedLastRunContext = sin.readMap()?.toMutableMap() ?: mutableMapOf(),
indexName = sin.readString(),
concreteIndexName = sin.readString(),
updatedIndexNames = sin.readStringList(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ data class Monitor(
schemaVersion = sin.readInt(),
inputs = sin.readList((Input)::readFrom),
triggers = sin.readList((Trigger)::readFrom),
uiMetadata = suppressWarning(sin.readMap()),
uiMetadata = sin.readMap()?.toMutableMap() ?: mutableMapOf(),
dataSources = if (sin.readBoolean()) {
DataSources(sin)
} else {
Expand Down Expand Up @@ -480,10 +480,5 @@ data class Monitor(
fun readFrom(sin: StreamInput): Monitor? {
return Monitor(sin)
}

@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): MutableMap<String, Any> {
return map as MutableMap<String, Any>
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ data class MonitorMetadata(
primaryTerm = sin.readLong(),
monitorId = sin.readString(),
lastActionExecutionTimes = sin.readList(ActionExecutionTime.Companion::readFrom),
lastRunContext = Monitor.suppressWarning(sin.readMap()),
sourceToQueryIndexMapping = sin.readMap() as MutableMap<String, String>
lastRunContext = sin.readMap()?.toMutableMap() ?: mutableMapOf(),
sourceToQueryIndexMapping = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as MutableMap<String, String>
)

override fun writeTo(out: StreamOutput) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import org.opensearch.OpenSearchException
import org.opensearch.Version
import org.opensearch.commons.alerting.alerts.AlertError
import org.opensearch.commons.alerting.util.optionalTimeField
import org.opensearch.commons.alerting.util.readMapAsMutableMap
import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.StreamOutput
import org.opensearch.core.common.io.stream.Writeable
Expand All @@ -36,7 +37,7 @@ data class MonitorRunResult<TriggerResult : TriggerRunResult>(
sin.readInstant(), // periodEnd
sin.readException(), // error
InputRunResults.readFrom(sin), // inputResults
suppressWarning(sin.readMap()) as Map<String, TriggerResult> // triggerResults
sin.readMapAsMutableMap() as Map<String, TriggerResult> // triggerResults
)

override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
Expand Down Expand Up @@ -72,11 +73,6 @@ data class MonitorRunResult<TriggerResult : TriggerRunResult>(
fun readFrom(sin: StreamInput): MonitorRunResult<TriggerRunResult> {
return MonitorRunResult(sin)
}

@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): Map<String, TriggerRunResult> {
return map as Map<String, TriggerRunResult>
}
}

@Throws(IOException::class)
Expand Down Expand Up @@ -147,7 +143,7 @@ data class InputRunResults(
val count = sin.readVInt() // count
val list = mutableListOf<Map<String, Any>>()
for (i in 0 until count) {
list.add(suppressWarning(sin.readMap())) // result(map)
list.add(sin.readMapAsMutableMap()) // result(map)
}
val pplCount = if (sin.version.onOrAfter(Version.V_3_7_0)) {
sin.readVInt()
Expand All @@ -157,7 +153,7 @@ data class InputRunResults(
val pplList = mutableListOf<Map<String, Any?>>()
if (sin.version.onOrAfter(Version.V_3_7_0)) {
for (i in 0 until pplCount) {
pplList.add(suppressWarning(sin.readMap())) // pplResults
pplList.add(sin.readMapAsMutableMap()) // pplResults
}
}
val pplNumResults = if (sin.version.onOrAfter(Version.V_3_7_0)) {
Expand All @@ -168,11 +164,6 @@ data class InputRunResults(
val error = sin.readException<Exception>() // error
return InputRunResults(list, error, null, pplList, pplNumResults)
}

@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): Map<String, Any> {
return map as Map<String, Any>
}
}

fun afterKeysPresent(): Boolean {
Expand All @@ -197,10 +188,11 @@ data class ActionRunResult(
) : Writeable, ToXContent {

@Throws(IOException::class)
@Suppress("UNCHECKED_CAST")
constructor(sin: StreamInput) : this(
sin.readString(), // actionId
sin.readString(), // actionName
suppressWarning(sin.readMap()), // output
sin.readMapAsMutableMap() as Map<String, String>, // output
sin.readBoolean(), // throttled
sin.readOptionalInstant(), // executionTime
sin.readException() // error
Expand Down Expand Up @@ -233,11 +225,6 @@ data class ActionRunResult(
fun readFrom(sin: StreamInput): ActionRunResult {
return ActionRunResult(sin)
}

@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): MutableMap<String, String> {
return map as MutableMap<String, String>
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package org.opensearch.commons.alerting.model

import org.opensearch.Version
import org.opensearch.commons.alerting.alerts.AlertError
import org.opensearch.commons.alerting.util.readMapAsMutableMap
import org.opensearch.core.common.io.stream.StreamInput
import org.opensearch.core.common.io.stream.StreamOutput
import org.opensearch.core.xcontent.ToXContent
Expand All @@ -29,7 +30,7 @@ open class QueryLevelTriggerRunResult(
triggerName = sin.readString(),
error = sin.readException(),
triggered = sin.readBoolean(),
actionResults = sin.readMap() as MutableMap<String, ActionRunResult>,
actionResults = sin.readMapAsMutableMap() as MutableMap<String, ActionRunResult>,
pplCustomQueryResults = if (sin.version.onOrAfter(Version.V_3_7_0)) {
sin.readList { it.readMap() }
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,4 @@ abstract class TriggerRunResult(
out.writeString(triggerName)
out.writeException(error)
}

companion object {
@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): MutableMap<String, ActionRunResult> {
return map as MutableMap<String, ActionRunResult>
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -294,11 +294,6 @@ data class Workflow(
return Workflow(sin)
}

@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): MutableMap<String, Any> {
return map as MutableMap<String, Any>
}

private const val DEFAULT_OWNER = "alerting"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ data class WorkflowRunResult(
val triggerResults: Map<String, ChainedAlertTriggerRunResult> = mapOf()
) : Writeable, ToXContent {

@Throws(IOException::class)
@Suppress("UNCHECKED_CAST")
@Throws(IOException::class)
constructor(sin: StreamInput) : this(
workflowId = sin.readString(),
workflowName = sin.readString(),
Expand All @@ -35,7 +35,7 @@ data class WorkflowRunResult(
executionEndTime = sin.readOptionalInstant(),
executionId = sin.readString(),
error = sin.readException(),
triggerResults = suppressWarning(sin.readMap()) as Map<String, ChainedAlertTriggerRunResult>
triggerResults = (sin.readMap()?.toMutableMap() ?: mutableMapOf()) as Map<String, ChainedAlertTriggerRunResult>
)

override fun writeTo(out: StreamOutput) {
Expand Down Expand Up @@ -73,10 +73,5 @@ data class WorkflowRunResult(
fun readFrom(sin: StreamInput): WorkflowRunResult {
return WorkflowRunResult(sin)
}

@Suppress("UNCHECKED_CAST")
fun suppressWarning(map: MutableMap<String?, Any?>?): Map<String, TriggerRunResult> {
return map as Map<String, TriggerRunResult>
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.commons.alerting.util

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

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