Update errors package & deserialize conjure errors #94

bmoylan · 2020-05-05T15:07:41Z

[break] Remove errors.paramterizer, replace with wparams.ParamStorer. The parameterizer type separated params into safe and unsafe substructures, which would be nice but is not in line with the conjure wire spec. In the case we can not determine a generated error type which knows which params are safe, all are considered unsafe.
[break] Remove errors.ErrorFromResponse which seems to be unused.
[break] errors.WriteErrorResponse now takes an Error instead of SerializableError for ease of use.
[feature] Add error type registry, a global map indicating how to unmarshal conjure errors received by the client based on the errorName. errors.UnmarshalError uses a registered type if possible, or falls back to genericError.
[improvement] Default error decoder uses errors.UnmarshalError if the response appears to be JSON.

TODO:

Document RegisterErrorType & improve validation
Update body_handler.go to attempt to unmarshal errors
Test, test, test

This change is

changelog-app · 2020-05-05T15:07:46Z

Generate changelog in `changelog/@unreleased`

Type

Description

Update errors package & deserialize conjure errors

[break] Remove errors.paramterizer, replace with wparams.ParamStorer. The parameterizer type separated params into safe and unsafe substructures, which would be nice but is not in line with the conjure wire spec. In the case we can not determine a generated error type which knows which params are safe, all are considered unsafe.
[break] Remove errors.ErrorFromResponse which seems to be unused.
[break] errors.WriteErrorResponse now takes an Error instead of SerializableError for ease of use.
[feature] Add error type registry, a global map indicating how to unmarshal conjure errors received by the client based on the errorName. errors.UnmarshalError uses a registered type if possible, or falls back to genericError.
[improvement] Default error decoder uses errors.UnmarshalError if the response appears to be JSON.

Check the box to generate changelog(s)

Generate changelog entry

bmoylan · 2020-05-05T20:22:58Z

conjure-go-client/httpclient/response_error_decoder_middleware.go

