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

bug: convert DbErrors to TokenserverErrors #1327

Merged
merged 12 commits into from
Jun 10, 2022

Conversation

ethowitz
Copy link
Contributor

@ethowitz ethowitz commented Jun 2, 2022

Description

The Tokenserver handler should always return a TokenserverError to ensure that errors are of the format expected by Tokenserver clients. In order to accomplish this, I needed to implement From<DbError> for TokenserverError. I treat any unhandled DbError as an internal error, since SQLAlchemy errors on the Python Tokenserver raise BackendErrors, which return 503s to the client.

Testing

I've added an integration test to ensure that a DbError response has the correct format

Closes #1316

@ethowitz ethowitz requested a review from a team June 2, 2022 19:39
@ethowitz ethowitz changed the title bug: convert DbErrors to TokenserverErrors bug: convert DbErrors to TokenserverErrors Jun 2, 2022
jrconlin
jrconlin previously approved these changes Jun 3, 2022
Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

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

I like this approach. I wonder how much I can steal for the autopush redo.

@ethowitz ethowitz force-pushed the bug/convert-dberrors-to-tokenservererrors branch from 0c74825 to 0d6ff3b Compare June 6, 2022 16:28
@ethowitz
Copy link
Contributor Author

ethowitz commented Jun 6, 2022

Just updated this comment to reflect the fact that we're returning 503 and not 500 for unhandled database errors

@ethowitz ethowitz requested a review from jrconlin June 6, 2022 16:29
@@ -52,31 +73,31 @@ impl TokenserverError {
}
}

pub fn invalid_key_id(description: &'static str) -> Self {
pub fn invalid_key_id(description: String) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively you could take description: &str for these to avoid to_owned() at the call sites. You'll need to pass &foo when you have owned Strings and it'll add an extra clone, but that's not the common case and IMO the small clone's worth it for convenience (but feel free to disagree and leave as is).

Copy link
Member

Choose a reason for hiding this comment

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

(and when you need to avoid that extra clone you can take an Into<Cow<&str>> but it's rarely worth that effort)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I had done it this way to make the clones obvious: in my mind, passing by reference suggests that the function doesn't need ownership, and it may suggest to the caller that the function is less expensive than it really is. But now I'm not so sure 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps having the function take Into<String> is a good compromise? It allows flexibility, but it signals to the caller that the function does in fact need ownership

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with how it is now 😄 but good point, Into<String> is simpler and works similarly and isn't too much trouble to specify as a generic type.

@ethowitz ethowitz merged commit 9bea328 into master Jun 10, 2022
@ethowitz ethowitz deleted the bug/convert-dberrors-to-tokenservererrors branch June 10, 2022 21:30
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.

Tokenserver: Convert DbErrors to TokenserverErrors before returning to the client
3 participants