-
Notifications
You must be signed in to change notification settings - Fork 0
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
GV-33 Create new HTTP Status Codes for backend APIs for Errors #6
base: main
Are you sure you want to change the base?
Conversation
…or checking if an email is a student. The bug is due to not using the await keyword
- Added specific error handling for missing authorization headers (401) - Ensured proper HTTP status codes (404, 500) are returned for key API operations - Updated error logging for better debugging
Amazing work! Can I ask how you tested the 500? Ideally the app shouldn't be directly throwing 500s unless something is realllllly broken |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, if we are responding to the client with an error, we should log the error and stack right before we send that error out to the client.
Also, we should be able to replace all of the try catches with a single one at the end of the router (remember, if something is erroring, it will bypass the return statements until it is caught. That means that at the base of the router, we can just wrap that, catch the error there, and do all of the general error handling and logging at that level. It's also a good solution because we should only be throwing 500s on errors we don't know about that get raised (hence the "internal service error" message).
const authEmail = await getEmailFromAuth(authHeader); | ||
adminStatus = await isAdmin(authEmail); | ||
} catch (error) { | ||
switch (error.constructor.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (error.constructor.name) { | |
switch (typeof err) { |
} | ||
const authEmail = await getEmailFromAuth(authHeader); | ||
adminStatus = await isAdmin(authEmail); | ||
} catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error
tends to be a protected verb, so we tend to prefer going with err
or something along those lines.
throw new AuthorizationError("Authorization Header is empty."); | ||
} | ||
const authEmail = await getEmailFromAuth(authHeader); | ||
adminStatus = await isAdmin(authEmail); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adminStatus = await isAdmin(authEmail); | |
const adminStatus = await isAdmin(authEmail); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally as an aside for now or later, but we can probably parallelize these with promise.all
. We should also return a 403 if the user isn't admin because they aren't going to magically get admin any time soon haha. See the difference between 401 and 403 on MDN
try { | ||
// Attempt to get max scores, if fails, return 404 | ||
maxScores = await getMaxScores(); | ||
} catch (error) { | ||
return res.status(404).json({ message: 'Error fetching max scores' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I think a bit too much of a blanket -- if getMaxScores errors for another reason, it should be a diff error (eg: if the db is down, it should throw a 500)
} | ||
return res.status(200).json(getStudentScoresWithMaxPoints(studentScores, maxScores)); | ||
} catch (error) { | ||
switch (error.constructor.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (error.constructor.name) { | |
switch (typeof err) { |
let maxPoints; | ||
try { | ||
maxPoints = await getTotalPossibleScore(); | ||
} catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error vs err
let maxScores; | ||
try { | ||
maxScores = await getMaxScores(); | ||
} catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error vs err
maxScores = await getMaxScores(); | ||
} catch (error) { | ||
return res.status(500).json({ message: "Max scores not found." }); | ||
} | ||
let studentTotalScore; | ||
let userGrades; | ||
if (isAdmin(email)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invert this statement and return early if the else statement is longer than the body and you return right after
try { | ||
userGrades = await getStudentScores(email); | ||
studentTotalScore = await getStudentTotalScore(email); | ||
} catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error vs err
userGrades = await getStudentScores(email); | ||
studentTotalScore = await getStudentTotalScore(email); | ||
} catch (error) { | ||
switch (error.constructor.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (error.constructor.name) { | |
switch (typeof err) { |
I made requests with fake emails that are not in the spreadsheet. |
The point is when some of these students are logged in at time t = 0 and successfully enters the app, and the student drops the class at time t = 1, at time t >= 2 when these API calls are invoked for time t >= 2, the data might not exist in the spreadsheet, which depends on when Director will sync the data if this applies (extreme edge case) |
Jira Ticket
Jira Ticket
Description
Create new HTTP Status Codes for backend APIs for Errors
Type of Change
Changes
Add 4xx and 5xx responses for API calls that return an error.
Testing
fake student grades:
Fail:
http://localhost/api/v2/students/johnsmith@berkeley.edu/projections
Success:
http://localhost/api/v2/students/gandhi@berkeley.edu/projections
Invalid token:
http://localhost/api/v2/isadmin
Valid token:
http://localhost/api/v2/isadmin
Student not found
http://localhost/api/v2/students/fake@gmail.com/grades
Login (when admin)
http://localhost/api/v2/login
Checklist
<ticket-id>/<brief-description-of-change>
[<ticket-id>] <brief-description-of-change>
Screenshots/Video
Additional Notes