diff --git a/CHANGELOG.md b/CHANGELOG.md index f59ddd6..8099885 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ - 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 @@ -21,6 +23,7 @@ - `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 diff --git a/guides/explanation/architecture.md b/guides/explanation/architecture.md index 8919948..5de07d2 100644 --- a/guides/explanation/architecture.md +++ b/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: diff --git a/lib/cache/term_encoder.ex b/lib/cache/term_encoder.ex index 5a14b77..8c083e5 100644 --- a/lib/cache/term_encoder.ex +++ b/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 diff --git a/test/cache/json_string_round_trip_test.exs b/test/cache/json_string_round_trip_test.exs new file mode 100644 index 0000000..19a3ea1 --- /dev/null +++ b/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 diff --git a/test/cache/term_encoder_test.exs b/test/cache/term_encoder_test.exs index 1ad6dc3..6d0c8c8 100644 --- a/test/cache/term_encoder_test.exs +++ b/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)