Refactor Collection model/controller to not use old Collection object, remove

server/Database.js

@@ -406,11 +406,6 @@ class Database {
     return Promise.all(oldBooks.map((oldBook) => this.models.book.saveFromOld(oldBook)))
   }
 
-  createBulkCollectionBooks(collectionBooks) {
-    if (!this.sequelize) return false
-    return this.models.collectionBook.bulkCreate(collectionBooks)
-  }
-
   createPlaylistMediaItem(playlistMediaItem) {
     if (!this.sequelize) return false
     return this.models.playlistMediaItem.create(playlistMediaItem)

server/controllers/CollectionController.js

@@ -5,13 +5,17 @@ const SocketAuthority = require('../SocketAuthority')
 const Database = require('../Database')
 
 const RssFeedManager = require('../managers/RssFeedManager')
-const Collection = require('../objects/Collection')
 
 /**
  * @typedef RequestUserObject
  * @property {import('../models/User')} user
  *
  * @typedef {Request & RequestUserObject} RequestWithUser
+ *
+ * @typedef RequestEntityObject
+ * @property {import('../models/Collection')} collection
+ *
+ * @typedef {RequestWithUser & RequestEntityObject} CollectionControllerRequest
  */
 
 class CollectionController {
@@ -25,36 +29,68 @@ class CollectionController {
    * @param {Response} res
    */
   async create(req, res) {
-    const newCollection = new Collection()
+    const reqBody = req.body || {}
-    req.body.userId = req.user.id
+
-    if (!newCollection.setData(req.body)) {
+    // Validation
+    if (!reqBody.name || !reqBody.libraryId) {
       return res.status(400).send('Invalid collection data')
     }
+    const libraryItemIds = (reqBody.books || []).filter((b) => !!b && typeof b == 'string')
+    if (!libraryItemIds.length) {
+      return res.status(400).send('Invalid collection data. No books')
+    }
 
-    // Create collection record
+    // Load library items
-    await Database.collectionModel.createFromOld(newCollection)
+    const libraryItems = await Database.libraryItemModel.findAll({
-
+      attributes: ['id', 'mediaId', 'mediaType', 'libraryId'],
-    // Get library items in collection
+      where: {
-    const libraryItemsInCollection = await Database.libraryItemModel.getForCollection(newCollection)
+        id: libraryItemIds,
-
+        libraryId: reqBody.libraryId,
-    // Create collectionBook records
+        mediaType: 'book'
-    let order = 1
-    const collectionBooksToAdd = []
-    for (const libraryItemId of newCollection.books) {
-      const libraryItem = libraryItemsInCollection.find((li) => li.id === libraryItemId)
-      if (libraryItem) {
-        collectionBooksToAdd.push({
-          collectionId: newCollection.id,
-          bookId: libraryItem.media.id,
-          order: order++
-        })
       }
-    }
+    })
-    if (collectionBooksToAdd.length) {
+    if (libraryItems.length !== libraryItemIds.length) {
-      await Database.createBulkCollectionBooks(collectionBooksToAdd)
+      return res.status(400).send('Invalid collection data. Invalid books')
     }
 
-    const jsonExpanded = newCollection.toJSONExpanded(libraryItemsInCollection)
+    /** @type {import('../models/Collection')} */
+    let newCollection = null
+
+    const transaction = await Database.sequelize.transaction()
+    try {
+      // Create collection
+      newCollection = await Database.collectionModel.create(
+        {
+          libraryId: reqBody.libraryId,
+          name: reqBody.name,
+          description: reqBody.description || null
+        },
+        { transaction }
+      )
+
+      // Create collectionBooks
+      const collectionBookPayloads = libraryItemIds.map((llid, index) => {
+        const libraryItem = libraryItems.find((li) => li.id === llid)
+        return {
+          collectionId: newCollection.id,
+          bookId: libraryItem.mediaId,
+          order: index + 1
+        }
+      })
+      await Database.collectionBookModel.bulkCreate(collectionBookPayloads, { transaction })
+
+      await transaction.commit()
+    } catch (error) {
+      await transaction.rollback()
+      Logger.error('[CollectionController] create:', error)
+      return res.status(500).send('Failed to create collection')
+    }
+
+    // Load books expanded
+    newCollection.books = await newCollection.getBooksExpandedWithLibraryItem()
+
+    // Note: The old collection model stores expanded libraryItems in the books property
+    const jsonExpanded = newCollection.toOldJSONExpanded()
     SocketAuthority.emitter('collection_added', jsonExpanded)
     res.json(jsonExpanded)
   }
@@ -75,7 +111,7 @@ class CollectionController {
   /**
    * GET: /api/collections/:id
    *
-   * @param {RequestWithUser} req
+   * @param {CollectionControllerRequest} req
    * @param {Response} res
    */
   async findOne(req, res) {
@@ -94,7 +130,7 @@ class CollectionController {
    * PATCH: /api/collections/:id
    * Update collection
    *
-   * @param {RequestWithUser} req
+   * @param {CollectionControllerRequest} req
    * @param {Response} res
    */
   async update(req, res) {
@@ -158,7 +194,7 @@ class CollectionController {
    *
    * @this {import('../routers/ApiRouter')}
    *
-   * @param {RequestWithUser} req
+   * @param {CollectionControllerRequest} req
    * @param {Response} res
    */
   async delete(req, res) {
@@ -178,7 +214,7 @@ class CollectionController {
    * Add a single book to a collection
    * Req.body { id: <library item id> }
    *
-   * @param {RequestWithUser} req
+   * @param {CollectionControllerRequest} req
    * @param {Response} res
    */
   async addBook(req, res) {
@@ -212,7 +248,7 @@ class CollectionController {
    * Remove a single book from a collection. Re-order books
    * TODO: bookId is actually libraryItemId. Clients need updating to use bookId
    *
-   * @param {RequestWithUser} req
+   * @param {CollectionControllerRequest} req
    * @param {Response} res
    */
   async removeBook(req, res) {
@@ -257,29 +293,31 @@ class CollectionController {
    * Add multiple books to collection
    * Req.body { books: <Array of library item ids> }
    *
-   * @param {RequestWithUser} req
+   * @param {CollectionControllerRequest} req
    * @param {Response} res
    */
   async addBatch(req, res) {
     // filter out invalid libraryItemIds
     const bookIdsToAdd = (req.body.books || []).filter((b) => !!b && typeof b == 'string')
     if (!bookIdsToAdd.length) {
-      return res.status(500).send('Invalid request body')
+      return res.status(400).send('Invalid request body')
     }
 
     // Get library items associated with ids
     const libraryItems = await Database.libraryItemModel.findAll({
+      attributes: ['id', 'mediaId', 'mediaType', 'libraryId'],
       where: {
-        id: {
+        id: bookIdsToAdd,
-          [Sequelize.Op.in]: bookIdsToAdd
+        libraryId: req.collection.libraryId,
-        }
+        mediaType: 'book'
-      },
-      include: {
-        model: Database.bookModel
       }
     })
+    if (!libraryItems.length) {
+      return res.status(400).send('Invalid request body. No valid books')
+    }
 
     // Get collection books already in collection
+    /** @type {import('../models/CollectionBook')[]} */
     const collectionBooks = await req.collection.getCollectionBooks()
 
     let order = collectionBooks.length + 1
@@ -288,10 +326,10 @@ class CollectionController {
 
     // Check and set new collection books to add
     for (const libraryItem of libraryItems) {
-      if (!collectionBooks.some((cb) => cb.bookId === libraryItem.media.id)) {
+      if (!collectionBooks.some((cb) => cb.bookId === libraryItem.mediaId)) {
         collectionBooksToAdd.push({
           collectionId: req.collection.id,
-          bookId: libraryItem.media.id,
+          bookId: libraryItem.mediaId,
           order: order++
         })
         hasUpdated = true
@@ -302,7 +340,8 @@ class CollectionController {
 
     let jsonExpanded = null
     if (hasUpdated) {
-      await Database.createBulkCollectionBooks(collectionBooksToAdd)
+      await Database.collectionBookModel.bulkCreate(collectionBooksToAdd)
+
       jsonExpanded = await req.collection.getOldJsonExpanded()
       SocketAuthority.emitter('collection_updated', jsonExpanded)
     } else {
@@ -316,7 +355,7 @@ class CollectionController {
    * Remove multiple books from collection
    * Req.body { books: <Array of library item ids> }
    *
-   * @param {RequestWithUser} req
+   * @param {CollectionControllerRequest} req
    * @param {Response} res
    */
   async removeBatch(req, res) {
@@ -329,9 +368,7 @@ class CollectionController {
     // Get library items associated with ids
     const libraryItems = await Database.libraryItemModel.findAll({
       where: {
-        id: {
+        id: bookIdsToRemove
-          [Sequelize.Op.in]: bookIdsToRemove
-        }
       },
       include: {
         model: Database.bookModel
@@ -339,6 +376,7 @@ class CollectionController {
     })
 
     // Get collection books already in collection
+    /** @type {import('../models/CollectionBook')[]} */
     const collectionBooks = await req.collection.getCollectionBooks({
       order: [['order', 'ASC']]
     })

server/models/Collection.js

@@ -1,7 +1,5 @@
 const { DataTypes, Model, Sequelize } = require('sequelize')
 
-const oldCollection = require('../objects/Collection')
-
 class Collection extends Model {
   constructor(values, options) {
     super(values, options)
@@ -26,12 +24,12 @@ class Collection extends Model {
   }
 
   /**
-   * Get all old collections toJSONExpanded, items filtered for user permissions
+   * Get all toOldJSONExpanded, items filtered for user permissions
    *
    * @param {import('./User')} user
    * @param {string} [libraryId]
    * @param {string[]} [include]
-   * @returns {Promise<oldCollection[]>} oldCollection.toJSONExpanded
+   * @async
    */
   static async getOldCollectionsJsonExpanded(user, libraryId, include) {
     let collectionWhere = null
@@ -79,8 +77,6 @@ class Collection extends Model {
     // TODO: Handle user permission restrictions on initial query
     return collections
       .map((c) => {
-        const oldCollection = this.getOldCollection(c)
-
         // Filter books using user permissions
         const books =
           c.books?.filter((b) => {
@@ -95,20 +91,14 @@ class Collection extends Model {
             return true
           }) || []
 
-        // Map to library items
-        const libraryItems = books.map((b) => {
-          const libraryItem = b.libraryItem
-          delete b.libraryItem
-          libraryItem.media = b
-          return this.sequelize.models.libraryItem.getOldLibraryItem(libraryItem)
-        })
-
         // Users with restricted permissions will not see this collection
-        if (!books.length && oldCollection.books.length) {
+        if (!books.length && c.books.length) {
           return null
         }
 
-        const collectionExpanded = oldCollection.toJSONExpanded(libraryItems)
+        this.books = books
+
+        const collectionExpanded = c.toOldJSONExpanded()
 
         // Map feed if found
         if (c.feeds?.length) {
@@ -153,69 +143,6 @@ class Collection extends Model {
     })
   }
 
-  /**
-   * Get old collection from Collection
-   * @param {Collection} collectionExpanded
-   * @returns {oldCollection}
-   */
-  static getOldCollection(collectionExpanded) {
-    const libraryItemIds = collectionExpanded.books?.map((b) => b.libraryItem?.id || null).filter((lid) => lid) || []
-    return new oldCollection({
-      id: collectionExpanded.id,
-      libraryId: collectionExpanded.libraryId,
-      name: collectionExpanded.name,
-      description: collectionExpanded.description,
-      books: libraryItemIds,
-      lastUpdate: collectionExpanded.updatedAt.valueOf(),
-      createdAt: collectionExpanded.createdAt.valueOf()
-    })
-  }
-
-  /**
-   *
-   * @param {oldCollection} oldCollection
-   * @returns {Promise<Collection>}
-   */
-  static createFromOld(oldCollection) {
-    const collection = this.getFromOld(oldCollection)
-    return this.create(collection)
-  }
-
-  static getFromOld(oldCollection) {
-    return {
-      id: oldCollection.id,
-      name: oldCollection.name,
-      description: oldCollection.description,
-      libraryId: oldCollection.libraryId
-    }
-  }
-
-  static removeById(collectionId) {
-    return this.destroy({
-      where: {
-        id: collectionId
-      }
-    })
-  }
-
-  /**
-   * Get old collection by id
-   * @param {string} collectionId
-   * @returns {Promise<oldCollection|null>} returns null if not found
-   */
-  static async getOldById(collectionId) {
-    if (!collectionId) return null
-    const collection = await this.findByPk(collectionId, {
-      include: {
-        model: this.sequelize.models.book,
-        include: this.sequelize.models.libraryItem
-      },
-      order: [[this.sequelize.models.book, this.sequelize.models.collectionBook, 'order', 'ASC']]
-    })
-    if (!collection) return null
-    return this.getOldCollection(collection)
-  }
-
   /**
    * Remove all collections belonging to library
    * @param {string} libraryId
@@ -286,64 +213,37 @@ class Collection extends Model {
   }
 
   /**
-   * Get old collection toJSONExpanded, items filtered for user permissions
+   * Get toOldJSONExpanded, items filtered for user permissions
    *
    * @param {import('./User')|null} user
    * @param {string[]} [include]
-   * @returns {Promise<oldCollection>} oldCollection.toJSONExpanded
+   * @async
    */
   async getOldJsonExpanded(user, include) {
-    this.books =
+    this.books = await this.getBooksExpandedWithLibraryItem()
-      (await this.getBooks({
-        include: [
-          {
-            model: this.sequelize.models.libraryItem
-          },
-          {
-            model: this.sequelize.models.author,
-            through: {
-              attributes: []
-            }
-          },
-          {
-            model: this.sequelize.models.series,
-            through: {
-              attributes: ['sequence']
-            }
-          }
-        ],
-        order: [Sequelize.literal('`collectionBook.order` ASC')]
-      })) || []
 
     // Filter books using user permissions
     // TODO: Handle user permission restrictions on initial query
-    const books =
+    if (user) {
-      this.books?.filter((b) => {
+      const books = this.books.filter((b) => {
-        if (user) {
+        if (b.tags?.length && !user.checkCanAccessLibraryItemWithTags(b.tags)) {
-          if (b.tags?.length && !user.checkCanAccessLibraryItemWithTags(b.tags)) {
+          return false
-            return false
+        }
-          }
+        if (b.explicit === true && !user.canAccessExplicitContent) {
-          if (b.explicit === true && !user.canAccessExplicitContent) {
+          return false
-            return false
-          }
         }
         return true
-      }) || []
+      })
 
-    // Map to library items
+      // Users with restricted permissions will not see this collection
-    const libraryItems = books.map((b) => {
+      if (!books.length && this.books.length) {
-      const libraryItem = b.libraryItem
+        return null
-      delete b.libraryItem
+      }
-      libraryItem.media = b
-      return this.sequelize.models.libraryItem.getOldLibraryItem(libraryItem)
-    })
 
-    // Users with restricted permissions will not see this collection
+      this.books = books
-    if (!books.length && this.books.length) {
-      return null
     }
 
-    const collectionExpanded = this.toOldJSONExpanded(libraryItems)
+    const collectionExpanded = this.toOldJSONExpanded()
 
     if (include?.includes('rssfeed')) {
       const feeds = await this.getFeeds()
@@ -357,10 +257,10 @@ class Collection extends Model {
 
   /**
    *
-   * @param {string[]} libraryItemIds
+   * @param {string[]} [libraryItemIds=[]]
    * @returns
    */
-  toOldJSON(libraryItemIds) {
+  toOldJSON(libraryItemIds = []) {
     return {
       id: this.id,
       libraryId: this.libraryId,
@@ -372,19 +272,19 @@ class Collection extends Model {
     }
   }
 
-  /**
+  toOldJSONExpanded() {
-   *
+    if (!this.books) {
-   * @param {import('../objects/LibraryItem')} oldLibraryItems
+      throw new Error('Books are required to expand Collection')
-   * @returns
+    }
-   */
+
-  toOldJSONExpanded(oldLibraryItems) {
+    const json = this.toOldJSON()
-    const json = this.toOldJSON(oldLibraryItems.map((li) => li.id))
+    json.books = this.books.map((book) => {
-    json.books = json.books
+      const libraryItem = book.libraryItem
-      .map((libraryItemId) => {
+      delete book.libraryItem
-        const book = oldLibraryItems.find((li) => li.id === libraryItemId)
+      libraryItem.media = book
-        return book ? book.toJSONExpanded() : null
+      return this.sequelize.models.libraryItem.getOldLibraryItem(libraryItem).toJSONExpanded()
-      })
+    })
-      .filter((b) => !!b)
+
     return json
   }
 }

server/models/CollectionBook.js

@@ -16,15 +16,6 @@ class CollectionBook extends Model {
     this.createdAt
   }
 
-  static removeByIds(collectionId, bookId) {
-    return this.destroy({
-      where: {
-        bookId,
-        collectionId
-      }
-    })
-  }
-
   static init(sequelize) {
     super.init(
       {

server/models/LibraryItem.js

@@ -123,7 +123,7 @@ class LibraryItem extends Model {
   }
 
   /**
-   * Currently unused because this is too slow and uses too much mem
+   *
    * @param {import('sequelize').WhereOptions} [where]
    * @returns {Array<objects.LibraryItem>} old library items
    */

server/objects/Collection.js

@@ -1,115 +0,0 @@
-const uuidv4 = require("uuid").v4
-
-class Collection {
-  constructor(collection) {
-    this.id = null
-    this.libraryId = null
-
-    this.name = null
-    this.description = null
-
-    this.cover = null
-    this.coverFullPath = null
-    this.books = []
-
-    this.lastUpdate = null
-    this.createdAt = null
-
-    if (collection) {
-      this.construct(collection)
-    }
-  }
-
-  toJSON() {
-    return {
-      id: this.id,
-      libraryId: this.libraryId,
-      name: this.name,
-      description: this.description,
-      cover: this.cover,
-      coverFullPath: this.coverFullPath,
-      books: [...this.books],
-      lastUpdate: this.lastUpdate,
-      createdAt: this.createdAt
-    }
-  }
-
-  toJSONExpanded(libraryItems, minifiedBooks = false) {
-    const json = this.toJSON()
-    json.books = json.books.map(bookId => {
-      const book = libraryItems.find(li => li.id === bookId)
-      return book ? minifiedBooks ? book.toJSONMinified() : book.toJSONExpanded() : null
-    }).filter(b => !!b)
-    return json
-  }
-
-  // Expanded and filtered out items not accessible to user
-  toJSONExpandedForUser(user, libraryItems) {
-    const json = this.toJSON()
-    json.books = json.books.map(libraryItemId => {
-      const libraryItem = libraryItems.find(li => li.id === libraryItemId)
-      return libraryItem ? libraryItem.toJSONExpanded() : null
-    }).filter(li => {
-      return li && user.checkCanAccessLibraryItem(li)
-    })
-    return json
-  }
-
-  construct(collection) {
-    this.id = collection.id
-    this.libraryId = collection.libraryId
-    this.name = collection.name
-    this.description = collection.description || null
-    this.cover = collection.cover || null
-    this.coverFullPath = collection.coverFullPath || null
-    this.books = collection.books ? [...collection.books] : []
-    this.lastUpdate = collection.lastUpdate || null
-    this.createdAt = collection.createdAt || null
-  }
-
-  setData(data) {
-    if (!data.libraryId || !data.name) {
-      return false
-    }
-    this.id = uuidv4()
-    this.libraryId = data.libraryId
-    this.name = data.name
-    this.description = data.description || null
-    this.cover = data.cover || null
-    this.coverFullPath = data.coverFullPath || null
-    this.books = data.books ? [...data.books] : []
-    this.lastUpdate = Date.now()
-    this.createdAt = Date.now()
-    return true
-  }
-
-  addBook(bookId) {
-    this.books.push(bookId)
-    this.lastUpdate = Date.now()
-  }
-
-  removeBook(bookId) {
-    this.books = this.books.filter(bid => bid !== bookId)
-    this.lastUpdate = Date.now()
-  }
-
-  update(payload) {
-    let hasUpdates = false
-    for (const key in payload) {
-      if (key === 'books') {
-        if (payload.books && this.books.join(',') !== payload.books.join(',')) {
-          this.books = [...payload.books]
-          hasUpdates = true
-        }
-      } else if (this[key] !== undefined && this[key] !== payload[key]) {
-        hasUpdates = true
-        this[key] = payload[key]
-      }
-    }
-    if (hasUpdates) {
-      this.lastUpdate = Date.now()
-    }
-    return hasUpdates
-  }
-}
-module.exports = Collection