Show better errors for naming collisions when embedding

[wolfgangwalther]

Against the current spec test schema:

GET /organizations?select=id,referee(id,name),referee(id,name)
{"code":"42712","details":null,"hint":null,"message":"table name \"organizations_referee_1\" specified more than once"}

GET /organizations?select=id,organizations!referee(id,name),organizations!auditor(id,name)
{"code":"42803","details":null,"hint":null,"message":"aggregate functions are not allowed in FROM clause of their own query level"}

While the first one is kind of ok, because we can guess what the problem is, the second one is not.

We should error out when the select spec we have results in two columns with the same name instead of trying to build a query.

[wolfgangwalther]

Maybe it would be more consistent to just throw away duplicate values, similar to how query strings are parsed in general.

Take this against our spec fixtures:

GET /rpc/getitemrange?min=1&min=2&max=4&max=5

This will use min=2&max=5 only.

How about we do the same in something like:

GET /table?select=col,col:other!hint(*),col:third(*)

in which case we would run it just as

GET /table?select=col:third(*)

[steve-chavez]

I think it'd be better to err and prevent confusions. Ignoring already caused problems as seen on https://github.com/PostgREST/postgrest/issues/1771 (wrong queries can remain unnoticed)

Maybe it would be more consistent to just throw away duplicate values, similar to how query strings are parsed in general. GET /rpc/getitemrange?min=1&min=2&max=4&max=5 This will use min=2&max=5 only.

This is usual behavior on web applications AFAICT but our select param doesn't have to follow that - we can make it strict.

[wolfgangwalther]

I looked into this a bit and I think this should be done at the planning stage, not at the parser stage, i.e. in Plan.hs. Probably initReadRequest? This is because, this needs to take all of the field name, the jsonpath and a potential alias into account to determine what the final output column name would be. Similar to how pgFmtSelectItem falls back to those:

https://github.com/PostgREST/postgrest/blob/9be6747b666833b25357763f579516dafdc2b080/src/PostgREST/Query/SqlFragment.hs#L236-L241

[steve-chavez]

Yes, agree. It should be done at the planner stage. Maybe by handling that the name or alias of the node is unique on the select tree.

[steve-chavez]

The error is bad when duplicating to-many ends:

curl 'localhost:3000/clients?select=projects(*),projects(*)'

{"code":"42803","details":null,"hint":null,
 "message":"aggregate functions are not allowed in FROM clause of their own query level"}

But not so bad on to-one ends:

curl 'localhost:3000/clients?select=projects(*),projects(*)'

{"code":"42712","details":null,"hint":null,
 "message":"table name \"projects_clients_1\" specified more than once"}