Skip to content

Commit

Permalink
Field keystone to non dynmic for casting (#10)
Browse files Browse the repository at this point in the history 
Only utilize dynamic fragment when it's needed to preserve previous casting behaviour.

* Expected fix for field casting from dynamic change

* first pass at tests

- missing coverage on nil comparision, I'm going to need to change the
  tests to a different schema to get full converage.

* Wordy things with routes

* Add tests for nillabe complex coverage

* Naming consistency

* Drop comments

* Release notes, for the fix
  • Loading branch information
@pmonson711
pmonson711 authored Mar 14, 2022
1 parent a6ef115 commit ebfb7f6
Show file tree
Hide file tree
Showing 7 changed files with 349 additions and 84 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ 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).

## 0.6.1 - 2022-03-14

### Fixed

- Fixed some cases where date / datetime casting suffered from the dynamic
expressions approach. The original Ecto integration queries now run unless a
fragment if supplied.

## 0.6.0 - 2022-03-10

### Changed
Expand Down
206 changes: 156 additions & 50 deletions lib/fob.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ defmodule Fob do
page_breaks = PageBreak.add_query_info(page_breaks, query)

query
|> apply_keyset_comparison(page_breaks, :strict)
|> route_keyset_comparison(page_breaks, :strict)
|> apply_limit(page_size)
end

Expand All @@ -39,15 +39,25 @@ defmodule Fob do
limit(queryable, ^page_size)
end

