Mint adapter #297

RyanSiu1995 · 2019-05-11T13:22:07Z

Regarding issue #282,
sorry for misopen of PR without verifying the build last time.
Several problems for mint integration

Mint is using two layers of keyword list for the connection opts. Also, SSL and TCP opts shares in the same opt list and the options from SSL is not compatible to TCP. It will make badarg if the user uses SSL options in TCP. So I do not use config :tesla, Mint, xxxxx to let user to have a global adapter configuration for Mint.
Ca cert for SSL in the unit test need to be imported from HTTPParrot so I have exposed the cacert in config :tesla, :cacert, [xxxx] for passing the unit test. I don't like the hacky way I do in the pr. Any idea on making this much more precise and concise?
The request body streaming in mint is not really good at all. It requires the user to specify the correct content length in order to pass all the unit tests. So I have made a function to get the full body before starting the request so I can always get the content length.
Welcome to have any kinds of comments. Thank you!!!!

codecov · 2019-05-11T13:32:09Z

Codecov Report

Merging #297 into master will increase coverage by 0.08%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   95.43%   95.51%   +0.08%     
==========================================
  Files          23       24       +1     
  Lines         460      513      +53     
==========================================
+ Hits          439      490      +51     
- Misses         21       23       +2

Impacted Files	Coverage Δ
lib/tesla/adapter/mint.ex	`96.22% <96.22%> (ø)`

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 40dc5ee...f57df1a. Read the comment docs.

codecov · 2019-05-11T13:32:09Z

Codecov Report

Merging #297 into master will decrease coverage by 0.31%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
- Coverage   95.43%   95.11%   -0.32%     
==========================================
  Files          23       24       +1     
  Lines         460      512      +52     
==========================================
+ Hits          439      487      +48     
- Misses         21       25       +4

Impacted Files	Coverage Δ
lib/tesla/adapter/mint.ex	`92.59% <92.59%> (ø)`
lib/tesla/middleware/decode_rels.ex	`100% <0%> (ø)`	⬆️
lib/tesla/builder.ex	`100% <0%> (ø)`	⬆️

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 40dc5ee...85f94c6. Read the comment docs.

lib/tesla/adapter/mint.ex

mix.exs

teamon · 2019-05-31T10:44:29Z

Wow! 👏

@ericmj would you mind taking a look at mint usage?

amatalai · 2019-05-31T11:35:13Z

If I have not overlooked anything, this implementation will create a new connection for every request and there will be no way to reuse it. The question is how fast will it work in comparison to other adapters.

teamon · 2019-05-31T12:46:57Z

I know, but given #301 I think this is a good starting point.

teamon

~~Could you explain why do we need this get_default_ca at all? Is this for production use or for tests only?~~ You explained that already, 🤦‍♂

Let me see if I can find any other non-hackish way to do this

teamon · 2019-06-03T09:59:10Z

config/config.exs

@@ -19,5 +19,5 @@ if Mix.env() == :test do

  config :tesla, MockClient, adapter: Tesla.Mock

-  config :tesla, cacert: ["./deps/httparrot/priv/ssl/server-ca.crt"]
+  config :tesla, Mint, cacert: ["./deps/httparrot/priv/ssl/server-ca.crt"]


I'd use

config :tesla, Tesla.Adapter.Mint, cacert: ["./deps/httparrot/priv/ssl/server-ca.crt"]

ericmj

There are performance downsides to this adapter. It starts a new connection for each request, unlike with for example hackney that uses a pool connection. Because of this, clients that use pools will likely be faster.

Since the connection is never reused you may want to set the connection: close header and explicitly close the connection. Otherwise you are likely to leak messages since you may still be getting messages from the socket after the request is finished.

lib/tesla/adapter/mint.ex

ericmj · 2019-06-03T20:27:57Z

lib/tesla/adapter/mint.ex

+
+  defp get_default_ca() do
+    env = Application.get_env(:tesla, Mint)
+    Keyword.get(env, :cacert)


