Kotlin: allow pluggable HTTP clients #1381

tjbanghart · 2023-10-13T06:17:31Z

👋👋 Thank you for contributing to Looker sdk-codegen (⚡️🍣)

👆 Make sure your pull request title follows Pull Request Title Guidelines from our Contribution guide
👉 Don't forget to replace these instructions with your ✨awesome✨ description of what this change actually does. Additionally, it's great to include context on how it works and why the change was needed.
👇 Edit "Developer Checklist" to reflect items relevant to this PR (and try to make sure to check everything off before asking for review)

Developer Checklist ℹ️

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Appropriate docs were updated (if necessary)

Fixes #1380 🦕

tjbanghart · 2023-10-14T18:00:44Z

kotlin/build.gradle

-    implementation "io.ktor:ktor-client:$ktorVersion"
-    implementation "io.ktor:ktor-client-okhttp:$ktorVersion"
-    implementation "io.ktor:ktor-client-json:$ktorVersion"
-    implementation "io.ktor:ktor-client-gson:$ktorVersion"


Removed for now but can add back if we decide to have a low-level ktor client to make the requests

tjbanghart · 2023-10-14T18:02:04Z

kotlin/src/main/com/looker/rtl/Transport.kt

+    APACHE("Apache HTTP Client"),
+    JAVA_NET("Native Java HTTP Client"),
+    // URL_FETCH("Google App Engine HTTP Client"), TODO: App Engine support? Requires App Engine SDK.
+    // KTOR("Kotlin based HTTP Client") TODO: Add ktor transport wrapper.


Is there a requirement to use ktor? If so I can make the wrapper class.

Apache and java.net are fairly standard.

There's not a requirement to use ktor although that appears to be the more "standard" HTTP client for Kotlin. Have you looked at the dependency size impact of the Apache and/or Java HTTP clients? Is Ktor a layer on top of Java HTTP? I haven't looked since we implemented the original SDK.

Prior to this change we were using the OkHttp engine but ktor supports several types including Apache: https://ktor.io/docs/http-client-engines.html.

In my understanding ktor and the Google HTTP Client do similar things in that they abstract "low-level" HTTP request/responses handling. ktor provides more Kotlin specific features, like coroutines, but I am not sure what else is gained. Both have Android specific transports as well if that is a requirement.

Ah. I think @brianmadden I talked about coroutines for ktor and that's why we wanted to use it, based on what he'd done in Krawler. That was a long time ago, though, so I'm not sure at this point.

jkaster · 2023-10-14T18:06:09Z

Did you run the kotlin smoke tests with this change?

…

On Sat, Oct 14, 2023, 7:02 PM TJ Banghart ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In kotlin/src/main/com/looker/rtl/Transport.kt <#1381 (comment)> : > - HEAD(io.ktor.http.HttpMethod.Head) - // TODO: Using the ktor-client-apache may support TRACE? +enum class HttpMethod { + GET, + POST, + PUT, + DELETE, + PATCH, + HEAD +} + +enum class HttpTransports(val label: String) { + APACHE("Apache HTTP Client"), + JAVA_NET("Native Java HTTP Client"), + // URL_FETCH("Google App Engine HTTP Client"), TODO: App Engine support? Requires App Engine SDK. + // KTOR("Kotlin based HTTP Client") TODO: Add ktor transport wrapper. Is there a requirement to use ktor? If so I can make the wrapper class. Apache and java.net are fairly standard. — Reply to this email directly, view it on GitHub <#1381 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABORGARTFKRED2GVK74252DX7LHSPAVCNFSM6AAAAAA56UT4H2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNZYGI3TENRXGM> . You are receiving this because your review was requested.Message ID: ***@***.***>

tjbanghart · 2023-10-14T20:11:56Z

Ah, let me double check that. I ran the unit tests in intellij but should probably use the script. I'll mark as WIP and test it out again.

tjbanghart · 2023-10-16T23:13:14Z

Okay, passing smoke tests but one issue with warnings about invalid cookies. I left a comment on the TODO

tjbanghart · 2023-10-16T23:19:54Z

kotlin/src/main/com/looker/rtl/Transport.kt

+                // TODO: fix bug upstream that does not pass client context to requests.
+                //  The Netscape `expire` datetime format is not compatible with the "default" and
+                //  will log invalid warnings.
+                val clientBuilder =


The response cookies we get back from Looker use the Wdy, DD-Mon-YY HH:MM:SS GMT format for expires. We should be able to push down the config here to the low-level Apache request but the request does not reference the client context. I believe it is a bug in the Google Java Client.

Note, this doesn't stop requests, but rather logs a confusing error. We still get a correct response back.

