Skip to content

Incorrect 404 error handling for mongoose findOne #3024

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

Closed
3 tasks
lindapaiste opened this issue Feb 15, 2024 · 7 comments
Closed
3 tasks

Incorrect 404 error handling for mongoose findOne #3024

lindapaiste opened this issue Feb 15, 2024 · 7 comments
Assignees
Labels
Area:Backend For server-side logic, APIs, or database functionality Area: Testing For tests, test coverage, or testing infrastructure

Comments

@lindapaiste
Copy link
Collaborator

Increasing Access

We want our APIs to return accurate and descriptive errors, even when provided with invalid arguments.

Feature enhancement details

Problem

When our project API is called with an invalid project id it will return a 200 success error code with the body null instead of the appropriate 404 not found error code.

To Reproduce

You can open the dev tools console on https://editor.p5js.org/ and paste the following snippet. You must be in a window with the editor due to cross-origin policies.

fetch("https://editor.p5js.org/editor/lindapaiste2/projects/wrong")
  .then(res => res.json().then(json => console.log('status', res.status, 'json', json)))

I'm not flagging this is a "bug" since it has no repercussions on the editor web app. You can't make this API call by navigating to an incorrect URL, since we will not load the page for a project which doesn't exist. The bug is only seen when querying the API directly.

Root Cause

The problem is in the following code in the project.controller:

Project.findOne({
user: user._id,
$or: [{ _id: projectId }, { slug: projectId }]
})
.populate('user', 'username')
.exec((err, project) => { // eslint-disable-line
if (err) {
console.log(err);
return res
.status(404)
.send({ message: 'Project with that id does not exist' });
}
return res.json(project);
});

The mongoose findOne function with not return an err when there is no document. This is considered normal behavior and not an error. It will return a null project, so we need to check for that. In other places we handle this with if (err || !project) {.

err here would be some unforeseen problem in mongoose, and should probably be a 500 internal server error instead of a 404 not found, in my opinion. I think we can let the error be thrown and caught by default express error-handling middleware?

Tasks:

  • Fix the above code to return a 404 when there is no project found.
  • Find other places in the code where we make this type of error
  • Create tests for the project.controller file to verify the current incorrect behavior and future correct behavior. We want to know that an API request with a valid username but an invalid projectId would return a 404 not found error.
@lindapaiste lindapaiste added Area: Testing For tests, test coverage, or testing infrastructure Area:Backend For server-side logic, APIs, or database functionality labels Feb 15, 2024
@mathanraj0601
Copy link

I like to work on this issue @lindapaiste can I get assigned?

@mathanraj0601
Copy link

Action Plan: Incorrect 404 error handling for mongoose findOne

  1. Conditional Handling: Update getProject to conditionally throw a 404 for a project not found and a 500 for unexpected errors.
  2. Identify Similar Areas: Scan the codebase for similar cases where findOne lacks proper handling for no document found. Implement consistent conditional handling.
  3. Write Tests: Write concise tests to ensure that a 404 is returned when the project is null and a 500 is returned for unexpected errors.

@Swarnendu0123
Copy link
Contributor

Swarnendu0123 commented Feb 17, 2024

Well I feel the correct implementation can be this bellow snnipid:

export function getProject(req, res) {
  const { project_id: projectId, username } = req.params;
  User.findByUsername(username, (err, user) => {
    if (!user) {
      return res
        .status(404)
        .send({ message: 'User with that username does not exist' });
    }
    Project.findOne({
      user: user._id,
      $or: [{ _id: projectId }, { slug: projectId }]
    })
      .populate('user', 'username')
      .exec((err, project) => {
        if (err) {
          console.log(err);
          return res.status(500).send({ message: 'Internal Server Error' });
        }
        if (!project) {
          return res
            .status(404)
            .send({ message: 'Project with that id does not exist' });
        }
        return res.json(project);
      });
  });
}

@lindapaiste
Copy link
Collaborator Author

@mathanraj0601 I've assigned you the issue. I would love for you to start with item 3 on your list and write some tests for the project.controller.js file. TBH we're kind of in the process of refactoring a lot of the API code so whatever issues you find in item 2 might already be addressed elsewhere.

@mathanraj0601
Copy link

mathanraj0601 commented Feb 22, 2024

Thanks, @lindapaiste. Could you please confirm if it's necessary to make the changes mentioned in item 1 and write test cases for that, or should I focus on writing test cases for the existing controller method which have test cases already? Let me know your preference! : )

@Jatin24062005
Copy link
Contributor

@raclim Can i work on these issue ? Or if it is fixed Can you please close the these comment.

@raclim
Copy link
Collaborator

raclim commented Feb 8, 2025

Thanks @Jatin24062005 for checking in on this! I think this issue has been addressed by #3025, so I'll close this out!

@raclim raclim closed this as completed Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:Backend For server-side logic, APIs, or database functionality Area: Testing For tests, test coverage, or testing infrastructure
Projects
None yet
Development

No branches or pull requests

5 participants