-
Notifications
You must be signed in to change notification settings - Fork 183
Feat kmp sdk #1040
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feat kmp sdk #1040
Conversation
…to feat-kmp-sdk
…to feat-kmp-sdk
…to feat-kmp-sdk
Certain data classes weren't serializing properly, moved actualSerializer to be above apiParams, added json encoding to data.
The socket was being created unnecessarily whenever a subscription was removed. This change ensures that the socket is only created when there are active channels.
templates/kmp/settings.gradle.kts
Outdated
} | ||
} | ||
|
||
rootProject.name = "Appwrite_KMP_SDK" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name this sdk-for-kmp
} else { | ||
engine { | ||
config { | ||
val trustManagerFactory = | ||
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()) | ||
trustManagerFactory.init(null as KeyStore?) | ||
val sslContext = SSLContext.getInstance("TLS").apply { | ||
init(null, trustManagerFactory.trustManagers, SecureRandom()) | ||
} | ||
sslSocketFactory( | ||
sslContext.socketFactory, | ||
trustManagerFactory.trustManagers[0] as X509TrustManager | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this else block be skipped? Is it the same as a the default behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with certificates, but from my understanding this uses a PKIX algorithm instead of Trust All. I'm not sure if that's the correct approach, but I figured if there was an option to not run setSelfSigned
then it should use a standard algorithm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the else block, since the behavior when not setting self-signed will be the same with or without it 👌
@@ -466,7 +466,7 @@ protected function getReturnType(array $method, array $spec, string $namespace, | |||
|
|||
$ret = $this->toPascalCase($method['responseModel']); | |||
|
|||
if ($this->hasGenericType($method['responseModel'], $spec)) { | |||
if ($this->hasGenericType($method['responseModel'], $spec) && $withGeneric) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for skipping the generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for situations like this:
serializer = io.appwrite.models.Document.serializer(actualSerializer)
without the skip I would get serializer = io.appwrite.models.Document<T>.serializer(actualSerializer)
I just found how it's been done for the same situation:
{{sdk.namespace | caseDot}}.models.{{ method.responseModel | caseUcfirst }}
I can change it to match the original method and get rid of this change if you like that better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep let's do it so we can handle both generics and non-generics in one shot
templates/android/library/src/main/java/io/package/WebAuthComponent.kt.twig
Outdated
Show resolved
Hide resolved
private val appVersion by lazy { | ||
try { | ||
val properties = Properties() | ||
properties.load(this.javaClass.getResourceAsStream("/version.properties")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe? Can we guarantee the file will always exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in a position to get the JVM stuff working properly right now so I was thinking of removing it entirely. I added it for a quick test that I was doing so I just did a port from the androidMain to jvmMain with chatGPT. I wasn't planning on this PR being a v1.0 or anything so I thought I'd add my JVM stuff to get the ball rolling. Let me know if you think JVM should just be removed in this case. It shouldn't be too difficult to add back since it's so similar to the android side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, we can remove for now and add other platform support in phases 👌
"origin" to "appwrite-jvm://app", | ||
"content-type" to "application/json", | ||
"origin" to "{{ spec.title | caseLower }}-jvm://app", | ||
"user-agent" to "JVM/$appVersion, ${System.getProperty("java.version")}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the first origin
templates/php/tests/IDTest.php.twig
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's changed in these php tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing, I think they got caught in me updating the line endings. I'll revert them
'chmod +x tests/sdks/kmp/gradlew', | ||
]; | ||
protected string $command = | ||
'docker run --rm --platform linux/amd64 --network="mockapi" -v $(pwd):/app -w /app/tests/sdks/kmp alvrme/alpine-android:android-34-jdk17 sh -c "./gradlew :shared:testDebugUnitTest --stacktrace -q && cat shared/result.txt"'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to specify platform here? I think this image is multi-arch right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for iOS and JVM too. We can also add them in .github/workflows/tests.yml so they run for each PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any luck here?
…to feat-kmp-sdk
…ded, Fix http/https check in Client
This commit refactors the OAuth2 authentication flow on iOS to use `ASWebAuthenticationSession` instead of `UIApplication.openURL`. This provides a more secure and user-friendly authentication experience. Key changes: - `WebAuthComponent.ios.kt`: - Replaced `UIApplication.openURL` with `ASWebAuthenticationSession`. - Added `PresentationContextProviderImpl` to provide a presentation anchor for the authentication session. - Updated `PendingAuth` to store the `ASWebAuthenticationSession`. - Improved error handling and cancellation. - Added `setCookieStorage` to allow passing the cookie storage instance. - Modified `handleIncomingCookie` to parse cookie details from the callback URL and store them using the provided `IosCookieStorage` and `NSUserDefaults`. - Added a `cleanup` method to cancel pending authentication sessions. - `OAuth2Extensions.kt`: - Updated `createOAuth2Session` and `createOAuth2Token` to use the new `WebAuthComponent` and pass the `iosCookieStorage`. - Ensured cookies are correctly constructed and stored. - Improved error handling for missing authentication cookies.
sourceSets { | ||
commonMain { | ||
dependencies { | ||
implementation("io.github.camka14.appwrite:sdk-for-kmp:0.1.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's template the group name and version here
@@ -0,0 +1,3 @@ | |||
<resources> | |||
<string name="app_name">Appwrite KMP SDK</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's template the values here
@@ -0,0 +1,16 @@ | |||
<resources xmlns:tools="http://schemas.android.com/tools"> | |||
<!-- Base application theme. --> | |||
<style name="Theme.AppwriteKMPSDK" parent="Theme.MaterialComponents.DayNight.DarkActionBar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's template
import okio.Path.Companion.toPath | ||
|
||
actual class Client constructor( | ||
private val context: Context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts here? If users don't pass application context specifically this will be a memory leak if the client outlives the context
templates/kmp/shared/src/androidMain/kotlin/io/package/HttpClientConfig.kt.twig
Show resolved
Hide resolved
storage = dataStoreCookieStorage | ||
} | ||
install(WebSockets) { | ||
pingInterval = 30.seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this integrate with our custom ping 🤔
val properties = mutableMapOf<Any?, Any?>( | ||
NSHTTPCookieName to storedCookie.name, | ||
NSHTTPCookieValue to storedCookie.value, | ||
NSHTTPCookiePath to storedCookie.path, | ||
NSHTTPCookieDomain to storedCookie.domain | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets also handle expires, max-age, samesite, httponly, and secure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OAuth methods should be handled in the service template so they appear along other Account
methods
} | ||
|
||
result.getOrNull()?.let { callbackUrl -> | ||
runBlocking { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid runBlocking here
private val appVersion by lazy { | ||
try { | ||
val properties = Properties() | ||
properties.load(this.javaClass.getResourceAsStream("/version.properties")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, we can remove for now and add other platform support in phases 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any luck here?
What does this PR do?
Adds a KMP SDK
Test Plan
Run KMPAndroid14Java17Test
Test OAuth2, cookies, and iOS file uploads using custom project in Android Studio
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
yes