Not sure how this works, but make sure the CA store Mint uses is not removed if a user does not explicitly the :cacert config.

There should be either some error reporting or some default, this will blow if Application.get_env(:tesla, Tesla.Adapter.Mint) returns nil

lib/tesla/adapter/mint.ex

ericmj · 2019-06-04T08:42:07Z

The request body streaming in mint is not really good at all. It requires the user to specify the correct content length in order to pass all the unit tests.

I have opened an issue to support chunked encoding for the request: elixir-mint/mint#174.

RyanSiu1995 · 2019-06-04T14:55:34Z

Thank you @ericmj and @teamon !
I have updated the code according to the comment.
For the cacert, I make it as a global configuration for Mint Adapter. It will serve as a global default cacert file for the adapter. Meaning every single call by that client will use that cacert.
For the request pooling, I will continue the development on issue #301.
Anyway, thank you very much!

codecov-io · 2019-06-04T14:58:19Z

Codecov Report

Merging #297 into master will increase coverage by 0.09%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   95.19%   95.28%   +0.09%     
==========================================
  Files          25       26       +1     
  Lines         520      573      +53     
==========================================
+ Hits          495      546      +51     
- Misses         25       27       +2

Impacted Files	Coverage Δ
lib/tesla/adapter/mint.ex	`96.22% <96.22%> (ø)`

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 9795075...b1e5c11. Read the comment docs.

RyanSiu1995 · 2019-06-04T14:58:27Z

And for the checks, I don't know how to fix the codecov. Some of the paths cannot be simply tested by HTTParrot.

RyanSiu1995 · 2019-06-12T06:23:43Z

@teamon @ericmj can you please help review the latest commit <3 Thank you!!

lib/tesla/adapter/mint.ex

teamon · 2019-06-24T17:15:30Z

config/config.exs

@@ -18,4 +18,7 @@ if Mix.env() == :test do
    sasl_error_logger: false

  config :tesla, MockClient, adapter: Tesla.Mock
+
+  config :tesla, Tesla.Adapter.Mint,
+    cacert: ["./deps/httparrot/priv/ssl/server-ca.crt"]


@ericmj This config won't end up in production builds, right? I mean, it will be nil by default so it should be safe to reference httparrot here.

I think it will be nil by default.

Correct, config.exs is not used from dependencies. So the Tesla.Adapter.Mint key will not be set.

teamon · 2019-06-24T17:16:42Z

lib/tesla/adapter/mint.ex

+
+  defp get_default_ca() do
+    env = Application.get_env(:tesla, Mint)
+    Keyword.get(env, :cacert)


There should be either some error reporting or some default, this will blow if Application.get_env(:tesla, Tesla.Adapter.Mint) returns nil

lib/tesla/adapter/mint.ex

RyanSiu1995 · 2019-06-25T02:18:56Z

I have done a simple research on the build issue.
Seem OTP 18.3 on xenial is no longer supported by travis ci.
So it got a failure while setting up the environment for opt_release: 18.3

Ref: https://travis-ci.community/t/erlang-18-3-couldnt-be-pulled/3836

teamon · 2019-07-22T11:31:55Z

@RyanSiu1995 please rebase on recent master that contains fixes for otp 18

…ter mint

Co-Authored-By: Alexander Strizhakov <[email protected]>

teamon requested a review from amatalai May 31, 2019 10:38

teamon reviewed May 31, 2019

View reviewed changes

lib/tesla/adapter/mint.ex Outdated Show resolved Hide resolved

lib/tesla/adapter/mint.ex Show resolved Hide resolved

mix.exs Outdated Show resolved Hide resolved

teamon reviewed Jun 3, 2019

View reviewed changes

ericmj reviewed Jun 4, 2019

View reviewed changes

alex-strizhakov reviewed Jun 19, 2019

View reviewed changes

lib/tesla/adapter/mint.ex Outdated Show resolved Hide resolved

teamon added the feature ✨ label Jun 24, 2019

teamon reviewed Jun 24, 2019

View reviewed changes

