Fixes DJ-2470 #14106
Fixes DJ-2470 #14106
Conversation
Adding invokeWorkflowForExternalUser
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe changes in this pull request involve a comprehensive update to the SDK, specifically transitioning from 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: 5
🧹 Outside diff range and nitpick comments (5)
packages/sdk/src/server/__tests__/server.test.ts (2)
551-554: InconsistentsecretKeyvalue in test initializationIn this test setup, the
secretKeyis set to"test", whereas in other tests it's set to"test-secret"or"test-secret-key". For consistency and to avoid confusion, consider using a uniform mock value forsecretKeyacross all tests.
626-629: InconsistentsecretKeyvalue in test initializationSimilar to a previous test, the
secretKeyhere is set to"test"instead of a consistent value like"test-secret-key". Aligning mock values across tests improves readability and reduces potential confusion.packages/sdk/src/server/index.ts (3)
10-13: Add JSDoc comments for theProjectKeystypeConsider adding JSDoc comments to the
ProjectKeystype to describe its purpose and usage. This will enhance code readability and help other developers understand how to use this type effectively.
15-18: Add JSDoc comments for thePipedreamOAuthClienttypeAdding JSDoc comments to the
PipedreamOAuthClienttype will improve maintainability and provide clarity on its intended use within the SDK.
842-844: Enhance error message for missing project public keyThe error message when
this.publicKeyis not set could be more informative:if (!this.publicKey) { throw new Error("Project public key is required to map the external user ID to the correct project"); }Consider providing guidance on how to supply the
publicKey, helping developers resolve the issue quickly.Update the error message to:
throw new Error( - "Project public key is required to map the external user ID to the correct project" + "Project public key is required. Please provide it via the 'project' option in 'CreateServerClientOpts'." );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/sdk/src/server/tests/server.test.ts (4 hunks)
- packages/sdk/src/server/index.ts (4 hunks)
🔇 Additional comments not posted (2)
packages/sdk/src/server/__tests__/server.test.ts (2)
667-762: Comprehensive tests forinvokeWorkflowForExternalUserThe new test suite for
invokeWorkflowForExternalUserthoroughly covers expected functionality and error handling scenarios, including header inclusion and validation of required parameters. This enhances the reliability of the code.
25-35: Verify all usages ofcreateClientare updated to the new signatureWhile this test confirms that
createClientworks with the newprojectparameter structure, it's important to ensure that all instances ofcreateClientin the codebase have been updated to match this new signature to prevent runtime errors.Run the following script to find usages of
createClientthat may need updating:✅ Verification successful
All usages of
createClienthave been verified to use the newprojectparameter structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `createClient` not using the new `project` parameter structure. # Test: Search for calls to `createClient` that don't include the `project` object. rg --type typescript --pcre2 'createClient\(\s*\{(?!.*project\s*:).*\}\s*\)'Length of output: 113
Script:
#!/bin/bash # Description: Find all usages of `createClient` not using the new `project` parameter structure. # Corrected Test: Search for calls to `createClient` that don't include the `project` object. rg --glob '*.ts' --glob '*.tsx' --pcre2 'createClient\(\s*\{(?!.*project\s*:).*\}\s*\)'Length of output: 89
| const client = new ServerClient( | ||
| { | ||
| publicKey: "test-public-key", | ||
| secretKey: "test-secret-key", | ||
| oauthClientId: "test-client-id", | ||
| oauthClientSecret: "test-client-secret", | ||
| project: { | ||
| publicKey: "test-public-key", | ||
| secretKey: "test", | ||
| }, | ||
| oauth: { | ||
| clientId: "test-client-id", | ||
| clientSecret: "test-client-secret", | ||
| }, | ||
| }, | ||
| oauthClientMock, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated ServerClient initialization
The initialization of ServerClient with mocked OAuth tokens in this test is repeated in other tests (e.g., lines 626-638 and 673-707). To improve maintainability and reduce code duplication, consider refactoring this setup into a shared helper function or utilizing a common beforeEach hook where appropriate.
packages/sdk/src/server/index.ts
Outdated
| public async invokeWorkflowForExternalUser(url: string, externalUserId: string, opts: RequestOptions = {}): Promise<unknown> { | ||
| const { | ||
| body, | ||
| headers = {}, | ||
| } = opts; | ||
|
|
||
| if (!externalUserId) { | ||
| throw new Error("External user ID is required"); | ||
| } | ||
|
|
||
| if (!this.publicKey) { | ||
| throw new Error("Project public key is required to map the external user ID to the correct project"); | ||
| } | ||
|
|
||
| return this.makeRequest("", { | ||
| ...opts, | ||
| baseURL: url, | ||
| method: opts.method || "POST", // Default to POST if not specified | ||
| headers: { | ||
| ...headers, | ||
| "Authorization": await this.oauthAuthorizationHeader(), | ||
| "X-PD-External-User-ID": externalUserId, | ||
| "X-PD-Project-Public-Key": this.publicKey, | ||
| }, | ||
| body, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the url parameter
In the invokeWorkflowForExternalUser method, the url parameter is used without validation. To prevent runtime errors and enhance security, consider validating the url to ensure it is a well-formed URL.
Add URL validation:
public async invokeWorkflowForExternalUser(
url: string,
externalUserId: string,
opts: RequestOptions = {},
): Promise<unknown> {
+ try {
+ new URL(url);
+ } catch {
+ throw new Error(`Invalid URL provided: ${url}`);
+ }
const {
body,
headers = {},
} = opts;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async invokeWorkflowForExternalUser(url: string, externalUserId: string, opts: RequestOptions = {}): Promise<unknown> { | |
| const { | |
| body, | |
| headers = {}, | |
| } = opts; | |
| if (!externalUserId) { | |
| throw new Error("External user ID is required"); | |
| } | |
| if (!this.publicKey) { | |
| throw new Error("Project public key is required to map the external user ID to the correct project"); | |
| } | |
| return this.makeRequest("", { | |
| ...opts, | |
| baseURL: url, | |
| method: opts.method || "POST", // Default to POST if not specified | |
| headers: { | |
| ...headers, | |
| "Authorization": await this.oauthAuthorizationHeader(), | |
| "X-PD-External-User-ID": externalUserId, | |
| "X-PD-Project-Public-Key": this.publicKey, | |
| }, | |
| body, | |
| }); | |
| } | |
| public async invokeWorkflowForExternalUser(url: string, externalUserId: string, opts: RequestOptions = {}): Promise<unknown> { | |
| try { | |
| new URL(url); | |
| } catch { | |
| throw new Error(`Invalid URL provided: ${url}`); | |
| } | |
| const { | |
| body, | |
| headers = {}, | |
| } = opts; | |
| if (!externalUserId) { | |
| throw new Error("External user ID is required"); | |
| } | |
| if (!this.publicKey) { | |
| throw new Error("Project public key is required to map the external user ID to the correct project"); | |
| } | |
| return this.makeRequest("", { | |
| ...opts, | |
| baseURL: url, | |
| method: opts.method || "POST", // Default to POST if not specified | |
| headers: { | |
| ...headers, | |
| "Authorization": await this.oauthAuthorizationHeader(), | |
| "X-PD-External-User-ID": externalUserId, | |
| "X-PD-Project-Public-Key": this.publicKey, | |
| }, | |
| body, | |
| }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/sdk/src/server/index.ts (1)
798-851: NewinvokeWorkflowForExternalUsermethod enhances API functionalityThe new method provides a valuable addition to the API, allowing invocation of workflows for specific external users. The implementation is well-structured and includes proper error checking.
Suggestion for improvement:
Consider adding input validation for theurlparameter to ensure it's a valid URL. This can help prevent runtime errors and enhance security.Example implementation:
public async invokeWorkflowForExternalUser( url: string, externalUserId: string, opts: RequestOptions = {} ): Promise<unknown> { try { new URL(url); } catch { throw new Error(`Invalid URL provided: ${url}`); } // ... rest of the method implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/sdk/src/server/index.ts (4 hunks)
🔇 Additional comments not posted (2)
packages/sdk/src/server/index.ts (2)
10-18: New type definitions enhance code structure and type safetyThe introduction of
ProjectKeysandPipedreamOAuthClienttypes improves code organization and provides better type safety for project-related and OAuth-related data structures.
26-57: Improved structure and deprecation notices inCreateServerClientOptsThe changes to
CreateServerClientOptsenhance the configuration structure by introducingprojectandoauthproperties, while properly deprecating the older properties. This approach facilitates a smooth transition to the new structure while maintaining backward compatibility.
packages/sdk/src/server/index.ts
Outdated
| if (opts.project) { | ||
| this.publicKey = opts.project.publicKey; | ||
| this.secretKey = opts.project.secretKey; | ||
| } |
There was a problem hiding this comment.
Consider clarifying precedence of new vs. deprecated properties
The current implementation overwrites the deprecated properties with the new ones from the project object. This behavior might lead to unexpected results if both sets of properties are provided.
Consider the following improvements:
- Add a warning log when both sets of properties are provided.
- Implement a clear precedence rule, e.g., prefer the new
projectobject over deprecated properties. - Document this behavior in the class or method documentation.
Example implementation:
if (opts.project) {
if (this.publicKey || this.secretKey) {
console.warn('Both deprecated and new project properties provided. Using new project properties.');
}
this.publicKey = opts.project.publicKey;
this.secretKey = opts.project.secretKey;
}
packages/sdk/src/server/index.ts
Outdated
| if (!oauth || !id || !secret) { | ||
| return; | ||
| } | ||
|
|
||
| const client = { | ||
| id: id ?? oauth.clientId, | ||
| secret: secret ?? oauth.clientSecret, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Fix logical error in OAuth client configuration
The current implementation has a logical error that prevents the OAuth client from being configured when only the new oauth object is provided. To address this issue and ensure proper configuration, please apply the following changes:
-if (!oauth || !id || !secret) {
+if (!(oauth || (id && secret))) {
return;
}
const client = {
- id: id ?? oauth.clientId,
- secret: secret ?? oauth.clientSecret,
+ id: oauth?.clientId ?? id,
+ secret: oauth?.clientSecret ?? secret,
};This change ensures that the OAuth client is configured correctly when either the new oauth object or the deprecated properties are provided.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!oauth || !id || !secret) { | |
| return; | |
| } | |
| const client = { | |
| id: id ?? oauth.clientId, | |
| secret: secret ?? oauth.clientSecret, | |
| }; | |
| if (!(oauth || (id && secret))) { | |
| return; | |
| } | |
| const client = { | |
| id: oauth?.clientId ?? id, | |
| secret: oauth?.clientSecret ?? secret, | |
| }; |
Correcting external user ID method. Correcting API request base method to handle authType
dylburger
changed the title
packages/sdk/src/browser/index.ts
Outdated
|
|
||
| /** | ||
| * A unique identifier for an app. | ||
| * The name slug for an app, a unique, human-readable identifier like "github" or "google_sheets". |
There was a problem hiding this comment.
|
|
||
| describe("ServerClient", () => { | ||
| let client: ServerClient; | ||
| let client: ServerClient; |
There was a problem hiding this comment.
Moving this up to the beforeEach block. Previously I'd been duplicating the client in many tests (see removal below)
| const getTokenMock = jest.fn().mockResolvedValue({ | ||
| token: { | ||
| access_token: "mocked-oauth-token", | ||
| it("should invoke a workflow with provided URL and body, with no auth type", async () => { |
There was a problem hiding this comment.
Happy path — default invokeWorkflow has an HTTPAuthType of None
|
|
||
| // Inject the mock oauthClient into the ServerClient instance | ||
| client = new ServerClient( | ||
| it("should invoke a workflow with OAuth auth type", async () => { |
There was a problem hiding this comment.
New test for OAuth
| ); | ||
| }); | ||
|
|
||
| it("should invoke a workflow with static bearer auth type", async () => { |
There was a problem hiding this comment.
New test for static bearer token (the expectation is that the customer passes this in themselves in the Authorization header, as we'll document
| }); | ||
| }); | ||
|
|
||
| describe("invokeWorkflowForExternalUser", () => { |
There was a problem hiding this comment.
New tests for invokeWorkflowForExternalUser
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pipedream-sdk-markdown-lint.yaml (1)
17-20: Consider adding configuration file for markdownlint.While the current setup works, consider adding a
.markdownlint.jsonor.markdownlint.yamlconfiguration file to customize the linting rules specific to your project's needs.Would you like me to help generate a base configuration file with recommended rules for your SDK documentation?
packages/sdk/src/server/index.ts (1)
751-846: Consider implementing rate limiting for workflow invocations.The workflow invocation methods could benefit from rate limiting to prevent abuse and ensure fair usage of resources.
Consider implementing a rate limiter using a token bucket algorithm:
interface RateLimitOptions { maxRequests: number; timeWindow: number; // in milliseconds } class RateLimiter { private tokens: number; private lastRefill: number; constructor(private options: RateLimitOptions) { this.tokens = options.maxRequests; this.lastRefill = Date.now(); } async acquire(): Promise<void> { this.refill(); if (this.tokens <= 0) { throw new Error('Rate limit exceeded'); } this.tokens--; } private refill(): void { const now = Date.now(); const timePassed = now - this.lastRefill; const refillAmount = Math.floor(timePassed / this.options.timeWindow * this.options.maxRequests); this.tokens = Math.min(this.options.maxRequests, this.tokens + refillAmount); this.lastRefill = now; } }This can be integrated into the workflow invocation methods to enforce rate limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/pipedream-sdk-markdown-lint.yaml(1 hunks)packages/sdk/src/server/index.ts(18 hunks)
🔇 Additional comments (1)
packages/sdk/src/server/index.ts (1)
10-60: LGTM! Well-structured type definitions and enums.
The new types and enums are well-documented and follow TypeScript best practices. The HTTPAuthType enum values are consistent with the backend implementation.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
.github/workflows/pipedream-sdk-markdown-lint.yaml (1)
21-24: Consider adding a markdownlint configuration file.While the linting action is correctly configured, consider adding a
.markdownlint.jsonor.markdownlint.yamlconfiguration file to ensure consistent linting rules across the SDK documentation.Example configuration file:
{ "default": true, "MD013": false, # Line length "MD033": false, # Inline HTML "MD041": false # First line in file should be a top level heading }packages/sdk/CONTRIBUTING.md (3)
16-19: Add comma for better readabilityAdd a comma after "tool" to improve sentence structure.
-If you prefer to use another tool make sure you use the same versions that are +If you prefer to use another tool, make sure you use the same versions that are🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: A comma might be missing here.
Context: ...stall ``` If you prefer to use another tool make sure you use the same versions tha...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
23-31: Consider mentioning root dependenciesThe instructions should mention that there are dependencies in the root
package.jsonthat might need to be installed first. This ensures a complete setup process.Consider adding:
Since other packages in this repository already use `pnpm`, we recommend you use it in this case too to keep your `node_modules` footprint low. + +Note: You may also need to install dependencies from the root `package.json`. Run +`pnpm install` from the root directory first.
34-45: Add context about build artifactsConsider adding information about:
- What artifacts are generated
- Where to find them
- Common troubleshooting steps
There's a script that you can invoke with `pnpm` to build the package artifacts: ```shell pnpm build
+This will generate the compiled artifacts in the
dist/directory. If you
+encounter any build errors, ensure all dependencies are installed correctly.You can also watch the code for changes, and automatically build a new artifact
after each change with thewatchscript:</blockquote></details> <details> <summary>packages/sdk/src/server/__tests__/server.test.ts (2)</summary><blockquote> `165-184`: **Consider testing additional OAuth error scenarios.** The OAuth token retrieval failure test only covers the "Invalid credentials" case. Consider adding tests for other common OAuth failure scenarios like network errors or token endpoint failures. --- `727-733`: **Consider additional validation test cases.** While testing for empty `externalUserId` is good, consider adding tests for: - null/undefined `externalUserId` - whitespace-only `externalUserId` - extremely long `externalUserId` values </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a97736d1c7a01c31afef0ff9ffab5debba5273b7 and 6596349521e92baac1341b84a950835e0a1033f5. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (6)</summary> * `.github/workflows/pipedream-sdk-markdown-lint.yaml` (1 hunks) * `.github/workflows/pipedream-sdk-test.yaml` (1 hunks) * `packages/sdk/CONTRIBUTING.md` (1 hunks) * `packages/sdk/src/server/__tests__/server.test.ts` (23 hunks) * `packages/sdk/src/server/index.ts` (18 hunks) * `packages/sdk/tsconfig.node.json` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * packages/sdk/tsconfig.node.json </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>packages/sdk/CONTRIBUTING.md</summary> [uncategorized] ~16-~16: A comma might be missing here. Context: ...stall ``` If you prefer to use another tool make sure you use the same versions tha... (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA) </details> </details> </details> <details> <summary>🔇 Additional comments (6)</summary> <details> <summary>.github/workflows/pipedream-sdk-markdown-lint.yaml (3)</summary> `3-8`: **LGTM! Well-configured triggers and path filters.** The workflow is correctly configured to run only when relevant Markdown files in the SDK package are modified. --- `9-11`: **LGTM! Effective concurrency configuration.** The concurrency settings will properly manage simultaneous workflow runs, canceling outdated ones. --- `6-7`: **Verify the existence of SDK documentation files.** Let's ensure there are markdown files to lint in the specified path. <details> <summary>✅ Verification successful</summary> **Markdown files exist in the SDK package - workflow path pattern is valid** The verification confirms the existence of multiple markdown files (`README.md`, `CHANGELOG.md`, `CONTRIBUTING.md`) in the `packages/sdk/` directory, which matches the workflow's path pattern `'packages/sdk/**/*.md'`. The workflow will correctly lint these documentation files. </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify the existence of markdown files in the SDK package # Expected: List of markdown files that will be linted by this workflow echo "Checking for markdown files in SDK package..." fd -e md . packages/sdk/Length of output: 205
.github/workflows/pipedream-sdk-test.yaml (1)
9-11: LGTM! Good addition of concurrency controls.
The concurrency configuration is well-implemented, helping to optimize CI resources by preventing redundant runs and automatically canceling outdated ones when new commits are pushed.
packages/sdk/src/server/__tests__/server.test.ts (2)
685-692: Duplicate client initialization detected.
Similar to the previous comment, this beforeEach block also duplicates client initialization logic.
736-816: LGTM: Comprehensive URL building test coverage.
The URL building tests are well-structured and cover various scenarios including:
- Input validation
- Protocol handling
- Path handling
- Custom domain support
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
💡 Codebase verification
Update actions/checkout to v4.1.7 for version consistency
Most workflows in the repository are using actions/checkout@v4.1.7, while this workflow and pipedream-sdk-markdown-lint.yaml are using the less specific actions/checkout@v4. For consistency across the codebase, update the version to v4.1.7.
.github/workflows/pipedream-sdk-test.yaml: Updateactions/checkout@v4toactions/checkout@v4.1.7.github/workflows/pipedream-sdk-markdown-lint.yaml: Updateactions/checkout@v4toactions/checkout@v4.1.7
🔗 Analysis chain
Verify compatibility with actions/checkout@v4.
The upgrade to actions/checkout@v4 is a good practice for keeping dependencies current. However, as this is a major version update, please ensure all workflow features remain compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other workflows using actions/checkout to ensure consistent versioning
# Test: Search for checkout action usage. Expect: All workflows should use v4 for consistency
echo "Checking for inconsistent versions of actions/checkout across workflows..."
rg -g "*.ya?ml" "uses: actions/checkout@" .github/workflows/
Length of output: 221
Script:
#!/bin/bash
# Let's try with a different approach using fd to find yaml files first
echo "GitHub workflow files in the repository:"
fd -e yml -e yaml . .github/workflows/
echo -e "\nChecking checkout action usage in these files:"
fd -e yml -e yaml . .github/workflows/ -x grep -l "actions/checkout@" {} \;
echo -e "\nDetailed checkout action versions in use:"
fd -e yml -e yaml . .github/workflows/ -x grep -H "actions/checkout@" {} \;
Length of output: 2284
| # Replace /path/to/pipedream with the actual path to this repository | ||
| grep nodejs /path/to/pipedream/packages/sdk/.tool-versions >> .tool-versions | ||
| asdf install | ||
| pnpm install @pipedream/sdk |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider automating Node.js version detection
The manual path replacement and version copying could be error-prone. Consider providing a script to automate this process.
-# Replace /path/to/pipedream with the actual path to this repository
-grep nodejs /path/to/pipedream/packages/sdk/.tool-versions >> .tool-versions
-asdf install
+# Create a setup script (e.g., setup.sh):
+#!/bin/bash
+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+SDK_DIR=$(find "$SCRIPT_DIR" -name "packages/sdk" -type d | head -n 1)
+if [ -n "$SDK_DIR" ]; then
+ grep nodejs "$SDK_DIR/.tool-versions" >> .tool-versions
+ asdf install
+else
+ echo "Error: Could not locate SDK directory"
+ exit 1
+fiCommittable suggestion skipped: line range outside the PR's diff.
packages/sdk/src/server/index.ts
Outdated
| export enum HTTPAuthType { | ||
| None = "none", | ||
| StaticBearer = "static_bearer_token", | ||
| OAuth = "oauth" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using const enum for better type safety.
The HTTPAuthType enum could benefit from being a const enum to ensure values are inlined at compile time and prevent runtime manipulation.
-export enum HTTPAuthType {
+export const enum HTTPAuthType {
None = "none",
StaticBearer = "static_bearer_token",
OAuth = "oauth"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export enum HTTPAuthType { | |
| None = "none", | |
| StaticBearer = "static_bearer_token", | |
| OAuth = "oauth" | |
| } | |
| export const enum HTTPAuthType { | |
| None = "none", | |
| StaticBearer = "static_bearer_token", | |
| OAuth = "oauth" | |
| } |
| public async invokeWorkflowForExternalUser( | ||
| url: string, | ||
| externalUserId: string, | ||
| opts: RequestOptions = {}, | ||
| ): Promise<unknown> { | ||
| const { headers = {} } = opts; | ||
|
|
||
| if (!externalUserId) { | ||
| throw new Error("External user ID is required"); | ||
| } | ||
|
|
||
| if (!this.oauthClient) { | ||
| throw new Error("OAuth is required for invoking workflows for external users. Please pass credentials for a valid OAuth client"); | ||
| } | ||
|
|
||
| return this.invokeWorkflow(url, { | ||
| ...opts, | ||
| headers: { | ||
| ...headers, | ||
| "Authorization": await this.oauthAuthorizationHeader(), | ||
| "X-PD-External-User-ID": externalUserId, | ||
| }, | ||
| body, | ||
| }); | ||
| }, HTTPAuthType.OAuth); // OAuth auth is required for invoking workflows for external users |
There was a problem hiding this comment.
Enhance error handling in workflow invocation.
The invokeWorkflowForExternalUser method could benefit from additional validation and error handling.
public async invokeWorkflowForExternalUser(
url: string,
externalUserId: string,
opts: RequestOptions = {},
): Promise<unknown> {
const { headers = {} } = opts;
- if (!externalUserId) {
+ if (!externalUserId?.trim()) {
throw new Error("External user ID is required");
}
+ if (!url?.trim()) {
+ throw new Error("Workflow URL is required");
+ }
if (!this.oauthClient) {
throw new Error("OAuth is required for invoking workflows for external users. Please pass credentials for a valid OAuth client");
}
+ try {
return this.invokeWorkflow(url, {
...opts,
headers: {
...headers,
"X-PD-External-User-ID": externalUserId,
},
}, HTTPAuthType.OAuth);
+ } catch (error) {
+ throw new Error(`Failed to invoke workflow for external user: ${error instanceof Error ? error.message : String(error)}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async invokeWorkflowForExternalUser( | |
| url: string, | |
| externalUserId: string, | |
| opts: RequestOptions = {}, | |
| ): Promise<unknown> { | |
| const { headers = {} } = opts; | |
| if (!externalUserId) { | |
| throw new Error("External user ID is required"); | |
| } | |
| if (!this.oauthClient) { | |
| throw new Error("OAuth is required for invoking workflows for external users. Please pass credentials for a valid OAuth client"); | |
| } | |
| return this.invokeWorkflow(url, { | |
| ...opts, | |
| headers: { | |
| ...headers, | |
| "Authorization": await this.oauthAuthorizationHeader(), | |
| "X-PD-External-User-ID": externalUserId, | |
| }, | |
| body, | |
| }); | |
| }, HTTPAuthType.OAuth); // OAuth auth is required for invoking workflows for external users | |
| public async invokeWorkflowForExternalUser( | |
| url: string, | |
| externalUserId: string, | |
| opts: RequestOptions = {}, | |
| ): Promise<unknown> { | |
| const { headers = {} } = opts; | |
| if (!externalUserId?.trim()) { | |
| throw new Error("External user ID is required"); | |
| } | |
| if (!url?.trim()) { | |
| throw new Error("Workflow URL is required"); | |
| } | |
| if (!this.oauthClient) { | |
| throw new Error("OAuth is required for invoking workflows for external users. Please pass credentials for a valid OAuth client"); | |
| } | |
| try { | |
| return this.invokeWorkflow(url, { | |
| ...opts, | |
| headers: { | |
| ...headers, | |
| "X-PD-External-User-ID": externalUserId, | |
| }, | |
| }, HTTPAuthType.OAuth); // OAuth auth is required for invoking workflows for external users | |
| } catch (error) { | |
| throw new Error(`Failed to invoke workflow for external user: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/sdk/src/server/index.ts (1)
352-373: Enhance OAuth error handling with specific error types.The OAuth client initialization could benefit from more specific error handling to help users troubleshoot common OAuth issues.
Apply this change:
private newOauthClient( { clientId, clientSecret }: OAuthCredentials, tokenHost: string, ) { if (!clientId || !clientSecret) { - throw new Error("OAuth client ID and secret are required"); + throw new Error("OAuth configuration error: Both client ID and secret are required"); } const client = { id: clientId, secret: clientSecret, }; - return new ClientCredentials({ - client, - auth: { - tokenHost, - tokenPath: "/v1/oauth/token", - }, - }); + try { + return new ClientCredentials({ + client, + auth: { + tokenHost, + tokenPath: "/v1/oauth/token", + }, + }); + } catch (error) { + throw new Error(`OAuth initialization failed: ${error instanceof Error ? error.message : 'Unknown error'}`); + }packages/sdk/src/server/__tests__/server.test.ts (4)
23-41: Consider extracting client initialization into a helper function.The client initialization logic is duplicated across multiple test blocks. Despite previous suggestions, this pattern continues throughout the file.
function createTestClient(options: Partial<BackendClientOpts> = {}) { return new BackendClient({ ...clientParams, ...options, }); } beforeEach(() => { setupOAuthMocks(); client = createTestClient(); customDomainClient = createTestClient({ workflowDomain: "example.com" }); });
Line range hint
616-682: Add comments explaining the OAuth token lifecycle test.While the test is well-structured, adding comments to explain the token lifecycle stages (initial token acquisition, expiration check, refresh) would improve maintainability.
describe("OAuth Token Handling", () => { it("should refresh token when expired", async () => { + // Stage 1: Setup initial expired token const expiredTokenMock = { token: { access_token: "expired-oauth-token", }, expired: jest.fn().mockReturnValue(true), }; + // Stage 2: Setup refresh token response const newTokenMock = { token: { access_token: "new-oauth-token", }, expired: jest.fn().mockReturnValue(false), }; + // Stage 3: Configure token lifecycle (expired -> refreshed) const getTokenMock = jest .fn() .mockResolvedValueOnce(expiredTokenMock) .mockResolvedValueOnce(newTokenMock);
684-750: Consider adding environment-specific test cases.The tests validate the presence of environment headers but don't verify behavior across different environments (e.g., development, staging).
Add test cases for different environments:
it("should support different environments", async () => { const environments = ["development", "staging", "production"]; for (const env of environments) { const client = new BackendClient({ ...clientParams, workflowDomain: "example.com", environment: env, }); // Test implementation const result = await client.invokeWorkflowForExternalUser( "https://example.com/workflow", "external-user-id", { body: { foo: "bar" } } ); expect(fetchMock).toHaveBeenCalledWith( expect.any(String), expect.objectContaining({ headers: expect.objectContaining({ "X-PD-Environment": env, }), }) ); } });
752-832: Add test cases for URL edge cases.While the current test coverage is good, consider adding tests for:
- URLs with query parameters
- URLs with fragments
- URLs with special characters in the path
describe("Edge cases", () => { it("should handle URLs with query parameters", () => { const input = "https://en123.example.com/foo?key=value"; const expected = "https://en123.example.com/foo?key=value"; expect(customDomainClient["buildWorkflowUrl"](input)).toBe(expected); }); it("should handle URLs with fragments", () => { const input = "https://en123.example.com/foo#section"; const expected = "https://en123.example.com/foo#section"; expect(customDomainClient["buildWorkflowUrl"](input)).toBe(expected); }); it("should handle URLs with encoded characters", () => { const input = "https://en123.example.com/foo%20bar"; const expected = "https://en123.example.com/foo%20bar"; expect(customDomainClient["buildWorkflowUrl"](input)).toBe(expected); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/sdk/package.json(3 hunks)packages/sdk/src/server/__tests__/server.test.ts(23 hunks)packages/sdk/src/server/index.ts(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/package.json
🧰 Additional context used
🪛 Biome
packages/sdk/src/server/index.ts
[error] 56-60: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
[error] 140-144: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
🔇 Additional comments (3)
packages/sdk/src/server/index.ts (3)
665-741: LGTM! Robust URL validation implementation.
The URL building logic includes:
- Proper input sanitization
- Protection against DNS rebinding attacks
- Comprehensive validation of endpoint ID format
- Clear error messages
775-813: LGTM! Well-implemented workflow invocation methods.
The implementation includes:
- Proper input validation
- Clear auth type handling
- Comprehensive error messages
- Proper external user ID validation
Also applies to: 848-872
334-349: 🛠️ Refactor suggestion
Add validation for environment parameter.
The constructor accepts any string for the environment parameter. Consider restricting it to specific valid environments to prevent configuration errors.
Apply this change:
constructor(opts: BackendClientOpts) {
- this.environment = opts.environment ?? "production";
+ const validEnvironments = ["production", "development"] as const;
+ const environment = opts.environment ?? "production";
+ if (environment && !validEnvironments.includes(environment as typeof validEnvironments[number])) {
+ throw new Error(`Invalid environment. Must be one of: ${validEnvironments.join(", ")}`);
+ }
+ this.environment = environment;Likely invalid or redundant comment.
| export const enum HTTPAuthType { | ||
| None = "none", | ||
| StaticBearer = "static_bearer_token", | ||
| OAuth = "oauth" | ||
| } |
There was a problem hiding this comment.
Convert const enums to regular enums to ensure compatibility.
The const enum usage in HTTPAuthType and AppAuthType can cause compatibility issues with bundlers and TypeScript's isolatedModules mode.
Apply these changes:
-export const enum HTTPAuthType {
+export enum HTTPAuthType {
None = "none",
StaticBearer = "static_bearer_token",
OAuth = "oauth"
}
-export const enum AppAuthType {
+export enum AppAuthType {
OAuth = "oauth",
Keys = "keys",
None = "none",
}Also applies to: 140-144
🧰 Tools
🪛 Biome
[error] 56-60: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
@pipedream/sdk 1.0.0 --------- Co-authored-by: Jay Vercellone <jay@pipedream.com> Co-authored-by: Jay Vercellone <jverce@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
WHY
Summary by CodeRabbit
New Features
BackendClientclass with updated methods for improved API interactions.Bug Fixes
Documentation
Chores