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

api: Improve http errors #2408

Merged
merged 4 commits into from
Sep 20, 2023
Merged

api: Improve http errors #2408

merged 4 commits into from
Sep 20, 2023

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented Sep 14, 2023

At first, I only wanted to fix some wrong connect error translation but then I realized we have many different ways of doing HTTP errors.

This PR makes sure we use the same function util.WriteError or util.WriteErrorWithStatus which generates the same HTTP JSON response as the connect API.

Fixes #2368

@cyriltovena cyriltovena requested a review from a team as a code owner September 14, 2023 20:31
@@ -20,11 +19,11 @@ func (f *Frontend) SelectMergeProfile(ctx context.Context, c *connect.Request[qu
ctx = connectgrpc.WithProcedure(ctx, querierv1connect.QuerierServiceSelectMergeProfileProcedure)
tenantIDs, err := tenant.TenantIDs(ctx)
if err != nil {
return nil, connect.NewError(http.StatusBadRequest, err)
return nil, connect.NewError(connect.CodeInvalidArgument, err)
Copy link
Contributor Author

@cyriltovena cyriltovena Sep 14, 2023

Choose a reason for hiding this comment

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

This is what paged me today.

Copy link
Contributor

@bryanhuhta bryanhuhta left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

pkg/util/error.go Outdated Show resolved Hide resolved
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Like to get the content type situation sorted before merge 🙂

pkg/util/error.go Outdated Show resolved Hide resolved
@cyriltovena cyriltovena enabled auto-merge (squash) September 20, 2023 07:58
@cyriltovena cyriltovena merged commit 5237bec into main Sep 20, 2023
14 of 15 checks passed
@cyriltovena cyriltovena deleted the improve-http-error branch September 20, 2023 08:19
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.

Wrong status code for query time range validation
3 participants