Add http transport

[luqasn]

addresses https://github.com/microsoft/thrifty/issues/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]

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%)	:arrow_up:

---

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.

[luqasn]

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]

Hi.

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

[luqasn]

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]

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 https://github.com/microsoft/thrifty/pull/449#issuecomment-1751950951, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACAW4X2LW2CWXTB3RNXPJXTX6JKYLAVCNFSM466HEBLKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZVGE4TKMBZGUYQ . You are receiving this because you commented.Message ID: @.***>

[luqasn]

Long time no see, @benjamin-bader :)

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

[benjamin-bader]

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]

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]

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.