(feat) Removing appSlug and oauthClientId from token create call #14123
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
Warning Rate limit exceeded@dylburger has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
docs-v2/pages/connect/quickstart.mdx (6)
188-218: LGTM! Consider adding type hints for better code clarity.The simplification of the
connect_token_createmethod aligns well with the PR objectives. The removal ofapp_slugandoauth_app_idparameters streamlines the API.Consider adding type hints to improve code clarity:
- def connect_token_create(self, opts): + def connect_token_create(self, opts: dict) -> dict:
Line range hint
252-275: LGTM! Consider enhancing exception handling.The simplification of the
connectTokenCreatemethod aligns well with the PR objectives. The removal ofappSlugandoauthClientIdparameters streamlines the API.Consider enhancing the exception handling to provide more specific error information:
- public String connectTokenCreate(String externalId) throws Exception { + public String connectTokenCreate(String externalId) throws IOException { // ... (existing code) + if (conn.getResponseCode() != HttpURLConnection.HTTP_OK) { + throw new IOException("HTTP error code: " + conn.getResponseCode()); + } return new String(conn.getInputStream().readAllBytes(), StandardCharsets.UTF_8); }
Line range hint
306-323: LGTM! Consider adding error handling for non-success status codes.The simplification of the
ConnectTokenCreatemethod aligns well with the PR objectives. The removal ofappSlugandoauthClientIdparameters streamlines the API.Consider adding error handling for non-success status codes:
public async Task<string> ConnectTokenCreate(string externalId) { // ... (existing code) var response = await client.PostAsync($"{baseURL}/v1/connect/tokens", content); + response.EnsureSuccessStatusCode(); return await response.Content.ReadAsStringAsync(); }
Line range hint
363-402: LGTM! Consider enhancing error handling.The simplification of the
ConnectTokenCreatemethod aligns well with the PR objectives. The removal ofappSlugandoauthClientIdparameters streamlines the API.Consider enhancing the error handling to provide more specific error information:
if err != nil { return nil, err } + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode) + } defer resp.Body.Close() body, _ := ioutil.ReadAll(resp.Body)
Line range hint
433-467: LGTM! Consider adding error handling for HTTP errors.The simplification of the
connectTokenCreatemethod aligns well with the PR objectives. The removal ofappSlugandoauthClientIdparameters streamlines the API.Consider adding error handling for HTTP errors:
$context = stream_context_create($options); - $result = file_get_contents($url, false, $context); + $result = @file_get_contents($url, false, $context); + if ($result === FALSE) { + $error = error_get_last(); + throw new Exception("HTTP request failed. Error: " . $error['message']); + } return json_decode($result, true);
Line range hint
497-514: LGTM! Consider adding error handling for non-success HTTP status codes.The simplification of the
connect_token_createmethod aligns well with the PR objectives. The removal ofapp_slugandoauth_app_idparameters streamlines the API.Consider adding error handling for non-success HTTP status codes:
res = Net::HTTP.start(uri.hostname, uri.port, use_ssl: true) do |http| http.request(req) end + res.value # Raises an exception for non-2xx status codes JSON.parse(res.body)docs-v2/pages/connect/api.mdx (3)
Line range hint
21-41: LGTM with a minor suggestion: Clear SDK installation instructionsThe installation instructions for both npm and <script> tag methods are well-presented and easy to follow.
Consider specifying a version number in the "latest version" example for the <script> tag method, or add a comment explaining that it will always fetch the latest version. This could help users understand the behavior more clearly. For example:
<!-- This will always fetch the latest version --> <script src="https://unpkg.com/@pipedream/sdk/dist/browser/index.js"></script>
Line range hint
43-108: LGTM with a suggestion: Comprehensive authentication instructionsThe authentication section provides clear instructions and code examples for both the TypeScript SDK (Server and Browser) and the REST API.
In the TypeScript SDK (Server) example, consider adding a note or example about securely handling API keys. For instance:
// Load API keys from environment variables const pd = createClient({ publicKey: process.env.PIPEDREAM_PUBLIC_KEY, secretKey: process.env.PIPEDREAM_SECRET_KEY, });This emphasizes the importance of not hardcoding sensitive information and using environment variables instead.
Line range hint
131-570: LGTM with a suggestion: Comprehensive API Reference for token creationThe API Reference section, particularly the "Tokens" subsection, provides detailed information about creating new tokens. The multi-language examples are a great addition, making it easy for developers using different programming languages to implement token creation.
In the TypeScript (Next.js) example, consider adding error handling to make the code more robust. For instance:
export async function serverConnectTokenCreate(opts: ConnectTokenCreateOpts): Promise<ConnectAPIResponse<ConnectTokenResponse>> { try { return await pd.connectTokenCreate(opts); } catch (error) { console.error("Error creating connect token:", error); throw error; // or handle the error as appropriate for your application } }This addition would help developers handle potential errors more gracefully in their applications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- docs-v2/pages/connect/api.mdx (12 hunks)
- docs-v2/pages/connect/quickstart.mdx (12 hunks)
- packages/sdk/src/server/index.ts (1 hunks)
🔇 Additional comments (9)
packages/sdk/src/server/index.ts (3)
Line range hint
52-67: LGTM: ConnectTokenCreateOpts type updated correctlyThe
ConnectTokenCreateOptstype has been successfully updated to remove theapp_slugandoauth_app_idproperties, aligning with the PR objectives. The addition of optionalsuccess_redirect_urianderror_redirect_uriproperties enhances flexibility in handling connection outcomes.
Line range hint
555-563: LGTM: connectTokenCreate method simplified and aligned with type changesThe
connectTokenCreatemethod has been successfully updated to reflect the changes in theConnectTokenCreateOptstype. The implementation now correctly spreads theoptsobject into the request body, maintaining backwards compatibility by mappingexternal_user_idtoexternal_id. This change simplifies the token creation process as intended.
Line range hint
52-67: Summary: Successful simplification of token creation processThe changes in this file successfully implement the PR objectives by simplifying the token creation process. The
ConnectTokenCreateOptstype andconnectTokenCreatemethod have been updated to removeapp_slugandoauth_app_id, while adding optional redirect URIs. These changes streamline the API usage and provide more flexibility for handling connection outcomes.The implementation maintains backwards compatibility by mapping
external_user_idtoexternal_idin the request body. Overall, these modifications enhance the usability of the SDK without introducing breaking changes.Also applies to: 555-563
docs-v2/pages/connect/quickstart.mdx (2)
Line range hint
1-1: LGTM! The usage example is consistent with the API changes.The TypeScript code snippet correctly demonstrates the simplified
serverConnectTokenCreatemethod call, using only theexternal_idparameter. This is consistent with the changes made across other language implementations.
Line range hint
1-1: Overall, the changes successfully simplify the Connect API across multiple languages.The modifications to the
connectTokenCreatemethod (and its equivalents in different languages) consistently remove theappSlugandoauthClientIdparameters, leaving only theexternal_idas the required parameter. This simplification aligns well with the PR objectives and improves the API's usability.The documentation has been updated accurately to reflect these changes across all provided language examples. This consistency will help developers understand and implement the simplified API correctly, regardless of their preferred programming language.
While the core changes are solid, consider implementing the suggested minor improvements in error handling and type hinting to further enhance the code quality and developer experience.
docs-v2/pages/connect/api.mdx (4)
Line range hint
1-13: LGTM: Well-structured introductionThe introductory section provides a clear overview of the Connect API & SDK Reference, including necessary imports and a helpful warning about the Base URL and authentication method.
Line range hint
15-19: LGTM: Clear presentation of REST API Base URLThe REST API Base URL is clearly presented in a code block, making it easy for developers to identify and use.
Line range hint
110-118: LGTM: Clear explanation of external usersThe "External users" section provides a concise explanation of the
external_idparameter and its usage. The mention of the 250-character limit is a helpful detail for developers.
Line range hint
120-129: LGTM: Well-presented rate limit informationThe "Rate limits" section clearly presents the rate limits for different API endpoints in a table format. The note about contacting support for higher rate limits is a helpful addition for users with more demanding requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/sdk/src/server/tests/server.test.ts (1 hunks)
🔇 Additional comments (1)
packages/sdk/src/server/__tests__/server.test.ts (1)
258-260: LGTM! Changes align with PR objectives.The modifications to the
connectTokenCreatetest accurately reflect the removal ofapp_slugandoauth_app_idparameters, which is consistent with the PR's goal of streamlining the token creation process.
| external_user_id: "user-id", | ||
| external_id: "user-id", | ||
| app_slug: "test-app", | ||
| oauth_app_id: undefined, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test cases for new optional parameters.
While the current changes look good, it would be beneficial to add test cases for the new optional parameters success_redirect_uri and error_redirect_uri. This will ensure comprehensive coverage of the updated connectTokenCreate method functionality.
Here's a suggested additional test case:
it("should create a connect token with optional redirect URIs", async () => {
fetchMock.mockResponseOnce(
JSON.stringify({
token: "connect-token-with-redirects",
expires_at: "2024-01-01T00:00:00Z",
}),
{
headers: {
"Content-Type": "application/json",
},
},
);
client = new ServerClient({
publicKey: "test-public-key",
secretKey: "test-secret-key",
});
const result = await client.connectTokenCreate({
external_user_id: "user-id",
success_redirect_uri: "https://example.com/success",
error_redirect_uri: "https://example.com/error",
});
expect(result).toEqual({
token: "connect-token-with-redirects",
expires_at: "2024-01-01T00:00:00Z",
});
expect(fetchMock).toHaveBeenCalledWith(
"https://api.pipedream.com/v1/connect/tokens",
expect.objectContaining({
method: "POST",
body: JSON.stringify({
external_user_id: "user-id",
external_id: "user-id",
success_redirect_uri: "https://example.com/success",
error_redirect_uri: "https://example.com/error",
}),
headers: expect.objectContaining({
"Authorization": expect.any(String),
"Content-Type": "application/json",
}),
}),
);
});…p-from-token-create-request
dylburger
deleted the
connect/removing-app-from-token-create-request
branch
1 participant
Summary by CodeRabbit
New Features
external_id.Documentation
These changes enhance the user experience by simplifying the connection process and providing clearer guidance in the documentation.