feat: optimize join query parsing fix #378

src/bone.js

@@ -1720,7 +1720,7 @@ class Bone {
 }
 
 const Spell_methods = [
-  'select', 'join', 'where', 'group', 'order', 'get', 'count', 'average', 'minimum', 'maximum', 'sum',
+  'select', 'join', 'where', 'group', 'order', 'get', 'count', 'average', 'minimum', 'maximum', 'sum', 'from',
 ];
 for (const method of Spell_methods) {
   Object.defineProperty(Bone, method, {

src/drivers/abstract/spellbook.js

@@ -2,65 +2,10 @@
 
 const SqlString = require('sqlstring');
 
-const { copyExpr, findExpr, walkExpr } = require('../../expr');
+const { findExpr, walkExpr } = require('../../expr');
 const { formatExpr, formatConditions, collectLiteral, isAggregatorExpr } = require('../../expr_formatter');
 const Raw = require('../../raw').default;
 
-/**
- * Create a subquery to make sure OFFSET and LIMIT on left table takes effect.
- * @param {Spell} spell
- */
-function createSubspell(spell) {
-  const { Model, columns, joins, whereConditions, orders } = spell;
-  const baseName = Model.tableAlias;
-  const subspell = spell.dup;
-
-  subspell.columns = [];
-  for (const token of columns) {
-    walkExpr(token, ({ type, qualifiers, value }) => {
-      if (type == 'id' && qualifiers[0] == baseName) {
-        subspell.columns.push({ type, value });
-      }
-    });
-  }
-
-  // If columns were whitelisted, make sure JOIN columns are included.
-  if (subspell.columns.length > 0) {
-    for (const qualifier in joins) {
-      const association = joins[qualifier];
-      walkExpr(association.on, ({ type, qualifiers, value }) => {
-        if (type == 'id' && qualifiers[0] == baseName) {
-          subspell.columns.push({ type, value });
-        }
-      });
-    }
-  }
-
-  // TODO: how to handle subqueries with GROUP?
-  subspell.groups = [];
-
-  subspell.whereConditions = [];
-  while (whereConditions.length > 0) {
-    const condition = whereConditions.shift();
-    const token = copyExpr(condition, ({ type, value }) => {
-      if (type === 'id') return { type, value };
-    });
-    subspell.whereConditions.push(token);
-  }
-
-  subspell.orders = [];
-  for (const order of orders) {
-    const [token, direction] = order;
-    const { type, qualifiers = [], value } = token;
-    if (type == 'id' && qualifiers[0] == baseName) {
-      subspell.orders.push([{ type, value }, direction]);
-      if (subspell.columns.length > 0) subspell.columns.push({ type, value });
-    }
-  }
-
-  return subspell;
-}
-
 /**
  * Make sure columns are qualified
  */
@@ -247,7 +192,8 @@ class SpellBook {
    * @param {Spell} spell
    */
   formatSelectWithoutJoin(spell) {
-    const { columns, whereConditions, groups, havingConditions, orders, rowCount, skip } = spell;
+    const { columns, whereConditions, groups, havingConditions, orders, rowCount, skip, Model } = spell;
+    const { escapeId } = Model.driver;
     const chunks = ['SELECT'];
     const values = [];
 
@@ -273,7 +219,8 @@ class SpellBook {
     const table = formatExpr(spell, spell.table);
     chunks.push(`FROM ${table}`);
     if (spell.table.value instanceof spell.constructor) {
-      chunks.push(`AS t${spell.subqueryIndex++}`);
+      const subTableAlias = spell.table.value.Model && spell.table.value.Model.tableAlias;
+      chunks.push(`AS ${subTableAlias? escapeId(subTableAlias) : `t${spell.subqueryIndex++}`}`);
     }
 
     // see https://dev.mysql.com/doc/refman/8.0/en/index-hints.html
@@ -534,24 +481,13 @@ class SpellBook {
     }
     chunks.push(selects.join(', '));
 
-    let hoistable = skip > 0 || rowCount > 0;
-    if (hoistable) {
-      function checkQualifier({ type, qualifiers = [] }) {
-        if (type === 'id' && qualifiers.length> 0 && !qualifiers.includes(baseName)) {
-          hoistable = false;
-        }
-      }
-      for (const condition of whereConditions) walkExpr(condition, checkQualifier);
-      for (const orderExpr of orders) walkExpr(orderExpr[0], checkQualifier);
-    }
-
-    if (hoistable) {
-      const subspell = createSubspell(spell);
-      const subquery = this.formatSelectWithoutJoin(subspell);
-      values.push(...subquery.values);
-      chunks.push(`FROM (${subquery.sql}) AS ${escapeId(baseName)}`);
+    const table = formatExpr(spell, spell.table);
+    chunks.push(`FROM ${table}`);
+    if (spell.table.value instanceof spell.constructor) {
+      const subTableAlias = spell.table.value.Model && spell.table.value.Model.tableAlias;
+      chunks.push(`AS ${subTableAlias? escapeId(subTableAlias) : `t${spell.subqueryIndex++}`}`);
     } else {
-      chunks.push(`FROM ${escapeId(Model.table)} AS ${escapeId(baseName)}`);
+      chunks.push(`AS ${escapeId(baseName)}`);
     }
 
     for (const qualifier in joins) {
@@ -581,10 +517,8 @@ class SpellBook {
     }
 
     if (orders.length > 0) chunks.push(`ORDER BY ${this.formatOrders(spell, orders).join(', ')}`);
-    if (!hoistable) {
-      if (rowCount > 0) chunks.push(`LIMIT ${rowCount}`);
-      if (skip > 0) chunks.push(`OFFSET ${skip}`);
-    }
+    if (rowCount > 0) chunks.push(`LIMIT ${rowCount}`);
+    if (skip > 0) chunks.push(`OFFSET ${skip}`);
     return { sql: chunks.join(' '), values };
   }
 

src/spell.js

@@ -385,6 +385,21 @@ class Spell {
     return { ...parseExpr(text), __expr: true };
   }
 
+  #emptySpell() {
+    Object.assign(this, {
+      columns: [],
+      whereConditions: [],
+      groups: [],
+      orders: [],
+      havingConditions: [],
+      joins: {},
+      skip: 0,
+      subqueryIndex: 0,
+      rowCount: 0,
+      skip: 0,
+    });
+  }
+
   get unscoped() {
     const spell = this.dup;
     spell.scopes = [];
@@ -790,6 +805,12 @@ class Spell {
    * @returns {Spell}
    */
   $with(...qualifiers) {
+    if (this.rowCount > 0 || this.skip > 0) {
+      const spell = this.dup;
+      this.#emptySpell();
+      this.table = { type: 'subquery', value: spell };
+    }
+
     for (const qualifier of qualifiers) {
       if (isPlainObject(qualifier)) {
         for (const key in qualifier) {

src/types/abstract_bone.d.ts

@@ -264,6 +264,8 @@ export class AbstractBone {
 
   static initialize(): void;
 
+  static from<T extends typeof AbstractBone>(table: string | Spell<T>): Spell<T>;
+
   constructor(values: { [key: string]: Literal }, opts?: { isNewRecord?: boolean });
 
   /**

test/integration/suite/associations.test.js

@@ -21,6 +21,11 @@ describe('=> Associations', function() {
     "Now you'll join him"
   ];
 
+  const comments1 = [
+    'Long may the sunshine!',
+    'Despicable outlanders!',
+  ];
+
   const tagNames = ['npc', 'boss', 'player'];
   const topicNames = ['nephalem', 'archangel', 'demon'];
 
@@ -55,6 +60,9 @@ describe('=> Associations', function() {
     await Promise.all(comments.map(content => {
       return Comment.create({ content, articleId: posts[0].id });
     }));
+    await Promise.all(comments1.map(content => {
+      return Comment.create({ content, articleId: posts[1].id });
+    }));
     await mapTags(posts[0], tags.slice(0, 2));
     await mapTags(posts[0], topics.slice(2, 3));
     await mapTags(posts[1], tags.slice(2, 3));
@@ -89,10 +97,17 @@ describe('=> Associations', function() {
   });
 
   it('Bone.hasMany', async function() {
-    const post = await Post.first.with('comments');
+    let post = await Post.first.with('comments');
     expect(post.comments.length).to.be.above(0);
     expect(post.comments[0]).to.be.a(Comment);
     expect(post.comments.map(comment => comment.content).sort()).to.eql(comments.sort());
+    post = await Post.first.offset(1).with('comments');
+    expect(post.comments.length).to.be.above(0);
+    expect(post.comments[0]).to.be.a(Comment);
+    expect(post.comments.map(comment => comment.content).sort()).to.eql(comments1.sort());
+    const posts = await Post.find().limit(100).offset(1).with('comments').limit(2);
+    assert.equal(posts.length, 1);
+    expect(posts[0].comments.map(comment => comment.content).sort()).to.eql(comments1.sort());
   });
 
   it('Bone.hasMany through', async function() {
@@ -155,6 +170,8 @@ describe('=> Associations', function() {
 
 describe('=> Associations order / offset / limit', function() {
   before(async function() {
+    await Post.remove({}, true);
+    await Comment.remove({}, true);
     const post1 = await Post.create({ title: 'New Post' });
     await Comment.create({ content: 'Abandon your foolish request!', articleId: post1.id });
     const post2 = await Post.create({ title: 'New Post 2' });
@@ -203,4 +220,109 @@ describe('=> Associations order / offset / limit', function() {
     const result = await Post.include('comments').select('content as cnt').order('cnt', 'desc').limit(1);
     assert.equal(result.length, 1);
   });
+
+  describe('should limit/offset subquery if limit/offset is set', function() {
+    it('on root query', async function() {
+      /*
+        sample data in db query all result:
+        [
+          { id: 1, title: 'New Post', comments: [ { id: 2, content: 'Now you\'ll join them } ] },
+          { id: 1, title: 'New Post', comments: [ { id: 1, content: 'Abandon your foolish request!' } ] },
+          { id: 2, title: 'New Post 2', comments: [ { id: 2, content: 'You are too late to save the child!' } ] },
+        ]
+      */
+      let posts = await Post.include('comments').limit(1).order('id');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post');
+      assert.equal(posts[0].comments.length, 1);
+
+      posts = await Post.include('comments').limit(1).offset(1).order('id');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post');
+      assert.equal(posts[0].comments.length, 1);
+
+      posts = await Post.include('comments').limit(1).offset(2).order('id');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post 2');
+      assert.equal(posts[0].comments.length, 1);
+      assert.equal(posts[0].comments[0].content, 'You are too late to save the child!');
+
+      /*
+        sample data in db query all result:
+        [
+          { id: 2, title: 'New Post 2', comments: [ { id: 2, content: 'You are too late to save the child!' } ] },
+          { id: 1, title: 'New Post', comments: [ { id: 2, content: 'Now you\'ll join them } ] },
+          { id: 1, title: 'New Post', comments: [ { id: 1, content: 'Abandon your foolish request!' } ] },
+        ]
+      */
+      posts = await Post.include('comments').limit(1).offset(0).order('id', 'desc');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post 2');
+      assert.equal(posts[0].comments.length, 1);
+      assert.equal(posts[0].comments[0].content, 'You are too late to save the child!');
+
+      /*
+        sample data in db query all result:
+        [
+          { id: 2, title: 'New Post 2', comments: [ { id: 2, content: 'You are too late to save the child!' } ] },
+          { id: 1, title: 'New Post', comments: [ { id: 1, content: 'Now you\'ll join them } ] },
+          { id: 1, title: 'New Post', comments: [ { id: 1, content: 'Abandon your foolish request!' } ] },
+        ]
+      */
+      posts = await Post.include('comments').order('id', 'desc');
+      assert.equal(posts.length, 2);
+      assert.equal(posts[0].title, 'New Post 2');
+      assert.equal(posts[0].comments.length, 1);
+      assert.equal(posts[0].comments[0].content, 'You are too late to save the child!');
+    });
+
+    it('on root query order join table', async function() {
+      /*
+        sample data in db query all result:
+        [
+          { id: 1, title: 'New Post', comments: [ { id: 2, content: 'Now you\'ll join them' } ] },
+          { id: 2, title: 'New Post 2', comments: [ { id: 2, content: 'You are too late to save the child!' } ] },
+          { id: 1, title: 'New Post', comments: [ { id: 1, content: 'Abandon your foolish request!' } ] },
+        ]
+      */
+      let posts = await Post.include('comments').limit(1).order('comments.id', 'desc');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post');
+      assert.equal(posts[0].comments.length, 1);
+      assert.equal(posts[0].comments[0].content, 'Now you\'ll join them');
+
+      posts = await Post.include('comments').limit(1).offset(1).order('comments.id', 'desc');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post 2');
+      assert.equal(posts[0].comments.length, 1);
+      assert.equal(posts[0].comments[0].content, 'You are too late to save the child!');
+
+      posts = await Post.include('comments').limit(1).offset(2).order('comments.id', 'desc');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post');
+      assert.equal(posts[0].comments.length, 1);
+      assert.equal(posts[0].comments[0].content, 'Abandon your foolish request!');
+
+    });
+
+    it('on subquery', async function() {
+      let post = await Post.first.with('comments');
+      assert.equal(post.title, 'New Post');
+      assert.equal(post.comments.length, 2);
+      post = await Post.first.offset(1).with('comments');
+      assert.equal(post.title, 'New Post 2');
+      assert.equal(post.comments.length, 1);
+    });
+
+    it('on subquery with order', async function() {
+      let posts = await Post.all.limit(1).order('id', 'desc').with('comments');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post 2');
+      assert.equal(posts[0].comments.length, 1);
+      posts = await Post.all.limit(1).order('id', 'asc').with('comments');
+      assert.equal(posts.length, 1);
+      assert.equal(posts[0].title, 'New Post');
+      assert.equal(posts[0].comments.length, 2);
+    });
+  });
 });

test/unit/drivers/abstract/spellbook.test.js

@@ -32,12 +32,13 @@ describe('=> Spellbook', function() {
 
       assert.equal(query.limit(10).toString(), heresql(function() {/*
         SELECT `posts`.*, `author`.*
-          FROM (SELECT * FROM `articles`
-                  WHERE `is_private` = true
-                    AND `author_id` IN (SELECT * FROM `users` WHERE `stauts` = 1)
-                    AND `gmt_deleted` IS NULL LIMIT 10) AS `posts`
+          FROM `articles` AS `posts`
       LEFT JOIN `users` AS `author`
             ON `posts`.`userId` = `author`.`id`
+      WHERE `posts`.`is_private` = true
+          AND `posts`.`author_id` IN (SELECT * FROM `users` WHERE `stauts` = 1)
+          AND `posts`.`gmt_deleted` IS NULL
+          LIMIT 10
       */}));
 
       assert.doesNotThrow(function() {