cirosantilli / node-express-sequelize-nextjs-realworld-example-app

Node.js + Express.js + Sequelize + SQLite/PostgreSQL + Next.js fullstack static/SSG/ISG Example Realworld App. Includes Heroku deployment, DB migration, DB and API unit testing, DB seeding, and ESLint linting setups.
https://cirosantilli-realworld-next.herokuapp.com
87 stars 300 forks source link

Optimize home queries a bit with joins #5

Open cirosantilli opened 2 years ago

cirosantilli commented 2 years ago

Article.toJson individually gets author. We should get them as a join instead. Same for tags. Otherwise we get 3 queries per row (article author, tag and tag count) / or 5 if logged in (+1 for user likes article, +1 for user follows author), which is insane.

Doing http://localhost:3000/api/articles?limit=2&offset=0 while logged off leads to the following reordered/organized list of requests according to DEBUG='*:sql:*':

  article find and count all, this is fine

  sequelize:sql:sqlite Executing (default): SELECT count(`Article`.`id`) AS `count` FROM `Article` AS `Article` LEFT OUTER JOIN `User` AS `author` ON `Article`.`authorId` = `author`.`id`; +5s
  sequelize:sql:sqlite Executing (default): SELECT `Article`.`id`, `Article`.`slug`, `Article`.`title`, `Article`.`description`, `Article`.`body`, `Article`.`createdAt`, `Article`.`updatedAt`, `Article`.`authorId`, `author`.`id` AS `author.id`, `author`.`username` AS `author.username`, `author`.`email` AS `author.email`, `author`.`bio` AS `author.bio`, `author`.`image` AS `author.image`, `author`.`hash` AS `author.hash`, `author`.`salt` AS `author.salt`, `author`.`createdAt` AS `author.createdAt`, `author`.`updatedAt` AS `author.updatedAt` FROM `Article` AS `Article` LEFT OUTER JOIN `User` AS `author` ON `Article`.`authorId` = `author`.`id` ORDER BY `Article`.`createdAt` DESC LIMIT 0, 2; +0ms

  post 499

  author: sequelize:sql:sqlite Executing (default): SELECT `id`, `username`, `email`, `bio`, `image`, `hash`, `salt`, `createdAt`, `updatedAt` FROM `User` AS `User` WHERE `User`.`id` = 9; +0ms
  tags: sequelize:sql:sqlite Executing (default): SELECT `Tag`.`id`, `Tag`.`name`, `Tag`.`createdAt`, `Tag`.`updatedAt`, `ArticleTag`.`createdAt` AS `ArticleTag.createdAt`, `ArticleTag`.`updatedAt` AS `ArticleTag.updatedAt`, `ArticleTag`.`articleId` AS `ArticleTag.articleId`, `ArticleTag`.`tagId` AS `ArticleTag.tagId` FROM `Tag` AS `Tag` INNER JOIN `ArticleTag` AS `ArticleTag` ON `Tag`.`id` = `ArticleTag`.`tagId` AND `ArticleTag`.`articleId` = 499; +0ms
  favoriteCount: sequelize:sql:sqlite Executing (default): SELECT COUNT(`User`.`id`) AS `count` FROM `User` AS `User` INNER JOIN `UserFavoriteArticle` AS `UserFavoriteArticle` ON `User`.`id` = `UserFavoriteArticle`.`userId` AND `UserFavoriteArticle`.`articleId` = 499; +1ms

 post 500: same 3 queries

Of those, we could remove all the users and tags queries with joins!

The users JOIN was already in place, we just weren't checking for it properly. Fixed with:

diff --git a/models/article.js b/models/article.js
index 1f76561..4a2ccd0 100644
--- a/models/article.js
+++ b/models/article.js
@@ -111,8 +111,8 @@ module.exports = (sequelize) => {
   }

   Article.prototype.toJson = async function(user) {
-    let authorPromise;
-    if (this.authorPromise === undefined) {
+    let authorPromise
+    if (!this.author) {
       authorPromise = this.getAuthor()
     } else {
       authorPromise = new Promise(resolve => {resolve(this.author)})

Had tried to fix it previously, but failed because this.authorPromise === undefined was always true!

The authorPromise.then(author => author.toProfileJSONFor(user)), would however make one extra query to see if the current user is following that user or not if were logged in. We would need to also join by following to fix that.

cirosantilli commented 2 years ago

OK, tags now fixed at: 7c37dfbfc7584370deb29bb167f5ac9c35ab746e

Ah, nevermind, reverted that, with that we only get the selected tag when doing the tag search.

Unerlying SQL solution at: https://stackoverflow.com/questions/25734598/get-all-posts-that-have-a-specific-tag-and-keep-all-other-tags-on-results-with-s/70435014#70435014 but needs to be ported to Sequelize. I'm unable to find out how, possible discussion: https://stackoverflow.com/questions/36969162/how-to-include-two-separate-references-of-same-model-using-sequelize-orm

Arghh reverting at def5faaa99de32c983e3933cae02f6dad86ab947 because it breaks count and thus pagination. Gotta add a test for that as well.