Fix OnDisconnect failing to updateChildren

[Daeda88]

These changes started as a way to fix an issue reported by @nbransby here

The reason for it was (I suspect) caused by a bug in Firebase Database OnDisconnect, where Javascript would update children with a Kotlin map rather than a Json object, which caused the wrong data to be sent and broke verification rules. Fixing this proved quite trivial.

However, it pointed to a design flaw in the SDK as a whole. Often an Any object is passed to set/update methods, while some constraints should apply. This is mostly cases such as create Document, where the value being written should be encoded as a Map of String and Any? (or in Javascripts case as Json). Often casting was done on the platform side (e.g. as Map<String, Any?> on JVM) which is problematic because A) generics get lost in casting, and B) now we have platform level exceptions for data that should be validated in common so a common exception can be thrown/handled. Furthermore, we have a lot of methods where a generically typed value can be provided, which is then encoded and force cast to not-null. Since null will always encode to null this is also incorrect.

Therefore, besides the straightforward fix I have made the following changes:

• Introduced an EncodedObject that is passed to internal API methods that only accept encoded data in the form of Map<String, Any?> (or the platform equivalent thereof)
• For generic methods that internally have their data encoded to an EncodedObject, the constraint has been changed from <T> to <T : Any> so that no null values can be provided.
• Restructured naming of Firebase internal classes a bit so that it is easier to access the platform native implementation
• On Firestore introduced the following methods: DocumentSnapshot.contains(fieldPath: FieldPath): Boolean, DocumentSnapshot.get(fieldPath: FieldPath, serverTimestampBehavior: ServerTimestampBehavior, buildSettings: DecodeSettings.Builder.() -> Unit): T and moved Query methods into the class itself (which is no longer expect actuals but uses the same NativeWrapper pattern as used elsewhere)
• On Database added FirebaseDatabase.goOnline() and FirebaseDatabase.goOffline() (there where required to actually test OnDisconnect.
• Database tests now point to the default database, because I couldnt get test with rules to work otherwise.

[nbransby]

are these methods suppose to be part of the public API? They don't have an equivalent methods in the official SDK, I'm assuming we don't have any documentation for them either, and personally I have no idea what 'encoded' means here?

[nbransby]

Are you proposing EncodedObject becomes part of the public API? Is that a good thing for API consumers?

[Daeda88]

I would've made it private, but it's in the common module used by firestore/database so it has to be a public class unfortunately. But I forgot to make the constructors private. I'll do that so it's at least inaccessible

[Daeda88]

Also added some extra documentation that explicitly recommends not to use it directly.

[nbransby]

Can you mark it PublishedApi?

[Daeda88]

That doesn't do anything here. Published api is to make internal methods and classes accessible to inline functions. But 'encodeAsObject' returns an EncodedObject, which is then used in say Firebase-database. Since that is a different module, it is public.

But I would argue the same issue pops up with all the encode methods. They are in the public api, but they shouldn't be used manually either. Sending a value that was encoded using 'encode' to Firestore will lead to double encoding, which would crash. But since those modules need to use it, it needs to be a public method.

What I think would help (but is probably out of scope here) is splitting up the 'firebase-common' module into 'firebase-common' and 'firebase-common-internal', with the latter only ever imported using implement rather than api and not advertised as existing. This will make it fairly clear that it is not for external use.

[Daeda88]

Btw, what I can (and probably will) do is make EncodedObject an expect interface so it's internals are better guarded. Still I would suggest considering explicit '.internal' packages

[Daeda88]

@nbransby I made an additional PR that would split up Firebase common into two modules. This may actually be nice since currently the firestore methods allow passing EncodeSettings.Builder, which would require users to import firebase-common.

With this implementation, the firebase-common module can be an API and the firebase-common-internal is imported using implements. I think it solves a lot of "is this public facing or not" issues

splendo#70

[nbransby]

@nbransby I made an additional PR that would split up Firebase common into two modules. This may actually be nice since currently the firestore methods allow passing EncodeSettings.Builder, which would require users to import firebase-common.

With this implementation, the firebase-common module can be an API and the firebase-common-internal is imported using implements. I think it solves a lot of "is this public facing or not" issues

splendo#70

this definitely makes the most sense

[Daeda88]

Merge it into this one then?

[nbransby]

Yep

[Daeda88]

Done :)

Simplified EncodedData a bit as well, it is now a sealed interface on the internal module, so its fairly well protected from users implementing it

[nbransby]

whats the purpose of still exposing this to clients?

[Daeda88]

Figured it would be useful. Moved to an internal get method now (cant be value due to JS weirdness)

[Daeda88]

Disabled the new DB test on Android due to a bug in the firebase-android-sdk: firebase/firebase-android-sdk#5870

[nbransby]

would it make sense to split each commonMain file into two, having separate files for the public and internal apis would make it easier to maintain (the firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt file is particularly huge)

[Daeda88]

Sure but can you review/merge #450 first, otherwise I have to track a bunch of merge conflicts

[Daeda88]

Alternatively, I would suggest splitting up the components themselves (e.g. a file for DocumentReference, Query, etc) into different files which makes actually finding what you need easier

[Daeda88]

Tracked master and moved all FIrestore internal classes to a .internal package. The same should probably be done for other modules, but since their files are smaller, its less of an issue to I left it as is for now. I'd still suggest splitting up the firestore file into multiple files, but I also left that out to keep this reviewable

[nbransby]

what about #450 (comment) ?

[Daeda88]

It's not impossible, still thinking about doing it right for JS. The trick is gonna be having an expect interface WithNative<T>. With on platform sides default implementations for android ios etc. Then just a matter of implementing the interface everywhere you need it. Tricky thing right now is JS, since its native classes are often exposed as interfaces and so you need an additional wrapper class there. If I can figure out a proper way so that WithNative exposes the interface rather than the wrapping class, it should be good.

Also, Im not sure how K2 handles it.

Ill try and implement it asap. You can decide whether you want to wait for that or just merge this first. Ill probably make a separate branch regardless to keep reviewing straightforward

[Daeda88]

Small update on it: I tried getting it to work, but its difficult for Javascript. Basically I can't do it without either exposing the wrapper class though the js value, or by making it an extended method still. I tried using delegation, which compiles, but will crash at runtime.

See https://github.com/splendo/firebase-kotlin-sdk/tree/feature/with-native

[nbransby]

In that case I think it would be best to change them all to extension properties and make this a breaking change, bumping the version to 2.0.0

[Daeda88]

See splendo#71

[nbransby]

See splendo#71

By boilerplate do you mean the single line per platform:
val SomePublicApi.js get() = ...
?

[Daeda88]

Also to make sure that all the import dev.gitlive.firebase.*.android can be replaced by a single import. And to get the pattern of having a native value standardized. It would've been more useful if I had been able to figure out the JS thing, but since I couldnt this is it. If you feel its overkill, the accessors already in this PR would basically do the same.

[nbransby]

I think what's already in the PR is fine but we need to change them to extensions everywhere so we won't need to make another breaking change in the future

[nbransby]

@Daeda88 merged this and made a start on changing them to extensions everywhere: #504

During this I noticed a number of your Native... classes are not internal due to the typealiases in android, can you think of another way round this? move into an internal package perhaps? Otherwise the Native... classes will leak into the public API

[Daeda88]

Well typealiases are inherently public I think. On the other pr I moved all of the Native Wrapper classes into an internal package though

[nbransby]

Which PR? @Daeda88

[Daeda88]

I meant this one splendo#71