Skip to content

Commit 391b8cb

Browse files
committed
fix: self-join if combination queries require more fields
In a query like this: ```elixir Post |> Ash.Query.combination_of([ Ash.Query.Combination.base( filter: expr(score == 10), select: [:id, :score], ), Ash.Query.Combination.union_all( filter: expr(score == 20), select: [:id, :score], ) ]) |> Ash.Query.filter(category == "category1") |> Ash.read!() ``` you'd get a SQL query something like this (before my fix): ```sql SELECT id, score FROM ( SELECT id, score FROM posts WHERE score = 10 UNION ALL SELECT id, score FROM posts WHERE score = 20 ) AS posts WHERE posts.category = "category1" ``` Which wouldn't work because `category` is not a part of the inner selects. Now we add a `JOIN` on primary key to the table, and then include all the fields we're missing from that table, in any conditions that we would reference a field that wasn't selected in the combinations.
1 parent dcdafd9 commit 391b8cb

File tree

2 files changed

+162
-6
lines changed

2 files changed

+162
-6
lines changed

lib/data_layer.ex

Lines changed: 121 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3262,6 +3262,8 @@ defmodule AshPostgres.DataLayer do
32623262

32633263
@impl true
32643264
def sort(query, sort, _resource) do
3265+
query = maybe_subquery_upgrade(query, {:sort, sort})
3266+
32653267
{:ok,
32663268
Map.update!(
32673269
query,
@@ -3272,12 +3274,18 @@ defmodule AshPostgres.DataLayer do
32723274

32733275
@impl true
32743276
def select(query, select, _resource) do
3277+
query = maybe_subquery_upgrade(query, {:select, select})
3278+
32753279
if query.__ash_bindings__.context[:data_layer][:combination_query?] ||
32763280
query.__ash_bindings__.context[:data_layer][:combination_of_queries?] do
32773281
binding = query.__ash_bindings__.root_binding
32783282

3279-
query =
3280-
from(row in Ecto.Query.exclude(query, :select), select: %{})
3283+
{query, select} =
3284+
if field_set = query.__ash_bindings__[:already_selected] do
3285+
{query, select -- field_set}
3286+
else
3287+
{from(row in Ecto.Query.exclude(query, :select), select: %{}), select}
3288+
end
32813289

32823290
Enum.reduce(select, query, fn field, query ->
32833291
from(row in query, select_merge: %{^field => field(as(^binding), ^field)})
@@ -3294,6 +3302,7 @@ defmodule AshPostgres.DataLayer do
32943302
end
32953303

32963304
def distinct_sort(query, sort, _) do
3305+
query = maybe_subquery_upgrade(query, {:distinct_sort, sort})
32973306
{:ok, Map.update!(query, :__ash_bindings__, &Map.put(&1, :distinct_sort, sort))}
32983307
end
32993308

@@ -3302,11 +3311,13 @@ defmodule AshPostgres.DataLayer do
33023311
# to come up with alternatives here.
33033312
@impl true
33043313
def distinct(query, distinct, resource) do
3314+
query = maybe_subquery_upgrade(query, {:distinct, distinct})
33053315
AshSql.Distinct.distinct(query, distinct, resource)
33063316
end
33073317

33083318
@impl true
33093319
def filter(query, filter, resource, opts \\ []) do
3320+
query = maybe_subquery_upgrade(query, {:filter, filter})
33103321
used_aggregates = Ash.Filter.used_aggregates(filter, [])
33113322

33123323
query =
@@ -3336,6 +3347,112 @@ defmodule AshPostgres.DataLayer do
33363347
end
33373348
end
33383349

3350+
defp maybe_subquery_upgrade(
3351+
%{__ash_bindings__: %{subquery_upgrade?: true}} = query,
3352+
_
3353+
) do
3354+
query
3355+
end
3356+
3357+
defp maybe_subquery_upgrade(query, type) do
3358+
fieldset = query.__ash_bindings__.context[:data_layer][:combination_fieldset]
3359+
3360+
if query.__ash_bindings__.context[:data_layer][:combination_of_queries?] && fieldset do
3361+
requires_join? =
3362+
case type do
3363+
{:filter, contents} ->
3364+
Enum.any?(
3365+
Ash.Filter.list_refs(contents),
3366+
&(&1.relationship_path != [] || &1.attribute.name not in fieldset)
3367+
)
3368+
3369+
{:calculations, calculations} ->
3370+
Enum.any?(calculations, fn {_, expr} ->
3371+
Enum.any?(
3372+
Ash.Filter.list_refs(expr),
3373+
&(&1.relationship_path != [] || &1.attribute.name not in fieldset)
3374+
)
3375+
end)
3376+
3377+
{sort, sorts} when sort in [:sort, :distinct, :distinct_sort] ->
3378+
Enum.any?(sorts, fn
3379+
{atom, _} when is_atom(atom) ->
3380+
atom not in fieldset
3381+
3382+
{%Ash.Query.Calculation{} = calc, _} ->
3383+
calc.opts
3384+
|> calc.module.expression(calc.context)
3385+
|> Ash.Filter.hydrate_refs(%{
3386+
resource: query.__ash_bindings__.resource,
3387+
parent_stack: query.__ash_bindings__[:parent_resources] || [],
3388+
public?: false
3389+
})
3390+
|> Ash.Filter.list_refs()
3391+
|> Enum.any?(&(&1.relationship_path != [] || &1.attribute.name not in fieldset))
3392+
3393+
_ ->
3394+
true
3395+
end)
3396+
3397+
{:select, select} ->
3398+
Enum.any?(select, &(&1 not in fieldset))
3399+
end
3400+
3401+
resource = query.__ash_bindings__.resource
3402+
3403+
if requires_join? do
3404+
primary_key = Ash.Resource.Info.primary_key(query.__ash_bindings__.resource)
3405+
3406+
if primary_key != [] && primary_key -- fieldset == [] do
3407+
dynamic =
3408+
Enum.reduce(primary_key, nil, fn key, expr ->
3409+
if is_nil(expr) do
3410+
Ecto.Query.dynamic([l, r], field(l, ^key) == field(r, ^key))
3411+
else
3412+
Ecto.Query.dynamic([l, r], field(l, ^key) == field(r, ^key) and ^expr)
3413+
end
3414+
end)
3415+
3416+
default_select =
3417+
MapSet.to_list(
3418+
Ash.Resource.Info.selected_by_default_attribute_names(
3419+
query.__ash_bindings__.resource
3420+
)
3421+
)
3422+
3423+
query_with_select =
3424+
from(sub in query,
3425+
join: row in ^query.__ash_bindings__.resource,
3426+
# why doesn't `.root_binding` work the way I expect it to here?
3427+
on: ^dynamic,
3428+
select: map(row, ^default_select),
3429+
select_merge: map(sub, ^fieldset)
3430+
)
3431+
3432+
from(row in subquery(query_with_select), as: ^0)
3433+
|> AshSql.Bindings.default_bindings(resource, AshPostgres.SqlImplementation)
3434+
|> Map.update!(
3435+
:__ash_bindings__,
3436+
&Map.merge(&1, %{
3437+
already_selected: fieldset,
3438+
subquery_upgrade?: true,
3439+
context: query.__ash_bindings__.context
3440+
})
3441+
)
3442+
else
3443+
raise """
3444+
Unsupported combination query. Combinations must select the primary key if referencing
3445+
any fields that are *not* selected by the combinations in filter, sort & distinct.
3446+
"""
3447+
end
3448+
else
3449+
query
3450+
end
3451+
else
3452+
query
3453+
end
3454+
end
3455+
33393456
@impl true
33403457
def add_aggregates(query, aggregates, resource) do
33413458
AshSql.Aggregate.add_aggregates(
@@ -3349,6 +3466,8 @@ defmodule AshPostgres.DataLayer do
33493466

33503467
@impl true
33513468
def add_calculations(query, calculations, resource, select? \\ true) do
3469+
query = maybe_subquery_upgrade(query, {:calculations, calculations})
3470+
33523471
AshSql.Calculation.add_calculations(
33533472
query,
33543473
calculations,

test/combination_test.exs

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ defmodule AshPostgres.CombinationTest do
110110
|> Ash.Changeset.for_create(:create, %{title: "post4"})
111111
|> Ash.create!()
112112

113-
assert [%Post{title: "post4"}, %Post{title: "post1"}] =
113+
assert [%{title: "post4"}, %{title: "post1"}] =
114114
Post
115115
|> Ash.Query.combination_of([
116116
Ash.Query.Combination.base(
@@ -122,7 +122,9 @@ defmodule AshPostgres.CombinationTest do
122122
limit: 1
123123
)
124124
])
125+
|> Ash.Query.sort(title: :desc)
125126
|> Ash.read!()
127+
|> Enum.map(&Map.take(&1, [:title]))
126128
end
127129

128130
test "you can define computed properties" do
@@ -143,6 +145,7 @@ defmodule AshPostgres.CombinationTest do
143145
|> Ash.Query.combination_of([
144146
Ash.Query.Combination.base(
145147
filter: expr(title == "post3"),
148+
select: [:id],
146149
limit: 1,
147150
calculations: %{
148151
post_group: calc(1, type: :integer),
@@ -151,16 +154,16 @@ defmodule AshPostgres.CombinationTest do
151154
),
152155
Ash.Query.Combination.union_all(
153156
filter: expr(title == "post1"),
157+
select: [:id],
154158
calculations: %{
155159
post_group: calc(2, type: :integer),
156160
common_value: calc(1, type: :integer)
157161
},
158162
limit: 1
159163
)
160164
])
161-
|> Ash.Query.distinct_sort([{calc(^combinations(:common_value)), :asc}])
162-
|> Ash.Query.sort([{calc(^combinations(:post_group)), :desc}])
163-
|> Ash.Query.distinct([{calc(^combinations(:common_value)), :asc}])
165+
|> Ash.Query.sort([{calc(^combinations(:post_group)), :asc}])
166+
|> Ash.Query.distinct([calc(^combinations(:common_value))])
164167
|> Ash.Query.calculate(:post_group, :integer, expr(^combinations(:post_group)))
165168
|> Ash.read!()
166169
end
@@ -389,5 +392,39 @@ defmodule AshPostgres.CombinationTest do
389392
assert "low" in groups
390393
assert "high" in groups
391394
end
395+
396+
test "combination with filters not included in the field set" do
397+
Post
398+
|> Ash.Changeset.for_create(:create, %{title: "post1", score: 10, category: "category1"})
399+
|> Ash.create!()
400+
401+
Post
402+
|> Ash.Changeset.for_create(:create, %{title: "post2", score: 10, category: "category2"})
403+
|> Ash.create!()
404+
405+
Post
406+
|> Ash.Changeset.for_create(:create, %{title: "post3", score: 20, category: "category3"})
407+
|> Ash.create!()
408+
409+
assert ["category1"] =
410+
Post
411+
|> Ash.Query.combination_of([
412+
Ash.Query.Combination.base(
413+
filter: expr(score == 10),
414+
select: [:id, :score],
415+
calculations: %{score_group: calc("low", type: :string)}
416+
),
417+
Ash.Query.Combination.union_all(
418+
filter: expr(score == 20),
419+
select: [:id, :score],
420+
calculations: %{score_group: calc("high", type: :string)}
421+
)
422+
])
423+
|> Ash.Query.filter(category == "category1")
424+
|> Ash.Query.distinct([{calc(^combinations(:score_group)), :asc}])
425+
|> Ash.Query.calculate(:upper_title, :string, expr(fragment("UPPER(?)", title)))
426+
|> Ash.read!()
427+
|> Enum.map(&to_string(&1.category))
428+
end
392429
end
393430
end

0 commit comments

Comments
 (0)