Extract a Brain module as an abstraction for persistence #11

stevegrossi · 2017-01-22T17:56:32Z

All the Redis stuff I was doing was fairly straightforward key-value manipulation. I'd like to create an Elix.Brain abstraction to get a few benefits:

a common interface for persistence—I've tried a few different approaches with the "lists" and "remind me" responders and have ideas about what works best. This will avoid needing to reinvent the wheel any time a responder needs to store state.
a default storage mechanism as an Agent so that tests can interact with an actual persistence layer instead of me having to create brittle, non-functional mock modules
an optional Redis storage plugin for to-disk persistence
eventually extracting this into a separate hedwig_brain library for sharing

Resolves #12

Tests failing like whoa, but it works in the console so it’s the tests’ problem Also, just `send`, no need to `reply`

as an alternative to Redis. Eventually both should be available as storage options. I don’t like the warning to group handle_call and handle_cast functions together. I get it for normal function clauses, but the GenServer API requires unrelated functions to have similar-looking callback clauses and I’d rather keep them separate than grouped.

neat that setup works for doctests, too!

stevegrossi · 2017-01-22T17:57:19Z

lib/elix/brain.ex

+  A general-purpose key-value storage mechanism.
+  """
+
+  # @TODO: should this handle encoding/decoding things?


I don’t love calling Brain.set in test setup, though, but I guess it makes sense. Starting the Brain process with existing data felt somehow cleaner than using its public API, but as I write that it feels like the opposite should be true.

stevegrossi · 2017-01-22T22:04:17Z

lib/elix/brain/redis_store.ex

+  def at_index(key, index) when is_binary(key) and is_integer(index) and index >= 0 do
+    command!(["LINDEX", key, index])
+    # @TODO: handle when key points to non-list
+    # @TODO: handle out of range


stevegrossi · 2017-01-22T22:04:17Z

lib/elix/brain/redis_store.ex

+  """
+  def at_index(key, index) when is_binary(key) and is_integer(index) and index >= 0 do
+    command!(["LINDEX", key, index])
+    # @TODO: handle when key points to non-list


stevegrossi · 2017-01-22T22:04:17Z

lib/elix/brain/redis_store.ex

+  def remove(key, item) when is_binary(key) and is_binary(item) do
+    command!(["LREM", key, 0, item])
+    # @TODO: handle when key points to non-list
+    # @TODO: handle when item is not in list?


stevegrossi · 2017-01-22T22:04:17Z

lib/elix/brain/redis_store.ex

+  """
+  def remove(key, item) when is_binary(key) and is_binary(item) do
+    command!(["LREM", key, 0, item])
+    # @TODO: handle when key points to non-list


stevegrossi · 2017-01-22T22:04:18Z

lib/elix/brain/redis_store.ex

+  """
+  def add(key, item) when is_binary(key) and is_binary(item) do
+    command!(["RPUSH", key, item])
+    # @TODO: handle when key points to non-list


stevegrossi · 2017-01-22T22:04:18Z

lib/elix/brain/redis_store.ex

+  """
+  def get(key) when is_binary(key) do
+    command!(["LRANGE", key, 0, -1])
+    # @TODO: handle when key points to non-list


stevegrossi · 2017-01-22T22:04:18Z

lib/elix/brain/redis_store.ex

+  A general-purpose key-value storage mechanism.
+  """
+
+  # @TODO: should this handle encoding/decoding things?


stevegrossi · 2017-01-29T14:22:53Z

lib/elix/message_scheduler/scheduled_message.ex

+  @enforce_keys ~w(message timestamp)a
+  defstruct ~w(message timestamp)a
+
+  def new(message, timestamp) when is_integer(timestamp) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:53Z

lib/elix/message_scheduler.ex

