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.

Sign up for GitHub

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

feat: Custom User-Agent strings #221

Closed
wants to merge 1 commit into from
Closed

feat: Custom User-Agent strings #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions clerk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ type BackendConfig struct {
// URL is the base URL to use for API endpoints.
// If it's not set, the default value for the Backend will be used.
URL *string
// UserAgentSuffix is appended to the User-Agent string on every
// request. Useful for support or debugging purposes.
UserAgentSuffix *string
}

// NewBackend returns a default backend implementation with the
Expand All @@ -160,8 +163,9 @@ func NewBackend(config *BackendConfig) Backend {
config.URL = String(APIURL)
}
return &defaultBackend{
HTTPClient: config.HTTPClient,
URL: *config.URL,
HTTPClient: config.HTTPClient,
URL: *config.URL,
UserAgentSuffix: config.UserAgentSuffix,
}
}

Expand Down Expand Up @@ -194,8 +198,9 @@ func SetBackend(b Backend) {
}

type defaultBackend struct {
HTTPClient *http.Client
URL string
HTTPClient *http.Client
URL string
UserAgentSuffix *string
}

// Call sends requests to the Clerk API and handles the responses.
Expand All @@ -219,7 +224,11 @@ func (b *defaultBackend) newRequest(ctx context.Context, apiReq *APIRequest) (*h
}
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", secretKey))
req.Header.Add("Content-Type", "application/json")
req.Header.Add("User-Agent", fmt.Sprintf("Clerk/%s SDK-Go/%s", clerkAPIVersion, sdkVersion))
userAgent := fmt.Sprintf("Clerk/%s SDK-Go/%s", clerkAPIVersion, sdkVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Just mentioning that Cloudflare analytics group by user agent, but I guess we can always filter by prefix!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I didn't know that. Do you think it's better to drop this change.

An alternative solution would be to leave the User-Agent string as is and maybe send a custom header instead. Would that be better?

Copy link
Member

Choose a reason for hiding this comment

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

@georgepsarakis Can't we just use the X-Clerk-SDK header?

Copy link
Member Author

Choose a reason for hiding this comment

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

The X-Clerk-SDK is already used, but we could add another one for debugging purposes, or to uniquely identify a consumer, like one of our partners or us.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, what I had in mind was to let the user set the User-Agent to whatever, not just the suffix, and rely on the X-Clerk-SDK for wherever we need to identify if it came from the SDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. That could work. However, I think we should have control over the User-Agent header, since it's a "standard" header but be loose in what we accept for custom ones.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit skeptical of this capability in general. Feels a bit weird to want to hijack the user agent header of an SDK. Do we have any requests about this? If not, then I'd go for what you proposed here @gkats:

An alternative solution would be to leave the User-Agent string as is and maybe send a custom header instead.

i.e., let the user add whatever extra headers they want in their requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

@agis we are following this approach in for BAPI requests by setting the request user-agent to {packageName}@{packageVersion}.

ref: https://github.com/clerk/javascript/blob/main/packages/backend/src/constants.ts#L4

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's an alternative, which uses a custom header. #252

if b.UserAgentSuffix != nil {
userAgent = fmt.Sprintf("%s (%s)", userAgent, *b.UserAgentSuffix)
}
req.Header.Add("User-Agent", userAgent)
req.Header.Add("X-Clerk-SDK", fmt.Sprintf("go/%s", sdkVersion))
req = req.WithContext(ctx)

Expand Down
16 changes: 10 additions & 6 deletions clerk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,18 @@ func TestNewBackend(t *testing.T) {
assert.Equal(t, APIURL, withDefaults.URL)

u := "https://some.other.url"
uaSuffix := "clerk.com"
httpClient := &http.Client{}
config := &BackendConfig{
URL: &u,
HTTPClient: httpClient,
URL: &u,
HTTPClient: httpClient,
UserAgentSuffix: &uaSuffix,
}
withOverrides, ok := NewBackend(config).(*defaultBackend)
require.True(t, ok)
assert.Equal(t, u, withOverrides.URL)
assert.Equal(t, httpClient, withOverrides.HTTPClient)
assert.Equal(t, uaSuffix, *withOverrides.UserAgentSuffix)
}

func TestGetBackend_DataRace(t *testing.T) {
Expand Down Expand Up @@ -155,10 +158,10 @@ func TestBackendCall_RequestHeaders(t *testing.T) {
// The client sets the Authorization header correctly.
assert.Equal(t, fmt.Sprintf("Bearer %s", secretKey), r.Header.Get("Authorization"))
// The client sets the User-Agent header.
assert.Equal(t, "Clerk/v1 SDK-Go/v2.0.0", r.Header.Get("User-Agent"))
assert.Equal(t, fmt.Sprintf("Clerk/v1 SDK-Go/%s (clerk.com)", sdkVersion), r.Header.Get("User-Agent"))
georgepsarakis marked this conversation as resolved.
Show resolved Hide resolved
assert.Equal(t, "application/json", r.Header.Get("Content-Type"))
// The client includes a custom header with the SDK version.
assert.Equal(t, "go/v2.0.0", r.Header.Get("X-Clerk-SDK"))
assert.Equal(t, fmt.Sprintf("go/%s", sdkVersion), r.Header.Get("X-Clerk-SDK"))

_, err := w.Write([]byte(`{}`))
require.NoError(t, err)
Expand All @@ -167,8 +170,9 @@ func TestBackendCall_RequestHeaders(t *testing.T) {

// Set up a mock backend which triggers requests to our test server above.
SetBackend(NewBackend(&BackendConfig{
HTTPClient: ts.Client(),
URL: &ts.URL,
HTTPClient: ts.Client(),
URL: &ts.URL,
UserAgentSuffix: String("clerk.com"),
}))

// Simulate usage for an API operation on a testResource.
Expand Down
Loading