Incorrect `selectAll` typing

[romeerez]

Hello dear maintainers, I found an incorrect typing in the Not null docs:

     jsonObjectFrom(
       eb.selectFrom('pet')
         // the problem is in this selectAll:
         .selectAll()
         .limit(1)
         .whereRef('person.id', '=', 'pet.owner_id')
     ).$notNull().as('pet')

SQL is correctly selecting * from the pet table, so only the pet columns are actually selected, but the typing combines columns of all tables.

// pet doesn't have a first_name, but no complaint from TS
persons[0]?.pet?.first_name

[igalklebanov]

Hey 👋

Thank you for submitting this crucial issue!

As a workaround, please scope the selectAll to get the expected results:

selectAll('pet')

[igalklebanov]

select * in this case, does indeed return only "pet"'s columns - without "person"'s columns. select "person".* is valid SQL. select "person"."first_name" is valid SQL as well.

To solve this we need to find a way to track the "external" query context (tables that exist in the main query's from clause) aside from the "local" query context (tables that exist in the subquery's from clause). As ExpressionBuilder passes the "external" query context tables to TB, TB currently represents the "total" query context ("external" and "local") and that is what's causing this issue.

My intuition points at an additional (4th) generic @ SelectQueryBuilder representing the tables from the "external" query context - what ExpressionBuilder knows about. The simplest solution, which should only affect SelectQueryBuild.selectAll() (select *) would be to exclude "external" query context tables from TB, while all other methods keep using TB as-is.

We are usually conservative when it comes to adding more generics. However, output type correctness is important, and these helpers are frequently used, so IMHO this time it's worth it.

[koskimas]

We are usually conservative when it comes to adding more generics. However, output type correctness is important, and these helpers are frequently used, so IMHO this time it's worth it.

I strongly disagree with this one. I definitely don't think we should add a fourth generic just for this case as it's easily fixed by using selectAll('pet'). Fixing this would be complicated and it'd only solve this one particular case.

[romeerez]

As an option, selectAll() without arguments could be deprecated, so users won't accidentally select everything from all the joined tables, and it would solve the typing issue.