Skip to content

Commit

Permalink
Fix: join queries and condition handling
Browse files Browse the repository at this point in the history 
Join queries isn't thoroughly tested yet. This commit fixes two issues
found during applying this module in serious use:

- Model.group().join() queries get derived columns left out;
- Long where conditions get truncated when formatted.
  • Loading branch information
@cyjake
cyjake committed Dec 18, 2017
1 parent 1d0e5f9 commit bcbdeb9
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 54 deletions.
5 changes: 4 additions & 1 deletion History.md
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
0.1.4 / 2017-12-??
0.1.4 / 2017-12-18
==================

* Fix: should format condition arrays by hand instead of hand it over to formatExpr
* Fix: whereConditions of subquery should retain the order of the whereConditions in major query
* Fix: calculated columns should be kept in the final columns when sorting out the attributes
* Fix: doesn't depend on co anymore

0.1.3 / 2017-12-17
Expand Down
25 changes: 14 additions & 11 deletions lib/expr.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,20 @@ function parseExpr(str, ...values) {
const value = values[valueIndex++]

next()
switch (typeof value) {
case 'array':
return { type: 'array', value }
case 'null':
return { type: 'null' }
case 'string':
return { type: 'string', value }
case 'number':
return { type: 'number', value }
default:
throw new Error(`Unexpected value ${value} at ${valueIndex - 1}`)
if (value == null) {
return { type: 'null' }
}
else if (Array.isArray(value)) {
return { type: 'array', value }
}
else if (typeof value == 'number') {
return { type: 'number', value }
}
else if (typeof value == 'string') {
return { type: 'string', value }
}
else {
throw new Error(`Unexpected value ${value} at ${valueIndex - 1}`)
}
}

Expand Down
101 changes: 60 additions & 41 deletions lib/spell.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,19 @@ function formatOrders(spell) {
return spell.orders.map(formatOrder)
}

function formatColumn(Model, name) {
const ast = walkExpr(parseExpr(name), node => {
if (node.type == 'id') node.value = Model.unalias(node.value)
})
function formatColumn(Model, token) {
if (typeof token == 'string') token = parseExpr(token)

if (ast.type == 'id') {
return formatIdentifier(ast)
if (Model) {
walkExpr(token, node => {
if (node.type == 'id') node.value = Model.unalias(node.value)
})
}

if (token.type == 'id') {
return formatIdentifier(token)
} else {
return formatExpr(ast)
return formatExpr(token)
}
}

Expand Down Expand Up @@ -257,22 +261,21 @@ function walkExpr(ast, fn) {
}

function formatSelectWithJoin(spell) {
const { Model, attributes, whereConditions, groups, orders, rowCount, skip, joins } = spell
const { Model, attributes, whereConditions, orders, rowCount, skip, joins } = spell
const subspell = spell.dup
const baseName = Model.alias
const selection = { [baseName]: new Set() }

for (const name of attributes) {
const { type, qualifiers, value } = parseExpr(name)
if (type == 'id') {
if (qualifiers && qualifiers[0] != baseName) {
const [qualifier] = qualifiers
if (!selection[qualifier]) selection[qualifier] = new Set()
selection[qualifier].add(value)
} else {
selection[baseName].add(value)
const token = parseExpr(name)
let qualifier = baseName
walkExpr(token, ({ type, qualifiers, value }) => {
if (type == 'id' && qualifiers && qualifiers[0] != baseName) {
[qualifier] = qualifiers
}
}
})
if (!selection[qualifier]) selection[qualifier] = new Set()
selection[qualifier].add(token)
}

for (const qualifier in joins) {
Expand All @@ -284,12 +287,19 @@ function formatSelectWithJoin(spell) {
walkExpr(relation.on, ({ type, qualifiers, value }) => {
if (type == 'id' && qualifiers && qualifiers[0] == baseName) {
const attrs = selection[qualifiers[0]]
if (attrs.size > 0) attrs.add(value)
if (attrs.size > 0) attrs.add({ type, qualifiers, value })
}
})
}

subspell.attributes = selection[baseName]
subspell.attributes = new Set()
for (const token of selection[baseName]) {
walkExpr(token, subtoken => {
const { type } = subtoken
if (type == 'id') delete subtoken.qualifiers
})
subspell.attributes.add(token.type == 'id' ? token.value : token)
}
selection[baseName] = new Set()

subspell.whereConditions = []
Expand All @@ -302,7 +312,7 @@ function formatSelectWithJoin(spell) {
}
})
if (internal) {
subspell.whereConditions.push(condition)
subspell.whereConditions.unshift(condition)
whereConditions.splice(i, 1)
}
}
Expand All @@ -313,17 +323,20 @@ function formatSelectWithJoin(spell) {
const { type, qualifiers, value } = parseExpr(name)
if (type == 'id' && !qualifiers || qualifiers[0] == baseName) {
subspell.orders.unshift([value, order])
orders[i] = [`${baseName}.${value}`, order]
orders[i] = value in Model.schema ? [`${baseName}.${value}`, order] : [value, order]
}
}

const columns = []
for (const qualifier in selection) {
const { Model: RefModel } = qualifier == baseName ? Model : joins[qualifier]
const RefModel = qualifier == baseName ? Model : joins[qualifier].Model
const attrs = selection[qualifier]
if (attrs.size > 0) {
for (const name of attrs) {
columns.push(`${escapeId(qualifier)}.${escapeId(RefModel.unalias(name))}`)
const token = typeof name == 'string'
? { type: 'id', qualifiers: [qualifier], value: name }
: name
columns.push(formatColumn(RefModel, token))
}
} else {
columns.push(`${escapeId(qualifier)}.*`)
Expand Down Expand Up @@ -381,16 +394,16 @@ function formatDelete(spell) {
}

function formatConditions(spell, conditions) {
const { Model } = spell
const ast = conditions.length > 1
? { type: 'op', name: 'and', args: conditions }
: conditions[0]

walkExpr(ast, node => {
if (node.type == 'id') node.value = Model.unalias(node.value)
})

return formatExpr(ast)
return conditions
.map(condition => {
walkExpr(condition, node => {
if (node.type == 'id') node.value = spell.Model.unalias(node.value)
})
return isLogicalOp(condition) && condition.name == 'or'
? `(${formatExpr(condition)})`
: formatExpr(condition)
})
.join(' AND ')
}

function formatInsert(spell) {
Expand Down Expand Up @@ -512,14 +525,20 @@ function isCalculation(func) {
*/
function Spell_deletedAtIsNull() {
const { Model, table, whereConditions } = this
for (const token of whereConditions) {
const { type, value } = token.args[0]
if (type == 'id' && value == 'deletedAt') return this
}
if (table.type == 'id' && Model.schema.deletedAt) {
return this.$where({ deletedAt: null })
const { schema } = Model

if (!schema.deletedAt) return
for (const condition of whereConditions) {
let found = false
walkExpr(condition, ({ type, value }) => {
if (type != 'id') return
if (value == 'deletedAt' || value == schema.deletedAt.column) {
found = true
}
})
if (found) return
}
return this
if (table.type == 'id') this.$where({ deletedAt: null })
}

class Spell {
Expand Down Expand Up @@ -615,7 +634,7 @@ class Spell {
}

$select(...names) {
if (names.length == 1) names = names[0].split(/\s+/)
if (names.length == 1 && !/ as /i.test(names)) names = names[0].split(/\s+/)
for (const name of names) this.attributes.add(name)
return this
}
Expand Down
53 changes: 52 additions & 1 deletion tests/test.bone.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,39 @@ describe('=> Query $op', function() {
})
})

describe('=> Where', function() {
before(async function() {
await Promise.all([
Post.create({ title: 'King Leoric', authorId: 1 }),
Post.create({ title: 'Skeleton King', authorId: 1 }),
Post.create({ title: 'Archbishop Lazarus', authorId: 2 })
])
})

after(async function() {
await Post.remove({}, true)
})

it('.where({ foo, bar })', async function() {
const posts = await Post.where({ title: ['King Leoric', 'Skeleton King'], authorId: 2 })
expect(posts).to.be.empty()
})

it('.where(query, ...values)', async function() {
const posts = await Post.where('title = ? and authorId = ?', ['King Leoric', 'Skeleton King'], 2)
expect(posts).to.be.empty()
})

it('.where(compoundQuery, ...values)', async function() {
const posts = await Post.where('authorId = ? || (title = ? && authorId = ?)', 2, 'King Leoric', 1).order('authorId')
expect(posts.length).to.be(2)
expect(posts[0].title).to.equal('King Leoric')
expect(posts[0].authorId).to.equal(1)
expect(posts[1].title).to.equal('Archbishop Lazarus')
expect(posts[1].authorId).to.equal(2)
})
})

describe('=> Scopes', function() {
before(async function() {
const results = await Promise.all([
Expand Down Expand Up @@ -802,7 +835,7 @@ describe('=> Group / Join / Subqueries', function() {
])
})

it('Bone.group().join()', async function() {
it('Bone.group() subquery', async function() {
const posts = await Post.find({
id: Comment.select('articleId').from(Comment.group('articleId').count().having('count > 0'))
}).with('attachment')
Expand All @@ -813,6 +846,24 @@ describe('=> Group / Join / Subqueries', function() {
expect(posts[1].title).to.eql('Archangel Tyrael')
expect(posts[1].attachment).to.be(null)
})

it('Bone.group().join()', async function() {
const comments = await Comment.select('count(id) as count').group('articleId').order('count')
.join(Post, 'comments.articleId = posts.id')
console.log(comments)
})

it('query / query.with() / query.count()', async function() {
const query = Post.find({ title: ['Archangel Tyrael', 'King Leoric'], deletedAt: null }).order('title')
const [ { count } ] = await query.count()
const posts = await query.with('attachment')
expect(posts.length).to.equal(count)
expect(posts[0]).to.be.a(Post)
expect(posts[0].title).to.equal('Archangel Tyrael')
expect(posts[0].attachment).to.be(null)
expect(posts[1].attachment).to.be.an(Attachment)
expect(posts[1].attachment.postId).to.equal(posts[1].id)
})
})

describe('=> Automatic Versioning', function() {
Expand Down
12 changes: 12 additions & 0 deletions tests/test.spell.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,18 @@ describe('=> Select', function() {
)
})

it('select with arbitrary function call', function() {
expect(Post.select('YEAR(createdAt)').toString()).to.equal(
'SELECT YEAR(`gmt_create`) FROM `articles` WHERE `gmt_deleted` IS NULL'
)
})

it('select as', function() {
expect(Post.select('COUNT(id) AS count').toString()).to.equal(
'SELECT COUNT(`id`) AS `count` FROM `articles` WHERE `gmt_deleted` IS NULL'
)
})

it('predefined hasOne join', function() {
expect(Post.select('title', 'createdAt').with('attachment').toString()).to.equal(
'SELECT `posts`.*, `attachment`.* FROM (SELECT `title`, `gmt_create`, `id` FROM `articles` WHERE `gmt_deleted` IS NULL) AS `posts` LEFT JOIN `attachments` AS `attachment` ON `posts`.`id` = `attachment`.`article_id`'
Expand Down

0 comments on commit bcbdeb9

Please sign in to comment.