Define behaviours for client and server adapters #200

avillen · 2021-07-07T08:20:39Z

Hi 👋

To give some context, I was trying to add tests to my integration using grpc. I found that you can set the adapter that you want on both client and server, and that makes it easier to test my code. I'm a big fan of Mox, I really think that simplifies the way to add tests, but we need a behaviour over we can create the mock.

This MR aims to create behaviours for both clients and servers, but for my personal use-case, with the client one would be enough. With this, an incomplete test example could be:

      expected_proto_response = ProtoSampe.new()

      grpc_channel = %GRPC.Channel{
        adapter: GRPCCLientMock
      }

      stream = %GRPC.Client.Stream{
        channel: grpc_channel,
        codec: GRPC.Codec.Proto,
        response_mod: ProtoSample
      }

      expect(GRPCCLientMock, :connect, fn _, _ -> {:ok, grpc_channel} end)
      expect(GRPCCLientMock, :send_request, fn _, _, _ -> stream end)
      expect(GRPCCLientMock, :recv_headers, fn _, _, _ -> {:ok, [], :fin} end)

      {:ok, data, _} =
        ProtoSample
        |> GRPC.Message.Protobuf.encode(expected_proto_response)
        |> GRPC.Message.to_data()

      expect(GRPCCLientMock, :recv_data_or_trailers, fn _, _, _ -> {:data, data} end)

      expect(GRPCCLientMock, :recv_data_or_trailers, fn _, _, _ ->
        {:trailers, %{"grpc-status" => "0"}}
      end)
      
     // Your request

I'm not a person with a lot of knowledge of both grpc and this library itself. This MR could be considered in WIP because and not pretty sure if I have properly defined the typespecs, or be closed without any problem if you consider.

Cheers and thanks for the library! Awesome job!

polvalente

I started looking into this for issue #199.

Since there was already your pull request opened, perhaps it's best we focus on it instead!

polvalente · 2022-07-02T01:42:33Z

lib/grpc/adapter/client.ex

+
+  @callback disconnect(channel) :: {:ok, channel} | {:error, any}
+
+  @type stream :: Stream.t()


I would use Stream.t() directly instead of adding this proxy type. Is there any reason for this path?

For clarity, I would also move all @type definitions to the top of the module

TBH, the only reason about using this proxy type is because I did it to all the types. Also the idea is that in this way it could be used in the specs of the definitions. But I don't have a strong reason to do that. How do you consider using the proxy type or using it directly inline?

Using the in-line type is better for readability:

# Module defs defmodule T do @type t :: integer end defmodule M do @type custom :: T.t() @spec f(x :: T.t(), y :: custom) :: T.t() def f(x, y), do: x + y end # iex calls iex(1)> t M @type custom() :: T.t() iex(2)> h M.f def f(x, y) @spec f(x :: T.t(), y :: custom()) :: T.t()

When exploring the API via IEx, the proxy type will force the user to call t module to get the typedefs and discover it is indeed a proxy

polvalente · 2022-07-02T01:43:24Z

lib/grpc/adapter/client.ex

+  alias GRPC.Client.Stream
+  alias GRPC.Channel
+
+  @type opts :: any


I think it's better to use opts :: keyword() instead (perhaps even avoiding a custom type, inlining this wherever needed).

This would warrant some changes to the current implementation, but provides a better interface

polvalente · 2022-07-02T01:44:40Z

lib/grpc/adapter/client.ex

+  @type message :: binary()
+  @callback send_request(stream, message, opts) :: stream
+
+  @type headers :: [{String.t(), String.t()}]


The current implementation would need to change, as I think it currently returns a string-map

totally right

polvalente · 2022-07-02T01:44:54Z

lib/grpc/adapter/client.ex

+  @callback send_request(stream, message, opts) :: stream
+
+  @type headers :: [{String.t(), String.t()}]
+  @type fin :: :fin | :nofin


A typedoc would be great here

polvalente · 2022-07-02T01:45:48Z

lib/grpc/adapter/client.ex

@@ -0,0 +1,27 @@
+defmodule GRPC.ClientAdapter do


