Add http transport #449

luqasn · 2021-06-18T19:47:53Z

addresses #370

Implementation based on the original thrift http transport with a few changes:

apache http client variant removed
- it was weird to have multiple different implementations in the same class
- I did not want to introduce an additional dependency
- okio is already a dependency, so maybe we want to base the transport on that?
tried to make the code a bit more kotlin idiomatic and safer

Even though there are no tests yet I wanted to collect some early feedback, especially on okio vs. HTTPUrlConnection. This needs to live in the jvm variant due to the availability of HTTPUrlConnection.

codecov-commenter · 2021-06-18T19:52:06Z

Codecov Report

Merging #449 (af78d7d) into master (e218285) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #449   +/-   ##
=========================================
  Coverage     59.52%   59.53%           
  Complexity      796      796           
=========================================
  Files            60       60           
  Lines          5534     5535    +1     
  Branches        876      876           
=========================================
+ Hits           3294     3295    +1     
  Misses         1996     1996           
  Partials        244      244

Impacted Files	Coverage Δ
.../com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt	`82.35% <0.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e218285...af78d7d. Read the comment docs.

benjamin-bader

This is a great start - thanks for picking this up! There are a few things I think we could change to make this fit seamlessly in with the rest of the project; noted inline. Also, I'd really like to see some sort of testing here. OkHttp's MockWebServer could be very handy for this, assuming it's still maintained and available (been a few years since I used it).

benjamin-bader · 2021-06-20T08:25:30Z

thrifty-runtime/src/jvmMain/kotlin/com/microsoft/thrifty/transport/THttpTransport.kt

+ *
+ * @see [THRIFT-970](https://issues.apache.org/jira/browse/THRIFT-970)
+ */
+open class THttpTransport(url: String) : Transport {


It's petty, but let's drop the T prefix. It's just ugly to me and, is out-of-step with the rest of the project, and (perhaps most importantly) doesn't deliver any actual information.

For context, the CompactProtocol class specifically is heavily based on TCompactProtocol but even there, the prefix had to go. TType retains its prefix only because Type is taken, and "T for Thrift" actually makes sense there.

benjamin-bader · 2021-06-21T04:01:12Z

thrifty-runtime/src/jvmMain/kotlin/com/microsoft/thrifty/transport/THttpTransport.kt

+ *
+ * @see [THRIFT-970](https://issues.apache.org/jira/browse/THRIFT-970)
+ */
+open class THttpTransport(url: String) : Transport {


Also, a substantive comment - can we please make an expect class in src/commonMain/kotlin/com/microsoft/thrifty/transport for this? Then make this one the actual?

Generally on JVM I favor using OkHttp over HttpURLConnection - it's much easier to use correctly and scalably. Is there a reason, aside from the initial implementation using it, to favor HttpURLConnection?

I suppose the fact that it's built in to the JDK is a point in its favor, along with not wanting to bring in extra dependencies. This is part of why I suggested making a separate module for this guy.

What benefit do we get from making it an expect/actual class? Why can't there just be different types of Transports implemented for different variants? This is a genuine question, I have no experience with Kotlin Multiplatform.

Yes, main reason is OOB-ness. I don't want to add something to the runtime that drags in a bunch of transitive dependencies. For that I would prefer the separate module. But I see nothing wrong with providing a basic HttpTransport with maybe not the best performance that needs no dependencies and is this simple as part of the "core" library.

Fair point re: "OOB-ness". My concern definitely is around the default easy path. Nothing's easier, dependency-wise, than what's built in. On the other hand, this is an Android library first and foremost, and on Android OkHttp is basically the standard anymore. I'm OK with leaving it out for now.

As far as expect/actual, this is the mechanism that lets us have interfaces in common code but have platform-specific implementations. In the eventual iOS release, we'll definitely want to use an HttpTransport. Having an expect class HttpTransport in common code is a prerequisite.

I was trying to understand the benefit of the expect over a plain interface for this usecase. We can already have different implementations of the http transport, they would all just implement Transport.

So if I understand correctly, expect just tells you "whatever platform this is ported to also needs to implement this thing", which to me is mainly useful for the "common" library to be able to call platform specific code without having the user pass an implementation to some constructor explicitly (there is always going to be exactly one implementation, no more, no less).
To me, plain old interface still sounds totally fitting to me for this use case. Consider the case that we add an OkHttpClient in a separate library as well. Would that then work with the expect? Or would this now be a "plain" implementation of Transport? If yes, why treat both implementations differently?

Please excuse all the text, I am mainly trying to wrap my head around Kotlin Multiplatform.

You're right that we could just have platform-specific types that implement Transport. The thing I think lacks there is that we won't have a single HttpTransport. That seems like a miss to me.

I never addressed you question about the benefit(s) of expect class over an interface. The difference is that an expect class is concrete - one can instantiate one as-is, without needing to consider what other type might implement the interface.

benjamin-bader · 2021-06-21T04:02:21Z

thrifty-runtime/src/jvmMain/kotlin/com/microsoft/thrifty/transport/TTransportException.kt

@@ -0,0 +1,6 @@
+package com.microsoft.thrifty.transport
+
+class TTransportException : Throwable {


I tend to think this exception doesn't buy us anything, and would prefer not to include it. We could use the perfectly-good ProtocolException in its stead.

ProtocolException unfortunately does not have the possibility to pass a cause (doesn't have that constructor implemented), and I really don't like wrapping exceptions in a way that loses information.
But I do agree with you that this does not provide much value, so I will go for just not catching and rethrowing these wrapped.

luqasn · 2021-06-22T21:04:11Z

This is a great start - thanks for picking this up! There are a few things I think we could change to make this fit seamlessly in with the rest of the project; noted inline. Also, I'd really like to see some sort of testing here. OkHttp's MockWebServer could be very handy for this, assuming it's still maintained and available (been a few years since I used it).

Yes, I am definitely planning to write tests still!

juequinterore · 2023-08-11T22:09:25Z

Hi.

I would like to ask, is there any plan to continue with this work?

luqasn · 2023-10-08T07:44:58Z

Sorry for the late reply. I picked this up again and have integration tests ready, but there are still issues with the plumbing (extension initialization order).
Will try to finish this ASAP.

juequinterore · 2023-10-08T11:35:19Z

Thanks for your work @luqasn! I have been using this code since couple of months ago and it has been working without any issue.

…

On Sun, 8 Oct 2023 at 2:45 AM Lucas Romero ***@***.***> wrote: Sorry for the late reply. I picked this up again and have integration tests ready, but there are still issues with the plumbing (extension initialization order). Will try to finish this ASAP. — Reply to this email directly, view it on GitHub <#449 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAW4X2LW2CWXTB3RNXPJXTX6JKYLAVCNFSM466HEBLKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZVGE4TKMBZGUYQ> . You are receiving this because you commented.Message ID: ***@***.***>

for spinning up a thrift http server for integration testing. The servlet namespace was renamed from javax.servlet to jakarta.servlet so that's why we need the newer version.

…tion in order to be able to introduce new, http based server for testing as well.

luqasn · 2023-10-09T18:24:26Z

Long time no see, @benjamin-bader :)

I finally finished the integration tests I promised, could you have a look? Thanks!

benjamin-bader · 2023-10-12T04:07:21Z

Hey, glad to see you @luqasn! The test code is pretty substantial - thanks for digging in to it. I don't know why the tests didn't run earlier, given that you've made PRs here before, but I just had to re-approve running the test suite. It failed the license-header check, which I fixed here.

I'm inclined to take this large PR as-is. Thanks very much for all of your hard work on it, and for coming back to finish it!

I might come back and refactor HttpTransport into an expect/actual type, so that we can use this in iOS (we have some rudimentary iOS support now), but that's somewhere off in the future.

benjamin-bader · 2023-10-12T18:44:09Z

Also fair warning for anyone following this PR - Microsoft has changed the way they handle publishing permissions to Maven Central, but started the tooling migration before Github support was done. This leaves Thrifty temporarily unable to make releases. I hear that in another month or so, this should be fixed, but in the meantime please be patient.

luqasn · 2023-10-12T21:13:29Z

Thanks for letting us know, I think after those two years one month more is not gonna bother anyone ;)

Anyone in a pinch could also use jitpack.io to get a "prerelease" version of thrifty-runtime. Brilliant service, though I can't vouch for their security. Be mindful of that.

Add http transport

af78d7d

benjamin-bader requested changes Jun 21, 2021

View reviewed changes

Implement review feedback

c89653b

Merge branch 'microsoft:master' into httptransport

78a05a3

luqasn added 3 commits October 9, 2023 20:13

Update libthrift to get properly working servlet support

d973500

for spinning up a thrift http server for integration testing. The servlet namespace was renamed from javax.servlet to jakarta.servlet so that's why we need the newer version.

refactor: move socket based test server implementation behind abstrac…

a873829

…tion in order to be able to introduce new, http based server for testing as well.

Add integration test for http protocol (client)

b11d796

Add missing license headers

ac20ca9

benjamin-bader approved these changes Oct 12, 2023

View reviewed changes

benjamin-bader merged commit 436e7f2 into microsoft:master Oct 12, 2023
3 checks passed

luqasn deleted the httptransport branch October 14, 2023 15:16

luqasn mentioned this pull request Nov 16, 2023

Support for THttpClient Transport #370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add http transport #449

Add http transport #449

luqasn commented Jun 18, 2021 •

edited

Loading

codecov-commenter commented Jun 18, 2021

benjamin-bader left a comment

benjamin-bader Jun 20, 2021

benjamin-bader Jun 20, 2021

benjamin-bader Jun 21, 2021

luqasn Jun 22, 2021

luqasn Jun 22, 2021

benjamin-bader Jun 22, 2021

luqasn Jun 24, 2021

benjamin-bader Jun 24, 2021

benjamin-bader Aug 10, 2021

benjamin-bader Jun 21, 2021

luqasn Jun 22, 2021

benjamin-bader Jun 22, 2021

luqasn commented Jun 22, 2021

juequinterore commented Aug 11, 2023

luqasn commented Oct 8, 2023

juequinterore commented Oct 8, 2023 via email

luqasn commented Oct 9, 2023

benjamin-bader commented Oct 12, 2023

benjamin-bader commented Oct 12, 2023

luqasn commented Oct 12, 2023

		@@ -0,0 +1,6 @@
		package com.microsoft.thrifty.transport

		class TTransportException : Throwable {

Add http transport #449

Add http transport #449

Conversation

luqasn commented Jun 18, 2021 • edited Loading

codecov-commenter commented Jun 18, 2021

Codecov Report

benjamin-bader 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

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

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

luqasn commented Jun 22, 2021

juequinterore commented Aug 11, 2023

luqasn commented Oct 8, 2023

juequinterore commented Oct 8, 2023 via email

luqasn commented Oct 9, 2023

benjamin-bader commented Oct 12, 2023

benjamin-bader commented Oct 12, 2023

luqasn commented Oct 12, 2023

luqasn commented Jun 18, 2021 •

edited

Loading