`defimplEx` pointing to an incorrect implementation

[WolfDan]

Hello ^^

I'm having an issue with the library, I don't know if I'm doing it in the wrong way, I just follow the code posted here, I'm implementing a protocol for a client that sends json messages, here my implementation:

Packet

import ProtocolEx

defprotocolEx Visionire.Network.Packet do
  def parse(from, data, state)

  @doc """
  A parse protocol, it takes the json data, and the client state, and dispatch the
  JSON data based on his content
  """
  def parse(
        %{"client" => data},
        %Visionire.Network.State.ClientState{} = state
      ) do
    message_id = Map.keys(data) |> List.first()
    parse(message_id, data, state)
  end
end

Login

import ProtocolEx

defimplEx Login, _, for: Visionire.Network.Packet do
  alias Visionire.Network.{Client, Server}
  alias Visionire.Network.State.ClientState
  alias Visionire.State.Player
  alias Visionire.Game.Path
  alias Visionire.Model.{User, Character}
  alias Visionire.Game.Auth

  # The state is already logged so we don't need to login again
  def parse("login", %{"login" => [login_id, _password]}, %ClientState{
        connection_state: :logged_in
      }) do
    Client.send(%{error: "id_in_use"})

    {:error, "The user #{login_id} is already logged"}
  end

  # Not connected and can login
  def parse(
        "login",
        %{"login" => [login_id, password]},
        %ClientState{connection_state: :new} = state
      ) do
    check_user_credentials(login_id, password, state)
  end
# more stuff
end

Map

import ProtocolEx

defimplEx Map, _, for: Visionire.Network.Packet do
  alias Visionire.Network.{Client}
  alias Visionire.Network.State.ClientState
  alias Visionire.Network.Utils

  def parse(
        "walk_to",
        %{"walk_to" => [x, y]},
        %ClientState{connection_state: :logged_in} = state
      ) do
    now = Utils.stamp_ms(:os.timestamp())
    # implementation
    {:ok, state}
  end
end

Chat

import ProtocolEx

defimplEx Chat, _, for: Visionire.Network.Packet do
  alias Visionire.Network.{Client, Server}
  alias Visionire.Network.State.ClientState

  def parse(
        "say",
        %{"say" => text},
        %ClientState{connection_state: :logged_in} = state
      ) do
    Server.broadcast_except(%{person_said: [state.player.user_id, text]})
    Client.send(%{result: "ok"})
    {:ok, state}
  end

  def parse(
        "whisper",
        %{"whisper" => [user_id, text]},
        %ClientState{connection_state: :logged_in} = state
      ) do
    Server.send_msg_to_user(%{person_said: [state.player.user_id, text]}, user_id)
    Client.send(%{result: "ok"})
    {:ok, state}
  end
end

Misc

import ProtocolEx

defimplEx Misc, _, for: Visionire.Network.Packet do
  alias Visionire.Network.{Client}
  alias Visionire.Network.State.ClientState
  alias Visionire.Network.Utils

  def parse(
        "get_time",
        "get_time",
        %ClientState{connection_state: :logged_in} = state
      ) do
    Client.send(%{result: %{ok: Utils.stamp_ms(:os.timestamp())}})
    {:ok, state}
  end
end

And I dispatch it like this:

with {:ok, data} <- Jason.decode(packet),
               {:ok, new_state} <- Packet.parse(data, state) do
# implementation
end

The client is sending this message "{"client":{"login":["sif","wolf"]}}" and I'm getting this error:

** (FunctionClauseError) no function clause matching in Visionire.Network.Packet.Misc.parse/3
    (visionire) lib/visionire/network/packets/misc.ex:8: Visionire.Network.Packet.Misc.parse("login", %{"login" => ["sif", "wolf"]}, %Visionire.Network.State.ClientState{client: #Port<0.17205>, connection_state: :new, login_id: nil, pid: #PID<0.280.0>, player: nil, uuid: "cf1fafe6-a2b4-45dd-bfca-ab51373388ec"})

As you can see it always match the Misc parser, is it a bug of the library or I miss something in the implementation?

Greetings 0/

[OvermindDL1]

The matcher on the implementations is _ on all of your implementations (in the post that was just a placeholder meaning 'put whatever you need to match on here'), meaning they all overlap, thus whatever gets called will be pretty random. I'm actually quite surprised you are not getting a compile-time warning about this, can you put this on github with a link to it so I can see why it's not causing a warning?

Either way, the matcher should be unique for each implementation (just like with normal Elixir Protocols), so for example instead of defimplEx Map, _, ... you'd want to do defimplEx Map, x when x in ["walk_to"], ... and instead of things like defimplEx Chat, _, ... you'd want to do something like defimplEx Chat, x when x in ["say", "whisper"], ..., and so forth that you would do for every implementation.

However that does mean there is duplication, which I don't like, that is why I have an undocumented feature in ProtocolEx specifically for use-cases like this, but it is undocumented because it has a couple of caveats, first an example, the Map implementation would be written like this instead:

import ProtocolEx

defimplEx Map, _, for: Visionire.Network.Packet, inline: [parse: 3] do
  def parse(
        "walk_to",
        %{"walk_to" => [x, y]},
        %Visionire.Network.State.ClientState{connection_state: :logged_in} = state
      ) do
    now = Visionire.Network.Utils.stamp_ms(:os.timestamp())
    # implementation
    {:ok, state}
  end
end

The only difference is that it is given an inline option, which takes a list of name: arity mappings of what to 'inline'. What this means is that the matcher will be ignored on the defimpl_ex itself and it will instead delegate it entirely to the defined functions itself, I.E. it is like copy/pasting the entire function into the protocol itself. Now the caveat means that the function has to be ENTIRELY self-contained. It cannot reference other local functions, it cannot reference aliases or imports outside of it, nothing. All function calls have to be completely and fully written out unless it is a local call to the protocol itself, not the implementation.

Consequently, this method is not suggested and instead only exists for micro-optimizations, which is definitely not the case here, so I'd recommend writing out the matchers properly. :-)

Just remember, the matchers on the defimpl_ex line itself generally must be unique or it falls back to priority ordering, and if that is unspecified then it falls back to alphanumeric ordering. :-)

Regardless though, if any further issues, try to reduce the problem to a reproduceable test-case that is either copy/pasteable into the shell or a link to a minimal git repo. :-)

 

On a secondary design issue, it looks like you are just extracting some random first key as per message_id = Map.keys(data) |> List.first() on the message then dispatching based on that, what if there are multiple entires on the map? Why is a map used at all if some random key is chosen to dispatch on? This should probably be a 2-tuple, not a map. :-)

[WolfDan]

Thank you @OvermindDL1 !

Actually I fixed it by passing the message struct as a matcher like this:

defimplEx ChatSay, %{"say" => text} when is_binary(text), for: Visionire.Network.Packet do
    alias Visionire.Network.{Client, Server}
    alias Visionire.Network.State.ClientState

    def parse_message(
          %{"say" => text},
          %ClientState{connection_state: :logged_in} = state
        ) do
      Server.broadcast_except(%{person_said: [state.player.user_id, text]})
      Client.send(%{result: "ok"})
      {:ok, state}
    end
  end

It works as expected now :D thank you for this amazing library ^^/

[OvermindDL1]

Actually I fixed it by passing the message struct as a matcher like this:

Yep, that's the way to do it especially if only one thing to match on, quite efficient. :-)