Firestore Where Clause extensions

[Daeda88]

The implementation of Query.where is somewhat limited currently: Some functionality is missing and iOS does not handle equals null correctly.

This PR fixes this by introducing a WhereClause that can easily be mixed and matched.

[nbransby]

Is there an underlying limitation to the implementation of Query.where via named arguments? Because if not I suggest we add the missing functionality and fix the issues without making breaking changes to the API.

If is it indeed an underlying limitation to expressing where clauses as named arguments and we do have to make breaking change to the API I would suggest we match the native android SDK API instead of creating a new API for everybody to learn. It's much easiest to point people towards the android docs than to write and maintain separate documentation for this project.

In fact when this project started, the Firebase Android SDK was java first and, for example, did not support consuming document snapshots as flows. It's nice to see that feature has now been added (as part of their new Kolin-first commitment) and its API is very similar to ours.

If they had already supported this then we would have matched that API just like we did for the ktx extensions such as FIrebase.firestore (which did exist at the time). My point being that now the android sdk is kotlin first we should aim to match the API in any new changes and only deviate in cases when is it not possible due to platform differences.

[Daeda88]

The API doesnt change that much compared to the one this project had before, though I did introduce some convenience methods.

Android (and by extension JS and iOS, though named differently) can do things like whereLessThan().whereArrayContains().whereEqualTo()

This SDK offers:

where(equalTo =)
where(lessThan = .., greaterThan = .., arrayContains = )
where(inArray = .., arrayContainsAny = ..)

This is already a bit inconsistent with the Android API in that lessThan/greaterThan are grouped together which they arent in the Android SDK. So we're already adding convenience methods. This just takes it one step further by using sealed classes to make parameters a bit more concise (and by grouping the lessThan and inArray where methods since there's not really a reason they cant be)

[nbransby]

I believe the grouping is inconsequential (a workaround for method signature clashes) and the API is designed to use a single named arg per method call and supports chaining just like the native APIs.

It doesn't matter so much if the API changes a little or a lot its more the case of does old code still compile or not, breaking changes such as these should only be introduced when unavoidable and as far as I can tell this does not appear to be the case here.

[Daeda88]

If only one argument at a time is used, these aren't breaking changes though. You can still call where(lessThan = ..) like before.

Nevertheless I can ungroup the lessThan and inArray calls if you prefer

[nbransby]

Ok thats good news, do we need the WhereClause class in that case then? Or if it is necessary can we make it internal?

[Daeda88]

I can make it internal yeah, if that's preferred. We don't "need" it, I just prefer folding a list of sealed classes over a huge nested if statement. It's more readable and more Kotlin like.

[nbransby]

Definitely more readable and more Kotlin like but less performant, I don't remember the original issue with the method signatures but maybe its a legacy one in an older Kotlin version. I wonder if it's now possible to simply use one method per clause and avoid the clashes with @JsName/@JvmName/@ObjCName

[Daeda88]

I've made the WhereClause internal.

On performance: Ive tested using value classes as instead of data classes, but the performance impact of that was slightly worse. Nevertheless, the time to process is a fraction of a millisecond and considering nobody's gonna be creating 1000s of where clauses per second, I doubt you'd see any noticeable performance impact from using sealed interfaces here. I would consider this the most stable and maintainable kotlin like implementation. See this example to see that the performance difference is minimal: https://pl.kotl.in/jJN_X6wqu

On method signatures: Kotlin does not support writing the same method with different parameter names. Since named arguments are not obligatory, the language considers them the same signature. This is regardless of platform so JvmName etc wont fix that. You'll see this is why Ive added the whereNot method for a notEquals (this cannot be grouped because null is meaningful for equals/notEquals as they take nullable properties on the platforms). So either we deprecate where in favour of whereEquals etc, or we group them as is. But the proper kotlin way would be using a sealed class as Ive implemented.

[nbransby]

Yes I am not particularly worried about performance tbh.

Looking at the docs seems like there is now support for or queries that introduce a new Filter class:
https://firebase.google.com/docs/firestore/query-data/queries#or_queries
https://firebase.google.com/docs/reference/android/com/google/firebase/firestore/Filter

If you ask me the syntax doesn't look great and certainly not Kotlin first! How would you handle those with your WhereClause sealed class?

[Daeda88]

Honestly, Id make it proper kotlin. Something like this:
https://pl.kotl.in/ITuvaxphp

So you can do:

query.where {
  "path" equalTo 0 and ( "otherPath" lessThan 8 or (field notIn listOf(1, 2, 3)))
}

[Daeda88]

If you like, I can change my PR to implement it like this

[nbransby]

that looks great! Is it capable of supporting all the queries possible with the Filter class?

[Daeda88]

In theory it should. One way to find out

[nbransby]

we could add this and keep but deprecate the old functions

[Daeda88]

Yeah Ill keep the existing functions, make them shortcut to this and deprecate them

[Daeda88]

Small note: your Java SDK doesnt seem to track this change yet. So For now Ill split sources between JVM and Android and on JVM I wont support the OR constraints

[Daeda88]

@nbransby Want to add a few more tests, but in principle its implemented :)

EDIT: Tests added

[nbransby]

Could you overload the in operator to avoid the quotes?

https://kotlinlang.org/docs/operator-overloading.html#in-operator

[Daeda88]

I cant overload it since the method to overload is contains which has a different signature. Ill rename to inArray

[Daeda88]

For similar reasons I cant make operator overloads for < > etc. They all go to the compareTo method and that isnt suitable for overloading in this case

[nbransby]

Would you like to add a paragraph to the the readme on your new kotlin-style where clauses in this PR?

[Daeda88]

Done. I removed the section on named arguments cause I couldnt find another example where its needed.

[nbransby]

This is a great contribution to the library, I'm sure people will enjoy using the new syntax, and, who knows, you might even inspire the official android sdk!