Incorrect 404 error handling for mongoose `findOne`

[lindapaiste]

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:

p5.js-web-editor/server/controllers/project.controller.js

Lines 94 to 107 in 6cac275

	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.

[mathanraj0601]

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

[mathanraj0601]

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]

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]

@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]

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]

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

[raclim]

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