Missing fields added to AuthResult #556

mr-kew · 2024-06-27T05:17:44Z

I added all the missing fields to AuthResult type and also made it's constructor public, so it can be used to pass data from platform specific methods back to commonMain.

Example:

I wanna use signInWithProvider which is available only in platformMain. It will probably never be available in commonMain because of the activity parameter. So I make a method for it. It takes commonMain type OAuthProvider.

Before this PR:

androidMain:

suspend fun main(activity: Activity) {
    val provider = getOAuthProvider()
    val result = Firebase.auth.signInWithProvider(activity, provider)
    processResult(result)
}

// This method cannot be moved into commonMain because of the [activity] parameter, but it should have commonMain parameters and return types, so the surrounding code can be moved into commonMain.
@Suppress("INVISIBLE_MEMBER", "INVISIBlE_REFERENCE") // This allows access to internal constructor and is bad
suspend fun FirebaseAuth.signInWithProvider(activity: Activity, provider: OAuthProvider) = suspendCoroutine { continuation ->
    this.android.startActivityForSignInWithProvider(activity, provider.android)
        .addOnSuccessListener { result ->
            val commonResult = AuthResult(result) // This is internal
            if (commonResult.user != null) {
                continuation.resume(commonResult)
            } else {
                continuation.resumeWithException(IllegalStateException())
            }
        }
        .addOnFailureListener {
            continuation.resumeWithException(it)
        }
}

commonMain:

// This code is the same for both/all platforms, so it makes sense to be in commonMain

fun getOAuthProvider() = ...

fun processResult(result: AuthResult) = ...

Issues

JVM version does not provide some fields at all. I used throw NotImplementedError() to communicate that in commonMain.
I have a problem with testing this. Can anyone give me some advice? I should verify whether all the fields are actually present in js, but I don't really know how to test that. With iOS and android it should be fine as the result is properly typed.

Adds missing fields to AuthResult and makes its constructor public

# Conflicts: # firebase-auth/src/androidMain/kotlin/dev/gitlive/firebase/auth/auth.kt # firebase-auth/src/commonMain/kotlin/dev/gitlive/firebase/auth/auth.kt # firebase-auth/src/iosMain/kotlin/dev/gitlive/firebase/auth/auth.kt # firebase-auth/src/jsMain/kotlin/dev/gitlive/firebase/auth/auth.kt # firebase-auth/src/jsMain/kotlin/dev/gitlive/firebase/auth/externals/auth.kt # firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt

firebase-auth/src/iosMain/kotlin/dev/gitlive/firebase/auth/auth.kt

nbransby · 2024-06-30T15:25:17Z

firebase-auth/src/androidMain/kotlin/dev/gitlive/firebase/auth/auth.kt

@@ -113,9 +113,28 @@ public actual class FirebaseAuth internal constructor(public val android: com.go
    public actual fun useEmulator(host: String, port: Int): Unit = android.useEmulator(host, port)
 }