@@ -73,3 +75,28 @@ func (d restErrorDecoder) DecodeError(resp *http.Response) error {
 func StatusCodeFromError(err error) (statusCode int, ok bool) {
 	return internal.StatusCodeFromError(err)
 }
+
+func ConjureErrorDecoder() ErrorDecoder {


This is currently opt-in, but we probably just want to make it the default. Alternatively, we could make conjure-go generate use of this param if we did not feel like changing existing behavior.

I'd argue for just making this the default. It seems strictly better to me, though the change of error message might be a little confusing at first. If we wanted to make it look the same for all existing use cases, we could make the fallback error message the same as the current restErrorDecoder one, but don't think that's really necessary.

asanderson15 · 2020-05-05T20:37:01Z

conjure-go-contract/errors/error_type_registry_test.go

+		assert.PanicsWithValue(t,
+			"Error type *string does not implement errors.Error interface",
+			func() {
+				RegisterErrorType("name2", reflect.TypeOf("string"))


nit: maybe change this to name3. I know the last registration should panic, but slightly clarifies that this is not another reuse case.

asanderson15 · 2020-05-05T20:43:21Z

conjure-go-contract/errors/generic_error.go

+func (e genericError) SafeParams() map[string]interface{} {
+	// Add errorInstanceId as safe param
+	idStorer := wparams.NewSafeParamStorer(map[string]interface{}{"errorInstanceId": e.errorInstanceID})
+	return wparams.NewParamStorer(e.params, idStorer).SafeParams()


Could we simplify this to:

safeParams := e.params.SafeParams() safeParams["errorInstanceId"] = e.errorInstanceID return safeParams

The syntax you have is nicer, but just feels like a lot of unnecessary work creating and copying things around to get to the same result.

I was trying to avoid mutating the underlying map, and NewParamStorer copies everything. I could also do that in here with something like

func (e genericError) SafeParams() map[string]interface{} { // Copy safe params map (so we don't mutate the underlying one) and add errorInstanceId safeParams := make(map[string]interface{}, len(e.params.SafeParams())+1) for k, v := range e.params.SafeParams() { safeParams[k] = v } safeParams["errorInstanceId"] = e.errorInstanceID return safeParams }

thoughts?

The reason mutating it is undesirable is it will end up in the marshalled payload

Got it, sorry, forgot that SafeParams() didn't copy things. Yeah, if you inlined the contents of the current code and your proposal above, the actual diff is fairly small so fine with either the original or your slightly more efficient proposal in the comment.

asanderson15 · 2020-05-05T20:48:07Z

conjure-go-contract/errors/generic_error_test.go

+	assert.Equal(t, params(e), params(unmarshalledError))
+}
+
+func params(storer wparams.ParamStorer) map[string]interface{} {


let's move this into the generic_error.go and have marshalParams call into it. Let's also use the code in marshalParams, which is more efficient rather than this function.

done, though it required moving this test into the package so we don't have to export mergeParams (not a big deal)

asanderson15 · 2020-05-05T21:07:15Z

conjure-go-contract/errors/http.go

+	// If we fail, use best-effort conversion to SerializableError.
+	if marshalledError == nil || err != nil {
+		// serializeError() handles param failures, so this should never fail
+		marshalledError, _ = codecs.JSON.Marshal(serializeError(e))


Isn't it safer to handle this error? If it throws, marshalledError will be nil. Not sure if that's a problem on w.Write.

Yeah, I fought with this for a long time. Maybe it's more understandable if I just bring serializeError into this file/function. I thought it was going to get used in more than one place but ended up being wrong.

The idea (which that helper function handles) is that if we fail to marshal it's going to be due to the params and not the primitive fields. So we null out the params on errors and just assume marshalling the other three fields will work.

I looked at w.Write and it looks like it should gracefully handle a nil for marshalledError, so I guess this is ok. Worst case, you don't get anything in the response body.

asanderson15 · 2020-05-05T21:10:34Z

conjure-go-contract/errors/unmarshal.go

+
+// UnmarshalError attempts to deserialize the message to a known implementation of Error.
+// Custom error types should be registered using RegisterErrorType.
+// If the ErrorName is not recognized, a genericError is returned with all unsafe params.


"is returned with all params marked unsafe"?

asanderson15 · 2020-05-05T21:15:49Z

conjure-go-contract/errors/unmarshal.go

+// If we fail to unmarshal to a generic SerializableError or to the type specified by ErrorName, an error is returned.
+func UnmarshalError(body []byte) (Error, error) {
+	var se SerializableError
+	if err := codecs.JSON.Unmarshal(body, &se); err != nil {


We could consider doing a shallower unmarshal here:

type errorName struct { Name string `json:"errorName"` } var name errorName if err := codecs.JSON.Unmarshal(body, &name); err != nil { }

That would be slightly faster (avoids unmarshaling the same fields twice) and also short-circuit 99% of the cases where we can't fall back to a SerializableError either. Could also use gjson to do effectively the same thing.

asanderson15 · 2020-05-06T00:48:19Z

conjure-go-contract/errors/unmarshal_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestUnmarshalError(t *testing.T) {


Is it worth adding a test case or two for totally garbage, unserializable errors (e.g., some plaintext, some nonconforming json, etc.)?

asanderson15 · 2020-05-06T00:54:03Z

conjure-go-contract/errors/unmarshal.go

+
+	instance := reflect.New(typ).Interface()
+	if err := codecs.JSON.Unmarshal(body, &instance); err != nil {
+		// TODO(bmoylan): Do we want to be more lenient and use a genericError if this can not unmarshal?


This seems fine to me - if it's supposed to be a registered type, I don't think we have to go any farther than our default fallback behavior of wrapping the body as unsafe.

asanderson15 · 2020-05-06T00:54:52Z

conjure-go-contract/errors/generic_error.go

 func (e genericError) MarshalJSON() ([]byte, error) {
-	marshalledParameters, err := codecs.JSON.Marshal(e.parameterizer)
+	marshalledParameters, err := codecs.JSON.Marshal(mergeParams(e.params))


Nit: can we replace all instances of *marshall* with *marshal*?

nmiyake

Looks great for the most part! Just left some minor comments/feedback.

Reviewed 3 of 6 files at r1, 6 of 10 files at r2, 3 of 4 files at r3, 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @asanderson15 and @bmoylan)

conjure-go-client/httpclient/response_error_decoder_middleware_test.go, line 116 at r5 (raw file):

}

func TestConjureErrorDecoder(t *testing.T) {

Can you add brief comment to describe the high-level behavior that's being tested here?

conjure-go-contract/errors/error.go, line 34 at r5 (raw file):

	// InstanceID returns unique identifier of this particular error instance.
	InstanceID() uuid.UUID
	// ParamStorer returns a set of named parameters detailing this particular error instance,

Comment should be updated now that we're fully embedding another interface (either just omit, or describe that this interface satisfies ParamStorer)

conjure-go-contract/errors/unmarshal.go, line 33 at r5 (raw file):

// If we fail to unmarshal to a generic SerializableError or to the type specified by ErrorName, an error is returned.
func UnmarshalError(body []byte) (Error, error) {
	var name errorNameAccessor // TODO(bmoylan) is gson's speed worth the dependency?

If errorNameAccessor is used only here (that is, you have it only to fit the shape that you want), you can just define the struct literal directly:

name := struct{
      Name string `json:"errorName"`
}{}

This is the pattern I usually use if we're doing a partial unmarshal in just one function, as it encapsulates the logic within the function (there's no need to reason about the errorNameAccessor type)

bmoylan

Reviewable status: 6 of 16 files reviewed, 11 unresolved discussions (waiting on @asanderson15, @bmoylan, and @nmiyake)

conjure-go-client/httpclient/response_error_decoder_middleware_test.go, line 116 at r5 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Can you add brief comment to describe the high-level behavior that's being tested here?

Done.

conjure-go-contract/errors/error.go, line 34 at r5 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Comment should be updated now that we're fully embedding another interface (either just omit, or describe that this interface satisfies ParamStorer)

Omitted

conjure-go-contract/errors/generic_error.go, line 83 at r2 (raw file):

Previously, asanderson15 (Adam Anderson) wrote…

Got it, sorry, forgot that SafeParams() didn't copy things. Yeah, if you inlined the contents of the current code and your proposal above, the actual diff is fairly small so fine with either the original or your slightly more efficient proposal in the comment.

Done.

conjure-go-contract/errors/http.go, line 38 at r2 (raw file):

Previously, asanderson15 (Adam Anderson) wrote…

I looked at w.Write and it looks like it should gracefully handle a nil for marshalledError, so I guess this is ok. Worst case, you don't get anything in the response body.

inlined so it's more obvious

conjure-go-contract/errors/unmarshal.go, line 33 at r5 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

If errorNameAccessor is used only here (that is, you have it only to fit the shape that you want), you can just define the struct literal directly:
name := struct{
      Name string `json:"errorName"`
}{}
This is the pattern I usually use if we're doing a partial unmarshal in just one function, as it encapsulates the logic within the function (there's no need to reason about the errorNameAccessor type)

Done.

conjure-go-contract/errors/unmarshal_test.go, line 29 at r5 (raw file):

Previously, asanderson15 (Adam Anderson) wrote…

Is it worth adding a test case or two for totally garbage, unserializable errors (e.g., some plaintext, some nonconforming json, etc.)?

Done. They produce kind of ugly error messages...

bmoylan

Reviewable status: 5 of 18 files reviewed, 10 unresolved discussions (waiting on @asanderson15 and @nmiyake)

conjure-go-client/httpclient/response_error_decoder_middleware.go, line 79 at r2 (raw file):

Previously, asanderson15 (Adam Anderson) wrote…

I'd argue for just making this the default. It seems strictly better to me, though the change of error message might be a little confusing at first. If we wanted to make it look the same for all existing use cases, we could make the fallback error message the same as the current restErrorDecoder one, but don't think that's really necessary.

Done!

bmoylan · 2020-05-06T21:19:13Z

OK @asanderson15 @nmiyake I'm happy with this branch as-is and would appreciate.a last pass.

My last commits contain some updates to the error decoding tests which demonstrate the different permutations of error messages.

asanderson15

Looks good. Could you quickly look over the TODOs and clean up any ones you don't want committed?

nmiyake

Thanks! Given that errors/error handling documentation was a TODO in the README before this change it's probably fine to keep it that way, but would be nice to get that filled out eventually :)

Reviewed 10 of 13 files at r6, 3 of 3 files at r7.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @asanderson15 and @bmoylan)

conjure-go-client/httpclient/response_error_decoder_middleware.go, line 78 at r7 (raw file):

	statusParam := werror.SafeParam("statusCode", resp.StatusCode)

	// TODO(bmoylan): If a byte buffer pool is configured, use it to avoid an allocation.

Can we open an issue and reference it as part of the TODO?

conjure-go-client/httpclient/response_error_decoder_middleware.go, line 89 at r7 (raw file):

	// If JSON, try to unmarshal as conjure error
	isJSON := strings.Contains(resp.Header.Get("Content-Type"), codecs.JSON.ContentType())
	if isJSON {

nit: inline declaration and check to make clear that variable is not needed after scope, and perhaps invert conditional so that happy path is at the higher indentation level?

bmoylan

Updated the readme!

Reviewable status: 17 of 19 files reviewed, 8 unresolved discussions (waiting on @asanderson15 and @nmiyake)

conjure-go-client/httpclient/response_error_decoder_middleware.go, line 78 at r7 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Can we open an issue and reference it as part of the TODO?

#98

conjure-go-client/httpclient/response_error_decoder_middleware.go, line 89 at r7 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

nit: inline declaration and check to make clear that variable is not needed after scope, and perhaps invert conditional so that happy path is at the higher indentation level?

Done.

nmiyake

Thanks for updating the README! Some minor typos, but I think this should be good to go once those are touched up!

Reviewed 2 of 2 files at r8.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @asanderson15 and @bmoylan)

README.md, line 55 at r8 (raw file):

httpclient supports a configurable `ErrorDecoder` for constructing an error given a particular response condition.
The default decoder, `restErrorDecoder`, handles responses with status codes greater than or equal to 400.
If the response as a Content-Type of `application/json`, we attempt to deserialize the body as a [Conjure error](https://github.com/palantir/conjure/blob/master/docs/spec/wire.md#55-conjure-errors).

typo: "response as a" -> "response has a"

README.md, line 59 at r8 (raw file):

A safeParam `statusCode` is added to the error and can be accessed with `httpclient.StatusCodeFromError`.

Error detection can be disabled with the ClientParam `httpclient.WithDisableRestErrors()` or overridded with `httpclient.WithErrorDecoder()`.

overridded -> overridden

nmiyake

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @asanderson15 and @bmoylan)

bmoylan added 2 commits May 4, 2020 15:02

WIP Error type registry

17e2036

Add errorInstanceId as safe param

ed24320

bmoylan requested a review from asanderson15 May 5, 2020 15:08

bmoylan mentioned this pull request May 5, 2020

Add RegisterErrorType API #95

Merged

bmoylan added 5 commits May 5, 2020 09:31

test registration

3ccce28

Move UnmarshalError to its own file, add tests

0de9ed4

Merge branch 'develop' into bm/error-registry

ad0c1b5

add custom error type

27f1816

Add ConjureErrorDecoder

a721f1d

bmoylan commented May 5, 2020

View reviewed changes

asanderson15 reviewed May 5, 2020

View reviewed changes

bmoylan added 3 commits May 5, 2020 14:43

CR1

e64b9bf

CR2

fed83f3

lint

46fd300

bmoylan requested a review from nmiyake May 5, 2020 23:28

asanderson15 reviewed May 6, 2020

View reviewed changes

nmiyake reviewed May 6, 2020

View reviewed changes

bmoylan added 3 commits May 6, 2020 08:33

CR

c2729db

CR

c17ef92

inline serializeError

61ba1b2

bmoylan commented May 6, 2020

View reviewed changes

bmoylan added 2 commits May 6, 2020 14:05

Add conjure handling to default restErrorDecoder

1c13abc

Add generated changelog entries

4b00254

bmoylan marked this pull request as ready for review May 6, 2020 21:13

bmoylan commented May 6, 2020

View reviewed changes

test httpclient.StatusCodeFromError

2acfc5c

bmoylan force-pushed the bm/error-registry branch from faa6c01 to 2acfc5c Compare May 6, 2020 21:19

asanderson15 reviewed May 6, 2020

View reviewed changes

remove todos

60e3b23

nmiyake requested changes May 6, 2020

View reviewed changes

bmoylan mentioned this pull request May 6, 2020

Use BytesBufferPool in restErrorDecoder #98

Open

bmoylan added 2 commits May 6, 2020 16:03

CR

2b6d87d

cr

c140541

bmoylan commented May 6, 2020

View reviewed changes

nmiyake requested changes May 6, 2020

View reviewed changes

typos

be333e3

nmiyake approved these changes May 6, 2020

View reviewed changes

bmoylan merged commit 0ba45f4 into develop May 6, 2020

bmoylan deleted the bm/error-registry branch May 6, 2020 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update errors package & deserialize conjure errors #94

Update errors package & deserialize conjure errors #94

bmoylan commented May 5, 2020 •

edited

changelog-app bot commented May 5, 2020 •

edited by bmoylan

bmoylan May 5, 2020 •

edited

asanderson15 May 6, 2020 •

edited

asanderson15 May 5, 2020

bmoylan May 5, 2020

asanderson15 May 5, 2020

bmoylan May 5, 2020

asanderson15 May 6, 2020

asanderson15 May 5, 2020

bmoylan May 5, 2020

asanderson15 May 5, 2020

bmoylan May 5, 2020

asanderson15 May 6, 2020

asanderson15 May 5, 2020

bmoylan May 5, 2020

asanderson15 May 5, 2020

bmoylan May 5, 2020

asanderson15 May 6, 2020

asanderson15 May 6, 2020

asanderson15 May 6, 2020

nmiyake left a comment

bmoylan left a comment

bmoylan left a comment

bmoylan commented May 6, 2020

asanderson15 left a comment

nmiyake left a comment

bmoylan left a comment

nmiyake left a comment

nmiyake left a comment

Update errors package & deserialize conjure errors #94

Update errors package & deserialize conjure errors #94

Conversation

bmoylan commented May 5, 2020 • edited

changelog-app bot commented May 5, 2020 • edited by bmoylan

Generate changelog in changelog/@unreleased

bmoylan May 5, 2020 • edited

Choose a reason for hiding this comment

asanderson15 May 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmiyake left a comment

Choose a reason for hiding this comment

bmoylan left a comment

Choose a reason for hiding this comment

bmoylan left a comment

Choose a reason for hiding this comment

bmoylan commented May 6, 2020

asanderson15 left a comment

Choose a reason for hiding this comment

nmiyake left a comment

Choose a reason for hiding this comment

bmoylan left a comment

Choose a reason for hiding this comment

nmiyake left a comment

Choose a reason for hiding this comment

nmiyake left a comment

Choose a reason for hiding this comment

bmoylan commented May 5, 2020 •

edited

changelog-app bot commented May 5, 2020 •

edited by bmoylan

Generate changelog in `changelog/@unreleased`

bmoylan May 5, 2020 •

edited

asanderson15 May 6, 2020 •

edited