Suggested change

defmodule GRPC.ClientAdapter do

defmodule GRPC.Client do

I think ClientAdapter is kinda redundant since the "adapter" part is actually what implements the behaviour specified here

The problem here is that, if we want to follow the same criteria with the GRPC.Server, we wont be able because there is already another file (lib/grpc/server.ex) with the same module name. WDYT about using GRPC.Adapter.Client and GRPC.Adapter.Server?

I think GRPC.Ports.Client/Server might be a better naming pattern (from the Ports and Adapters design pattern), and then we could have GRPC.Adapters.Client.Gun or something like that

Just my opinion, curently we have lib/grpc/client and lib/grpc/server which provides client code and server code separately. How about we move the adapter behaviour to inside those folders? Like:

# For server adapter behaviour. lib/grpc/server/adapter.ex # For client adapter behaviour. lib/grpc/client/adapter.ex

I tend to prefer grouping all behaviours under a given namespace to make it easy to find all of them (so adapters.xxx), but I'm not set too hard on either option :)

I was thinking about grouping the code by context, something like this:

/client/

stream.ex

adapter.ex

adapter/

gun.ex

/server.ex

/server/

stream.ex

supervisor.ex

adapter.ex

adapter/

cowboy.ex

cowboy/

handler.ex

WDYT? You guys have a bigger picture and will work on the project more than me 😅 , so let me know which one suits better to you 😄

BTW, @polvalente if you are in a rush to merge this, please, go ahead and take the PR if you want!! In the end, I can only take this from time to time...

No rush at all! I think this discussion is somewhat important and relevant to this PR as well

I agree with the suggested tree, but I would have the adapters directory instead of adapter

I agree with the suggested tree, but I would have the adapters directory instead of adapter

Nice! Agree with that! I'll push a commit with that ASAP 😄

Something like this? WDYT?

polvalente · 2022-07-02T01:46:09Z

lib/grpc/adapter/client.ex

@@ -0,0 +1,27 @@
+defmodule GRPC.ClientAdapter do
+  @moduledoc false


Adding a moduledoc here is really important so that this module is discoverable in the docs

polvalente · 2022-07-02T01:46:29Z

lib/grpc/adapter/server.ex

@@ -0,0 +1,24 @@
+defmodule GRPC.ServerAdapter do
+  @moduledoc false


Basically the same considerations as the client behaviour

polvalente · 2022-07-02T01:49:21Z

Also, disclaimer: I'm not a maintainer for the library and have just added my 2 cents because I'm interested in having this library work in production

sleipnir · 2022-07-02T14:38:17Z

Also, disclaimer: I'm not a maintainer for the library and have just added my 2 cents because I'm interested in having this library work in production

Hi Paulo, it's a pleasure to speak with you again.
I decided to comment since you gave me the cue.
Today this project doesn't seem to be being maintained by anyone, there are numerous requests for PRs and Issues open without any response. For that reason I created a fork of the project called Falco to try to continue, we are also trying to improve the performance of the library that has suffered a lot in the gRPCs benchmark tests.
If you want to take a look at the project https://github.com/eigr/falco

**Note, some of the PRs open here I already include in Falco. But we are not yet ready for production.I'm sharing this just for your knowledge.

polvalente · 2022-07-02T16:17:08Z

Also, disclaimer: I'm not a maintainer for the library and have just added my 2 cents because I'm interested in having this library work in production

Hi Paulo, it's a pleasure to speak with you again.
I decided to comment since you gave me the cue.
Today this project doesn't seem to be being maintained by anyone, there are numerous requests for PRs and Issues open without any response. For that reason I created a fork of the project called Falco to try to continue, we are also trying to improve the performance of the library that has suffered a lot in the gRPCs benchmark tests.
If you want to take a look at the project https://github.com/eigr/falco

**Note, some of the PRs open here I already include in Falco. But we are not yet ready for production.I'm sharing this just for your knowledge.

@sleipnir that's awesome to know! For sure I'll take a look into it!

polvalente · 2022-07-17T01:34:51Z

