Skip to content

Chore/sentry errors #238

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Chore/sentry errors #238

wants to merge 4 commits into from

Conversation

tobias-oetzel
Copy link
Contributor

we currently do not get any error logs besides the general error handler, that does not report to sentry because there is no error wrapping

we currently do not get any error logs besides the general error handler, that does not report to sentry because there is no error wrapping
@tobias-oetzel tobias-oetzel requested a review from a team as a code owner May 19, 2025 08:29
@CLAassistant
Copy link

CLAassistant commented May 19, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

@pteich pteich left a comment

Choose a reason for hiding this comment

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

I had some comments and suggestions. Exporting error vars is optional (could make sense, though)

extracted a sctruct because there were too many params
separated the actual check from the preparation
reported only client errors to sentry
Copy link

@pteich pteich left a comment

Choose a reason for hiding this comment

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

Approved with comments

tenantID, err := openmfpcontext.GetTenantFromContext(ctx)
if err != nil {
return nil, err
if !res.Allowed {
Copy link

Choose a reason for hiding this comment

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

Are we sure res is always not nil if no error occured?

log.Error().Err(err).Str("user", req.TupleKey.User).Msg("authorization check failed")
return nil, err
}
func (ac *authChecker) extractValuesToCheckFromRequest(ctx context.Context, entityParamName string, entityTypeParamName *string, entityType *string) (context.Context, bool, string, string, string, error) {
Copy link

Choose a reason for hiding this comment

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

This would be one of the rare situations where named return values could be useful both for documentation and for preventing returning a lot of "" without knowing what it actually is and returning the (empty) variables instead.

Copy link

Choose a reason for hiding this comment

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

The name extractValue is a bit misleading as it does extract but also creates a new context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants