Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Query findById methods #15337

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sderrow
Copy link
Contributor

@sderrow sderrow commented Apr 1, 2025

Summary

Even though query.d.ts says otherwise, Query does NOT in fact have the following methods:

  • findById
  • findByIdAndUpdate
  • findByIdAndDelete

You can see as much if you try to call them. Here's a reproducible example:

import mongoose from "mongoose";

type Person = {
  _id: mongoose.Types.ObjectId;
  name: string;
  email: string;
  archived: boolean;
  createdAt: number;
  updatedAt: number;
};

const personSchema = new mongoose.Schema<Person>({
  name: { type: String, required: true },
  email: { type: String, required: true, index: true },
  archived: { type: Boolean, default: false },

  createdAt: { type: Number },
  updatedAt: { type: Number },
});

const Person = mongoose.model("Person", personSchema);

async function run() {
  await mongoose.connect("mongodb://127.0.0.1:27017");

  await mongoose.connection.dropDatabase();
  console.log("Database dropped");

  const person = await Person.create({ name: "Alice", email: "alice@example.com" });
  console.log("Person created");

  try {
    const p = await Person.find({}).findById(person._id);
    console.log("[success] Query.prototype.findById", p?.id);
  } catch (err: any) {
    console.error("[error] Query.prototype.findById failed:", err.message);
  }

  try {
    await Person.find({}).findByIdAndUpdate(person._id, { email: "a@b.com" });
    console.log("[success] Query.prototype.findByIdAndUpdate");
  } catch (err: any) {
    console.error("[error] Query.prototype.findByIdAndUpdate failed:", err.message);
  }

  try {
    await Person.find({}).findByIdAndDelete(person._id);
    console.log("[success] Query.prototype.findByIdAndDelete");
  } catch (err: any) {
    console.error("[error] Query.prototype.findByIdAndDelete failed:", err.message);
  }

  try {
    const p = await Person.find({}).findOne({ _id: person._id });
    console.log("[success] Query.prototype.findOne", p?.id);
  } catch (err: any) {
    console.error("[error] Query.prototype.findOne failed:", err.message);
  }

  try {
    const p = await Person.find({}).findOneAndUpdate({ _id: person._id }, { email: "a@b.com" });
    console.log("[success] Query.prototype.findOneAndUpdate", p?.id);
  } catch (err: any) {
    console.error("[error] Query.prototype.findOneAndUpdate failed:", err.message);
  }

  try {
    const p = await Person.find({}).findOneAndDelete({ _id: person._id });
    console.log("[success] Query.prototype.findOneAndDelete", p?.id);
  } catch (err: any) {
    console.error("[error] Query.prototype.findOneAndDelete failed:", err.message);
  }
}

void run()

That example gives zero Typescript warnings because it thinks findById, findByIdAndUpdate, and findByIdAndDelete are all valid methods on Query. But when you actually run it, you get the following output:

Database dropped
Person created
[error] Query.prototype.findById failed: Person.find(...).findById is not a function
[error] Query.prototype.findByIdAndUpdate failed: Person.find(...).findByIdAndUpdate is not a function
[error] Query.prototype.findByIdAndDelete failed: Person.find(...).findByIdAndDelete is not a function
[success] Query.prototype.findOne 67ec4b44052e05360699693f
[success] Query.prototype.findOneAndUpdate 67ec4b44052e05360699693f
[success] Query.prototype.findOneAndDelete 67ec4b44052e05360699693f

One place where this distinction matters is with the accessibleBy casl plugin. This comment references the problem, but no action was taken to address the root cause.

I'm not sure if the type declarations were included by mistake (here and here), or if the implementations used to exist and were deleted at some point.

I don't see the harm in including the implementations, so that's what this PR adds. Basic tests included.

@sderrow
Copy link
Contributor Author

sderrow commented Apr 1, 2025

@vkarpov15 sorry to tag you, but I'm not sure the best way to request a review. Let me know if you have any thoughts on this one.

@vkarpov15 vkarpov15 requested a review from Copilot April 2, 2025 17:26
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements the Query prototype methods findById, findByIdAndUpdate, and findByIdAndDelete to address discrepancies in type definitions versus actual functionality. It also adds corresponding tests to verify the correct behavior of these methods.

  • Implements findById, findByIdAndUpdate, and findByIdAndDelete in lib/query.js.
  • Adds tests in test/query.test.js to validate these new query methods.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/query.test.js New tests added to verify the behavior of the query methods
lib/query.js Implements the new findById, findByIdAndUpdate, and findByIdAndDelete methods

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nitpick, but LGTM otherwise 👍

@sderrow sderrow requested a review from vkarpov15 April 3, 2025 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants