fix: cache a JSON string and get a string back

CHANGELOG.md

@@ -14,13 +14,16 @@
 - fix: `Cache.ConCache.get_or_store/3` followed by `get/1` no longer raises. `get_or_store/3` writes through ConCache directly, bypassing the encode in `put/3`, so the matching `get/1` tried to `binary_to_term/1` a raw term.
 - fix: a binary value wrapped in braces but not valid JSON (eg `"{oops}"`) no longer raises `Jason.DecodeError` on read. `Cache.TermEncoder.decode/1` used `Jason.decode!/1`, and now falls back to returning the binary unchanged.
 - fix: raw `Cache.ETS` operations (`match_object/1`, `select/1`, `tab2list/0`, `foldl/2`) now see the terms that were `put`, rather than the opaque encoded binaries they used to return.
+- fix: caching a JSON string hands back the string. `encode/2` stored a brace-wrapped binary unencoded, so `decode/1` had to guess what it was looking at — `put(:k, ~s({"a": 1}))` followed by `get(:k)` returned `%{"a" => 1}`, a `String` in and a `Map` out. Binaries are now always run through `:erlang.term_to_binary/1`, and `decode/1` keys off the external term format version byte rather than the shape of the payload, so nothing is guessed.
+- fix: `decode/1` no longer raises on a binary that is not an encoded term. It used to reach `:erlang.binary_to_term/1` for anything that was not digits or brace-wrapped, which raised `ArgumentError` on a value written into the store by something other than this library.
 
 ## Breaking Changes
 
 - Values held by native-term adapters are now stored as terms rather than encoded binaries. This is not observable through `get/1`, `put/3` and `delete/1`, which round-trip exactly as before. It is observable if you read the underlying store directly (`:ets.lookup/2`, `:persistent_term.get/1`, `ConCache.get/2`) or through the raw ETS API — those now return terms, which is what they were always meant to return.
 - `Cache.DETS` is unchanged and still encodes, so existing `.dets` files stay readable. `Cache.ETS` with `:rehydration_path` also still encodes, so existing table dumps stay loadable.
 - `Cache.HashRing`, and `Cache.MultiLayer` under `broadcast_mode: :replicate`, also still encode. Those strategies hand the stored value to another node, so a rolling deploy has 0.4.x and 0.5.x reading each other's writes for the same key and they have to agree on the representation. Their wire format is unchanged from 0.4.x and a mixed-version cluster is safe.
 - An in-memory cache populated by an older version and read by this one would return raw binaries, but ETS, Agent, PersistentTerm and ConCache do not survive a restart, and every representation that outlives a node — disk, Redis, another node — is still encoded, so there is no upgrade path on which that can happen.
+- A brace-wrapped or all-digit binary is now stored encoded rather than raw. Keys written by an earlier version are not in external term format, so they still decode the way they always did: a raw JSON string in Redis reads back as a map, a raw digit string as an integer. Only values written from this version on are type-stable. Code that was reading those keys out of Redis with another tool and expecting readable JSON gets an encoded term instead — write JSON through `json_set/3` (RedisJSON), which is a separate path and unchanged.
 
 # 0.4.9
 

guides/explanation/architecture.md

@@ -62,6 +62,12 @@ Adapters declare this through the optional `c:Cache.native_term_storage?/1` call
 resolved once at compile time, so there is no runtime branch on the read or write path.
 The callback is optional and defaults to encoding, so third-party adapters are unaffected.
 
+An encoded value is read back by its format rather than by its shape. External term format
+always begins with the version byte `131` and every value this library encodes is in it, so
+decoding never guesses at a payload. A stored value that is *not* in that format was written
+either by an older version or by something other than this library, and keeps the reading it
+has always had — a raw JSON string decodes to a map, a raw digit string to an integer.
+
 ## Sandboxing for Tests
 
 A unique feature of ElixirCache is its sandboxing capability for tests. When you enable sandboxing:

lib/cache/term_encoder.ex

@@ -140,34 +140,33 @@ defmodule Cache.TermEncoder do
     )
   end
 
+  # An integer is handed to the store as itself, so a store that understands numbers
+  # still sees one. `decode/1` reads it back through the legacy branch below.
   def encode(term, _) when is_integer(term) do
     term
   end
 
-  def encode(term, _) when is_binary(term) do
-    if to_string(term) =~ ~r/^{.*}$/ do
-      term
-    else
-      :erlang.term_to_binary(term)
-    end
-  end
-
   def encode(term, _compression_level) do
     :erlang.term_to_binary(term)
   end
 
+  # External term format always starts with the version byte 131, and every value this
+  # library encodes goes through `:erlang.term_to_binary/1`, so the first byte is the
+  # whole decision. Nothing has to be guessed from the shape of the payload.
+  def decode(<<131, _rest::binary>> = encoded) do
+    encoded
+    |> :erlang.binary_to_term()
+    |> maybe_decode_binary_struct_error()
+  end
+
+  # Not written by this version of the encoder: a value stored before binaries were
+  # encoded unconditionally, or one written straight into the store by something else.
+  # Both are read the way they always were.
   def decode(binary) when is_binary(binary) do
     cond do