-public actual class AuthResult internal constructor(public val android: com.google.firebase.auth.AuthResult) {
+public actual class AuthResult(


why cant the constructor stay internal?

I just feel like there is no need for it to be internal. Having it public just allows more flexibility for the library. People can define their own extensions for stuff that is platform specific etc (like my example) and still keep the common types from the library.

I mean sure, in my example I could redefine the AuthResult in my own code and use that instead and this PR kinda removes the need for that code all together. But still the constructor being internal seems like an unnecessary obstacle forcing me to write something that is already there, while offering not much benefit elsewhere.

It's definitely up to discussion, depends if there are any more serious reasons for it to be internal in the first place.

nbransby · 2024-06-30T15:27:07Z

Thanks for this. Issue 1 is fine. Issue 2 you can just write a test to get the new fields you've added and assert they are the values you expect, the test will run on all platforms and will fail on JS if you have got a property name wrong

mr-kew · 2024-06-30T17:26:56Z

@nbransby

Thanks for this. Issue 1 is fine. Issue 2 you can just write a test to get the new fields you've added and assert they are the values you expect, the test will run on all platforms and will fail on JS if you have got a property name wrong

Well the problem is that I don't really know what values to expect from the testing FirebaseApp. Also should the tests be running on my device? They are all crashing on FirebaseNetworkException for me. I never worked with kmp js and dont have much experience with this library either, so I really have no idea what to do.

nbransby · 2024-07-01T02:29:11Z

Ok well commit the test, let the CI run it and we'll take it from there

nbransby · 2024-07-12T12:16:06Z

@mr-kew you can review the test reports now to see what is failing, also maybe add some messages to the assert calls so you know which ones are failing. Also the lint is failing too

mr-kew · 2024-07-15T09:09:02Z

@nbransby Hm, I have no idea why the lint fails. It fails in jvmMain, but I really didn't do many changes there. It's mostly empty computed properties with throw NotImplementedError() inside.

The tests are failing on the first assert for credential. I think the problem is that all of the added fields are optional and therefore doesn't have to be provided by the firebase in all cases. I think modifying the testing FirebaseApp so it actually returns all the additional info, will be needed.

nbransby · 2024-07-15T09:14:21Z

scroll up and it tells you what the issue are:

/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:132:17: Lint error > [standard:statement-wrapping] Missing newline after '{'
> Task :firebase-firestore:lintKotlinAndroidUnitTest
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:132:45: Lint error > [standard:statement-wrapping] Missing newline before '}'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:134:17: Lint error > [standard:statement-wrapping] Missing newline after '{'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:134:45: Lint error > [standard:statement-wrapping] Missing newline before '}'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:139:17: Lint error > [standard:statement-wrapping] Missing newline after '{'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:139:45: Lint error > [standard:statement-wrapping] Missing newline before '}'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:141:17: Lint error > [standard:statement-wrapping] Missing newline after '{'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:141:45: Lint error > [standard:statement-wrapping] Missing newline before '}'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:143:17: Lint error > [standard:statement-wrapping] Missing newline after '{'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:143:45: Lint error > [standard:statement-wrapping] Missing newline before '}'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:145:17: Lint error > [standard:statement-wrapping] Missing newline after '{'
/home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/src/jvmMain/kotlin/dev/gitlive/firebase/auth/auth.kt:145:45: Lint error > [standard:statement-wrapping] Missing newline before '}'

nbransby · 2024-07-15T09:14:50Z

I think modifying the testing FirebaseApp so it actually returns all the additional info, will be needed.

How do we do this?

mr-kew · 2024-07-15T09:49:12Z

I think modifying the testing FirebaseApp so it actually returns all the additional info, will be needed.

How do we do this?

I have no idea, I was hoping you would help me with that. It seems the tests are accessing some regular firebase project, but I don't have a permissions (and don't really want them) to do anything with that.

nbransby · 2024-07-15T09:55:18Z

Yes I can edit it just not sure what you need doing

mr-kew · 2024-07-15T10:06:37Z

Yes I can edit it just not sure what you need doing

Hmm, I'm really not entirely sure either. In my project, the additional info & credential come from the SSO. To test it we are using Okta, which then after signInWithProvider returns given additional info. I did not set it up, so I sadly don't know much more about it.

From the Firebase docs it looks like the additional info is indeed not null only while using the signInWithProvider. I was hoping it could be done using regular email+password sign in, but sadly it seems not, so you would have to setup something like that.

Additional issue is that the okta sign in redirects to okta sign in form in browser, so idk if it is possible in unit tests without running the app.

nbransby · 2024-07-15T10:14:27Z

Ok well for now I guess we can simply update the test to confirm the values are null - that will at least assert no exceptions are thrown

mr-kew · 2024-07-15T10:21:34Z

Yeah, that makes sense. If someone discovers it's not working later, it can be solved as a separate issue.

Tests for null using optional unwrap don't make much sense

mr-kew · 2024-07-16T12:23:58Z

@nbransby Android & JVM tests are now passing, but iOS and JS does not show my assert messages so I don't know how exactly is the test failing. Can I somehow access the file with results?

There were failing tests. See the report at: file:///Users/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-auth/build/reports/tests/iosSimulatorArm64Test/index.html

nbransby · 2024-07-17T03:07:08Z

You can find the test reports here: https://github.com/GitLiveApp/firebase-kotlin-sdk/actions/runs/9940703433?pr=556#artifacts

Exact values differ on different platforms

nbransby · 2024-07-25T13:23:14Z

JS tests still falling with
FirebaseAuthException: app/app-deleted: Firebase: Firebase App named '[DEFAULT]' already deleted (app/app-deleted).

firebase-auth/src/jsMain/kotlin/dev/gitlive/firebase/auth/externals/auth.kt

mr-kew · 2024-08-16T14:11:04Z

@nbransby Yeah, it might be JSON, nice find.

But from the exception it seems like there is an issue with FirebaseApp initialization in the tests. So the test itself does not even run, I think. So I still can't verify it.

mr-kew · 2024-08-28T14:39:58Z

@nbransby So I fixed the js tests (it was about the position of the test code in a file), but I had to make them more forgiving because in JS the additionalUserInfo is not provided at all, so I cannot test it's properties.

But I think it is fine

If someone discovers it's not working later, it can be solved as a separate issue.

nbransby · 2024-08-28T15:37:27Z

Ok, btw there are conflicts you'll need to resolve before we can merge it in

firebase-auth/src/androidMain/kotlin/dev/gitlive/firebase/auth/auth.kt

firebase-auth/src/iosMain/kotlin/dev/gitlive/firebase/auth/auth.kt

firebase-auth/src/jsMain/kotlin/dev/gitlive/firebase/auth/auth.kt

mr-kew · 2024-08-30T14:16:57Z

@nbransby Really thank you for all the help. It was quite complicated and I wouldn't have managed it all on my own.

nbransby · 2024-08-31T03:59:41Z

No problem, it was 99% all you!

mr-kew and others added 4 commits June 27, 2024 06:45

Missing fields added to AuthResult

377b887

Adds missing fields to AuthResult and makes its constructor public

Merge branch 'master' into auth-result

4885a5d

Merge branch 'master' into auth-result

dcd28c7

nbransby reviewed Jun 30, 2024

View reviewed changes

firebase-auth/src/iosMain/kotlin/dev/gitlive/firebase/auth/auth.kt Outdated Show resolved Hide resolved

nbransby reviewed Jun 30, 2024

View reviewed changes

Removed unused import

31d8466

mr-kew and others added 2 commits July 1, 2024 06:58

AuthResult test

37867cf

Merge branch 'master' into auth-result

3a7f273

mr-kew added 2 commits July 15, 2024 10:01

Added messages to asserts in AuthResult test

93b9dda

JVM lint issues fixed

ef5e5d9

JVM lint issues properly fixed

be998c3

Public api updated

f2c2c51

mr-kew added 5 commits July 15, 2024 12:31

Public api fix

ed24041

AuthResult structure test allows null values

434abaa

AuthResult structure test unwrap fixed

84cdf2c

Tests for null using optional unwrap don't make much sense

AuthResult structure additionalUserInfo exists

a18e3f2

AuthResult structure isNewUser default respected

79a7516

AuthResult structure test made more forgiving

ac4cffa

Exact values differ on different platforms

nbransby reviewed Jul 25, 2024

View reviewed changes

firebase-auth/src/jsMain/kotlin/dev/gitlive/firebase/auth/externals/auth.kt Outdated Show resolved Hide resolved

mr-kew added 4 commits August 16, 2024 16:13

Merge branch 'master' into auth-result

277e7f5

AuthResult map parsing fixed in JS

e5af67d

Used Json? instead od dynamic for better clarity

9681cf4

AuthResult structure JS test fixed

fad3664

mr-kew added 3 commits August 29, 2024 08:03

Merge branch 'master' into auth-result

60e42c0

Merge cleanup

b566e31

API dump

1a9a14a

nbransby reviewed Aug 29, 2024

View reviewed changes

firebase-auth/src/androidMain/kotlin/dev/gitlive/firebase/auth/auth.kt Outdated Show resolved Hide resolved

nbransby reviewed Aug 29, 2024

View reviewed changes

firebase-auth/src/iosMain/kotlin/dev/gitlive/firebase/auth/auth.kt Outdated Show resolved Hide resolved

nbransby reviewed Aug 29, 2024

View reviewed changes

firebase-auth/src/jsMain/kotlin/dev/gitlive/firebase/auth/auth.kt Outdated Show resolved Hide resolved

Native properties moved into extensions

3eb1a0c

nbransby approved these changes Aug 30, 2024

View reviewed changes

nbransby added 2 commits August 30, 2024 12:32

Merge branch 'master' into auth-result

ecfa47d

Update auth.kt

9e8ef04

nbransby merged commit fc7cb62 into GitLiveApp:master Aug 30, 2024
50 checks passed

mr-kew deleted the auth-result branch August 30, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing fields added to AuthResult #556

Missing fields added to AuthResult #556

mr-kew commented Jun 27, 2024 •

edited

Loading

nbransby Jun 30, 2024

mr-kew Jun 30, 2024

nbransby commented Jun 30, 2024

mr-kew commented Jun 30, 2024

nbransby commented Jul 1, 2024

nbransby commented Jul 12, 2024

mr-kew commented Jul 15, 2024

nbransby commented Jul 15, 2024

nbransby commented Jul 15, 2024

mr-kew commented Jul 15, 2024

nbransby commented Jul 15, 2024

mr-kew commented Jul 15, 2024 •

edited

Loading

nbransby commented Jul 15, 2024

mr-kew commented Jul 15, 2024

mr-kew commented Jul 16, 2024

nbransby commented Jul 17, 2024

nbransby commented Jul 25, 2024

mr-kew commented Aug 16, 2024

mr-kew commented Aug 28, 2024 •

edited

Loading

nbransby commented Aug 28, 2024

mr-kew commented Aug 30, 2024

nbransby commented Aug 31, 2024

Missing fields added to AuthResult #556

Missing fields added to AuthResult #556

Conversation

mr-kew commented Jun 27, 2024 • edited Loading

Example:

Issues

nbransby Jun 30, 2024

Choose a reason for hiding this comment

mr-kew Jun 30, 2024

Choose a reason for hiding this comment

nbransby commented Jun 30, 2024

mr-kew commented Jun 30, 2024

nbransby commented Jul 1, 2024

nbransby commented Jul 12, 2024

mr-kew commented Jul 15, 2024

nbransby commented Jul 15, 2024

nbransby commented Jul 15, 2024

mr-kew commented Jul 15, 2024

nbransby commented Jul 15, 2024

mr-kew commented Jul 15, 2024 • edited Loading

nbransby commented Jul 15, 2024

mr-kew commented Jul 15, 2024

mr-kew commented Jul 16, 2024

nbransby commented Jul 17, 2024

nbransby commented Jul 25, 2024

mr-kew commented Aug 16, 2024

mr-kew commented Aug 28, 2024 • edited Loading

nbransby commented Aug 28, 2024

mr-kew commented Aug 30, 2024

nbransby commented Aug 31, 2024

mr-kew commented Jun 27, 2024 •

edited

Loading

mr-kew commented Jul 15, 2024 •

edited

Loading

mr-kew commented Aug 28, 2024 •

edited

Loading