aggregation: json.Number is now validated in go 1.6

[brenol]

Please use the following questions as a guideline to help me answer
your issue/question without further inquiry. Thank you.

Which version of Elastic are you using?

[X] elastic.v2 (for Elasticsearch 1.x)
[X] elastic.v3 (for Elasticsearch 2.x)

Any steps to reproduce the behavior?

Marshal the output from the aggregation.

Since Go1.6, json.Number is now validated. Which means the following code:
http://play.golang.org/p/Xx4GjIgL_X
is now invalid. (You can try to run it in playground and it does not work).

I found this issue in the following line of code:

elastic/search_aggs.go

Line 1011 in ebe16b7

json.Unmarshal(*v, &a.KeyNumber)

It means that if the key is not a number, the library will cause a error when Marshalling the aggregation response, which would not happen in go1.5.3.

Although a very specific case, I think the library should handle this case gracefully, trying to parse the value before unmarshalling it in a json.Number.

Thanks!

[olivere]

@brenol Hi... thanks for reporting. I'm a bit busy recently. I hope to look at it this weekend.

[brenol]

hey @olivere, no problem! I know this is happening only during Marshalling at the moment but I actually believe this is a bug and it isn't supposed to be like this - it should also be validated in Unmarshalling as per the issue I found in golang/go.

Thanks!

[olivere]

Hmm... I'm not really sure what your proposing. This is the current version:

// AggregationBucketKeyItem is a single bucket of an AggregationBucketKeyItems structure.
type AggregationBucketKeyItem struct {
    Aggregations

    Key       interface{} //`json:"key"`
    KeyNumber json.Number
    DocCount  int64 //`json:"doc_count"`
}

// UnmarshalJSON decodes JSON data and initializes an AggregationBucketKeyItem structure.
func (a *AggregationBucketKeyItem) UnmarshalJSON(data []byte) error {
    var aggs map[string]*json.RawMessage
    dec := json.NewDecoder(bytes.NewReader(data))
    dec.UseNumber()
    if err := dec.Decode(&aggs); err != nil {
        return err
    }
    if v, ok := aggs["key"]; ok && v != nil {
        json.Unmarshal(*v, &a.Key)
        json.Unmarshal(*v, &a.KeyNumber)
    }
    if v, ok := aggs["doc_count"]; ok && v != nil {
        json.Unmarshal(*v, &a.DocCount)
    }
    a.Aggregations = aggs
    return nil
}

If you want me to change it to ...

// AggregationBucketKeyItem is a single bucket of an AggregationBucketKeyItems structure.
type AggregationBucketKeyItem struct {
    Aggregations

    Key       interface{} //`json:"key"`
    KeyNumber json.Number
    DocCount  int64 //`json:"doc_count"`
}

// UnmarshalJSON decodes JSON data and initializes an AggregationBucketKeyItem structure.
func (a *AggregationBucketKeyItem) UnmarshalJSON(data []byte) error {
    var aggs map[string]*json.RawMessage
    dec := json.NewDecoder(bytes.NewReader(data))
    dec.UseNumber()
    if err := dec.Decode(&aggs); err != nil {
        return err
    }
    if v, ok := aggs["key"]; ok && v != nil {
        json.Unmarshal(*v, &a.Key)
        // Changes here...
        var n json.Number
        if err := json.Unmarshal(*v, &n); err != nil {
                return err
        }
        json.Unmarshal(*v, &n)
    }
    if v, ok := aggs["doc_count"]; ok && v != nil {
        json.Unmarshal(*v, &a.DocCount)
    }
    a.Aggregations = aggs
    return nil
}

... we would crash all the cases where the bucket key is, with a purpose, NOT a number, but e.g. a string. The problem was IIRC that ES returns different types for bucket keys: numbers, strings etc. So I silently swallowed the error of not correctly reading the json.Number (which is okay with bucket keys of type string). It is up to the user to use KeyNumber because she's the only one who knows if the bucket keys will be numbers or not.

Or am I missing something? If so, a failing test case would really help... :-)

[brenol]

Hi Olivere,

I'm sorry.

I just noticed that there's a bug in the stdlib not specifically when unmarshalling strings into json.Numbers, but when unmarshalling strings into json.Numbers inside structs.

I actually opened golang/go#14702 as it does not follow what's happening in this test case:
http://play.golang.org/p/WSL3wYvMC7

If you change it into this:

   var n json.Number
        if err := json.Unmarshal(*v, &n); err != nil {
                return err
        }

Then it'll really just break. I think you should change it to a type switch so this issue won't happen in the near future, as it's actually a bug at the moment in the stdlib that only happens when unmarshalling into a struct field.

Thanks!

[maps90]

any update on this?

[olivere]

No. To be honest, I don't know what to do about this in Elastic. If you can, please provide a failing test case.

[brenol]

As I said, unfortunately this only happens when Unmarshalling into a struct/a map. In case of elastic you're unmarshalling into a struct and this is why it breaks.

If they actually fix it in golang/go#14702 then it'll break in elastic. However no one looked into it there and I'm not sure.

Thanks

[olivere]

But Elastic is deliberately swallowing errors. What will break? What am I missing?

[maps90]

i think I have a workaround on this issue, I set my struct as an interface and then type switching as brenol suggested.

Thanks.

[olivere]

There's 3 tests regarding the handling of bucket keys in Elastic.

1. Buckets with string keys
2. Buckets with numeric keys
3. Buckets with boolean keys

How do I change any of these to get to a reproducible, failing test case?

[olivere]

Closing due to inactivity.