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

Consider rfc7807 error response handling #6

Open
jarrettv opened this issue Mar 20, 2022 · 6 comments
Open

Consider rfc7807 error response handling #6

jarrettv opened this issue Mar 20, 2022 · 6 comments

Comments

@jarrettv
Copy link

jarrettv commented Mar 20, 2022

It appears don will fallback to a 500 error and uses a simple format { "message": "Internal Server Error" }

It would be nice if it used a standard response format with a method of hooking in validation errors like shown in the spec.

Full example below

package settings

import (
	"context"
	"github.com/Example/go-auth/auth"
	"github.com/abemedia/go-don/problems"
)

type SettingsGetReq struct {
	Session auth.Session `context:"auth-session"`
	Code    string       `path:"code"`
}

type SettingsPutReq struct {
	TargetCode string       `path:"code"`
	Session    auth.Session `context:"auth-session"`
	Settings
}

func SettingsGet(ctx context.Context, req SettingsGetReq) (*Settings, error) {
	if !req.Session.Permit("settings-read") {
		return nil, problems.NewPermError(req.Session.Username)
	}

	s, err := get(req.Code)

	if s == nil && err == nil {
		return nil, problems.NewNotFoundError()
	} else if err != nil {
		return nil, problems.NewUnexpError(err)
	}

	return s, nil
}

func SettingsPut(ctx context.Context, req SettingsPutReq) (*Settings, error) {
	if !req.Session.Permit("settings-manage") {
		return nil, problems.NewPermError(req.Session.Username)
	}

	fieldErrors := req.validate(req.TargetCode)
	if fieldErrors != nil {
		return nil, problems.NewValidError(fieldErrors)
	}

	err := put(&req.Settings)
	if err != nil {
		return nil, problems.NewUnexpError(err)
	}

	return &req.Settings, nil
}
@jarrettv
Copy link
Author

If there was an easy way to take over the default error handler. Maybe something similar to panic handler?

@abemedia
Copy link
Owner

abemedia commented Mar 22, 2022

An error handler is a great idea. I've been working on decoupling the handler from the config though as handlers that were wrapped in middleware were panicking, due to the config not getting set so I'm not sure how one would make the error bubble up to API (which has access to the error handler) without using panic.

A workaround might be to either use the panic handler, or setting the response type to any and returning them as normal responses. If they implement StatusCoder we'd still end up with the correct response code.

@jarrettv
Copy link
Author

Here was my simple solution
image

@abemedia
Copy link
Owner

abemedia commented Mar 24, 2022

The thing is that we then lose the encoding of different formats. I'm thinking maybe just implement the marshaler types in the error and then in error encoding check if it implements marshaler before using default marshaling.

So to have a custom error that works in all formats you'd implement encoding.TextMarshaler, JSON.Marshaler, XML.Marshaler and yaml.Marshaler.

You'd probably also want to implement don.StatusCoder to give it a custom status code.

I think the simplest way to achieve all of this would be having a StructuredError helper function which you can just pass a struct and a status code to.

@abemedia
Copy link
Owner

By the way I noticed you're still working on the previous version of Don. I migrated it from standard net/http to using valyala/fasthttp after running some benchmarks and seeing the latter was 10x faster.

@jarrettv
Copy link
Author

I went back and forked from net/http as we don't (yet) need the performance of fasthttp. The fasthttp change requires us to wrap/update all our middleware.

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

No branches or pull requests

2 participants