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

Return structured errors from the selectors API #3998

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Jul 25, 2024

Summary

In order to tell the user what's wrong with a selector they are about to
take into use, let's introduce a structured way of returning errors to
the selector API.

The errors can be represented as JSON in order for clients to be able to
parse the error without knowing much about the type.

Related: #3725

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

with the added unit tests and as part of a larger topic branch

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@jhrozek jhrozek requested a review from a team as a code owner July 25, 2024 10:54
@@ -38,8 +39,110 @@ var (
// ErrResultUnknown is returned when the result of a selector expression is unknown
// this tells the caller to try again with more information
ErrResultUnknown = errors.New("result is unknown")
// ErrUnsupported is returned when the entity type is not supported
ErrUnsupported = errors.New("unsupported entity type")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error will be easier to test once we expose the errors through handlers

var jsonString string

if errors.As(err, &ce) {
jsonString = ce.Error()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might look like an odd test because go code could very well just look at the contents of the struct, but the thinking was to include the JSON representation as HTTP body when manipulating selectors from handlers throws an error, then FE would be able to parse the JSON which would include the location of the failed CEL expression.

Is that an acceptable pattern or would we prefer to add an error message to the protobuf message?

@coveralls
Copy link

Coverage Status

coverage: 54.402% (+0.09%) from 54.316%
when pulling 0ef5a6c on jhrozek:selectors_structured_errors
into 1cff724 on stacklok:main.

ErrDetails: errDetailsFromCelIssues(source, issues),
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

should these errors wrap a common one to ease error handling? https://pkg.go.dev/errors you could implement Unwraps for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a really good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. We could even use errors.Join(ErrCheckFailed, ErrParse) for parse and join with a check error for the check but I'm not sure if it's worth it.

dmjb
dmjb previously approved these changes Jul 25, 2024
In order to tell the user what's wrong with a selector they are about to
take into use, let's introduce a structured way of returning errors to
the selector API.

The errors can be represented as JSON in order for clients to be able to
parse the error without knowing much about the type.
@jhrozek jhrozek merged commit 5707473 into mindersec:main Jul 26, 2024
21 checks passed
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.

4 participants