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

Replace grpcstatus.Errorf with connect.Error #741

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Conversation

chacha912
Copy link
Contributor

What this PR does / why we need it:

Previously, there was an issue in Dashboard where errors were not being properly distinguished as ConnectError. To address this problem, I replaced the usage of gRPC status error with connect.NewError.

  • as-is:
    image
  • to-be:
    image

Which issue(s) this PR fixes:

Related #703

Special notes for your reviewer:

I'm unsure about how to validate this change in the test code, as it is currently passing test using the existing approach.

t.Run("authorization webhook test", func(t *testing.T) {
ctx := context.Background()
authServer, token := newAuthServer(t)
// project with authorization webhook
project.AuthWebhookURL = authServer.URL
_, err := adminCli.UpdateProject(
ctx,
project.ID.String(),
&types.UpdatableProjectFields{
AuthWebhookURL: &project.AuthWebhookURL,
},
)
assert.NoError(t, err)
// client with token
cli, err := client.Dial(
svr.RPCAddr(),
client.WithAPIKey(project.PublicKey),
client.WithToken(token),
)
assert.NoError(t, err)
defer func() { assert.NoError(t, cli.Close()) }()
assert.NoError(t, cli.Activate(ctx))
defer func() { assert.NoError(t, cli.Deactivate(ctx)) }()
doc := document.New(helper.TestDocKey(t))
assert.NoError(t, cli.Attach(ctx, doc))
// client without token
cliWithoutToken, err := client.Dial(
svr.RPCAddr(),
client.WithAPIKey(project.PublicKey),
)
assert.NoError(t, err)
defer func() { assert.NoError(t, cliWithoutToken.Close()) }()
err = cliWithoutToken.Activate(ctx)
assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err))
// client with invalid token
cliWithInvalidToken, err := client.Dial(
svr.RPCAddr(),
client.WithAPIKey(project.PublicKey),
client.WithToken("invalid"),
)
assert.NoError(t, err)
defer func() { assert.NoError(t, cliWithInvalidToken.Close()) }()
err = cliWithInvalidToken.Activate(ctx)
assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err))
})

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@chacha912 chacha912 requested review from hackerwins and krapie January 1, 2024 12:50
Copy link

codecov bot commented Jan 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (34d960b) 49.27% compared to head (f85034a) 49.25%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #741      +/-   ##
==========================================
- Coverage   49.27%   49.25%   -0.02%     
==========================================
  Files          69       69              
  Lines       10081    10084       +3     
==========================================
  Hits         4967     4967              
- Misses       4598     4601       +3     
  Partials      516      516              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

Seems like I have forgot to update some of error handling codes on interceptors. Thanks for the updates 👍🏼

I'm unsure about how to validate this change in the test code, as it is currently passing test using the existing approach.

auth_webhook_test.go does not validates admin interceptor. Since this code is only related to admin operations, you need to add new test on admin_test.go.

it will look something like this:

t.Run("authorization test", func(t *testing.T) {
	// try to do admin operation without authorization.
	adminCli2, err := admin.Dial(defaultServer.RPCAddr(), admin.WithInsecure(true))
	assert.NoError(t, err)
	defer func() {
		adminCli2.Close()
	}()
	_, err = adminCli2.GetProject(ctx, "default")
	assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err))
})

Co-authored-by: Yourim Cha <chacha912@users.noreply.github.com>
Co-authored-by: Youngteac Hong <susukang98@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Jan 3, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. 👍

@hackerwins hackerwins merged commit 0cb7cec into main Jan 3, 2024
3 checks passed
@hackerwins hackerwins deleted the connect-error branch January 3, 2024 10:34
@hackerwins hackerwins changed the title Fix Issue with Non-ConnectError Replace grpcstatus.Errorf with connect.Error Jan 3, 2024
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.

5 participants