vitaly-t / pg-promise

PostgreSQL interface for Node.js
https://vitaly-t.github.io/pg-promise
MIT License
3.47k stars 217 forks source link

pgp.helpers.insert: table should accept fully qualified name as a string #931

Closed jmealo closed 6 months ago

jmealo commented 6 months ago

Expected behavior

pgp.helpers.insert(..., ..., 'schema.table'); 
pgp.helpers.insert(..., ..., 'database.schema.table');

If fully qualified table names aren't allowed, this should throw telling you to pass it as an object.

Actual behavior

The entire table name is quoted, causing hard to track down issues about the relation not existing.

Steps to reproduce

Try to pass a fully qualified table name as a string.

Environment

vitaly-t commented 6 months ago

Dots are allowed within table unicode names, so there is nothing to throw an error about. You just need to use TableName object when schema.table are used.

Also, there is no such thing as database.schema.table in PostgreSQL. Databases are represented only by the connection.

vitaly-t commented 6 months ago

And you can always use your own way of processing table names like that.

Below is an example of splitting such table names:

function _T(data: TemplateStringsArray) {
    const [schema, table] = data[0].split('.');
    return {table: {schema, table}};
}

Usage example:

const cs = new pgp.helpers.ColumnSet([ 'id', 'name' ], _T`schema.table`);
jmealo commented 6 months ago

So it doesn't support fully qualified names? The kind that work via the SQL interface? You introduced your own incompatible with the actual database API? 🤔

https://www.postgresql.org/docs/current/ddl-schemas.html

For context: most folks don't even know about search paths, this seems like a foot canon. More folks use fully qualified names versus than search path.

tableNameWithoutSchema or unqualifiedTableName might be a better parameter name to clue folks into the issue.

vitaly-t commented 6 months ago

This library does not parse schema from table for query generation, which is not needed there. And it handles schema separately, because it is needed for easy separation when any schema override is used, so it can be switched easily.

In your comments, you seem to tie together unrelated things, for something that you want. I gave the solution to your original post - use of TableName type to wrap table + schema easily.

P.S. You may have seen this in other libraries, that incorrectly parse dots into path. In PostgreSQL, you can create a table with name "a.b.c", for example, and then those assumptions will break. This library handles all scenarios for handling Unicode SQL names, not just some subset.

jmealo commented 6 months ago

@vitaly-t: Thanks for your patience. I have a fever and troubleshooting this took longer than I'd like. I guess there's no good option to avoid this foot canon. Your implementation is pragmatic. Closing now.

vitaly-t commented 6 months ago

Following this, I added helper _TN into v11.7.0 of the driver, which was then followed by a few fixes and improvements, through version 11.7.8

@jmealo