From 8e18a9b84825d446bc81c5f79b90f942423d55fb Mon Sep 17 00:00:00 2001 From: Steven Cahill Date: Sat, 12 Sep 2026 14:21:04 +0000 Subject: [PATCH 1/2] Render unary minus instead of falling through to "?" `expr/3` had no clause for `{:-, _, [arg]}`, so a negated value reached the catch-all at the bottom of the function and was emitted as a bare `?`. That fallback is silent: the expression disappears from the SQL, the parameter it was standing in for is still planned and passed, and the statement runs and returns a plausible-looking wrong answer rather than failing. Ecto plans a negative literal as unary minus once a query is cached, which is how `ago/2` reaches the adapter, so this is on the path of any expiry check. Binary arithmetic (`+`, `-`, `*`, `/`) has no clause either and still falls through the same way; that is left alone here. --- lib/ecto/adapters/libsql/connection.ex | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/ecto/adapters/libsql/connection.ex b/lib/ecto/adapters/libsql/connection.ex index 72da826..40b463e 100644 --- a/lib/ecto/adapters/libsql/connection.ex +++ b/lib/ecto/adapters/libsql/connection.ex @@ -1217,6 +1217,13 @@ defmodule Ecto.Adapters.LibSql.Connection do [expr(arg, sources, query) | " IS NULL"] end + # Ecto plans a negative literal as unary minus, so `ago(14, "day")` arrives as + # `{:datetime_add, _, [_, {:-, _, [14]}, "day"]}` once the query is cached. Without + # this clause the count falls through to the catch-all "?" and the interval is lost. + defp expr({:-, _, [arg]}, sources, query) do + [?-, ?(, expr(arg, sources, query), ?)] + end + defp expr({:not, _, [arg]}, sources, query) do ["NOT (", expr(arg, sources, query), ?)] end From 8c242c56e44dac0e181118008bddcbb09d35b4e5 Mon Sep 17 00:00:00 2001 From: Steven Cahill Date: Sat, 12 Sep 2026 14:21:09 +0000 Subject: [PATCH 2/2] Support datetime_add and date_add so ago/2 keeps its interval `ago/2` and `from_now/2` lower to `datetime_add`/`date_add`, and neither had an `expr/3` clause, so both fell through to the catch-all `?`. The interval was dropped entirely: only the datetime parameter bound, and where: t.inserted_at > ago(14, "day") became `WHERE inserted_at > ?` bound to `utc_now()` - the window never applied. Nothing raised, because Ecto plans both parameters regardless of how many placeholders the adapter emits and SQLite ignores an unreferenced one, so every `ago/2` comparison quietly degraded to `column > now()`. Rows more than a second old read as out of range, and expiry checks built on this - a session token's validity window, for instance - were wrong in the unsafe direction of rejecting live rows while never actually enforcing the interval. Timestamps are written by `datetime_encode/1` via `DateTime.to_iso8601/1`, so the result is formatted to match: comparisons here are textual and a `datetime()`-shaped `YYYY-MM-DD HH:MM:SS` would not order against stored `YYYY-MM-DDTHH:MM:SSZ`. The two agree to the second, so a value carrying sub-second precision, or a naive column with no `Z`, can only fall on the wrong side of a bound when it ties the cutoff to the exact second. SQLite has no week, millisecond or microsecond modifier, so those are converted to ones it does have; it accepts a fractional count. The count is rendered through `expr/3` rather than interpolated, so an interpolated `ago(^n, "day")` still binds as a parameter. --- lib/ecto/adapters/libsql/connection.ex | 50 ++++++++ test/ecto_datetime_interval_test.exs | 162 +++++++++++++++++++++++++ 2 files changed, 212 insertions(+) create mode 100644 test/ecto_datetime_interval_test.exs diff --git a/lib/ecto/adapters/libsql/connection.ex b/lib/ecto/adapters/libsql/connection.ex index 40b463e..f187c0a 100644 --- a/lib/ecto/adapters/libsql/connection.ex +++ b/lib/ecto/adapters/libsql/connection.ex @@ -1391,11 +1391,61 @@ defmodule Ecto.Adapters.LibSql.Connection do defp expr(false, _sources, _query), do: "0" defp expr(nil, _sources, _query), do: "NULL" + # `ago/2` and `from_now/2` lower to these. Without a clause they fall through to + # the catch-all below, which emits a bare "?" - so the interval is dropped, only + # the datetime parameter binds, and the comparison silently degrades to + # `column > now()`. Every expiry window built on `ago/2` then matches nothing + # older than the current second. + # + # Timestamps are stored by `datetime_encode/1` as `DateTime.to_iso8601/1`, so the + # result has to be formatted the same way to compare correctly as text. The two + # formats agree to the second; a value carrying sub-second precision, or a naive + # column with no "Z", can therefore land on the wrong side of a bound only when it + # ties the cutoff to the exact second. + defp expr({:datetime_add, _, [datetime, count, interval]}, sources, query) do + [ + "strftime('%Y-%m-%dT%H:%M:%SZ', ", + expr(datetime, sources, query), + ", ", + interval_modifier(count, interval, sources, query), + ?) + ] + end + + defp expr({:date_add, _, [date, count, interval]}, sources, query) do + [ + "date(", + expr(date, sources, query), + ", ", + interval_modifier(count, interval, sources, query), + ?) + ] + end + # Default fallback for unsupported expressions defp expr(_expr, _sources, _query) do "?" end + # Builds a SQLite time modifier such as '-14 days'. The count stays an expression + # rather than being interpolated, so an interpolated `ago(^n, "day")` still binds + # as a parameter. SQLite has no week/millisecond/microsecond modifiers, so those + # are converted to ones it does have - it accepts a fractional count. + defp interval_modifier(count, interval, sources, query) do + value = expr(count, sources, query) + + {amount, unit} = + case interval do + "week" -> {[?(, value, ") * 7"], "days"} + "millisecond" -> {[?(, value, ") / 1000.0"], "seconds"} + "microsecond" -> {[?(, value, ") / 1000000.0"], "seconds"} + unit when unit in ~w(year month day hour minute second) -> {value, unit <> "s"} + other -> raise ArgumentError, "unsupported interval for libSQL: #{inspect(other)}" + end + + ["CAST(", amount, " AS TEXT) || ' ", unit, ?'] + end + defp combination(%{combinations: []}, _as_prefix), do: [] defp combination(%{combinations: combinations}, as_prefix) do diff --git a/test/ecto_datetime_interval_test.exs b/test/ecto_datetime_interval_test.exs new file mode 100644 index 0000000..07446f1 --- /dev/null +++ b/test/ecto_datetime_interval_test.exs @@ -0,0 +1,162 @@ +defmodule EctoLibSql.EctoDatetimeIntervalTest do + @moduledoc """ + Coverage for `ago/2` and `from_now/2`, which Ecto lowers to `datetime_add` and + `date_add`. + + Neither had an `expr/3` clause, so both fell through to the catch-all that emits a + bare "?" and the interval was dropped: `inserted_at > ago(14, "day")` became + `inserted_at > ?` bound to the current time. Nothing raised - Ecto plans the + parameters regardless of how many placeholders the adapter emits, and SQLite + ignores an unreferenced one - so the comparison silently degraded to + `column > now()` and rejected anything older than the current second while never + enforcing the interval at all. + """ + use ExUnit.Case, async: false + + import Ecto.Query + + defmodule TestRepo do + use Ecto.Repo, otp_app: :ecto_libsql, adapter: Ecto.Adapters.LibSql + end + + defmodule Event do + use Ecto.Schema + + schema "interval_events" do + field(:name, :string) + field(:at, :utc_datetime) + field(:on_date, :date) + end + end + + setup do + unique_id = :erlang.unique_integer([:positive]) + test_db = "z_ecto_libsql_test-datetime_interval-#{unique_id}.db" + + {:ok, pid} = TestRepo.start_link(database: test_db, pool_size: 1, name: TestRepo) + + Ecto.Adapters.SQL.query!(TestRepo, """ + CREATE TABLE IF NOT EXISTS interval_events ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT, + at TEXT, + on_date TEXT + ) + """) + + on_exit(fn -> + if Process.alive?(pid) do + try do + :ok = Supervisor.stop(pid) + catch + :exit, _ -> :ok + end + end + + # Give SQLite time to release the file handles before removing them. + Process.sleep(50) + for f <- Path.wildcard(test_db <> "*"), do: File.rm(f) + end) + + :ok + end + + defp at!(name, amount, unit) do + at = DateTime.add(DateTime.utc_now(:second), amount, unit) + TestRepo.insert!(%Event{name: name, at: at}) + end + + defp names(query), do: query |> TestRepo.all() |> Enum.sort() + + describe "ago/2" do + test "keeps rows inside the window and drops rows outside it" do + at!("now", 0, :second) + at!("day-13", -13, :day) + at!("day-15", -15, :day) + at!("year-ago", -365, :day) + + assert names(from(e in Event, where: e.at > ago(14, "day"), select: e.name)) == + ["day-13", "now"] + end + + test "a row hours old is still inside a day-scale window" do + at!("hours-ago", -6, :hour) + + # The bug made every window behave as `> now()`, so this vanished entirely. + assert names(from(e in Event, where: e.at > ago(14, "day"), select: e.name)) == + ["hours-ago"] + end + + test "minute and hour units resolve to their own scale" do + at!("min-10", -10, :minute) + at!("min-20", -20, :minute) + + assert names(from(e in Event, where: e.at > ago(15, "minute"), select: e.name)) == + ["min-10"] + + assert names(from(e in Event, where: e.at > ago(1, "hour"), select: e.name)) == + ["min-10", "min-20"] + end + + test "week, millisecond and microsecond are converted to units SQLite has" do + at!("day-3", -3, :day) + at!("day-10", -10, :day) + + assert names(from(e in Event, where: e.at > ago(1, "week"), select: e.name)) == ["day-3"] + + at!("recent", -2, :second) + + assert names(from(e in Event, where: e.at > ago(5000, "millisecond"), select: e.name)) == + ["recent"] + + assert names(from(e in Event, where: e.at > ago(5_000_000, "microsecond"), select: e.name)) == + ["recent"] + end + + test "an interpolated count still binds as a parameter" do + at!("day-3", -3, :day) + at!("day-10", -10, :day) + + days = 7 + query = from(e in Event, where: e.at > ago(^days, "day"), select: e.name) + + assert names(query) == ["day-3"] + + # The count is rendered through expr/3 rather than interpolated into the SQL, + # so an interpolated value stays a bound parameter. + {sql, _params} = Ecto.Adapters.SQL.to_sql(:all, TestRepo, query) + assert sql =~ "strftime" + assert sql =~ "days" + end + + test "the interval reaches the SQL rather than being dropped" do + {sql, _params} = + Ecto.Adapters.SQL.to_sql(:all, TestRepo, from(e in Event, where: e.at > ago(14, "day"))) + + assert sql =~ "strftime" + assert sql =~ "days" + refute sql =~ ~r/"at" > \?\s*\)/ + end + end + + describe "from_now/2" do + test "selects rows ahead of now" do + at!("past", -1, :day) + at!("soon", 60, :second) + at!("later", 3, :day) + + assert names(from(e in Event, where: e.at < from_now(1, "day"), select: e.name)) == + ["past", "soon"] + end + end + + describe "date_add" do + test "applies the interval to a date column" do + today = Date.utc_today() + TestRepo.insert!(%Event{name: "d-3", on_date: Date.add(today, -3)}) + TestRepo.insert!(%Event{name: "d-30", on_date: Date.add(today, -30)}) + + assert names(from(e in Event, where: e.on_date > ago(7, "day"), select: e.name)) == ["d-3"] + end + end +end