@@ -23,17 +25,22 @@ defmodule Elix.MessageScheduler do
      :ok

  """
-  def send_at(message, timestamp) do
-    GenServer.cast(__MODULE__, {:schedule, {message, timestamp}})
+  def send_at(timestamp, message) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:53Z

lib/elix/message_scheduler.ex

  end

-  def handle_cast({:schedule, {message, timestamp} = scheduled_message}, state) do
-    @store.add(message, timestamp)
+  def handle_cast({:schedule, %ScheduledMessage{} = scheduled_message}, state) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:54Z

lib/elix/brain/redis_store.ex

+  @doc """
+  Sets a key to a value
+  """
+  def set(key, value) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:54Z

lib/elix/brain/redis_store.ex

+  @doc """
+  Returns all strings stored under a key
+  """
+  def get(key) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:54Z

lib/elix/brain/redis_store.ex

+  @doc """
+  Deletes the value stored under a key
+  """
+  def delete(key) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:54Z

lib/elix/brain/redis_store.ex

+  @doc """
+  Stores an additional string under a key
+  """
+  def add(key, item) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:56Z

lib/elix/brain.ex

+    @store.set(key, val)
+  end
+
+  def all(key) when is_binary(key) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:56Z

lib/elix/brain.ex

+    @store.get(key) || []
+  end
+
+  def delete(key) when is_binary(key) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:57Z

lib/elix/brain.ex

+    @store.delete(key)
+  end
+
+  def add(key, item) when is_binary(key) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T14:22:57Z

lib/elix/brain.ex

+    @store.add(key, item)
+  end
+
+  def remove(key, item) when is_binary(key) do


Functions should have a @SPEC type specification.

Need a way to test this separately, but coupling the store to the environment makes that hard

stevegrossi · 2017-01-29T16:21:33Z

lib/elix/brain/redis_store.ex

+    {:ok, _pid} = Redix.start_link(redis_url, name: :redis)
+  end
+
+  def set(key, list) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:33Z

lib/elix/brain/redis_store.ex

+    end)
+  end
+
+  def get(key) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:33Z

lib/elix/brain/redis_store.ex

+    Enum.map(list, &decode_if_necessary/1)
+  end
+
+  def delete(key) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:34Z

lib/elix/brain/redis_store.ex

+    command!(["DEL", key])
+  end
+
+  def add(key, item) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:34Z

lib/elix/brain/redis_store.ex

+    command!(["RPUSH", key, encode_if_necsssary(item)])
+  end
+
+  def remove(key, item) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:34Z

lib/elix/brain/redis_store.ex

+    command!(["LREM", key, 0, encode_if_necsssary(item)])
+  end
+
+  def at_index(key, index) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:34Z

lib/elix/brain/redis_store.ex

+    decode_if_necessary(item)
+  end
+
+  def encode_if_necsssary(term) do


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:34Z

lib/elix/brain/redis_store.ex

+  def decode_if_necessary(@encoded_binary_prefix <> binary) do
+    :erlang.binary_to_term(binary)
+  end
+  def decode_if_necessary(binary), do: binary


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T16:21:35Z

lib/elix/brain.ex

+    @store.remove(key, item)
+  end
+
+  def at_index(key, index) when is_binary(key)


Functions should have a @SPEC type specification.

stevegrossi · 2017-01-29T18:22:19Z

Ebert has finished reviewing this Pull Request and has found:

4 possible new issues (including those that may have been commented here).
20 fixed issues! 🎉

You can see more details about this review at https://ebertapp.io/github/indyelixir/elix/pulls/11.

stevegrossi · 2017-01-29T18:22:18Z

lib/elix/brain/redis_store.ex

+  end
+
+  @spec get(String.t) :: any
+  def delete(key) do


Functions should have a @SPEC type specification.

I think this one does, Ebert. Update: whoops, nope I badly copy-pasta'd 🍝

stevegrossi · 2017-01-29T18:22:19Z

lib/elix/brain/redis_store.ex

+  end
+
+  @spec add(String.t, any) :: any
+  def remove(key, item) do


Functions should have a @SPEC type specification.

Another badly copy-pasted type signature.

stevegrossi · 2017-01-29T18:22:19Z

lib/elix/brain/redis_store.ex

+  end
+
+  @spec add(String.t, integer) :: any
+  def at_index(key, index) do


Functions should have a @SPEC type specification.

Another badly copy-pasted type signature.

stevegrossi · 2017-01-30T12:46:12Z

lib/elix/brain/process_store.ex

+  def set(key, value) do
+    Agent.update(__MODULE__, fn (state) ->
+      Map.put(state, key, value)
+    end)


This could perhaps be rewritten as:

Agent.update(__MODULE__, &Map.put(&1, key, value))

I ended up rewriting a number of these to use the capture syntax.

stevegrossi · 2017-01-30T12:47:20Z

lib/elix/brain/redis_store.ex

+  @encoded_binary_prefix "binterm:"
+
+  def start_link do
+    redis_url = System.get_env("REDIS_URL") || "redis://"


I should perhaps also check for Application.get_env(:brain, :redis_url) to allow configuration via config.exs in case folks don't want to use ENV vars.

This seems like a case where you don' want a default value in production

Why's that? To prevent people from using a Redis instance they may not intend? I generally agree about having a higher bar for intentionality in production.

Pretty much. If part of your test setup clears the database, you don't want that to accidentally happen in production. It still can but you should roadblock as much as possible. And halt running when a step (configuration) has been skipped.

stevegrossi · 2017-01-30T12:49:41Z

lib/elix/brain/redis_store.ex

+    command!(["DEL", key])
+    Enum.each(list, fn (item) ->
+      command!(["RPUSH", key, encode_if_necsssary(item)])
+    end)


This should probably use a MULTI transaction (or whatever Redix provides for that) so that these operations are atomic and the key isn't deleted unless adding the new values also succeeds.

I ended up wrapping this in a MULTI transaction, and also taking advantage of the fact that RPUSH can take an arbitrary number of items in order to append them all in a single operation:

def set(key, list) do transaction fn -> command!(["DEL", key]) items = Enum.map(list, &encode_if_necsssary(&1)) command!(["RPUSH", key] ++ items) end end

https://github.com/stevegrossi/hedwig_brain/blob/master/lib/brain/redis_store.ex#L18

stevegrossi · 2017-01-30T12:52:13Z

lib/elix/brain/redis_store.ex

+    else
+      @encoded_binary_prefix <> :erlang.term_to_binary(term)
+    end
+  end


Whoops, I could totally replace the conditional here with two function clauses, like I did for decode_if_necessary...

stevegrossi · 2017-01-30T12:53:20Z

lib/elix/responders/lists.ex

@@ -11,7 +11,7 @@ defmodule Elix.Responders.Lists do
  show lists - Displays all lists
  """
  respond ~r/show lists\Z/i, msg do
