Skip to content
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

Incorrect selectAll typing #1059

Open
romeerez opened this issue Jun 25, 2024 · 6 comments
Open

Incorrect selectAll typing #1059

romeerez opened this issue Jun 25, 2024 · 6 comments
Labels
bug Something isn't working typescript Related to Typescript wontfix This will not be worked on

Comments

@romeerez
Copy link

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 igalklebanov added bug Something isn't working typescript Related to Typescript labels Jun 25, 2024
@igalklebanov
Copy link
Member

Hey 👋

Thank you for submitting this crucial issue!

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

selectAll('pet')

@igalklebanov
Copy link
Member

igalklebanov commented Jun 25, 2024

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
Copy link
Member

koskimas commented Jun 25, 2024

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.

@igalklebanov igalklebanov added the wontfix This will not be worked on label Jun 26, 2024
@romeerez
Copy link
Author

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.

@koskimas
Copy link
Member

koskimas commented Jul 9, 2024

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.

select * could and should almost always be replaced by select table.* for sure, but there might be some legitimate use cases for select *. Preventing people from from writing it is too opinionated IMO.

@hughgardiner

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working typescript Related to Typescript wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants