feat: update license handshake to use rateLimitTier#692
feat: update license handshake to use rateLimitTier#692
Conversation
- Rename Client.licenseType field to rateLimitTier - Update FetchLicense() to parse rateLimitTier from JSON response - Rename SetLicenseType() to SetRateLimitTier() - Update addDescopeHeaders() to send tier value in x-descope-license header - Update handshake code in client.go to use new method names - Update all tests: tier values (tier1/tier2/tier4) instead of license type names - All 9 license-related tests passing Ref: descope/etc#14245
|
✅ Code review completed successfully |
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK to use rateLimitTier instead of licenseType in the license handshake mechanism and header injection, aligning with a backend API change. The change affects how the SDK fetches and stores license information during client initialization.
Changes:
- Renamed field and method from
licenseType/SetLicenseType()torateLimitTier/SetRateLimitTier()throughout the codebase - Updated JSON response parsing to expect
rateLimitTierfield from the license endpoint - Updated all tests to use tier values (tier1-tier4) instead of named license types
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| descope/client/client.go | Updated client initialization to use renamed FetchLicense() return value and SetRateLimitTier() method |
| descope/api/client.go | Renamed struct field from licenseType to rateLimitTier, updated JSON parsing, and renamed setter method |
| descope/api/client_test.go | Updated all test cases to use tier values (tier1-tier4) and renamed test methods/variables for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if rateLimitTier, err := mgmtClient.FetchLicense(ctx); err != nil { | ||
| logger.LogInfo("License handshake failed, continuing without header: %v", err) | ||
| } else { | ||
| mgmtClient.SetLicenseType(licenseType) | ||
| mgmtClient.SetRateLimitTier(rateLimitTier) |
There was a problem hiding this comment.
The test file descope/client/client_test.go at line 140 still uses the old licenseType field format in the mock response. This test should be updated to use rateLimitTier instead to match the updated API contract. The mock response currently returns {"licenseType":"enterprise"} but should return {"rateLimitTier":"tier4"} to be consistent with the changes in this PR.
![shuni-bot-dev[bot]](https://avatars.githubusercontent.com/u/97479186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Straightforward rename from licenseType to rateLimitTier in the license handshake and header injection. Clean mechanical change — but one test mock was left behind.
Sniffed out 2 issues:
- 1 🟠 HIGH:
descope/client/client_test.go:140—TestFetchLicenseSuccessmock still returns{"licenseType":"enterprise"}butFetchLicense()now parsesrateLimitTier. The JSON fieldlicenseTypeis silently ignored, soresp.RateLimitTieris empty, causingFetchLicenseto return an error. The test still passes becauseNewWithConfighandles that error gracefully (log + continue), but the test now validates the failure path, not success. The mock should be{"rateLimitTier":"tier4"}. - 1 🟢 LOW: Struct field alignment off by one space
See inline comments for details. Needs a bath!
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| if licenseType, err := mgmtClient.FetchLicense(ctx); err != nil { | ||
| if rateLimitTier, err := mgmtClient.FetchLicense(ctx); err != nil { |
There was a problem hiding this comment.
🟠 HIGH: The corresponding test TestFetchLicenseSuccess in client_test.go:140 still returns the old JSON format:
Body: io.NopCloser(strings.NewReader(`{"licenseType":"enterprise"}`))Since FetchLicense() now parses rateLimitTier, this mock response yields an empty string, causing FetchLicense to return an error ("empty rate limit tier returned from server"). The test still passes because this line gracefully handles the error — but TestFetchLicenseSuccess is now silently testing the failure/graceful-degradation path, not the success path. The license header will never be set.
Fix: update client_test.go:140 mock to {"rateLimitTier":"tier4"}.
| Conf ClientParams | ||
| sdkInfo *sdkInfo | ||
| licenseType string // License type from handshake (free/pro/enterprise) | ||
| rateLimitTier string // Rate limit tier from handshake (tier1/tier2/tier3/tier4) |
There was a problem hiding this comment.
🟢 LOW: Minor alignment inconsistency — other fields in this struct align types at column 18 (after tab). rateLimitTier has 4 spaces (column 17) instead of 5 (column 18).
| rateLimitTier string // Rate limit tier from handshake (tier1/tier2/tier3/tier4) | |
| rateLimitTier string // Rate limit tier from handshake (tier1/tier2/tier3/tier4) |
Summary
Update SDK to use
rateLimitTierfield instead oflicenseTypein license handshake and header injection.Changes
License Handshake
FetchLicense()to parserateLimitTierfrom responseSetLicenseType()→SetRateLimitTier()Client.licenseTypefield →Client.rateLimitTierHeader Injection
x-descope-licenseheadertier1,tier2,tier3, ortier4Tests
Tier Values
tier1- Free licensetier2- Pro licensetier3- Growth licensetier4- Enterprise licenseTesting
go test ./descope/...All 39 tests passing.
Backward Compatibility
Old SDK versions will fail handshake gracefully (log + continue) when backend is updated. No breaking changes for existing deployments.
Related
Part of rate limit tier implementation (descope/etc#14245)