RyanSiu1995 force-pushed the mint-adapter branch from 46ed476 to 9fd478f Compare June 25, 2019 01:56

RyanSiu1995 added 10 commits July 29, 2019 14:48

Setup happy path logic for adapter mint

2acabfa

Handle stream request body for Adapter Mint

22e5b44

Fix the URI parsing before request in Adapter Mint

eca9cf3

Fix streaming request body problem and add multipart support for adap…

ee6a72d

…ter mint

Update the mint adapter docs

b2117e9

Throwing error tuple when encountering mint error

e57d697

Fix the unit tests for mint

0dad600

Update README about the cacert and minimum elixir version for mint

c28cccc

Bump the minimum elixir version as 1.5.0

ae07e69

Make the cacert as the property for Mint

b487fde

RyanSiu1995 and others added 6 commits July 29, 2019 14:50

Remove unnecessary check for http method in mint adapter

2d57f4e

Improve the usage in mint library

046822b

Refactor the default cacert as global cacert

9e7b7d2

Fix the incorrect order of stream_response

eef0adc

Co-Authored-By: Alexander Strizhakov <[email protected]>

Refactor the codes for mint adapter

8abab5d

Remove OTP 18.3 due to the unsupport from travis

b1e5c11

RyanSiu1995 force-pushed the mint-adapter branch from 8d3e6e6 to b1e5c11 Compare July 29, 2019 06:50

RyanSiu1995 closed this Aug 17, 2019

RyanSiu1995 reopened this Aug 17, 2019

teamon merged commit 755708c into elixir-tesla:master Aug 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mint adapter #297

Mint adapter #297

RyanSiu1995 commented May 11, 2019

codecov bot commented May 11, 2019

codecov bot commented May 11, 2019 •

edited

Loading

teamon commented May 31, 2019

amatalai commented May 31, 2019

teamon commented May 31, 2019

teamon left a comment •

edited

Loading

teamon Jun 3, 2019

ericmj left a comment

ericmj Jun 3, 2019

teamon Jun 24, 2019

ericmj commented Jun 4, 2019

RyanSiu1995 commented Jun 4, 2019

codecov-io commented Jun 4, 2019 •

edited by codecov bot

Loading

RyanSiu1995 commented Jun 4, 2019

RyanSiu1995 commented Jun 12, 2019

teamon Jun 24, 2019

RyanSiu1995 Jun 25, 2019

ericmj Jun 25, 2019

teamon Jun 24, 2019

RyanSiu1995 commented Jun 25, 2019

teamon commented Jul 22, 2019

Mint adapter #297

Mint adapter #297

Conversation

RyanSiu1995 commented May 11, 2019

codecov bot commented May 11, 2019

Codecov Report

codecov bot commented May 11, 2019 • edited Loading

Codecov Report

teamon commented May 31, 2019

amatalai commented May 31, 2019

teamon commented May 31, 2019

teamon left a comment • edited Loading

Choose a reason for hiding this comment

teamon Jun 3, 2019

Choose a reason for hiding this comment

ericmj left a comment

Choose a reason for hiding this comment

ericmj Jun 3, 2019

Choose a reason for hiding this comment

teamon Jun 24, 2019

Choose a reason for hiding this comment

ericmj commented Jun 4, 2019

RyanSiu1995 commented Jun 4, 2019

codecov-io commented Jun 4, 2019 • edited by codecov bot Loading

Codecov Report

RyanSiu1995 commented Jun 4, 2019

RyanSiu1995 commented Jun 12, 2019

teamon Jun 24, 2019

Choose a reason for hiding this comment

RyanSiu1995 Jun 25, 2019

Choose a reason for hiding this comment

ericmj Jun 25, 2019

Choose a reason for hiding this comment

teamon Jun 24, 2019

Choose a reason for hiding this comment

RyanSiu1995 commented Jun 25, 2019

teamon commented Jul 22, 2019

codecov bot commented May 11, 2019 •

edited

Loading

teamon left a comment •

edited

Loading

codecov-io commented Jun 4, 2019 •

edited by codecov bot

Loading