From ee010a24c064debaf18d0f8945ff526ee512da35 Mon Sep 17 00:00:00 2001 From: Paul Monson <4119175+pmonson711@users.noreply.github.com> Date: Thu, 11 Aug 2022 11:59:42 -0500 Subject: [PATCH 1/5] Allow computed selects that are not orderd by. - currently fob throws errors when joined and an expression is used in the select. - Note to select and order by an expression, you still need to use a Ecto fragment --- lib/fob/ordering.ex | 10 ++- test/fob/computed_select_test.exs | 112 ++++++++++++++++++++++++++++++ test/support/left_join_schemas.ex | 1 + 3 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 test/fob/computed_select_test.exs diff --git a/lib/fob/ordering.ex b/lib/fob/ordering.ex index e9bc134..5c1ced7 100644 --- a/lib/fob/ordering.ex +++ b/lib/fob/ordering.ex @@ -82,10 +82,14 @@ defmodule Fob.Ordering do expr: {:%{}, _, [{:|, _, [{:&, _, [0]}, merges]}]} } }) do - Map.new(merges, fn {toplevel_name, - {{:., _, [{:&, _, [table]}, name_in_table]}, _, _}} -> - {{table, name_in_table}, toplevel_name} + Map.new(merges, fn + {toplevel_name, {{:., _, [{:&, _, [table]}, name_in_table]}, _, _}} -> + {{table, name_in_table}, toplevel_name} + + {_toplevel_name, _ast_expr} = expr -> + {:merged_computed_column, expr} end) + |> Map.delete(:merged_computed_column) end def selection_mapping(%Query{}), do: %{} diff --git a/test/fob/computed_select_test.exs b/test/fob/computed_select_test.exs new file mode 100644 index 0000000..2e3b9bc --- /dev/null +++ b/test/fob/computed_select_test.exs @@ -0,0 +1,112 @@ +defmodule Fob.ComputedSelectTest do + use Fob.RepoCase + alias Fob.Cursor + alias Ecto.Multi + + describe "single table" do + setup do + [schema: SimplePrimaryKeySchema, repo: Fob.Repo] + end + + setup c do + records = for n <- 1..20, do: %{id: n} + + Multi.new() + |> Multi.insert_all(:seeds, c.schema, records) + |> c.repo.transaction() + + :ok + end + + test "when selecting with computed query, we can get each page", c do + cursor = + Cursor.new( + from( + s in c.schema, + select: %{ + id: s.id, + computed_column: 1 >= 5 + }, + order_by: [asc: s.id] + ), + c.repo, + _initial_page_breaks = nil, + 5 + ) + + assert {records, cursor} = Cursor.next(cursor) + assert Enum.map(records, & &1.id) == Enum.to_list(1..5) + + assert {records, cursor} = Cursor.next(cursor) + assert Enum.map(records, & &1.id) == Enum.to_list(6..10) + + assert {records, cursor} = Cursor.next(cursor) + assert Enum.map(records, & &1.id) == Enum.to_list(11..15) + + assert {records, cursor} = Cursor.next(cursor) + assert Enum.map(records, & &1.id) == Enum.to_list(16..20) + + # end of the data set + assert {[], _cursor} = Cursor.next(cursor) + end + end + + describe "left join" do + setup do + [repo: Fob.Repo, trunk_schema: TrunkSchema, child_schema: ChildSchema] + end + + setup c do + trunk_records = for n <- 1..20, do: %{id: n - 1, child: 20 - n} + + child_records = + for n <- 1..20 do + %{id: n - 1, name: String.pad_trailing("", n, "a")} + end + + Multi.new() + |> Multi.insert_all(:trunk_seeds, c.trunk_schema, trunk_records) + |> Multi.insert_all(:child_seeds, c.child_schema, child_records) + |> c.repo.transaction() + + :ok + end + + test """ + we can sort _ascending_ by a left-joined field, + even if renamed in the select clause + """, + c do + child_schema = c.child_schema + + cursor = + Cursor.new( + from(t in c.trunk_schema, + left_join: c in ^child_schema, + on: t.child == c.id, + select: %{t | child_name: c.name, computed: c.id + t.id}, + order_by: [asc: c.name, desc: t.id] + ), + c.repo, + nil, + 5 + ) + + {records, cursor} = Cursor.next(cursor) + + records + |> Enum.with_index(1) + |> Enum.each(fn {record, index} -> + assert String.length(record.child_name) == index + end) + + {records, _cursor} = Cursor.next(cursor) + + records + |> Enum.with_index(6) + |> Enum.each(fn {record, index} -> + assert String.length(record.child_name) == index + end) + end + end +end diff --git a/test/support/left_join_schemas.ex b/test/support/left_join_schemas.ex index bc73480..a9822b5 100644 --- a/test/support/left_join_schemas.ex +++ b/test/support/left_join_schemas.ex @@ -7,6 +7,7 @@ defmodule TrunkSchema do schema "trunk_schema" do field :child, :integer field :child_name, :string, virtual: true + field :computed, :boolean, virtual: true end end From f2af8772a40a3d57fd732546c53622b7c198d177 Mon Sep 17 00:00:00 2001 From: Paul Monson <4119175+pmonson711@users.noreply.github.com> Date: Thu, 11 Aug 2022 12:07:46 -0500 Subject: [PATCH 2/5] Lower duplicate code, remove unneeded asserts --- test/fob/computed_select_test.exs | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/test/fob/computed_select_test.exs b/test/fob/computed_select_test.exs index 2e3b9bc..9055467 100644 --- a/test/fob/computed_select_test.exs +++ b/test/fob/computed_select_test.exs @@ -3,7 +3,7 @@ defmodule Fob.ComputedSelectTest do alias Fob.Cursor alias Ecto.Multi - describe "single table" do + describe "query with single table" do setup do [schema: SimplePrimaryKeySchema, repo: Fob.Repo] end @@ -51,17 +51,17 @@ defmodule Fob.ComputedSelectTest do end end - describe "left join" do + describe "query with left join" do setup do [repo: Fob.Repo, trunk_schema: TrunkSchema, child_schema: ChildSchema] end setup c do - trunk_records = for n <- 1..20, do: %{id: n - 1, child: 20 - n} + trunk_records = for n <- 1..10, do: %{id: n - 1, child: 10 - n} child_records = - for n <- 1..20 do - %{id: n - 1, name: String.pad_trailing("", n, "a")} + for n <- 1..10 do + %{id: n - 1, name: String.pad_trailing("", n, "b")} end Multi.new() @@ -73,8 +73,7 @@ defmodule Fob.ComputedSelectTest do end test """ - we can sort _ascending_ by a left-joined field, - even if renamed in the select clause + we can select a computed value with a left-joined field """, c do child_schema = c.child_schema @@ -92,21 +91,8 @@ defmodule Fob.ComputedSelectTest do 5 ) - {records, cursor} = Cursor.next(cursor) - - records - |> Enum.with_index(1) - |> Enum.each(fn {record, index} -> - assert String.length(record.child_name) == index - end) - - {records, _cursor} = Cursor.next(cursor) - - records - |> Enum.with_index(6) - |> Enum.each(fn {record, index} -> - assert String.length(record.child_name) == index - end) + assert {_records, cursor} = Cursor.next(cursor) + assert {_records, _cursor} = Cursor.next(cursor) end end end From 80d29227b3cd5140c761d8c0bb531398103a9b00 Mon Sep 17 00:00:00 2001 From: Paul Monson <4119175+pmonson711@users.noreply.github.com> Date: Thu, 11 Aug 2022 12:51:12 -0500 Subject: [PATCH 3/5] Switch to Enum.reduce from Map.new for filtering --- lib/fob/ordering.ex | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/fob/ordering.ex b/lib/fob/ordering.ex index 5c1ced7..903f12b 100644 --- a/lib/fob/ordering.ex +++ b/lib/fob/ordering.ex @@ -82,14 +82,14 @@ defmodule Fob.Ordering do expr: {:%{}, _, [{:|, _, [{:&, _, [0]}, merges]}]} } }) do - Map.new(merges, fn - {toplevel_name, {{:., _, [{:&, _, [table]}, name_in_table]}, _, _}} -> - {{table, name_in_table}, toplevel_name} + Enum.reduce(merges, %{}, fn + {toplevel_name, {{:., _, [{:&, _, [table]}, name_in_table]}, _, _}}, + acc -> + Map.put(acc, {table, name_in_table}, toplevel_name) - {_toplevel_name, _ast_expr} = expr -> - {:merged_computed_column, expr} + {_toplevel_name, _ast_expr}, acc -> + acc end) - |> Map.delete(:merged_computed_column) end def selection_mapping(%Query{}), do: %{} From d07a26d5e0b298be4852982b7de8d2cf2a2340b6 Mon Sep 17 00:00:00 2001 From: Paul Monson <4119175+pmonson711@users.noreply.github.com> Date: Thu, 11 Aug 2022 13:05:11 -0500 Subject: [PATCH 4/5] Adding a change log document --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef45acd..7a3fee1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## 1.0.2 - 2022-08-11 + +### Fixed + +- Fixed issues which caused select some expressions to fail when used with join + queries. + ## 1.0.1 - 2022-05-12 ### Added From 3c27accad47dd878c081ff081eb4d65e1eec224d Mon Sep 17 00:00:00 2001 From: Paul Monson <4119175+pmonson711@users.noreply.github.com> Date: Thu, 11 Aug 2022 13:55:48 -0500 Subject: [PATCH 5/5] Some additional clarity in the tests --- test/fob/computed_select_test.exs | 10 ++++++---- test/support/left_join_schemas.ex | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/test/fob/computed_select_test.exs b/test/fob/computed_select_test.exs index 9055467..2eaaf9e 100644 --- a/test/fob/computed_select_test.exs +++ b/test/fob/computed_select_test.exs @@ -25,7 +25,7 @@ defmodule Fob.ComputedSelectTest do s in c.schema, select: %{ id: s.id, - computed_column: 1 >= 5 + computed_column: s.id >= 5 }, order_by: [asc: s.id] ), @@ -83,7 +83,7 @@ defmodule Fob.ComputedSelectTest do from(t in c.trunk_schema, left_join: c in ^child_schema, on: t.child == c.id, - select: %{t | child_name: c.name, computed: c.id + t.id}, + select: %{t | child_name: c.name, id_next: t.id + 1}, order_by: [asc: c.name, desc: t.id] ), c.repo, @@ -91,8 +91,10 @@ defmodule Fob.ComputedSelectTest do 5 ) - assert {_records, cursor} = Cursor.next(cursor) - assert {_records, _cursor} = Cursor.next(cursor) + {records, cursor} = Cursor.next(cursor) + assert Enum.all?(records, &(&1.id_next == &1.id + 1)) + {records, _cursor} = Cursor.next(cursor) + assert Enum.all?(records, &(&1.id_next == &1.id + 1)) end end end diff --git a/test/support/left_join_schemas.ex b/test/support/left_join_schemas.ex index a9822b5..95390be 100644 --- a/test/support/left_join_schemas.ex +++ b/test/support/left_join_schemas.ex @@ -7,7 +7,7 @@ defmodule TrunkSchema do schema "trunk_schema" do field :child, :integer field :child_name, :string, virtual: true - field :computed, :boolean, virtual: true + field :id_next, :integer, virtual: true end end