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

Implemented select syntax: select({ alias: 'column' }) #2227

Merged
merged 5 commits into from Oct 16, 2017

Conversation

@ivanfilhoz
Copy link
Contributor

@ivanfilhoz ivanfilhoz commented Sep 14, 2017

Attending to @fengb's proposal on issue #1034, this PR adds support for this notation:

knex.select({title, description, createdTime: 'created_time'}).from('tasks')

Which compiles as:

select `title`, `description`, `created_time` as `createdTime` from `tasks`
@@ -134,7 +134,17 @@ describe("QueryBuilder", function() {
});
});

it("basic alias", function() {
it("basic select with alias as property-value pairs", function() {
testsql(qb().select({bar: 'foo'}).from('users'), {

This comment has been minimized.

@elhigu

elhigu Sep 14, 2017
Member

should test also .select('colName', { bar: 'foo'}) and .select(['colName', { bar: 'foo'}])

This comment has been minimized.

@ivanfilhoz

ivanfilhoz Sep 14, 2017
Author Contributor

Sure, you're right.

const raw = this.unwrapRaw(value);
if (raw) return raw;
if (typeof value === 'number') return value;
return this._wrapString(value + '');
switch (typeof value) {

This comment has been minimized.

@elhigu

elhigu Sep 14, 2017
Member

looks correct place to implement this (the same place where 'as something' is parsed 👍

This comment has been minimized.

@ivanfilhoz

ivanfilhoz Sep 14, 2017
Author Contributor

A type check at the top of it? I thought it would make more sense to separate the methods, since the function is named wrapString (thus not looking intuitively able to handle an object). Also, there are other methods in the formatter class which call the alias helper function, like outputQuery (for aliased subqueries).

This comment has been minimized.

@elhigu

elhigu Sep 14, 2017
Member

I meant that looks like the place where you chose to implement this feature seems correct and logical. This was not a critic of any kind :) I agree that those implementations were good to be in separate methods as you did.

This comment has been minimized.

@ivanfilhoz

ivanfilhoz Sep 14, 2017
Author Contributor

I misunderstood, sorry!

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 14, 2017

I think the code looks good, but this still needs link to the documentation PR

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 14, 2017

And thanks for the PR :) I'm not big fan of adding more different ways to do the same thing, but since other people was fine with this I have no problem merging this in when docs are ready and some additional tests has been added.

@ivanfilhoz
Copy link
Contributor Author

@ivanfilhoz ivanfilhoz commented Sep 14, 2017

Okay, also sending a PR to the docs repo. I agree adding different ways can lead to some trouble in general, but since I introduced knex into my team, it's a consensus that the current 'foo AS bar' notation feels hacky.

I'm sure people will find this feature very useful. For example:

const taskModel = {
  name,
  description,
  createdTime: 'created_time',
  totalAssignees: function () {
    this.count().from('tasks_assignees').where('tasks_assignees.id', 'tasks.id')
  }
}
let tasks = await knex('tasks').select(taskModel)
console.log(tasks) // outputs a nice formatted list

EDIT: Here is the docs PR: knex/documentation#52

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 15, 2017

Could you still tell what does

knex({ a: 'table1', b: 'table2' }).toSQL()

and

knex(knex.raw('??', ['foo as bar'])).toSQL()

and

knex(knex.raw('??', [{bar: 'foo' }])).toSQL()

output after this change?

@elhigu elhigu mentioned this pull request Sep 28, 2017
@serprex
Copy link
Contributor

@serprex serprex commented Oct 11, 2017

Before:

> knex({ a: 'table1', b: 'table2' }).toSQL().sql
'select * from "[object Object]"'
> knex(knex.raw('??', ['foo as bar'])).toSQL().sql
'select * from "foo" as "bar"'
> knex(knex.raw('??', [{bar: 'foo' }])).toSQL().sql
'select * from ""'

After:

> knex({ a: 'table1', b: 'table2' }).toSQL().sql
'select * from "table1" as "a", "table2" as "b"'
> knex(knex.raw('??', ['foo as bar'])).toSQL().sql
'select * from "foo" as "bar"'
> knex(knex.raw('??', [{bar: 'foo' }])).toSQL().sql
ReferenceError: value is not defined
    at Formatter.parseObject (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:170:45)
    at Formatter.wrap (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:124:21)
    at Formatter.parseObject (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:174:34)
    at Formatter.wrap (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:124:21)
    at Formatter.parseObject (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:174:34)
    at Formatter.wrap (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:124:21)
    at QueryCompiler_PG.get (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:656:52)
    at QueryCompiler_PG.columns (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:230:78)
    at C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:147:30
    at Array.map (<anonymous>)
    at QueryCompiler_PG.select (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:146:33)
    at QueryCompiler_PG.toSQL (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:108:27)
    at Builder.toSQL (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\builder.js:115:44)
    at repl:1:39
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:239:29)

I attempted to fix the naming typo of value to first but then:

> knex(knex.raw('??', [{bar: 'foo' }])).toSQL().sql
TypeError: Cannot set property '[object Object]' of undefined
    at Builder.setTypeParser (C:\Users\pdube\Mozaik\backend\node_modules\pg-types\index.js:36:28)
    at Formatter.compileCallback (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:186:14)
    at Formatter.parseObject (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:170:29)
    at Formatter.wrap (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:124:21)
    at Formatter.parseObject (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:174:34)
    at Formatter.wrap (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:124:21)
    at Formatter.parseObject (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:174:34)
    at Formatter.wrap (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:124:21)
    at Formatter.parseObject (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:174:34)
    at Formatter.wrap (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\formatter.js:124:21)
    at QueryCompiler_PG.get (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:656:52)
    at QueryCompiler_PG.columns (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:230:78)
    at C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:147:30
    at Array.map (<anonymous>)
    at QueryCompiler_PG.select (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:146:33)
    at QueryCompiler_PG.toSQL (C:\Users\pdube\Mozaik\backend\node_modules\knex\lib\query\compiler.js:108:27)
@elhigu
Copy link
Member

@elhigu elhigu commented Oct 11, 2017

I think that the last example should also fixed to work, but it can be implemented later also later. Do you think you could made the knex.raw('??', [ {a: 'foo' }]) to work too?

@elhigu
Copy link
Member

@elhigu elhigu commented Oct 14, 2017

I started check this out if this can be fixed to work with knex raw too.

@elhigu elhigu force-pushed the ivanfilhoz:master branch from 6c3ef9f to 98d95c2 Oct 16, 2017
@elhigu
Copy link
Member

@elhigu elhigu commented Oct 16, 2017

Now this seems to work also for identifiers inside knex.raw and allows to use this syntax to alias subqueries 🎉

@elhigu elhigu merged commit 11536f9 into knex:master Oct 16, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants