Raise when empty list is given to values/2

[greg-rychlewski]

Closes #4522

[fuelen]

I think it would be great to mention this in docs as well. It is not obvious that empty list must be handled explicitly. I caught this error in production.

[greg-rychlewski]

@fuelen Thank you that's a good callout. I will update the docs.

Out of curiosity do you know of any other standard way that this is handled?

[fuelen]

Out of curiosity do you know of any other standard way that this is handled?

If we talk about elegant ways, then I don't know any

[greg-rychlewski]

Ok I'll merge it this way since I wasn't able to find a way to represent an empty values list. If that changes I'll submit a PR to the adapters to change the SQL.

Thank you for the report.

[josevalim]

I have asked chatgpt and it suggested generating (SELECT NULL WHERE false), which I think is better than failing. WDYT?

[fuelen]

It will work, but it is no longer a VALUES statement 🙂 Don't know whether this is a problem.
Columns and types should be defined as well, so in a more complex scenario it will look like this

SELECT * FROM (SELECT NULL::uuid, NULL::text, NULL::int WHERE false) AS v1 ("id", "name", "age")

[josevalim]

I don't think it is a problem in this case, but maybe @greg-rychlewski disagrees. :)

[greg-rychlewski]

I think it could be a bit jarring to see such a different query in the logs if you're investigating unexpected input. But I also agree it is not good that it raises.

Since I can't think of anything better then I would go with it because I agree raising is the worst option.

[warmwaffles]

@josevalim is it really wise to send that along the connection and wait for the db to respond though?

[greg-rychlewski]

It could be part of a left join like this

select * from test left join (select null where false) as n  on true;

[greg-rychlewski]

I am still a bit unsure but after thinking about it more, it might not be so bad to let the query go through and then raise at the driver level. My reasoning:

1. It keeps the query the same as the user wrote it, so that it is not surprising to see something different in your logs.
2. It is closer to the behaviour you will get in other systems like psql. Where you will get an error if you try to use an empty values list.
3. It leaves the option open for databases that want to support empty values lists. I don't know of any but I know in general some databases introduce nonstandard behaviour for convenience.

Probably point 2 is the most contentious because sometimes Ecto helps and sometimes it doesn't. In this case I could see a case for saying "ok you asked for an empty list, you got it". Similar to if you ask not a non-existent table.

If we go this route then I still to clean up the SQL because we have some issues like missing types.

[josevalim]

Any route is fine by me. 👍