Alternatively, we could use disableCookieManagement() so the low-level client doesn't try to validate cookies. This prevents the warning and we get the desired response. The SDK usually shouldn't care about cookies right? Wouldn't the required auth token header make cookies redundant?

We definitely have responses that would set cookies and those would be needed (the ironically named cookieless embed is one example) but this could be a case of worrying about it when we discover someone needs it. The only concern I have is whether the Looker Android mobile client reads any cookies from Looker, but it probably doesn't? I've not really looked at how cookies are used in non-browser situations and someone could implement a custom payload parser if it really was required as long as we have the hooks available to get the original request.

Do we have a test that verifies a payload with missing "required" fields still gets parsed correctly? Or are you still using GSon for parsing with whichever client? I'll look at the code now that I'm on a bigger screen.

jkaster

I have questions but nothing that blocks approval for this PR

jkaster · 2023-10-17T22:12:18Z

kotlin/src/main/com/looker/rtl/APIMethods.kt

@@ -41,27 +41,27 @@ open class APIMethods(val authSession: AuthSession) {
        }
    }

-    inline fun <reified T> get(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {
+    inline fun <reified T : Any> get(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {


was this update required by the new Kotlin version? Just curious because it doesn't look like the Kotlin version got updated.

jkaster · 2023-10-17T22:14:11Z

kotlin/src/main/com/looker/rtl/ErrorDoc.kt

@@ -30,7 +30,7 @@ class ErrorDocItem(var url: String)
 /** Structure of the error code document index */
 typealias ErrorCodeIndex = HashMap<String, ErrorDocItem>

- interface IErrorDocLink {
+interface IErrorDocLink {


thanks for the reformatting correction. Weird we had leading spaces in the interface declarations.

jkaster · 2023-10-17T22:18:53Z

kotlin/src/main/com/looker/rtl/GsonObjectParser.kt.kt

+import java.lang.reflect.Type
+import java.nio.charset.Charset
+
+/** Custom GSON based parser for deserialization. */


Can you document why this is required now?

tjbanghart force-pushed the tjbanghart/google-java-http branch 2 times, most recently from 3157dc4 to 58c10e0 Compare October 14, 2023 17:59

tjbanghart requested a review from jkaster October 14, 2023 18:00

tjbanghart commented Oct 14, 2023

View reviewed changes

tjbanghart requested review from jkaster and removed request for jkaster October 14, 2023 20:12

tjbanghart commented Oct 16, 2023

View reviewed changes

jkaster previously approved these changes Oct 17, 2023

View reviewed changes

tjbanghart added 4 commits October 17, 2023 16:43

Replace ktor with Google Java HTTP client

e51153d

Remove ktor and clean up methods

68440de

Fix error handling and cookie changes

95dc732

Disable cookie managment for Apache transport

ec84830

tjbanghart dismissed jkaster’s stale review via ec84830 October 17, 2023 23:44

tjbanghart force-pushed the tjbanghart/google-java-http branch from 140c013 to ec84830 Compare October 17, 2023 23:44

Rebase sdk/4.0 changes

9b6f85c

jkaster approved these changes Oct 17, 2023

View reviewed changes

tjbanghart merged commit 5407e63 into main Oct 18, 2023
11 checks passed

tjbanghart deleted the tjbanghart/google-java-http branch October 18, 2023 02:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kotlin: allow pluggable HTTP clients #1381

Kotlin: allow pluggable HTTP clients #1381

tjbanghart commented Oct 13, 2023 •

edited

tjbanghart Oct 14, 2023

tjbanghart Oct 14, 2023

jkaster Oct 17, 2023

tjbanghart Oct 17, 2023

jkaster Oct 17, 2023

jkaster commented Oct 14, 2023 via email

tjbanghart commented Oct 14, 2023

tjbanghart commented Oct 16, 2023

tjbanghart Oct 16, 2023 •

edited

jkaster Oct 17, 2023

jkaster left a comment

jkaster Oct 17, 2023

jkaster Oct 17, 2023

jkaster Oct 17, 2023

Kotlin: allow pluggable HTTP clients #1381

Kotlin: allow pluggable HTTP clients #1381

Conversation

tjbanghart commented Oct 13, 2023 • edited

👋👋 Thank you for contributing to Looker sdk-codegen (⚡️🍣)

Developer Checklist ℹ️

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkaster commented Oct 14, 2023 via email

tjbanghart commented Oct 14, 2023

tjbanghart commented Oct 16, 2023

tjbanghart Oct 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjbanghart commented Oct 13, 2023 •

edited

tjbanghart Oct 16, 2023 •

edited