@avillen let me know if you want me to take over this PR

avillen · 2022-07-17T09:28:03Z

hi @polvalente!! thanks for your review 😄 I can take care of apply your suggestions, but where are you saying to apply those changes, on this repo or in this one https://github.com/eigr/falco?

polvalente · 2022-07-17T11:02:09Z

hi @polvalente!! thanks for your review 😄 I can take care of apply your suggestions, but where are you saying to apply those changes, on this repo or in this one https://github.com/eigr/falco?

Here! I'm a maintainer for the repo now.
The idea is to concentrate the efforts on this repo.

I'm in touch with the people at falco and at another fork as well, so we can bring the best changes from the forks here

avillen · 2022-07-18T08:24:54Z

hi @polvalente!! thanks for your review 😄 I can take care of apply your suggestions, but where are you saying to apply those changes, on this repo or in this one https://github.com/eigr/falco?

Here! I'm a maintainer for the repo now. The idea is to concentrate the efforts on this repo.

I'm in touch with the people at falco and at another fork as well, so we can bring the best changes from the forks here

Nice to hear that! I'll take care of it along the week 😄

avillen

@polvalente I was reading your suggestions and I totally agree with them, I just have a few smaill questions

avillen · 2022-07-18T22:45:44Z

lib/grpc/adapter/client.ex

@@ -0,0 +1,27 @@
+defmodule GRPC.ClientAdapter do


The problem here is that, if we want to follow the same criteria with the GRPC.Server, we wont be able because there is already another file (lib/grpc/server.ex) with the same module name. WDYT about using GRPC.Adapter.Client and GRPC.Adapter.Server?

avillen · 2022-07-18T22:46:02Z

lib/grpc/adapter/client.ex

+
+  @callback disconnect(channel) :: {:ok, channel} | {:error, any}
+
+  @type stream :: Stream.t()


TBH, the only reason about using this proxy type is because I did it to all the types. Also the idea is that in this way it could be used in the specs of the definitions. But I don't have a strong reason to do that. How do you consider using the proxy type or using it directly inline?

avillen · 2022-07-18T22:46:08Z

lib/grpc/adapter/client.ex

+  @type message :: binary()
+  @callback send_request(stream, message, opts) :: stream
+
+  @type headers :: [{String.t(), String.t()}]


totally right

avillen · 2022-07-18T22:51:13Z

lib/grpc/adapter/client.ex

@@ -0,0 +1,27 @@
+defmodule GRPC.ClientAdapter do
+  @moduledoc false


avillen · 2022-07-18T22:58:48Z

lib/grpc/adapter/client.ex

+  @callback send_request(stream, message, opts) :: stream
+
+  @type headers :: [{String.t(), String.t()}]
+  @type fin :: :fin | :nofin


…thub.com:avillen/grpc into define-behaviours-for-client-and-server-adapters

config/config.exs

polvalente · 2022-07-19T19:51:51Z

lib/grpc/server.ex

  def start_endpoint(endpoint, port, opts \\ []) do
    servers = endpoint.__meta__(:servers)
    servers = GRPC.Server.servers_to_map(servers)
-    adapter = Keyword.get(opts, :adapter, GRPC.Adapter.Cowboy)
+    adapter = Keyword.get(opts, :adapter, GRPC.Server.Adapters.Cowboy)


Could you replace all of these Keyword.get(a, b, c) with Keyword.get(a, b) || c?

Just the ones this PR touches is fine, though

polvalente · 2022-07-19T19:52:32Z

lib/grpc/server.ex

+  @type endpoint :: Adapter.endpoint()
+  @type server_port :: Adapter.server_port()
  @type servers_list :: module | [module]
+  @type servers_map :: Adapter.servers_map()
+  @type stream :: Adapter.stream()
+  @type headers :: Adapter.headers()


let's avoid proxy types

polvalente · 2022-07-19T19:53:16Z

lib/grpc/server/adapter.ex

+  @type reply :: binary()
+  @callback send_reply(state, reply, opts) :: any()
+
+  @type headers :: map()


Shall we rework headers into keyword() on a separate PR?

