Fix nested query issue

[johannes-scharlach]

Fixes #142.

The added test case describes the scenario quite well: nesting with query added a query node to bool where it should have added a must. When I tried to fix it, it became clear that using filter in the context of 'nested' or 'has_parent' has only worked with elasticsearch version 1.x.

This MR represents a breaking change for people using elasticsearch v 1.x!

We could work around this by changing some of the inner workings of bodybuilder: We know the version of the query language once build() is executed, so if we defer the execution of all makeFilter and makeQuery calls to only be executed once build is called, we can pass along the ES version and will be fine.

[johannes-scharlach]

I've added a test to illustrate what needs to be different between v1 and v2 syntax

[johannes-scharlach]

@danpaz I think I'd like to go for a v3 of bodybuilder soon, which would drop support for elasticsearch v1 syntax. That might also allow us to shorten and simplify some of the generated bools.

I imagine this fix to be part of that bodybuilder v3. What are your thoughts on that?

[danpaz]

@johannes-scharlach Agreed this is a breaking change and a solution like parsing the version from build will work. Sounds like it could be tricky to implement cleanly, but I don't know if that warrants a major version bump to v3.

What do you think about implementing the backwards compatible bugfix here in v2, then coming up with a plan for v3 that includes things like dropping ES 1.x syntax, simplifying the generated bools, etc.?

[danpaz]

I think there's a typo here v1 syntax

[johannes-scharlach]

So I've thought about the issue brought up in this MR again and thought about ways of solving it:

1. One idea was to call the nested callback later. That would be a breaking change though, since the nested query function might return a different result when called at a later time. It will also make it more difficult to debug weird nested issues.
2. So instead of passing in the version when calling .build, we could allow passing the target ES version when initiating BodyBuilder like bodyBuilder('v2'). Since nested queries have worked for ES v1 and not for v2 syntax, we'd need to require that for 'v2'. It feels wrong to me to add compatability features for v2 v1 syntax, what do you think?
3. we could rethink build and buildV1, which currently don't interfere with any of the queries but just merge them – but this seems like a major change that we'd only do to accommodate compatability with v1

I'd say we start working on a first alpha version of 3.0 with this change, breaking compatability with v1. I also have some other ideas for 3.0 that we should start talking about, but I don't see us releasing this in a backwards compatible version right now.

[danpaz]

@johannes-scharlach

It feels wrong to me to add compatability features for v2 syntax, what do you think?

I think I follow, but do you mean it feels wrong to add compatibility for Elasticsearch v2 syntax? In the past I've been following the EOL dates to decide when it's fair to drop support for past versions. Looks like 2.x will be EOL in Feb 2018.

I'm on board with what you're thinking, though. A breaking change seems inevitable and we should aim to implement this change as part of a major version bump.

[johannes-scharlach]

Sorry, I mean v1 syntax. We've reached EOL for elasticsearch 1.x, so I'm not very motivated to jump through hoops to fix issues that only occur when you're using an older version of elasticsearch.

[danpaz]

Ah, yes. I'm in favor of dropping 1.x syntax as well, that version of Elasticsearch has been EOL since Jan 2017.

[johannes-scharlach]

In that case

1. We can remove the v1 compatability test I've added and merge this MR
2. I have other changes based on top of this, e.g. switching to Ramda, that we can discuss
3. We can also discuss what else should go in v3 – some ideas are already in discussion in Option to ignore filters when value is undefined #123

[danpaz]

To be clear I think it makes sense to drop v1 compatibility as part of a major version bump to bodybuilder 3.0. We probably don't want this to land in bodybuilder 2.0 as it could break things for existing users. Is that what you were thinking as well?

[johannes-scharlach]

Yes, that's what I was thinking, too