fix: Types on overridden findByPk methods (#5908)

server/commands/documentMover.ts

@@ -1,5 +1,6 @@
 import invariant from "invariant";
 import { Transaction } from "sequelize";
+import { ValidationError } from "@server/errors";
 import { traceFunction } from "@server/logging/tracing";
 import { User, Document, Collection, Pin, Event } from "@server/models";
 import pinDestroyer from "./pinDestroyer";
@@ -50,7 +51,10 @@ async function documentMover({
   }
 
   if (document.template) {
-    invariant(collectionId, "collectionId should exist");
+    if (!document.collectionId) {
+      throw ValidationError("Templates must be in a collection");
+    }
+
     document.collectionId = collectionId;
     document.parentDocumentId = null;
     document.lastModifiedById = user.id;
@@ -142,7 +146,8 @@ async function documentMover({
         }).findByPk(collectionId, {
           transaction,
         });
-        invariant(newCollection, "collection should exist");
+        invariant(newCollection, "Collection not found");
+
         result.collections.push(newCollection);
 
         await Document.update(

server/models/Collection.ts

@@ -1,9 +1,16 @@
+/* eslint-disable lines-between-class-members */
 import find from "lodash/find";
 import findIndex from "lodash/findIndex";
 import remove from "lodash/remove";
 import uniq from "lodash/uniq";
 import randomstring from "randomstring";
-import { Identifier, Transaction, Op, FindOptions } from "sequelize";
+import {
+  Identifier,
+  Transaction,
+  Op,
+  FindOptions,
+  NonNullFindOptions,
+} from "sequelize";
 import {
   Sequelize,
   Table,
@@ -362,8 +369,17 @@ class Collection extends ParanoidModel {
    * Overrides the standard findByPk behavior to allow also querying by urlId
    *
    * @param id uuid or urlId
-   * @returns collection instance
+   * @param options FindOptions
+   * @returns A promise resolving to a collection instance or null
    */
+  static async findByPk(
+    id: Identifier,
+    options?: NonNullFindOptions<Collection>
+  ): Promise<Collection>;
+  static async findByPk(
+    id: Identifier,
+    options?: FindOptions<Collection>
+  ): Promise<Collection | null>;
   static async findByPk(
     id: Identifier,
     options: FindOptions<Collection> = {}

server/models/Document.ts

@@ -1,8 +1,9 @@
+/* eslint-disable lines-between-class-members */
 import compact from "lodash/compact";
 import isNil from "lodash/isNil";
 import uniq from "lodash/uniq";
 import randomstring from "randomstring";
-import type { SaveOptions } from "sequelize";
+import type { Identifier, NonNullFindOptions, SaveOptions } from "sequelize";
 import {
   Sequelize,
   Transaction,
@@ -52,6 +53,11 @@ import Length from "./validators/Length";
 
 export const DOCUMENT_VERSION = 2;
 
+type AdditionalFindOptions = {
+  userId?: string;
+  includeState?: boolean;
+};
+
 @DefaultScope(() => ({
   attributes: {
     exclude: ["state"],
@@ -429,13 +435,30 @@ class Document extends ParanoidModel {
     return this.scope(["defaultScope", collectionScope, viewScope]);
   }
 
+  /**
+   * Overrides the standard findByPk behavior to allow also querying by urlId
+   *
+   * @param id uuid or urlId
+   * @param options FindOptions
+   * @returns A promise resolving to a collection instance or null
+   */
   static async findByPk(
-    id: string,
-    options: FindOptions<Document> & {
-      userId?: string;
-      includeState?: boolean;
-    } = {}
+    id: Identifier,
+    options?: NonNullFindOptions<Document> & AdditionalFindOptions
+  ): Promise<Document>;
+  static async findByPk(
+    id: Identifier,
+    options?: FindOptions<Document> & AdditionalFindOptions
+  ): Promise<Document | null>;
+  static async findByPk(
+    id: Identifier,
+    options: (NonNullFindOptions<Document> | FindOptions<Document>) &
+      AdditionalFindOptions = {}
   ): Promise<Document | null> {
+    if (typeof id !== "string") {
+      return null;
+    }
+
     // allow default preloading of collection membership if `userId` is passed in find options
     // almost every endpoint needs the collection membership to determine policy permissions.
     const scope = this.scope([

server/queues/processors/RevisionsProcessor.ts

@@ -1,4 +1,3 @@
-import invariant from "invariant";
 import revisionCreator from "@server/commands/revisionCreator";
 import { Revision, Document, User } from "@server/models";
 import { DocumentEvent, RevisionEvent, Event } from "@server/types";
@@ -22,8 +21,8 @@ export default class RevisionsProcessor extends BaseProcessor {
 
         const document = await Document.findByPk(event.documentId, {
           paranoid: false,
+          rejectOnEmpty: true,
         });
-        invariant(document, "Document should exist");
         const previous = await Revision.findLatest(document.id);
 
         // we don't create revisions if identical to previous revision, this can

server/routes/api/documents/documents.ts

@@ -928,8 +928,8 @@ router.post(
     // reload to get all of the data needed to present (user, collection etc)
     const reloaded = await Document.findByPk(document.id, {
       userId: user.id,
+      rejectOnEmpty: true,
     });
-    invariant(reloaded, "document not found");
 
     ctx.body = {
       data: await presentDocument(reloaded),

server/utils/jwt.ts

@@ -1,5 +1,4 @@
 import { subMinutes } from "date-fns";
-import invariant from "invariant";
 import JWT from "jsonwebtoken";
 import { Team, User } from "@server/models";
 import { AuthenticationError } from "../errors";
@@ -85,8 +84,9 @@ export async function getUserForEmailSigninToken(token: string): Promise<User> {
     }
   }
 
-  const user = await User.scope("withTeam").findByPk(payload.id);
-  invariant(user, "User not found");
+  const user = await User.scope("withTeam").findByPk(payload.id, {
+    rejectOnEmpty: true,
+  });
 
   try {
     JWT.verify(token, user.jwtSecret);