-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add explicit handling for rate-limit errors #1033
Changes from 5 commits
f78e918
1be05e6
ad11cb8
cdbdf7d
49089c3
4af4a79
6009111
ad87f37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
export class NoApiKeyProvidedException extends Error { | ||
readonly status: number = 500; | ||
readonly name: string = 'NoApiKeyProvidedException'; | ||
readonly message: string = | ||
readonly status = 500; | ||
readonly name = 'NoApiKeyProvidedException'; | ||
readonly message = | ||
`Missing API key. Pass it to the constructor (new WorkOS("sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU")) ` + | ||
`or define it in the WORKOS_API_KEY environment variable.`; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { RequestException } from '../interfaces/request-exception.interface'; | ||
|
||
export class RateLimitExceededException | ||
extends Error | ||
implements RequestException | ||
{ | ||
readonly name = 'RateLimitExceededException'; | ||
readonly status = 429; | ||
|
||
constructor( | ||
readonly message: string, | ||
readonly requestID: string, | ||
/** | ||
* The number of seconds to wait before retrying the request. | ||
*/ | ||
readonly retryAfter: number | null, | ||
) { | ||
super(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export interface RequestException { | ||
readonly status: number; | ||
readonly message: string; | ||
readonly requestID: string; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import { | |
OauthException, | ||
} from './common/exceptions'; | ||
import { WorkOS } from './workos'; | ||
import { RateLimitExceededException } from './common/exceptions/rate-limit-exceeded.exception'; | ||
|
||
describe('WorkOS', () => { | ||
beforeEach(() => fetch.resetMocks()); | ||
|
@@ -198,6 +199,7 @@ describe('WorkOS', () => { | |
); | ||
}); | ||
}); | ||
|
||
describe('when the api responds with a 400 and an error/error_description', () => { | ||
it('throws an OauthException', async () => { | ||
fetchOnce( | ||
|
@@ -222,6 +224,30 @@ describe('WorkOS', () => { | |
}); | ||
}); | ||
|
||
describe('when the api responses with a 429', () => { | ||
it('throws a RateLimitExceededException', async () => { | ||
fetchOnce( | ||
{ | ||
message: 'Too many requests', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this verbatim what we return from the API? I'd like there to be more information in here, like a link to the rate limiting docs or a callout to reach out on support@ if they want to discuss higher rate limits. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is, and agreed that we should have more information in the |
||
}, | ||
{ | ||
status: 429, | ||
headers: { 'X-Request-ID': 'a-request-id', 'Retry-After': '10' }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is 10 seconds the default or is that just for this test? If it's the default returned by the API then that feels way too high. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just a test value. IIRC, the actual |
||
}, | ||
); | ||
|
||
const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU'); | ||
|
||
await expect(workos.get('/path')).rejects.toStrictEqual( | ||
new RateLimitExceededException( | ||
'Too many requests', | ||
'a-request-id', | ||
10, | ||
), | ||
); | ||
}); | ||
}); | ||
|
||
describe('when the entity is null', () => { | ||
it('sends a null body', async () => { | ||
fetchOnce(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import { | |
UnauthorizedException, | ||
UnprocessableEntityException, | ||
OauthException, | ||
RateLimitExceededException, | ||
} from './common/exceptions'; | ||
import { | ||
GetOptions, | ||
|
@@ -214,6 +215,15 @@ export class WorkOS { | |
requestID, | ||
}); | ||
} | ||
case 429: { | ||
const retryAfter = headers.get('Retry-After'); | ||
|
||
throw new RateLimitExceededException( | ||
data.message, | ||
requestID, | ||
retryAfter ? Number(retryAfter) : null, | ||
); | ||
} | ||
Comment on lines
+218
to
+226
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a breaking change? Existing users may be doing an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, one option would be to make Though I'd like to hear opinions before I make that change. EDIT: It might be a breaking change either way since even if the base class stays the same, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strongly prefer having While someone out there might be relying on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made the minimal change of only having the new exception inherit from |
||
default: { | ||
if (error || errorDescription) { | ||
throw new OauthException( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my initial pass I realized the new
RateLimitExceededException
was missingstatus
. It seemed worth having something like this in order for all exception classes to be roughly shaped the same.Arguable we should have a base-class instead, but I opted for an
interface
for now since it only adds an additional compile-time check, and we can decide later.