-    reply(msg, render_items(List.all_names))
+    send(msg, render_items(List.all_names))


Not really related, but I should show some kind of "There are no lists yet" message when List.all_names is empty. Currently, the robot just doesn't reply.

Should List.allnames return a nullobject-ey thing so you don't need to change this logic?

I ended up using a case:

response = case List.all_names do [] -> "There are no lists yet." lists -> render_items(lists) end

...but I'd be very interested in null-object-like patterns in Elixir. The only (crazy) thing I can think of offhand is a custom struct that you defimpl enumerable on so when I Enum.map over the result of List.all_names it returns a string. But that feels pretty heavy-handed.

I think this is reasonable. That is effectively null object. Really the decision is where does the defaulting happen. In this case it might make sense to push it down and have a NullList object that gets returned from the List module. Without typing it hard to say what I would like more.

stevegrossi · 2017-01-30T12:54:12Z

test/brain/redis_store_test.exs

+
+  setup do
+    RedisStore.start_link
+    Redix.command!(:redis, ["FLUSHDB"])


Maybe this should be a function on RedisStore? Feels a little awkward reaching down into its Redix dependency directly.

Seems like a good idea.

Ended up adding a RedisStore.delete_all function. I considered adding something similar to the ProcessStore but figure I can wait until there's actually a use for it. I also considered giving it a clever name like Brain.amnesia! but when I get clever I tend to regret it.

janxious

Some belated feedback. Overall excellent

janxious · 2017-02-06T01:50:32Z

lib/elix/brain.ex

+  end
+
+  @doc """
+  Stores a list at the given key, overwriting the existing value if present.


I feel like this is not the right documentation.

🍝 Good catch, thanks.

janxious · 2017-02-06T01:52:40Z

lib/elix/brain/redis_store.ex

+  @encoded_binary_prefix "binterm:"
+
+  def start_link do
+    redis_url = System.get_env("REDIS_URL") || "redis://"


This seems like a case where you don' want a default value in production

janxious · 2017-02-06T01:53:04Z

lib/elix/brain/redis_store.ex

+    command!(["DEL", key])
+    Enum.each(list, fn (item) ->
+      command!(["RPUSH", key, encode_if_necsssary(item)])
+    end)


janxious · 2017-02-06T01:55:35Z

lib/elix/brain/store.ex

@@ -0,0 +1,11 @@
+defmodule Elix.Brain.Store do
+  @callback start_link :: any


Haven't seen @callback before AFAIK. What does that do? This looks dangerously like a header file 😁

@callback is part of definining behaviours (docs). Here I'm saying that any module that implements @behaviour Elix.Brain.Store (i.e. the redis and process stores) must implement a start_link function with the signature declared here. If it doesn't, we get a compiler warning, so I've found it a handy way to gently enforce a consistent API when using polymorphism. Though I do find @callback to be a confusing name for this.

janxious · 2017-02-06T01:57:41Z

lib/elix/message_scheduler.ex

@@ -64,4 +71,14 @@ defmodule Elix.MessageScheduler do
  defp heartbeat do
    Process.send_after(self(), :heartbeat, :timer.seconds(1))
  end
+
+  # This is unnecessary for the process brain. I feel like


Concur with comment. Perhaps should have been a TODO

Oh yeah, and I can totally do this now, as the Redis brain already handles encoding and decoding itself. Good catch!

janxious · 2017-02-06T01:58:49Z

lib/elix/responders/lists.ex

@@ -11,7 +11,7 @@ defmodule Elix.Responders.Lists do
  show lists - Displays all lists
  """
  respond ~r/show lists\Z/i, msg do
