Firestore customizable encoding for where clauses and update methods

[Daeda88]

Fixes #604 and trumps #605

This solution extends all update methods in Firestore to support more advanced update methods. String fields and FieldPaths can now be used interchangably and custom serializer can be passed.

documentRef.update("field" to "value, "otherField" to 1)

becomes

documentRef.update {
     // specifying build settings is optional
    buildSettings = {}

    "field" to "value"
    FieldPath("otherField") to 1
    "customField".to(CustomValueSerializer(), customValue)
}

Note that the existing vararg pairs can still be used.

Similar support has been added to where clauses as well as startAt/endAt/etc:

query.startAt {
    add(value)
    addWithStrategy(CustomValueSerializer(), customValue)
 }

There is a small regression on the existing update methods in that

public fun update(documentRef: DocumentReference, vararg fieldsAndValues: Pair<String, Any?>, buildSettings: EncodeSettings.Builder.() -> Unit)changed to public fun update(documentRef: DocumentReference, buildSettings: EncodeSettings.Builder.() -> Unit, vararg fieldsAndValues: Pair<String, Any?>) due to Kotlin not being able to figure out which update method to use.

Furthermore, I've split the Firestore tests into a few different files and extended them so they clean up all data after completion.

Lastly, I discovered bug #613 and fixed it

[Daeda88]

I still need to add Readme/some tests, but feedback would already be appreciated @nbransby as I wont be able to continue until next week

[Daeda88]

Now fully ready for review @nbransby / @Reedyuk . See description above for details of changes.

[Daeda88]

Will anyone ever review this @nbransby 😅

[nbransby]

Sorry been a crazy month, will hopefully get to it soon

[nbransby]

There is a small regression on the existing update methods in that

Does that make this a breaking change?

[nbransby]

I still need to add Readme/some tests, but feedback would already be appreciated @nbransby as I wont be able to continue until next week

looks great to me, I would like to see more comprehensive documentation in the README that would also help me understand the intended usage.

[Daeda88]

There is a small regression on the existing update methods in that

Does that make this a breaking change?

Yeah, its breaking. I went from:

update(docRef, "path" to value) { encodeDefaults = false }

to

update(docRef, { encodeDefaults = false }, "path" to value)

The reason I had to do something is because the new update method has the signature:

update(docRef) {
  "path" to value
}

and since the original method takes a vararg, it can be called as update(docRef) { encodeDefaults = false }, which means the compiler wont know wich method to call. I considered alternatives:

1. Change the original code to always take at least one path (i.e. fun update(docRef, firstValue: Pair<String, Any>, vararg others: Pair<String, Any>) but that would mean you woundnt be able to call it with update(docRef, *listOfPairs.toTypedArray()) which I assume is a common usecase.
2. Name the new update function something else, e.g. updateWith, but I though the downside of that was that it sort of breaks the agreement that we name stuff like it is named in the native libraries
3. I do this approach, knowing that the encodeSettings closure is relatively new and will thus probably be not used that much yet.
4. Keep the old signature and the new one as is now, but that means you have to use named arguments for the closure which sucks.

I think 1 is a definitive no-go. If you prefer we do 2, Im fine with that as well. Its always gonna be a tradeoff so its pick your poison basically.

As for documentation, Ill change that as soon as we agree on this point :)

[nbransby]

I spent long enough trying to think of additional alternatives without joy so, I favour 2 and calling it updateFields

[Daeda88]

Done @nbransby

[nbransby]

does all the DefaultImpls need to be exposed?

[Daeda88]

Looks like this is coming in from Kotlin. By extending the Encoder interface we inherit some default implementations. This is what that looks like from a JVM api perspective. There is no "DefaultImpls" class anywhere, the file is literally this:

public interface FirebaseEncoder : Encoder

[nbransby]

does this need to be public?

[Daeda88]

No, removed

[nbransby]

exposing constructor?

[Daeda88]

FilterBuilder uses it in its inline implementation. Marked as internal @PublishedAPI in code, so not actually visible for the kotlin side

[nbransby]

does startAtFieldValues/startAfterFieldValues/endAtFieldValues/endBeforeFieldValues suffer the same signature clash issue as update or can they renamed to startAt/startAfter/endAt/endBefore` overrides?

[Daeda88]

@nbransby @Reedyuk Sorry for the looooooong delay in picking this up. Priorities just got in the way so I never got around to addressing this.

I've tracked latest master, corrected the readme and where possible moved things out of the public API.

On startAtFieldValues etc, yes, unfortunately
public fun startAt(builder: FieldValuesDSL.() -> Unit)
would clash with the existing
public fun startAt(vararg fieldValues: Any?, buildSettings: EncodeSettings.Builder.() -> Unit)
as kotlin would not know which closure to use if you just call startAt {}, assuming fieldValues to be empty. The alternative is of course to replace the latter with public fun startAt(firstFieldValue: Any?, vararg fieldValues: Any?, buildSettings: EncodeSettings.Builder.() -> Unit), as calling startAt without any fieldValues in the first place is probably wrong, but since that would break the API Im guessing nobody is in favor of that.

On some of the things in the API: see individual comments.

[DatL4g]

@Daeda88 is this still a thing?

If so, it would be really nice if you rebase and create a PR here: https://github.com/DatL4g/firebase-kotlin-sdk

[nbransby]

@Daeda88 looks good, lets deprecate the old methods (update, startAt, endAt) with the plan to rename these new ones eventually in a future 3.x release then I will merge

[Daeda88]

Ok, I'm on holiday the upcoming week, will do that when I'm back

[Daeda88]

@nbransby added deprecation/updateWith annotation. Hope its finally good to go! (almost exactly a year further but worth it 😂 )

[Daeda88]

Glad to see its approved. Im assuming you're holding of on merging due to the CI issue, but I have no idea what's causing it as API dump doesnt trigger any changes for me locally

[nbransby]

@Reedyuk any ideas on that?

[Reedyuk]

Seems to be that there is changes it wants to push but doesn't have the permission to push.
The intention was to get it to autocommit the changes but i think we are going to have to just make it fail instead.
The issue is because of permissions and forks, i guess its because the action runs on gitlives workspace and so doesnt have write access to the forked branch.

[Reedyuk]

Im not sure if @Daeda88 is on a mac, but it could be that we generate the ios api too now, whereas previously it was only done on jvm and js

[Daeda88]

@Reedyuk API is only for JVM targets (android/jvm) and I am running on Mac regardless. So that cant be it. Running it now with a git status command to see what its trying to commit. Also, what's the point of the auto commit task if it doesnt work?