defp apply_keyset_comparison(
defp route_keyset_comparison(routeable_page_break, acc) do
case routeable_page_break do
{page_break, nil} ->
apply_keyset_comparison_field(page_break, acc)

{page_break, expr} ->
apply_keyset_comparison_expression(page_break, expr, acc)
end
end

defp route_keyset_comparison(
%Ecto.Query{} = query,
nil = _page_breaks,
_comparison_strictness
) do
query
end

defp apply_keyset_comparison(
defp route_keyset_comparison(
%Ecto.Query{} = query,
[_ | _] = page_breaks,
comparison_strictness
Expand All @@ -58,88 +68,184 @@ defmodule Fob do

where_clause =
remaining_breaks
|> PageBreak.wrap_field_or_alias(query)
|> Enum.reduce(initial_acc, &apply_keyset_comparison/2)
|> PageBreak.wrap_to_routeable(query)
|> Enum.reduce(initial_acc, &route_keyset_comparison/2)

where(query, ^where_clause)
end

defp apply_keyset_comparison(page_break, accumulator)
defp apply_keyset_comparison_field(page_break, acc)

# --- value is nil

defp apply_keyset_comparison(
{%PageBreak{
direction: direction,
value: nil
}, field_or_alias},
defp apply_keyset_comparison_field(
%PageBreak{
direction: direction,
value: nil,
table: table,
column: column
},
acc
)
when direction in [:asc, :asc_nulls_last, :desc_nulls_last] do
dynamic(^field_or_alias |> is_nil() and ^acc)
dynamic([{t, table}], field(t, ^column) |> is_nil() and ^acc)
end

defp apply_keyset_comparison(
{%PageBreak{
direction: direction,
value: nil
}, field_or_alias},
defp apply_keyset_comparison_field(
%PageBreak{
direction: direction,
value: nil,
table: table,
column: column
},
acc
)
when direction in [:desc, :desc_nulls_first, :asc_nulls_first] do
dynamic(
not is_nil(^field_or_alias) or
(^field_or_alias |> is_nil() and ^acc)
[{t, table}],
not is_nil(field(t, ^column)) or
(field(t, ^column) |> is_nil() and ^acc)
)
end

# --- value is non-nil
defp apply_keyset_comparison_field(
%PageBreak{
direction: direction,
value: value,
table: table,
column: column
},
acc
)
when direction in [:asc, :asc_nulls_last] do
dynamic(
[{t, table}],
field(t, ^column) > ^value or field(t, ^column) |> is_nil() or
(field(t, ^column) == ^value and ^acc)
)
end

defp apply_keyset_comparison_field(
%PageBreak{
direction: :asc_nulls_first,
value: value,
table: table,
column: column
},
acc
) do
dynamic(
[{t, table}],
field(t, ^column) > ^value or (field(t, ^column) == ^value and ^acc)
)
end

defp apply_keyset_comparison(
{%PageBreak{
direction: direction,
value: value
}, field_or_alias},
defp apply_keyset_comparison_field(
%PageBreak{
direction: direction,
value: value,
table: table,
column: column
},
acc
)
when direction in [:desc, :desc_nulls_first] do
dynamic(
[{t, table}],
field(t, ^column) < ^value or (field(t, ^column) == ^value and ^acc)
)
end

defp apply_keyset_comparison_field(
%PageBreak{
direction: :desc_nulls_last,
value: value,
table: table,
column: column
},
acc
) do
dynamic(
[{t, table}],
field(t, ^column) < ^value or field(t, ^column) |> is_nil() or
(field(t, ^column) == ^value and ^acc)
)
end

defp apply_keyset_comparison_expression(
%PageBreak{
direction: direction,
value: nil
},
expression,
acc
)
when direction in [:asc, :asc_nulls_last, :desc_nulls_last] do
dynamic(^expression |> is_nil() and ^acc)
end

defp apply_keyset_comparison_expression(
%PageBreak{
direction: direction,
value: nil
},
expression,
acc
)
when direction in [:desc, :desc_nulls_first, :asc_nulls_first] do
dynamic(
not is_nil(^expression) or
(^expression |> is_nil() and ^acc)
)
end

defp apply_keyset_comparison_expression(
%PageBreak{
direction: direction,
value: value
},
expression,
acc
)
when direction in [:asc, :asc_nulls_last] do
dynamic(
^field_or_alias > ^value or ^field_or_alias |> is_nil() or
(^field_or_alias == ^value and ^acc)
^expression > ^value or ^expression |> is_nil() or
(^expression == ^value and ^acc)
)
end

defp apply_keyset_comparison(
{%PageBreak{
direction: :asc_nulls_first,
value: value
}, field_or_alias},
defp apply_keyset_comparison_expression(
%PageBreak{
direction: :asc_nulls_first,
value: value
},
expression,
acc
) do
dynamic(^field_or_alias > ^value or (^field_or_alias == ^value and ^acc))
dynamic(^expression > ^value or (^expression == ^value and ^acc))
end

defp apply_keyset_comparison(
{%PageBreak{
direction: direction,
value: value
}, field_or_alias},
defp apply_keyset_comparison_expression(
%PageBreak{
direction: direction,
value: value
},
expression,
acc
)
when direction in [:desc, :desc_nulls_first] do
dynamic(^field_or_alias < ^value or (^field_or_alias == ^value and ^acc))
dynamic(^expression < ^value or (^expression == ^value and ^acc))
end

defp apply_keyset_comparison(
{%PageBreak{
direction: :desc_nulls_last,
value: value
}, field_or_alias},
defp apply_keyset_comparison_expression(
%PageBreak{
direction: :desc_nulls_last,
value: value
},
expression,
acc
) do
dynamic(
^field_or_alias < ^value or ^field_or_alias |> is_nil() or
(^field_or_alias == ^value and ^acc)
^expression < ^value or ^expression |> is_nil() or
(^expression == ^value and ^acc)
)
end

Expand Down Expand Up @@ -236,8 +342,8 @@ defmodule Fob do
stop = stop |> PageBreak.add_query_info(query) |> reverse()

query
|> apply_keyset_comparison(start, :lenient)
|> apply_keyset_comparison(stop, :lenient)
|> route_keyset_comparison(start, :lenient)
|> route_keyset_comparison(stop, :lenient)
end

defp reverse(nil), do: nil
Expand Down
12 changes: 6 additions & 6 deletions lib/fob/ordering.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@ defmodule Fob.Ordering do
# in a query

alias Ecto.Query
import Ecto.Query
alias Fob.FragmentBuilder
require Fob.FragmentBuilder

@typep table :: nil | non_neg_integer()
@typep expr :: Macro.t()

@type t :: %__MODULE__{
table: table(),
column: atom(),
direction: :asc | :desc,
field_or_alias: any()
maybe_expression: nil | expr()
}

defstruct ~w[table column direction field_or_alias]a
defstruct ~w[table column direction maybe_expression]a

@spec config(%Query{}) :: [t()]
def config(%Query{order_bys: orderings} = query) do
Expand All @@ -39,7 +39,7 @@ defmodule Fob.Ordering do
direction: direction,
column: column,
table: table,
field_or_alias: dynamic([{t, table}], field(t, ^column))
maybe_expression: nil
}
end

Expand All @@ -51,7 +51,7 @@ defmodule Fob.Ordering do

column = FragmentBuilder.column_for_query_fragment(frag, query)

field_or_alias =
dyn_expression =
Fob.FragmentBuilder.build_from_existing(
[{t, table}],
frag
Expand All @@ -61,7 +61,7 @@ defmodule Fob.Ordering do
direction: direction,
column: column,
table: table,
field_or_alias: field_or_alias
maybe_expression: dyn_expression
}
end

Expand Down
8 changes: 4 additions & 4 deletions lib/fob/page_break.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,16 @@ defmodule Fob.PageBreak do
%__MODULE__{page_break | table: order.table, direction: order.direction}
end

def wrap_field_or_alias(page_breaks, %Ecto.Query{} = query)
def wrap_to_routeable(page_breaks, %Ecto.Query{} = query)
when is_list(page_breaks) do
ordering_config = Ordering.config(query)

Enum.map(page_breaks, &wrap_field_or_alias(&1, ordering_config))
Enum.map(page_breaks, &wrap_to_routeable(&1, ordering_config))
end

def wrap_field_or_alias(%{column: column} = page_break, ordering_config) do
def wrap_to_routeable(%{column: column} = page_break, ordering_config) do
order = Enum.find(ordering_config, fn order -> column == order.column end)
{page_break, order.field_or_alias}
{page_break, order.maybe_expression}
end

@doc since: "0.2.0"
Expand Down
Loading

0 comments on commit ebfb7f6

Please sign in to comment.