-    reply(msg, render_items(List.all_names))
+    send(msg, render_items(List.all_names))


Should List.allnames return a nullobject-ey thing so you don't need to change this logic?

janxious · 2017-02-06T01:59:58Z

test/brain/redis_store_test.exs

+
+  setup do
+    RedisStore.start_link
+    Redix.command!(:redis, ["FLUSHDB"])


Seems like a good idea.

stevegrossi added 9 commits January 22, 2017 08:51

Use ScheduledMessage struct

a77e631

Begin extracting some persistence into Elix.Brain

ef9fab4

Refer to Brain in Elix Lists

77a665d

Tests failing like whoa, but it works in the console so it’s the tests’ problem Also, just `send`, no need to `reply`

Get list tests passing

d9ec0cf

neat that setup works for doctests, too!

Extract setup for brain tests

c3d3447

New Brain.set function

a591de5

Rename Brain.all to Brain.get to match Brain.set

efaac57

Yay, list responder tests passing with proper setup!

790d298

stevegrossi commented Jan 22, 2017

View reviewed changes

stevegrossi added 3 commits January 22, 2017 14:23

And we don’t need RedixMock anymore

2411800

Permanently supervise a Brain process

56274e1

I don’t love calling Brain.set in test setup, though, but I guess it makes sense. Starting the Brain process with existing data felt somehow cleaner than using its public API, but as I write that it feels like the opposite should be true.

Separate brain backends

032a83f

stevegrossi commented Jan 22, 2017

View reviewed changes

stevegrossi added 3 commits January 27, 2017 20:13

Fixup tests

16b032f

Rename and reorganize some functions

b69d71c

Rewrite ProcessStore as an Agent

a1e1645

stevegrossi commented Jan 29, 2017

View reviewed changes

stevegrossi added 2 commits January 29, 2017 10:24

Get brain tests passing with RedisStore

6f97c7b

Need a way to test this separately, but coupling the store to the environment makes that hard

Separate tests for RedisStore

140498e

stevegrossi commented Jan 29, 2017

View reviewed changes

lib/elix/brain/redis_store.ex

end)

end

def get(key) do

Copy link

Member Author

stevegrossi Jan 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions should have a @SPEC type specification.

stevegrossi commented Jan 29, 2017

View reviewed changes

Final API changes, some typespecs and docs update

212b57c

stevegrossi commented Jan 29, 2017

View reviewed changes

stevegrossi merged commit 786694d into master Jan 29, 2017

stevegrossi deleted the stevegrossi/brain branch January 29, 2017 18:37

stevegrossi commented Jan 30, 2017

View reviewed changes

janxious reviewed Feb 6, 2017

View reviewed changes

stevegrossi mentioned this pull request Feb 6, 2017

Don't rely on a default Redis URL in production stevegrossi/hedwig_brain#1

Open

		@@ -0,0 +1,11 @@
		defmodule Elix.Brain.Store do
		@callback start_link :: any

Extract a Brain module as an abstraction for persistence #11

Extract a Brain module as an abstraction for persistence #11

Conversation

stevegrossi commented Jan 22, 2017 • edited Loading

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

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

stevegrossi commented Jan 29, 2017

Choose a reason for hiding this comment

stevegrossi Jan 30, 2017 • edited Loading

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

stevegrossi Jan 30, 2017 • edited Loading

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

janxious 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

stevegrossi commented Jan 22, 2017 •

edited

Loading

stevegrossi Jan 30, 2017 •

edited

Loading

stevegrossi Jan 30, 2017 •

edited

Loading