-      binary =~ ~r/^\d+$/ ->
-        String.to_integer(binary)
-
-      binary =~ ~r/^{.*}$/ ->
-        decode_json(binary)
-
-      true ->
-        binary
-        |> :erlang.binary_to_term()
-        |> maybe_decode_binary_struct_error
+      binary =~ ~r/^\d+$/ -> String.to_integer(binary)
+      binary =~ ~r/^{.*}$/ -> decode_json(binary)
+      true -> binary
     end
   end
 

test/cache/json_string_round_trip_test.exs

@@ -0,0 +1,90 @@
+defmodule Cache.JsonStringRoundTripTest do
+  @moduledoc """
+  A binary was stored unencoded when it happened to look like JSON, and `decode/1` then
+  had to guess what it was looking at — so caching a JSON string handed back a map.
+  These tests hold the round trip type-stable while keeping values written by earlier
+  versions readable.
+  """
+
+  use ExUnit.Case, async: true
+
+  defmodule RedisCache do
+    use Cache, adapter: Cache.Redis, name: :json_round_trip_redis, opts: [uri: "redis://localhost:6379"]
+  end
+
+  defmodule ETSCache do
+    use Cache, adapter: Cache.ETS, name: :json_round_trip_ets, opts: []
+  end
+
+  setup do
+    start_supervised!({Cache, [RedisCache, ETSCache]})
+
+    :ok
+  end
+
+  describe "&put/3 of a binary that looks like something else" do
+    test "a JSON object string comes back as the same string" do
+      value = ~s({"user": "mika", "roles": ["admin"]})
+
+      assert :ok === RedisCache.put("json_object", value)
+      assert {:ok, value} === RedisCache.get("json_object")
+
+      assert :ok === ETSCache.put(:json_object, value)
+      assert {:ok, value} === ETSCache.get(:json_object)
+    end
+
+    test "a brace-wrapped string that is not JSON comes back unchanged" do
+      assert :ok === RedisCache.put("not_json", "{oops not json}")
+      assert {:ok, "{oops not json}"} === RedisCache.get("not_json")
+    end
+
+    test "a string of digits stays a string" do
+      assert :ok === RedisCache.put("digits", "42")
+      assert {:ok, "42"} === RedisCache.get("digits")
+    end
+
+    test "an integer stays an integer" do
+      assert :ok === RedisCache.put("integer", 42)
+      assert {:ok, 42} === RedisCache.get("integer")
+    end
+
+    test "a binary that starts with the external term format version byte survives" do
+      value = <<131, 104, 2, "not really a term">>
+
+      assert :ok === RedisCache.put("term_lookalike", value)
+      assert {:ok, value} === RedisCache.get("term_lookalike")
+    end
+
+    test "the stored bytes are external term format, not the raw string" do
+      assert :ok === RedisCache.put("stored_shape", ~s({"a": 1}))
+      stored = Cache.Redis.command!(:json_round_trip_redis, ["GET", "json_round_trip_redis:stored_shape"])
+
+      assert <<131, _rest::binary>> = stored
+      assert :erlang.binary_to_term(stored) === ~s({"a": 1})
+    end
+  end
+
+  describe "&Cache.TermEncoder.decode/1 of values written by an earlier version" do
+    test "a raw JSON string still decodes to a map" do
+      assert %{"a" => 1} === Cache.TermEncoder.decode(~s({"a": 1}))
+    end
+
+    test "a raw digit string still decodes to an integer" do
+      assert 42 === Cache.TermEncoder.decode("42")
+    end
+
+    test "a raw string that is neither is returned unchanged" do
+      assert "just a string" === Cache.TermEncoder.decode("just a string")
+    end
+
+    test "a key written by an earlier version reads back the way it always did" do
+      Cache.Redis.command!(:json_round_trip_redis, [
+        "SET",
+        "json_round_trip_redis:legacy_json",
+        ~s({"written_by": "0.4.9"})
+      ])
+
+      assert {:ok, %{"written_by" => "0.4.9"}} === RedisCache.get("legacy_json")
+    end
+  end
+end

test/cache/term_encoder_test.exs

@@ -8,10 +8,12 @@ defmodule Cache.TermEncoderTest do
       assert 1 === TermEncoder.encode(1, nil)
     end
 
-    test "encodes JSON properly" do
+    test "encodes a JSON string as a term rather than handing it through unencoded" do
       json = Jason.encode!(%{"a" => 1})
+      encoded = TermEncoder.encode(json, nil)
 
-      assert json === TermEncoder.encode(json, nil)
+      assert <<131, _rest::binary>> = encoded
+      assert :erlang.binary_to_term(encoded) === json
     end
 
     test "encodes terms properly" do
@@ -22,11 +24,11 @@ defmodule Cache.TermEncoderTest do
   end
 
   describe "&decode/1" do
-    test "decodes integers properly" do
+    test "decodes an unencoded integer written by an earlier version" do
       assert 123 === TermEncoder.decode("123")
     end
 
-    test "decodes JSON properly" do
+    test "decodes an unencoded JSON string written by an earlier version" do
       json = Jason.encode!(%{"a" => 1})
 
       assert %{"a" => 1} === TermEncoder.decode(json)