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

Projects
None yet
3 participants
@ivanslf
Contributor

ivanslf 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

Collaborator

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

This comment has been minimized.

@ivanslf

ivanslf Sep 14, 2017

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

Collaborator

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

This comment has been minimized.

@ivanslf

ivanslf Sep 14, 2017

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

Collaborator

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.

@ivanslf

ivanslf Sep 14, 2017

Contributor

I misunderstood, sorry!

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Sep 14, 2017

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

@elhigu

This comment has been minimized.

Collaborator

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.

@ivanslf

This comment has been minimized.

Contributor

ivanslf 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

This comment has been minimized.

Collaborator

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 referenced this pull request Sep 28, 2017

Closed

Are self-joins possible? #582

@serprex

This comment has been minimized.

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

elhigu commented Oct 14, 2017

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

@elhigu

This comment has been minimized.

Collaborator

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 tgriesser:master Oct 16, 2017

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