Sub-metrics parser

[bermanjosh]

The fromJSON instances of the various "result" types for Aggregations (TermsResult, DateHistogramResult, DateRangeResult) all parse for sub-aggregations with a static key "aggregations" ( here, here and here).

I think that might be a mistake. Nearest I can tell, sub-aggregations are returned with the name of the aggregation as the key. You can see an example from the Elastic docs, the returned JSON looks like:

  "aggregations": {
      "colors": {
         "buckets": [
            {
               "key": "red",
               "doc_count": 4,
               "avg_price": { 
                  "value": 32500
               }
            }
         ]
      }
   }

I didn't see a test case for sub-aggregations (unless I missed it, which is totally possible).

@chrisguiney @alistair @MichaelXavier @MHova : What do you think? Thanks

[MHova]

Ah interesting @bermanjosh. I want to make sure that this is indeed a bug with the library. Can you write failing test cases that demonstrate this problem against a live ES instance for each of the major ES versions supported by Bloodhound?

[bermanjosh]

@MHova I confirmed this locally while I was working on it today. What're you looking for, some branch pushed up for travis specifically to fail? Or just some minimal-code here? I'm happy to do either, I just wasn't clear which you meant.

I've also started working on a rough solution, essentially just passing the object of any unused fields as a sub-aggregation, and letting the user call the aggregation name while making a Result. I'm building it off the state of my fork for #158, so I don't think I'll be able to PR it until after that's accepted / merged.

[MHova]

Sorry let me clarify. Yes, I'd like to see a branch with a failing test for TravisCI to run. Specifically, I'd like to see tests for all the ES 1.x versions.

Is Bloodhound working correctly for some older version of ES, but breaks for newer versions due to an ES API change? Or have we always been broken? If it's the former case, will the bug fix cause us to have to drop support for older versions of ES?

[bitemyapp]

We enforced 1.x correctness only, with some small (but irritating) breakages on 2.x and 5.x. We're now heading towards splitting the API into version-specific modules to resolve the problem.

[MHova]

Ah okay so what does this mean for @bermanjosh's efforts? Is he all cleared to fix the bug without worrying about 1.x compatibility?

[bitemyapp]

If a feature exists in V1 and V5 and has a bug, both modules need to be fixed. If a feature exists exclusively in 5.x, then a fix addressing that alone is fine.

[bermanjosh]

@MHova This should fail: https://github.com/bermanjosh/bloodhound/commits/badAggTest

I'll try push a fix on that branch some time over the weekend.

[MHova]

Sweet thanks @bermanjosh

[bermanjosh]

@MHova I pushed a commit to that branch which fix up the sub-aggregations parsing. It's a little brute-force, but I don't see any other option short of forcing aggregation names with some kind of unique prefix (which I'd rather not do).

(I forgot to remove ES2 from the .travis.yml file. I fixed that, but you can also see on the earlier test that it worked for both ES1.7 and ES5)

[MHova]

@bermanjosh Looked over the commits. Makes sense to me :+1:

[bermanjosh]

@MHova Ok. I'll PR it after #158 is merged (it's built on top of that).