florinpatrascu / bolt_sips

Neo4j driver for Elixir
Apache License 2.0
258 stars 49 forks source link

Proposal: Proposal for encoding #60

Closed dominique-vassard closed 5 years ago

dominique-vassard commented 5 years ago

Hi, I've worked a little bit on the "encoding issue" #57 , and found it less easy than I thought. Here is a proposal to open the discussion.

Encoding contains 3 part:

Bolt.Sips.ResponseEncoder

User will use this lib to encode its data. Calls share the same signature: ResponseEncoder.encode(data_to_encode, targeted_format) with targeted_format being a atom. This lib does nothing more that calling encoding libs whic hare protocols

Protocols

Stored in : lib/bolt_sips/reponse_encoder/. Protocols who managed one specific format. Though I identified two different ways of using them:

Encoder derivation

Stored in : lib/bolt_sips/response_encoder/format_name Poison and /Jason are popular json encoding library and have default implementation for this protocol make working with bolt_sips easier and transparent. You'll see that both implementation are present and not only one with some config. This is because I ask myself: what if there is project out there that uses both libs? I don't know if there some widely used xml lib but an implementation could be added.

Usage

Vanilla usage is like this:

iex> long_result = [
  %{
    "rel" => %Bolt.Sips.Types.Relationship{
      end: 30,
      id: 5,
      properties: %{
        "created" => %Bolt.Sips.Types.DateTimeWithTZOffset{
          naive_datetime: ~N[2016-05-24 13:26:08.543],
          timezone_offset: 7200
        }
      },
      start: 69,
      type: "UPDATED_TO_"
    },
    "t1" => %Bolt.Sips.Types.Node{
      id: 69,
      labels: ["Test"],
      properties: %{
        "created" => %Bolt.Sips.Types.DateTimeWithTZOffset{
          naive_datetime: ~N[2016-05-24 13:26:08.543],
          timezone_offset: 7200
        },
        "uuid" => 12345
      }
    },
    "t2" => %Bolt.Sips.Types.Node{
      id: 30,
      labels: ["Test"],
      properties: %{"uuid" => 6789}
    }
  }
]
iex(9)> Bolt.Sips.ResponseEncoder.encode(long_result, :json)
{:ok,
 "[{\"rel\":{\"end\":30,\"id\":5,\"properties\":{\"created\":\"2016-05-24T13:26:08.543+02:00\"},\"start\":69,\"type\":\"UPDATED_TO_\"},\"t1\":{\"id\":69,\"labels\":[\"Test\"],\"properties\":{\"created\":\"2016-05-24T13:26:08.543+02:00\",\"uuid\":12345}},\"t2\":{\"id\":30,\"labels\":[\"Test\"],\"properties\":{\"uuid\":6789}}}]"}

# Use JSON encoding lib (enabled by protocol implementation)
iex(10)> Jason.encode(long_result)
{:ok,
 "[{\"rel\":{\"end\":30,\"id\":5,\"properties\":{\"created\":\"2016-05-24T13:26:08.543+02:00\"},\"start\":69,\"type\":\"UPDATED_TO_\"},\"t1\":{\"id\":69,\"labels\":[\"Test\"],\"properties\":{\"created\":\"2016-05-24T13:26:08.543+02:00\",\"uuid\":12345}},\"t2\":{\"id\":30,\"labels\":[\"Test\"],\"properties\":{\"uuid\":6789}}}]"}
iex(11)> Poison.encode(long_result)
{:ok,
 "[{\"t2\":{\"properties\":{\"uuid\":6789},\"labels\":[\"Test\"],\"id\":30},\"t1\":{\"properties\":{\"uuid\":12345,\"created\":\"2016-05-24T13:26:08.543+02:00\"},\"labels\":[\"Test\"],\"id\":69},\"rel\":{\"type\":\"UPDATED_TO_\",\"start\":69,\"properties\":{\"created\":\"2016-05-24T13:26:08.543+02:00\"},\"id\":5,\"end\":30}}]"}

Default implementation can be overridden at 2 levels:

This allows fine-grained encoding options, even using user-defined types.

I know this is not perfect but it's a good start, what do you think?

florinpatrascu commented 5 years ago

this is awesome job, man! Thank you for dedicating time to this. Do you think we should implement the json implementation for two different libs; Jason and Poison, respectively? Shouldn't we just let the user clarify the library she desires for encoding, i.e. using a simple config like: :bolt_sips, :json_library, Poison? While our default dependency would be: Jason, for example?!

This way, we could have something like:


defmodule Bolt.Sips do 
  def json_library do
    Application.get_env(:bolt_sips, :json_library, Jason)
  end
end

then use it throughout the driver like this: ... |> BoltSips.json_library().encode(), or similar?

Obviously we would display a warning in case the :json_library config is missing from the user's config or app's env, so that we can let him know we're falling back on our (default) implementation; the Jason lib in this case, as mocked in the code above.

Thoughts?

dominique-vassard commented 5 years ago

In fact, something like ... |> BoltSips.json_library().encode() already exists. It's the following usage: ... |> Bolt.Sips.ResponseEncoder.encode(:json). In this case, using Jason or Poison is up to us as it is an internal process. I choose Jason over Poison because it's more performant and more widely used (or will be). The fact that the user uses another json library in his/her project has no impact here.

Regarding the Jason and Poison implementation you can found in lib/bolt_sips/response_encoder/json/jason.ex and lib/bolt_sips/response_encoder/json/poison.ex, their sole purpose is: magic :)

Let's say you're working on a API using Phoenix. As you know, Phoenix automatically convert elixir types in valid json via Jason. Without additional implementation, to make it work properly your view have to look like this:

def render("result.json", query_result) do
  %{data: Bolt.Sips.ResponseEncoder.encode(query_result, :json)}
end

With Jason implementation, you only need to write the same as usual:

def render("result.json", query_result) do
  %{data: query_result}
end

Phoenix calls Jason.encode which takes in account the implementation for the Sips types and everything is transparent for user.

I agree that this is a little bit out of the scope of the driver and I was thinking of just adding some doc about this, but it's convenient to have a default implementation. Then why a Json implementation AND a Poison implementation. I believe that in some cases, users will need both i.e one database used by phoenix 1.2 back office and
phoeinx 1.4 front office. And to be totally honest, I didn't a working solution to pass dynamically a module to deifmpl like this:

defimpl MyDynamicLib.Encoder, for: ...

I tried attributes, functions, Module.concat, none of these work. When I was considering the macro solution, I find this was over engineered and decided to keep the 2 implementations. Not the best solutions but it has advantages.

florinpatrascu commented 5 years ago

Thank you for the clarifications, and yeah you're right about the users who might be forced to use older versions of phoenix, due to legacy constraints. True. So let's do this, Sir :)

dominique-vassard commented 5 years ago

I'm on it :)

dominique-vassard commented 5 years ago

And here is the final version. 2 things:

florinpatrascu commented 5 years ago

very cool!

florinpatrascu commented 5 years ago

thank you @dominique-vassard! 🥇 🥇 🥇

dominique-vassard commented 5 years ago

You're welcome.