Skip to content

ledger: wrap response errors with HTTP metadata#4

Open
adamdecaf wants to merge 1 commit into
RealImage:masterfrom
moov-io:log-request-errors
Open

ledger: wrap response errors with HTTP metadata#4
adamdecaf wants to merge 1 commit into
RealImage:masterfrom
moov-io:log-request-errors

Conversation

@adamdecaf

Copy link
Copy Markdown

I found enhancing the error helps to debug problems. I don't consider the URL (i.e. account ID) sensitive since it would end up in load balancer logs.

@sudhirj

sudhirj commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Looks fine to me, @ilayaraja89 ?

@ilayaraja89-zz

Copy link
Copy Markdown
Contributor

@sudhirj @adamdecaf Client applications might want to differentiate between 4xx and 5xx responses from Ledger. But now both are generalised as error, skipping the switch case of http status codes.

https://github.com/RealImage/qledger-sdk-go/blob/master/transactions.go#L39
https://github.com/RealImage/qledger-sdk-go/blob/master/transactions.go#L48

@adamdecaf

Copy link
Copy Markdown
Author

With Go's current error handling the best people can do is match on == or strings.Contains(err.Error(), ...) to determine which error was returned. This change only breaks strict equality, ==.

@ilayaraja89-zz

Copy link
Copy Markdown
Contributor

I think, in client applications, comparing against variables like ErrTransactionDuplicate, ErrAccountConflict will be more handy compared to inspecting error texts.

Eg. https://github.com/RealImage/qledger-sdk-go/blob/master/transactions_test.go#L108

@adamdecaf Can you also check whether tests pass after this change ?

@adamdecaf

Copy link
Copy Markdown
Author

They don't pass. This change would break the comparison you're talking about. I'm ok closing this change if it won't work.

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.

3 participants