fix: return better error messages for grpc-gateway errors #1397

yquansah · 2023-03-13T20:44:58Z

Currently, we return a very "go" specific type of error messaging when body inputs fail to unmarshal against the protobuf schema:

{
    "code": 3,
    "message": "json: cannot unmarshal bool into Go value of type map[string]json.RawMessage",
    "details": []
}

This error messaging does not really tell users what is going on with their inputs to the API, so this PR serves to change the error messaging to something like the following:

{
    "code": 3,
    "message": "invalid values for key(s) in json body",
    "details": []
}

Fixes: FLI-129
Fixes: FLI-130

codecov-commenter · 2023-03-13T20:48:52Z

Codecov Report

Merging #1397 (40e6d47) into main (1273bed) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main    #1397   +/-   ##
=======================================
  Coverage   78.17%   78.17%           
=======================================
  Files          43       43           
  Lines        3239     3239           
=======================================
  Hits         2532     2532           
  Misses        565      565           
  Partials      142      142

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

markphelps · 2023-03-13T22:53:20Z

Impl looks good to me minus one comment.

Could we add a unit or integration test as well?

GeorgeMac · 2023-03-14T09:38:24Z

rpc/flipt/marshaller.go

+	err := c.Decoder.Decode(v)
+	if err != nil {
+		if _, ok := err.(*json.UnmarshalTypeError); ok {
+			return fmt.Errorf("invalid values for key(s) in json body")


This looks great. One extra suggestion: could we slip in a logger and log line in here?
Something to emit the original error out via logs.

error level is the obvious choice. However, I wonder if this actually makes more sense at a debug level.
Just thinking if someone spams the API with a bad request then debug level might be more forgiving.

@GeorgeMac Agreed, especially if someones client is configured badly. I can emit a debug level log here first than we can adjust if necessary.

💯 sounds great!

yquansah · 2023-03-14T12:58:07Z

@markphelps Definitely. I wanted to see if this was the right direction before I added test. Will add those tests though!

markphelps · 2023-03-13T22:52:13Z

rpc/flipt/marshaller.go

+	err := c.Decoder.Decode(v)
+	if err != nil {
+		if _, ok := err.(*json.UnmarshalTypeError); ok {
+			return fmt.Errorf("invalid values for key(s) in json body")


nit: could prob just use errors.New here since we aren't formatting anything

@markphelps Done! Thanks.

an integration test for the behavior.

markphelps · 2023-03-14T16:00:13Z

@yquansah this should fix both FLI-129 and FLI-130 right?

markphelps

lgtm!! thank you ♥️

GeorgeMac

Wicked 💪

GeorgeMac · 2023-03-15T09:38:31Z

@all-contributors please add @yquansah for code

allcontributors · 2023-03-15T09:38:40Z

@GeorgeMac

I've put up a pull request to add @yquansah! 🎉

* main: (36 commits) feat(sdk/go): thread client token on RPC calls when provided (#1389) docs: update .all-contributorsrc [skip ci] docs: update README.md [skip ci] chore(deps): bump google.golang.org/protobuf in /_tools (#1401) chore(deps): bump google.golang.org/protobuf from 1.29.0 to 1.29.1 (#1400) fix: return better error messages for grpc-gateway errors (#1397) fix: import for cockroach db (#1399) chore(deps): bump golang.org/x/tools from 0.6.0 to 0.7.0 in /_tools (#1394) chore: update readme w better usecase verbiage (#1395) fix(sdk/go): documentation and example corrections chore(deps): bump google.golang.org/protobuf from 1.28.1 to 1.29.0 (#1393) chore(deps): bump google.golang.org/protobuf in /_tools (#1391) chore(sdk/go): fix README markdown style fix(sdk/go): drop support for single field message mangling chore(sdk/go): add README chore(gowork): go edit -fmt feat(sdk/go): add doc.go feat(sdk/go): add static client token provider refactor(sdk/go): move grpc transport into its own subpackage chore(proto): regenerate RPC using protoc-gen-go-grpc v1.3.0 ...

* namespaces: (31 commits) Release/1.19 (#1403) feat(sdk/go): thread client token on RPC calls when provided (#1389) docs: update .all-contributorsrc [skip ci] docs: update README.md [skip ci] chore(deps): bump google.golang.org/protobuf in /_tools (#1401) chore(deps): bump google.golang.org/protobuf from 1.29.0 to 1.29.1 (#1400) fix: return better error messages for grpc-gateway errors (#1397) fix: import for cockroach db (#1399) chore(deps): bump golang.org/x/tools from 0.6.0 to 0.7.0 in /_tools (#1394) chore: update readme w better usecase verbiage (#1395) fix(sdk/go): documentation and example corrections chore(deps): bump google.golang.org/protobuf from 1.28.1 to 1.29.0 (#1393) chore(deps): bump google.golang.org/protobuf in /_tools (#1391) chore(sdk/go): fix README markdown style fix(sdk/go): drop support for single field message mangling chore(sdk/go): add README chore(gowork): go edit -fmt feat(sdk/go): add doc.go feat(sdk/go): add static client token provider refactor(sdk/go): move grpc transport into its own subpackage ...

pull-request-size bot added the size/S label Mar 13, 2023

GeorgeMac reviewed Mar 14, 2023

View reviewed changes

markphelps reviewed Mar 14, 2023

View reviewed changes

pull-request-size bot added size/M and removed size/S labels Mar 14, 2023

yquansah force-pushed the yq-grpc-gateway-error branch from de889c4 to 11ea5d0 Compare March 14, 2023 15:42

yquansah added 2 commits March 14, 2023 10:47

fix: return better error messages for grpc-gateway errors

38cd018

feat: slip logger in to the special marhsaller, and also add

51088be

an integration test for the behavior.

yquansah force-pushed the yq-grpc-gateway-error branch from 11ea5d0 to 51088be Compare March 14, 2023 15:47

yquansah marked this pull request as ready for review March 14, 2023 15:52

yquansah requested a review from a team as a code owner March 14, 2023 15:52

yquansah requested review from markphelps and GeorgeMac March 14, 2023 15:52

markphelps approved these changes Mar 14, 2023

View reviewed changes

GeorgeMac approved these changes Mar 14, 2023

View reviewed changes

Merge branch 'main' into yq-grpc-gateway-error

40e6d47

yquansah added the automerge Used by Kodiak bot to automerge PRs label Mar 14, 2023

kodiakhq bot merged commit 1628eca into main Mar 14, 2023

kodiakhq bot deleted the yq-grpc-gateway-error branch March 14, 2023 17:59

allcontributors bot mentioned this pull request Mar 15, 2023

docs: add yquansah as a contributor for code #1402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: return better error messages for grpc-gateway errors #1397

fix: return better error messages for grpc-gateway errors #1397

yquansah commented Mar 13, 2023 •

edited

Loading

codecov-commenter commented Mar 13, 2023 •

edited

Loading

markphelps commented Mar 13, 2023

GeorgeMac Mar 14, 2023

yquansah Mar 14, 2023

GeorgeMac Mar 14, 2023

yquansah commented Mar 14, 2023

markphelps Mar 13, 2023 •

edited

Loading

yquansah Mar 14, 2023

markphelps commented Mar 14, 2023

markphelps left a comment

GeorgeMac left a comment

GeorgeMac commented Mar 15, 2023

allcontributors bot commented Mar 15, 2023

fix: return better error messages for grpc-gateway errors #1397

fix: return better error messages for grpc-gateway errors #1397

Conversation

yquansah commented Mar 13, 2023 • edited Loading

codecov-commenter commented Mar 13, 2023 • edited Loading

Codecov Report

markphelps commented Mar 13, 2023

GeorgeMac Mar 14, 2023

Choose a reason for hiding this comment

yquansah Mar 14, 2023

Choose a reason for hiding this comment

GeorgeMac Mar 14, 2023

Choose a reason for hiding this comment

yquansah commented Mar 14, 2023

markphelps Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

yquansah Mar 14, 2023

Choose a reason for hiding this comment

markphelps commented Mar 14, 2023

markphelps left a comment

Choose a reason for hiding this comment

GeorgeMac left a comment

Choose a reason for hiding this comment

GeorgeMac commented Mar 15, 2023

allcontributors bot commented Mar 15, 2023

yquansah commented Mar 13, 2023 •

edited

Loading

codecov-commenter commented Mar 13, 2023 •

edited

Loading

markphelps Mar 13, 2023 •

edited

Loading