mbuhot / eskotlin

Elasticsearch Query DSL for Kotlin
MIT License
136 stars 28 forks source link

Conditional queries - take 2 #5

Closed mbuhot closed 7 years ago

mbuhot commented 7 years ago

@puug What do you think of this approach for the conditional queries / params.

The gist is to use a new helper funtion runIf to wrap any block of code that is conditional.

The stdlib has the run function:

public inline fun <T, R> T.run(block: T.() -> R): R = block()

And runIf adds the boolean condition with nullable result:

fun <T, R> T.runIf(condition: Boolean, f: T.() -> R): R? {
    return if (condition) {
        f()
    } else {
        null
    }

Which gets used like:

val query = bool {
    must {
        term { "user" to "kimchy" }
    }
    runIf(true) {
        filter {
            term { "tag" to "true" }
        }
    }
    runIf(false) {
        filter {
            term { "tag" to "false" }
        }
    }
    runIf(false) {
        must_not {
            range {
                "age" to {
                    from = 10
                    to = 20
                }
            }
        }
    }
    should = listOf(
            term { "tag" to "wow" },
            runIf(false) { term { "tag" to "elasticsearch" } })
    minimum_should_match = 1
    boost = 1.0f
}

All the new tests you added pass, I just moved the true / false out from the query builder function calls into enclosing runIf calls.

puug commented 7 years ago

I like the simplification. I wonder if we can take it further though, using existing control structures.

val query = bool {
    must {
        term { "user" to "kimchy" }
    }
   if (true) {
        filter {
            term { "tag" to "true" }
        }
    }
    if (false) {
        filter {
            term { "tag" to "false" }
        }
    }
    if (false) {
        must_not {
            range {
                "age" to {
                    from = 10
                    to = 20
                }
            }
        }
    }
    should = listOf(
            term { "tag" to "wow" },
            if (false) { term { "tag" to "elasticsearch" } } else null
    minimum_should_match = 1
    boost = 1.0f
}

I gave it a quick whirl and tests passed. Happy to do the legwork and update my PR if you like this approach

mbuhot commented 7 years ago

@puug: Yeah I like using plain-old if !

One gotcha might be cases where a QueryBuilder is being created inside a listOf, you need to throw in the else { null } block:

With if/else

  should = listOf(
    if (isAmazing()) { term { "tag" to "wow" } } else { null },
    term { "tag" to "elasticsearch" })

With runIf

  should = listOf(
    runIf (isAmazing()) { term { "tag" to "wow" } },
    term { "tag" to "elasticsearch" })
puug commented 7 years ago

Yep the else null could become annoying for that scenario. Could support both?

mbuhot commented 7 years ago

Could support both?

Should be fine. Use if when the result isn't being passed directly into another function, and runIf as a convenience when you need it.

mbuhot commented 7 years ago

Closing this one, regular kotlin if/else should be good enough.