Scripted Metric Aggregation Failure During Serialization

[jdconrad]

A change made in (#62884) appears to have caused a regression. Though, I'm not sure if this is actually a "fixed" bug, and just happens to be a breaking change. A class cast exception appears when returning from a combine script as part of a scripted metric aggregation when the return is a Map<Long, Object> where serialization appears to only accept Map<String, Object>.

Steps to reproduce:

1. Run elasticsearch 7.10.0

curl -X PUT "localhost:9200/testindex" -H 'Content-Type: application/json' -d'
{
  "mappings": {
    "properties": {
      "instanceId": {
        "store": true,
        "type": "long"
      }
    }
  }
}'

curl -X POST "localhost:9200/testindex/_doc" -H 'Content-Type: application/json' -d'
{
  "instanceId": 1
}
'

curl -X POST "localhost:9200/_search" -H 'Content-Type: application/json' -d'
{
"query": {
    "match_all": {}
  },
  "aggs": {
    "profit": {
      "scripted_metric": {
        "init_script": "state.test = [:]",
        "map_script": "state.test[doc.instanceId.value] = 1",
        "combine_script": "return state.test",
        "reduce_script": "return 0"
      }
    }
  }
}'

Steps 2 and 3 are separate because it seems like the response was masked if done together.

I found this by bisecting with the last known good commit as cd584d49dcc3819821c677973e388f4103f89418
and the first bad commit as 78a93dc18fb3db9a1b9785de34753b499f8bcbf8
from the root branch tag of v7.10.0

cc @jimczi

[elasticmachine]

Pinging @elastic/es-analytics-geo (Team:Analytics)

[jimczi]

I don't think you need #62884 to reproduce the issue. It will happen on every serialization of the aggregation response. The only difference in 7.10 is that it reproduces with a single node since we serialize all aggregations to get their estimated size. So I think it's reproducible before 7.10 with any search that spans a shard search on a different node. I did not test this yet though.
In any case the underlying issue here is that we don't accept a Map<Long, Object> in the output of the scripted metric.
We shouldn't be lenient when checking the class of the key in https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java#L899, only strings should be accepted. So in my opinion this is not a breaking change, just a leniency that we need to fix with a clear error.

However there's a bug in the handling of the exception due to #6288. We don't expect an exception when we retrieve the size of an aggregation, I'll fix this in a separate PR independently of what we decide on the format that we accept.

[jdconrad]

@jimczi Thank you for the analysis here. I'm definitely onboard with closing down this leniency.