I'd say yes! I can take care of it 😄

polvalente · 2022-07-19T19:53:57Z

lib/grpc/stub.ex

@@ -98,6 +98,8 @@ defmodule GRPC.Stub do
    end
  end

+  @type channel :: GRPC.ClientAdapter.channel()


polvalente · 2022-07-19T19:54:15Z

lib/grpc/stub.ex

@@ -125,7 +127,7 @@ defmodule GRPC.Stub do
    * `:accepted_compressors` - tell servers accepted compressors, this can be used without `:compressor`
    * `:headers` - headers to attach to each request
  """
-  @spec connect(String.t(), Keyword.t()) :: {:ok, GRPC.Channel.t()} | {:error, any}
+  @spec connect(String.t(), Keyword.t()) :: {:ok, channel} | {:error, any}


I think we should use GRPC.Channel.t() here as it was before

wingyplus · 2022-07-19T21:39:25Z

lib/grpc/client/adapter.ex

+  alias GRPC.Client.Stream
+  alias GRPC.Channel
+
+  @type opts :: any


Should be a keyword?

I think currently the code passes maps as options. We need to rework both this and header maps to become keywords and lists of tuples instead

…apters

avillen · 2022-07-21T09:03:56Z

@polvalente @wingyplus I think I have address all your suggestions, but pretty sure I'm missing something 😅 Do you mind taking a look again? Those missing things I think that we can take care of it in another PR:
#200 (comment)
#200 (comment)

wingyplus · 2022-07-21T12:37:15Z

lib/grpc/server/adapter.ex

+
+  alias GRPC.Server.Adapters.Cowboy.Handler
+
+  @callback start(atom, %{String.t() => [module]}, non_neg_integer(), Keyword.t()) ::


Should be atom() instead of just atom?

module might be the same

Suggested change

@callback start(atom, %{String.t() => [module]}, non_neg_integer(), Keyword.t()) ::

@callback start(atom(), %{String.t() => [module()]}, non_neg_integer(), Keyword.t()) ::

wingyplus · 2022-07-21T12:38:32Z

lib/grpc/server/adapter.ex

+  alias GRPC.Server.Adapters.Cowboy.Handler
+
+  @callback start(atom, %{String.t() => [module]}, non_neg_integer(), Keyword.t()) ::
+              {atom, any, non_neg_integer}


Suggested change

{atom, any, non_neg_integer}

{atom(), any(), non_neg_integer()}

wingyplus · 2022-07-21T12:38:57Z

lib/grpc/server/adapter.ex

+  @callback start(atom, %{String.t() => [module]}, non_neg_integer(), Keyword.t()) ::
+              {atom, any, non_neg_integer}
+
+  @callback stop(atom, %{String.t() => [module]}) :: :ok | {:error, :not_found}


Suggested change

@callback stop(atom, %{String.t() => [module]}) :: :ok | {:error, :not_found}

@callback stop(atom(), %{String.t() => [module()]}) :: :ok | {:error, :not_found}

I think we can standardize on having parens :)

Agree and done (at least everywhere I see 😂 ), but I think that we should think about how we can add a validation about that 😅

wingyplus · 2022-07-21T12:44:30Z

Just a quick look from my mobile:

Found type declaration inconsistency ( () vs no () ).
- I already add code suggestion, but not all places. You may take a look at it.
I think we should add @impl true to callback function in cowboy adapter and removing the typespec from it.

lib/grpc/client/adapter.ex

avillen · 2022-07-21T14:26:45Z

Just a quick look from my mobile:

Found type declaration inconsistency ( () vs no () ).

I already add code suggestion, but not all places. You may take a look at it.

I think we should add @impl true to callback function in cowboy adapter and removing the typespec from it.

@wingyplus I think it is done 😄

polvalente

Looks good to me!

lib/grpc/client/adapter.ex

lib/grpc/stub.ex

avillen and others added 4 commits July 6, 2021 14:24

Create server and client behaviours

3a94839

Share types with the types of the behaviours

ffbf54a

Add behaviours definition on test adapters

503628a

Add disconnect to behaviour definition

2bc735a

polvalente reviewed Jul 2, 2022

View reviewed changes

polvalente mentioned this pull request Jul 18, 2022

Ideal way to test servers and clients? #162

Closed

avillen commented Jul 18, 2022

View reviewed changes

polvalente mentioned this pull request Jul 19, 2022

Support more HTTP2 clients #199

Closed

avillen added 2 commits July 19, 2022 20:46

Split client and server domains

29e296b

Merge branch 'define-behaviours-for-client-and-server-adapters' of gi…

852a2e1

…thub.com:avillen/grpc into define-behaviours-for-client-and-server-adapters

polvalente reviewed Jul 19, 2022

View reviewed changes

config/config.exs Show resolved Hide resolved

polvalente reviewed Jul 19, 2022

View reviewed changes

wingyplus reviewed Jul 19, 2022

View reviewed changes

avillen added 5 commits July 21, 2022 09:41

Replace all Keyword.get(a, b, c) with Keyword.get(a, b) || c

22b2bfb

avoid proxy types

4a3385d

Add some documentation

a56dc8f

Complete missing func implementations in test client adapter

ab821d3

Merge branch 'master' into define-behaviours-for-client-and-server-ad…

63466fc

…apters

wingyplus reviewed Jul 21, 2022

View reviewed changes

lib/grpc/client/adapter.ex Outdated Show resolved Hide resolved

avillen added 2 commits July 21, 2022 16:20

Add parenthesis on every type definition

9311dc8

Replace specs with impl true on adapters implemetations

1c76965

polvalente approved these changes Jul 21, 2022

View reviewed changes

lib/grpc/client/adapter.ex Outdated Show resolved Hide resolved

lib/grpc/client/adapter.ex Outdated Show resolved Hide resolved

lib/grpc/client/adapter.ex Outdated Show resolved Hide resolved

lib/grpc/stub.ex Show resolved Hide resolved

polvalente added 3 commits July 21, 2022 12:57

Update lib/grpc/client/adapter.ex

b9058dc

Update lib/grpc/client/adapter.ex

6349609

Update lib/grpc/client/adapter.ex

f14df0c

polvalente merged commit 736c878 into elixir-grpc:master Jul 21, 2022

avillen deleted the define-behaviours-for-client-and-server-adapters branch July 21, 2022 17:53


		@callback disconnect(channel) :: {:ok, channel} \| {:error, any}

		@type stream :: Stream.t()

		@@ -0,0 +1,27 @@
		defmodule GRPC.ClientAdapter do
		@moduledoc false

		@@ -0,0 +1,24 @@
		defmodule GRPC.ServerAdapter do
		@moduledoc false


		alias GRPC.Server.Adapters.Cowboy.Handler

		@callback start(atom, %{String.t() => [module]}, non_neg_integer(), Keyword.t()) ::

	@callback start(atom, %{String.t() => [module]}, non_neg_integer(), Keyword.t()) ::
	@callback start(atom(), %{String.t() => [module()]}, non_neg_integer(), Keyword.t()) ::

	{atom, any, non_neg_integer}
	{atom(), any(), non_neg_integer()}

	@callback stop(atom, %{String.t() => [module]}) :: :ok \| {:error, :not_found}
	@callback stop(atom(), %{String.t() => [module()]}) :: :ok \| {:error, :not_found}

Define behaviours for client and server adapters #200

Define behaviours for client and server adapters #200

Conversation

avillen commented Jul 7, 2021

polvalente 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

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

polvalente commented Jul 2, 2022

sleipnir commented Jul 2, 2022

polvalente commented Jul 2, 2022

polvalente commented Jul 17, 2022

avillen commented Jul 17, 2022

polvalente commented Jul 17, 2022

avillen commented Jul 18, 2022

avillen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avillen commented Jul 21, 2022

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

wingyplus commented Jul 21, 2022 • edited Loading

avillen commented Jul 21, 2022

polvalente left a comment

Choose a reason for hiding this comment

wingyplus commented Jul 21, 2022 •

edited

Loading