-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: allow to-many ordering #4592
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.
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,35 +106,29 @@ spec = describe "related queries" $ do | |
| , matchHeaders = [matchContentTypeJson] | ||
| } | ||
|
|
||
| it "fails when is not a to-one relationship" $ do | ||
| get "/clients?select=*,projects(*)&order=projects(id)" `shouldRespondWith` | ||
| [json|{ | ||
| "code":"PGRST118", | ||
| "details":"'clients' and 'projects' do not form a many-to-one or one-to-one relationship", | ||
| "hint":null, | ||
| "message":"A related order on 'projects' is not possible" | ||
| }|] | ||
| { matchStatus = 400 | ||
| it "works on a to-many relationship" $ do | ||
| get "/clients?select=*,projects(*)&order=projects(id)&projects.order=id.asc" `shouldRespondWith` | ||
| [json|[ | ||
| {"id":1,"name":"Microsoft","projects":[{"id":1,"name":"Windows 7","client_id":1},{"id":2,"name":"Windows 10","client_id":1}]}, | ||
| {"id":2,"name":"Apple","projects":[{"id":3,"name":"IOS","client_id":2},{"id":4,"name":"OSX","client_id":2}]} | ||
| ]|] | ||
|
Comment on lines
+109
to
+114
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there are multiple rows in the to-many end, how do we decide which row we order into?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you mean exactly? can you please give me an example?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be misinterpreting @steve-chavez, but in this example the nested relationship has two rows for the first "projects":[
{"id":1,"name":"Windows 7","client_id":1},
{"id":2,"name":"Windows 10","client_id":1}
]So, does it take From what I see in other example, it should order the top relationship according to the first element of the nested relationship (in this case
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I understand what you mean. It works like the internal postgres array sorting mechanism. I attached an example image from my application. For your info, the application uses the uuid behind the displayName of the account to sort. Example matrix: results in:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@Fridious Could you answer/clarify the above? I don't quite get your example.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, so basically this is the main query without the SELECT "test"."clients".*,
COALESCE("clients_projects_1"."clients_projects_1", '[]') AS "projects"
FROM "test"."clients"
LEFT JOIN LATERAL
(SELECT json_agg("clients_projects_1" ORDER BY "clients_projects_1"."id" ASC)::jsonb AS "clients_projects_1",
array_agg("clients_projects_1"."id" ORDER BY "clients_projects_1"."id") AS "clients_projects_1_ord_1"
FROM
(SELECT "projects_1".*
FROM "test"."projects" AS "projects_1"
WHERE "projects_1"."client_id" = "test"."clients"."id"
ORDER BY "projects_1"."id" ASC) AS "clients_projects_1") AS "clients_projects_1" ON TRUE
ORDER BY "clients_projects_1"."clients_projects_1_ord_1"So it would order by the result of the insert into test.projects values (20,'Test',1);Ordering the internal by curl 'localhost:3000/clients?select=*,projects(*)&projects.order=id.desc&order=projects(id).asc'[
{"id":1,"name":"Microsoft","projects":[
{"id": 20, "name": "Test", "client_id": 1},
{"id": 2, "name": "Windows 10", "client_id": 1},
{"id": 1, "name": "Windows 7", "client_id": 1}
]},
{"id":2,"name":"Apple","projects":[
{"id": 4, "name": "OSX", "client_id": 2},
{"id": 3, "name": "IOS", "client_id": 2}
]}
]Is this expected? If so it looks confusing from a user perspective.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I explained it a little too confusing. To me, this looks correct.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, so
There are many new things here since we didn't allow to-many ordering before, that's why we're doing many questions here to understand better the implementation. The next step would be to check the integration with other features like aggregates, spread operator, nested resources, etc. I'll be reviewing those and check if anything breaks (these would need to be added to tests too).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, thanks for the clarification. Please let me know if there’s anything you’d like me to change or add on my side.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For starters, this would need a Documentation entry, but in order to do that we need to define the use case with clarity as mentioned here #4592 (comment) (what exactly does the user expect to get with this feature). |
||
| { matchStatus = 200 | ||
| , matchHeaders = [matchContentTypeJson] | ||
| } | ||
| get "/clients?select=*,pros:projects(*)&order=pros(id)" `shouldRespondWith` | ||
| [json|{ | ||
| "code":"PGRST118", | ||
| "details":"'clients' and 'pros' do not form a many-to-one or one-to-one relationship", | ||
| "hint":null, | ||
| "message":"A related order on 'pros' is not possible" | ||
| }|] | ||
| { matchStatus = 400 | ||
| get "/clients?select=*,pros:projects(*)&order=pros(id)&pros.order=id.asc" `shouldRespondWith` | ||
| [json|[ | ||
| {"id":1,"name":"Microsoft","pros":[{"id":1,"name":"Windows 7","client_id":1},{"id":2,"name":"Windows 10","client_id":1}]}, | ||
| {"id":2,"name":"Apple","pros":[{"id":3,"name":"IOS","client_id":2},{"id":4,"name":"OSX","client_id":2}]} | ||
| ]|] | ||
| { matchStatus = 200 | ||
| , matchHeaders = [matchContentTypeJson] | ||
| } | ||
| get "/designers?select=id,computed_videogames(id)&order=computed_videogames(id).desc" `shouldRespondWith` | ||
| [json|{ | ||
| "code":"PGRST118", | ||
| "details":"'designers' and 'computed_videogames' do not form a many-to-one or one-to-one relationship", | ||
| "hint":null, | ||
| "message":"A related order on 'computed_videogames' is not possible" | ||
| }|] | ||
| { matchStatus = 400 | ||
| get "/designers?select=id,computed_videogames(id)&order=computed_videogames(id).desc&computed_videogames.order=id.asc" `shouldRespondWith` | ||
| [json|[ | ||
| {"id":2,"computed_videogames":[{"id":3},{"id":4}]}, | ||
| {"id":1,"computed_videogames":[{"id":1},{"id":2}]} | ||
| ]|] | ||
| { matchStatus = 200 | ||
| , matchHeaders = [matchContentTypeJson] | ||
| } | ||
|
|
||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these test case changes are changing something like
order=x(y)toorder.x=y. But that's not the same, right?I believe the
order.x=yordering worked before already and just sorts the embedding, not the top-level.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I will adjust the tests. thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my personal use case, I can now use these queries:
curl "http://localhost:3000/person?select=sys_id,first_name,last_name,relation_owned_persons(source_id,reference:sys_user(sys_display_name))&order=relation_owned_persons(source_id).asc&offset=0&limit=10"