Skip to content

fix: unexpected results when embedding the same table twice #4104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: unexpected results when embedding the same table twice #4104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/PostgREST/Plan.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,7 @@ updateNode f (targetNodeName:remainingPath, a) (Right (Node rootNode forest)) =
updateNode f (remainingPath, a) (Right target)
where
findNode :: Maybe ReadPlanTree
findNode = find (\(Node ReadPlan{relName, relAlias} _) -> relName == targetNodeName || relAlias == Just targetNodeName) forest
findNode = find (\(Node ReadPlan{relName, relAlias} _) -> fromMaybe relName relAlias == targetNodeName) forest

mutatePlan :: Mutation -> QualifiedIdentifier -> ApiRequest -> SchemaCache -> ReadPlanTree -> Either Error MutatePlan
mutatePlan mutation qi ApiRequest{iPreferences=Preferences{..}, ..} SchemaCache{dbTables, dbRepresentations} readReq =
Expand Down
12 changes: 11 additions & 1 deletion test/spec/Feature/Query/QuerySpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,16 @@ spec = do
{ "id":4,"children":[]}
]|] { matchHeaders = [matchContentTypeJson] }

it "works when embedding the same table more than once" $
get "/places?select=name,visits(id,start_time,visit_type),work_visits:visits(id,start_time,visit_type)&id=eq.1&visits.visit_type=neq.work&visits.start_time=gt.20250101+00:00&work_visits.visit_type=eq.work&work_visits.start_time=gt.20250101+00:00" `shouldRespondWith`
[json|[
{
"name":"Lake",
"visits":[{"id": 1, "start_time": "2025-01-01T10:00:00", "visit_type": "vacation"}, {"id": 2, "start_time": "2025-01-01T15:00:00", "visit_type": "vacation"}],
"work_visits":[{"id": 3, "start_time": "2025-01-01T20:00:00", "visit_type": "work"}]
}
]|] { matchHeaders = [matchContentTypeJson] }

describe "ordering response" $ do
it "by a column asc" $
get "/items?id=lte.2&order=id.asc"
Expand Down Expand Up @@ -1138,7 +1148,7 @@ spec = do
{ matchHeaders = [matchContentTypeJson] }

it "ordering embeded entities with alias" $
get "/projects?id=eq.1&select=id, name, the_tasks:tasks(id, name)&tasks.order=name.asc" `shouldRespondWith`
get "/projects?id=eq.1&select=id, name, the_tasks:tasks(id, name)&the_tasks.order=name.asc" `shouldRespondWith`
Copy link
Member Author

@laurenceisla laurenceisla May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice that we allow using the table name as filter even when the alias is present. This is the only test that mentions this, but I don't think this is expected, right?

From now on, this would fail with an error like:

GET /projects?id=eq.1&select=id, name, the_tasks:tasks(id, name)&tasks.order=name.asc
{
  "code":"PGRST108",
  "details":null,
  "hint":"Verify that 'tasks' is included in the 'select' query parameter.",
  "message":"'tasks' is not an embedded resource in this request"
}

If this change is OK then more info to the hint would be needed like: "If it has an alias, then use it instead of the resource name".

If using the resource name as well as the alias is expected, then this would be a breaking change and the solution would be different. Maybe throw an error when there may be ambiguity (like in #2529).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From now on, this would fail with an error like:

This would also fail for filters, limits, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as you're only using one embedding for the same table, but with an alias, this is a working query right now. So, I'd say we only fix this in a major, otherwise people can't update to the latest patch without risking to break something that works.

[json|[{"id":1,"name":"Windows 7","the_tasks":[{"id":2,"name":"Code w7"},{"id":1,"name":"Design w7"}]}]|]
{ matchHeaders = [matchContentTypeJson] }

Expand Down
17 changes: 15 additions & 2 deletions test/spec/fixtures/data.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,8 +961,21 @@ INSERT INTO artists
VALUES (1, 'duster'), (2, 'black country, new road'), (3, 'bjork');

TRUNCATE TABLE albums CASCADE;
INSERT INTO albums
INSERT INTO albums
VALUES (1, 'stratosphere', 1),
(2, 'ants from up above',2),
(2, 'ants from up above',2),
(3, 'vespertine',3),
(4, 'contemporary movement', 1);

TRUNCATE TABLE places CASCADE;
INSERT INTO places (name)
VALUES ('Lake'), ('Mountain'), ('Beach');

TRUNCATE TABLE visits CASCADE;
INSERT INTO visits (place_id, start_time, end_time, visit_type)
VALUES (1, '2025-01-01 10:00','2025-01-01 11:00', 'vacation'),
(1, '2025-01-01 15:00','2025-01-01 16:00', 'vacation'),
(1, '2025-01-01 20:00', '2025-01-01 21:00', 'work'),
(2, '2024-11-01 09:00','2024-11-01 10:00', 'vacation'),
(3, '2024-12-02 13:00','2024-12-02 14:00', 'vacation'),
(1, '2023-01-02 20:00','2023-01-01 21:00', 'work');
15 changes: 15 additions & 0 deletions test/spec/fixtures/schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3820,3 +3820,18 @@ comment on function test.collision_test_func(id integer) is 'fizzbuzz';
create or replace function test.delete_all_items() returns setof items as $$
delete from items where id <= 15 returning *; -- deletes 15 items, then return them
$$ language sql;

create type visit_type as enum ('vacation', 'work');

create table places (
id int primary key generated always as identity,
name text not null
);

create table visits (
id int primary key generated always as identity,
place_id int not null references places(id),
start_time timestamp,
end_time timestamp,
visit_type visit_type
);