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

please hint about missing "as()" when from(subquery) is missing it #3485

Closed
kapouer opened this issue Oct 16, 2019 · 11 comments · Fixed by #3496
Closed

please hint about missing "as()" when from(subquery) is missing it #3485

kapouer opened this issue Oct 16, 2019 · 11 comments · Fixed by #3496

Comments

@kapouer
Copy link
Contributor

@kapouer kapouer commented Oct 16, 2019

Environment

Knex version: 0.19.5
Database + version: postgresql 11
OS: linux/debian

Feature discussion / request

I find myself often spending hours trying to figure how to make

knex('table').select(rawstr).from(knex('table').where(obj))

work.
It's very simple:

knex('table').select(rawstr).from(knex('table').where(obj).as('sub'))

However knex outputs a wrong sql statement (missing parenthesis) and when debugging, it looks like a bug in knex.

I think it would be nice to users if a missing as() could be detected and a warning printed.

@elhigu

This comment has been minimized.

Copy link
Member

@elhigu elhigu commented Oct 16, 2019

Generally knex cannot know if parenthesis are required or not, but in this particular case I believe that knex knows enough about query semantics to be able to add them (and to avoid adding them twice).

I would be happy to accept PR for implementing this (with integration test for all the dialects and also with case where .as is used that it doesn't create double parenthesis).

@edvardchen

This comment has been minimized.

Copy link
Contributor

@edvardchen edvardchen commented Oct 19, 2019

Anyone work no it ? If not, I can take it

@kapouer

This comment has been minimized.

Copy link
Contributor Author

@kapouer kapouer commented Oct 23, 2019

@edvardchen that'd be nice !

@edvardchen

This comment has been minimized.

Copy link
Contributor

@edvardchen edvardchen commented Oct 23, 2019

@kapouer kinda busy this week. I would do it in the weekend. Sorry to reply so late

edvardchen pushed a commit to edvardchen/knex that referenced this issue Oct 24, 2019
@edvardchen

This comment has been minimized.

Copy link
Contributor

@edvardchen edvardchen commented Oct 24, 2019

Almost done. I would add more test before submitting PR

@edvardchen

This comment has been minimized.

Copy link
Contributor

@edvardchen edvardchen commented Oct 27, 2019

All dialects except sqlite3 require that subquery must have an alias.

  1. Should we add an anonymous alias for them like T,
  2. or just let the dialects to complain?
    The error messages thrown from dialects are really comprehensible.

@kapouer @elhigu @kibertoad

edvardchen pushed a commit to edvardchen/knex that referenced this issue Oct 27, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Oct 27, 2019

@edvardchen What's the advantage of anonymous aliases, if they are not referenced anywhere in the query anyway? If messages are explicit enough for users to understand what is happening, I think that's good enough.

@edvardchen

This comment has been minimized.

Copy link
Contributor

@edvardchen edvardchen commented Oct 28, 2019

Anonymous aliases can make the query executable even not being referenced. Like:

select id from (select id from my_tab) as T

But I worry that it may not be expected and would cause confusions

@kapouer

This comment has been minimized.

Copy link
Contributor Author

@kapouer kapouer commented Oct 28, 2019

An interesting thread to read about bringing anonymous aliases to postgresql.
@edvardchen the main problem seems to be sure anonymous aliases won't conflict with anything.
Maybe it's safer stick to whatever SQL dialect is available.

@edvardchen

This comment has been minimized.

Copy link
Contributor

@edvardchen edvardchen commented Oct 28, 2019

OK.

edvardchen pushed a commit to edvardchen/knex that referenced this issue Oct 28, 2019
edvardchen pushed a commit to edvardchen/knex that referenced this issue Oct 28, 2019
edvardchen pushed a commit to edvardchen/knex that referenced this issue Oct 28, 2019
edvardchen pushed a commit to edvardchen/knex that referenced this issue Oct 28, 2019
edvardchen pushed a commit to edvardchen/knex that referenced this issue Oct 28, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

@kibertoad kibertoad commented Oct 29, 2019

Released in 0.20.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.