Continue HTTP response code review from PR#1598 (Issue 1647)

[kujoon226]

Description

This pull request continues the work started in PR #1598 for Issue #1647.

The original commit chain from PR #1598 has been preserved as requested in the issue description. We added a missing '500 INTERNAL SERVER ERROR' response in src/server/routes/conversions.js, specifically for base route so the route returns an appropriate response when an exception occurs.

Fixes #1647

Contributors:

• Kujoon Kwon (@kujoon226 )
• Neil Cabanilla (@neilcabanilla)

Type of change

(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])

• Note merging this changes the database configuration.
• This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

• [x ] I have followed the OED pull request ideas
• [x ] I have removed text in ( ) from the issue request
• [x ] You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

Some routes protected by AdminAuthMiddleWare could not be fully exercised. Although administtrator authentication was successful, the tested failure scenarios did not reach the intended route-level catch blocks. We still need to verify those exception paths return 500 Internal Server Error.

[huss]

@kujoon226 & @neilcabanilla Can I ask if the linked issue is labeled as partly addressed because of the limitation in the description about testing or is there another reason? If it is that, are you still trying and making progress? Would getting any help be of interest?

[kujoon226]

@huss, yes the "Partly Addresses" label was mainly because of testing limitations.

After opening this PR, we've been able to verify additional routes and here's where we are now:

1. POST /api/conversions/edit (around line 91):

} catch (err) {
   log.error(`Error while editing conversion with error(s): ${err}`);
   failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR,
       `Error while editing conversion with error(s): ${err}`);
}

We have not been able to naturally reach this catch. We tested valid edits (200). Nonexistent conversions (200), and stopping the database. When database is stopped, AdminAuthMiddleware returns 401 before this request reaches this route.

2. POST /api/conversions/addConversion (around line 159):

} catch (err) {
    log.error(`Error while inserting new conversion with error(s): ${err}`);
    failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR,
        `Error while inserting new conversion with error(s): ${err}`);
}

We were able to verify this catch. Submitting the same conversion twice directly through API returned 500 INTERNAL SERVER ERROR. We also confirmed the admin UI prevents duplicate conversions before the request is sent. We think this should stay as 500 since this catch handles database errors that happen during the insert. The normal admin UI already prevents duplicate before even sending the request, so reaching this catch usually means the request was made directly through API or another unexpected database error occured.

3. POST /api/conversions/delete (around line 220):

} catch (err) {
    log.error(`Error while deleting conversion and updating meters/groups: ${err}`);
    failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR,
        `Error while deleting conversion and updating meters/groups: ${err}`);
}

This had same result as the edit route. Valid requests return 200. Invalid request return 400. Nonexistent meter IDs still return 200, and stopping database causes same issue and just return 401 before catch is reached.

4. POST /api/groups/create (around line 320):

} else {
    log.error(`Error while inserting new group ${err}`, err);
    failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR,
        err.toString() + ' with detail ' + err['detail']);
}

We were able to verify by creating a group with a nonexistent child group (childGroups: [999999]), which caused the transaction to fail and returned 500 INTERNAL SERVER ERROR. We believe this 500 should stay as is since it represents unexpected database transaction faliures while creating a group and attaching its child groups/meters. Duplicate group names are already handled as 400 separately, so this catch is specifically for other internal database errors.

We have not found a natural way to reach the conversions/edit and conversions/delete catch blocks. Valid requests succeed, invalid requests are handled earlier, nonexistent IDs do not throw errors, and stopping database causes AdminAuthMiddleware to return 401 before the route is even reached. We would appreciate guidence on the preferred way to test these two exception paths.

[huss]

@kujoon226 Thank you for the detailed information. My thoughts are below.

@huss, yes the "Partly Addresses" label was mainly because of testing limitations.

After opening this PR, we've been able to verify additional routes and here's where we are now:

1. POST /api/conversions/edit (around line 91):

} catch (err) {
   log.error(`Error while editing conversion with error(s): ${err}`);
   failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR,
       `Error while editing conversion with error(s): ${err}`);
}

We have not been able to naturally reach this catch. We tested valid edits (200). Nonexistent conversions (200), and stopping the database. When database is stopped, AdminAuthMiddleware returns 401 before this request reaches this route.

I've had to generate errors many times to test unusual code so here is my go to technique in this situation. I modify the request so it is invalid within the route. Here is what I did to that route:

		try {
			request.body.sourceId = -99;
			const updatedConversion = new Conversion(req.body.sourceId, req.body.destinationId, req.body.bidirectional,
				req.body.slope, req.body.intercept, req.body.note);
			await updatedConversion.update(conn);

I received Internal Server Error popup on the client web page then.

2. POST /api/conversions/addConversion (around line 159):

Seems fine.

3. POST /api/conversions/delete (around line 220):

} catch (err) {
    log.error(`Error while deleting conversion and updating meters/groups: ${err}`);
    failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR,
        `Error while deleting conversion and updating meters/groups: ${err}`);
}

This had same result as the edit route. Valid requests return 200. Invalid request return 400. Nonexistent meter IDs still return 200, and stopping database causes same issue and just return 401 before catch is reached.

I think the same technique above will work here. My quick look saw sourceId used for the variable here.

4. POST /api/groups/create (around line 320):

Seems fine.

We have not found a natural way to reach the conversions/edit and conversions/delete catch blocks. Valid requests succeed, invalid requests are handled earlier, nonexistent IDs do not throw errors, and stopping database causes AdminAuthMiddleware to return 401 before the route is even reached. We would appreciate guidence on the preferred way to test these two exception paths.

See above. Could you test the other two to see if you can get the result you want? Let me know if you have any issues. Once that is done and any needed changes are made, you can update the description to Fixes from Partially Fixes so the issue will automatically close (assuming you think that is appropriate). If you then put a comment in the PR about the status I will look at the PR for a careful review.

As always, please let me know any questions/thoughts or items that are unclear.

[neilcabanilla]

Hello @huss,

We were able to test the other two routes (conversions/edit & conversions/delete) by temporarily adding the line of code you've mentioned previously. We were able to get to the catch block for those two routes and it shows us the INTERNAL_SERVER_ERROR (code 500) that we were testing for. We believe everything looks appropriate and everything looks good to go.

Please let us know if you have anything you would like us to change or add.

Thank you so much!