Small bug fixes, refactors in workflow invocation based on Danny's tests #14054
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new mock implementation for the Changes
Possibly related PRs
Suggested labels
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: 1
Outside diff range and nitpick comments (2)
packages/sdk/__mocks__/simple-oauth2.ts (1)
1-14: LGTM! The mock implementation looks good.The mock correctly simulates the behavior of the
ClientCredentialsclass from thesimple-oauth2library. It provides agetTokenmethod that returns a mock token object with the necessary properties and methods.This setup allows for testing components that depend on the
simple-oauth2library without making actual OAuth requests.Consider adding a way to customize the
access_tokenvalue and theexpiredmethod's return value in the mock. This would allow for more flexible testing scenarios.For example, you could accept optional parameters in the mock implementation to set these values:
jest.mock("simple-oauth2", () => { return { ClientCredentials: jest.fn().mockImplementation((options) => { const { access_token = "mocked-oauth-token", expired = false } = options || {}; return { getToken: jest.fn().mockResolvedValue({ token: { access_token, }, expired: jest.fn().mockReturnValue(expired), }), }; }), }; });This way, you can customize the mock behavior when needed:
const clientCredentials = new ClientCredentials({ access_token: "custom-token", expired: true, });packages/sdk/src/server/__tests__/server.test.ts (1)
266-270: Remove unnecessaryoauth_app_idfield with undefined valueIn the request body of the
connectTokenCreatetest,oauth_app_idis set toundefined. When serializing to JSON withJSON.stringify, properties withundefinedvalues are omitted. Including this field may cause confusion. It's clearer to omit the field entirely when it isn't needed.Apply this diff to remove the unnecessary field:
body: JSON.stringify({ external_id: "user-id", app_slug: "test-app", - oauth_app_id: undefined, }),
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/sdk/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (7)
- packages/sdk/mocks/simple-oauth2.ts (1 hunks)
- packages/sdk/jest.config.js (1 hunks)
- packages/sdk/jest.setup.js (1 hunks)
- packages/sdk/package.json (2 hunks)
- packages/sdk/src/server/tests/server.test.ts (1 hunks)
- packages/sdk/src/server/index.ts (22 hunks)
- packages/sdk/tsconfig.node.json (1 hunks)
Additional context used
Biome
packages/sdk/src/server/index.ts
[error] 466-466: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (31)
packages/sdk/jest.setup.js (2)
1-3: LGTM!The setup file correctly initializes
jest-fetch-mockfor mocking thefetchAPI in Jest tests. This will facilitate easier and more controlled testing of components that rely on thefetchAPI.
3-3: Verify the impact of the globalfetchassignment on the codebase.Assigning the global
fetchvariable to the mock implementation provided byjest-fetch-mockcould potentially affect other parts of the codebase that rely on the originalfetchimplementation.Run the following script to verify the usage of the global
fetchvariable:If there are occurrences of
fetchoutside of test files or setup files, it might indicate that the global assignment could affect the behavior of those parts of the codebase. In such cases, consider using a different approach for mockingfetchin tests, such as using a library likenockormswthat can intercept and mock network requests without modifying the globalfetchvariable.Verification successful
Verified: The global
fetchassignment is confined to test and setup files.No usages of
fetchoutside of test or setup files were found, ensuring that assigningglobal.fetchtofetchMockdoes not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the global `fetch` variable in the codebase. # Test 1: Search for occurrences of `global.fetch` in the codebase. # Expect: Only occurrences in test files or setup files. rg --type js $'global\.fetch' # Test 2: Search for occurrences of `fetch` that are not preceded by `global.` in the codebase. # Expect: No occurrences outside of test files or setup files. rg --type js $'(?<!global\.)fetch'Length of output: 342
Script:
#!/bin/bash # Description: Re-verify occurrences of `fetch` not preceded by `global.` using PCRE2. # Test 2: Search for occurrences of `fetch` that are not preceded by `global.` in the codebase. # Expect: No occurrences outside of test files or setup files. rg --pcre2 --type js '(?<!global\.)fetch'Length of output: 348
packages/sdk/tsconfig.node.json (6)
4-4: Verify compatibility with target JavaScript environments.The change from
"ES6"to"es2020"for thetargetoption is approved. However, please ensure that the target JavaScript environments (browsers, Node.js versions, etc.) support ECMAScript 2020 features.
5-5: LGTM!The change from
["ES6"]to["esnext"]for theliboption is approved. This allows the use of the latest ECMAScript built-in API declarations in the codebase.
11-11: LGTM!The addition of the
allowSyntheticDefaultImportsoption set totrueis approved. This allows the use of default imports from modules that don't have a default export, which is useful when working with CommonJS modules.
12-12: Update module imports, if necessary.The change from
"NodeNext"to"node"for themoduleResolutionoption is approved. However, please ensure that the module imports in the codebase are updated to align with the Node.js CommonJS module resolution strategy, if necessary.
14-14: LGTM!The change from
["node"]to["node", "jest", "jest-fetch-mock"]for thetypesoption is approved. This allows the use of type declarations for Node.js, Jest, and Jest Fetch Mock in the codebase.
3-3: Verify compatibility and update module imports/exports.The change from
"NodeNext"to"esnext"for themoduleoption is approved. However, please ensure the following:
- Verify that the target Node.js version supports the latest ECMAScript features.
- Update the module imports and exports in the codebase to align with the new module code generation, if necessary.
Run the following script to verify the module usage:
packages/sdk/jest.config.js (1)
1-28: LGTM!The Jest configuration file is well-structured and follows the recommended practices for a TypeScript project. The use of
ts-jestpreset, along with the specified test environment, setup file, roots, test match patterns, module file extensions, and transformation rule, provides a solid foundation for running tests effectively.The configuration ensures that TypeScript files are properly transformed during testing, and the setup file allows for any necessary setup or mocking to be performed before running the tests. The roots and test match patterns help Jest locate and run the relevant test files, while the transformation rule ensures that TypeScript files are processed correctly with the specified TypeScript configuration.
Overall, this Jest configuration file is a great addition to the project and should enable smooth testing of the TypeScript codebase.
packages/sdk/package.json (4)
24-26: LGTM!Specifying the required Node.js version using the
"engines"field is a good practice. Setting it to 18.0.0 or higher ensures that the package leverages the latest features and improvements.
38-38: LGTM!Adding the
"build:browser"script improves the build process by generating the browser-specific version of the package using the appropriate TypeScript configuration.
39-39: LGTM!Adding the
"test"script is a great improvement as it enables running tests using Jest. This enhances the testing capabilities of the package and aligns with the PR objective of implementing changes based on tests.
45-51: LGTM!The added development dependencies significantly enhance the testing setup of the package:
@types/fetch-mockand@types/jestprovide type definitions for better type checking and IDE support.jestis a powerful testing framework that enables writing and running tests effectively.jest-fetch-mockallows mockingfetchrequests in tests, which is crucial for testing network-related functionality.ts-jestenables running TypeScript tests with Jest, ensuring type safety in the test suite.These additions align perfectly with the PR objective of implementing changes based on tests conducted by Danny.
packages/sdk/src/server/index.ts (12)
111-115: LGTM!The
AuthTypeenum is correctly defined with appropriate string literal values to represent the authentication types for an app.
227-227: LGTM!The optional
credentialsproperty is correctly added to theAccounttype with the appropriate typeRecord<string, string>. The comment clarifies that the property is included only when theinclude_credentialsparameter is set totrue.
Line range hint
250-269: LGTM!The
RequestOptionsinterface is updated to include thebodyproperty explicitly, allowing various data types such as objects, strings,FormData,URLSearchParams, ornull. This change provides flexibility in specifying the request body.
294-299: LGTM!The properties of the
ServerClientclass are updated to makesecretKey,publicKey, andoauthClientoptional, and theenvironmentproperty is removed. These changes align with the updates made to theCreateServerClientOptstype.
302-323: LGTM!The constructor of the
ServerClientclass is updated to remove theenvironmentproperty and add an optionaloauthClientparameter. The logic for configuring the OAuth client is updated based on the presence of theoauthClientparameter, allowing for providing a pre-configured OAuth client for testing purposes.
355-358: Verify the availability ofpublicKeyandsecretKey.The
connectAuthorizationHeadermethod now directly uses thepublicKeyandsecretKeyproperties without checking for their presence. Please ensure that these properties are always available when the method is called to avoid potential runtime errors.
Line range hint
360-388: LGTM!The
oauthAuthorizationHeadermethod is updated to include retry logic for handling expiredoauthToken. It introduces a maximum number of attempts and a delay between retries to prevent potential infinite loops and avoid rapid retries. If the maximum number of attempts is reached and theoauthTokenis still expired, an appropriate error is thrown.
Line range hint
399-470: LGTM!The
makeRequestmethod is updated to handle different types of request bodies, includingFormData,URLSearchParams, and strings. It introduces aprocessedBodyvariable to store the processed request body, which is then assigned to thebodyproperty of therequestOptionsobject. The error handling is improved to attempt parsing the response as JSON if the content type indicates JSON, otherwise, it falls back to raw text.
474-498: LGTM!The
makeAuthorizedRequestmethod is updated to accept anauthTypeparameter, which determines the type of authorization header to include in the request. It now supports both OAuth and Connect authorization headers based on theauthTypevalue. Theheadersobject is updated to include the appropriate authorization header based on theauthType.
506-518: LGTM!The
makeApiRequestmethod is updated to callmakeAuthorizedRequestwith"oauth"as theauthType. This change simplifies the method by delegating the authorization header inclusion to themakeAuthorizedRequestmethod and ensures that the OAuth authorization header is used for API requests.
520-532: LGTM!The
makeConnectRequestmethod is updated to callmakeAuthorizedRequestwith"connect"as theauthType. This change simplifies the method by delegating the authorization header inclusion to themakeAuthorizedRequestmethod and ensures that the Connect authorization header is used for Connect API requests. Prepending/connectto thepathparameter ensures that the correct API endpoint is called for Connect requests.
Line range hint
699-765: LGTM!The
getProjectInfomethod is added to retrieve project information by making a GET request to the/projects/infoendpoint using themakeConnectRequestmethod. It returns a promise that resolves to the project info response.The
invokeWorkflowmethod is updated to accept abodyparameter as part of theoptsobject. Thebodyparameter is included in the request options when making the request to the workflow URL. TheAuthorizationheader is set using theoauthAuthorizationHeadermethod to include the OAuth access token.packages/sdk/src/server/__tests__/server.test.ts (6)
128-176: Tests formakeApiRequestcorrectly verify OAuth authorization handlingThe tests effectively mock the OAuth client and validate that the
makeApiRequestmethod includes the correct Authorization header. This ensures that API requests are properly authenticated using OAuth tokens.
200-232: Validation of Connect API requests is thoroughThe test for
makeConnectRequestcorrectly checks that the necessary authorization headers are included for Connect API requests. This ensures that requests to the Connect API are properly authenticated.
280-315: Retrieving accounts with query parameters is correctly testedThe
getAccountsmethod test accurately includes query parameters and checks that accounts are retrieved as expected. Passing parameters likeinclude_credentialsensures the API handles query parameters properly.
422-442: Deletion of accounts is properly verifiedThe test for
deleteAccountconfirms that the correct HTTP method (DELETE) is used and that the client handles the 204 No Content response appropriately.
530-588: Workflow invocation test correctly handles OAuth authenticationThe
invokeWorkflowtest ensures that workflows are invoked with the proper URL, body, and headers, including the OAuth token. This verifies that the client can invoke workflows while maintaining valid authentication.
590-653: OAuth token refresh logic is correctly testedThe test for token refreshing accurately simulates an expired token scenario and validates that a new token is fetched and used in subsequent requests. This ensures the client can maintain authentication even when tokens expire.
| * The environment in which the server is running (e.g., "production", "development"). | ||
| * The public API key for accessing the service. | ||
| */ | ||
| environment?: string; |
There was a problem hiding this comment.
This is a weird diff but I was removing environment
| secretKey: "test-secret-key", | ||
| }); | ||
|
|
||
| const result = await client["makeRequest"]("/test-path", { |
There was a problem hiding this comment.
Why do we have to do client["makeRequest"] instead of client.makeRequest? Is it because of mocking? Or because the method is private?
There was a problem hiding this comment.
I'd rather focus the tests on the public interface instead.
There was a problem hiding this comment.
I think it makes sense to make makeRequest public as a mechanism for people making their own Pipedream API requests that aren't covered by the SDK anyway, so I changed that and corrected these tests
| if (authType === "oauth") { | ||
| headers["Authorization"] = await this.oauthAuthorizationHeader(); | ||
| } else { | ||
| headers["Authorization"] = this.connectAuthorizationHeader(); | ||
| } |
There was a problem hiding this comment.
I'm now more convinced that we should split these clients into ConnectClient and ApiClient, WDYT?
We could also rename ServerClient (too verbose IMHO) to just Client, make it abstract, and implement the HTTP calling logic only.
abstract class Client {
private baseUrl: string;
constructor(baseUrl: string) {
this.baseUrl = baseUrl;
}
makeRequest(
method: "GET" | "POST",
path: string,
headers: object,
payload?: unknown,
) {
// Do all the stuff that it does now
}
}
class ConnectClient extends Client {
private publicKey: string;
private secretKey: string;
constructor(publicKey: string, secretKey: string) {
super("https://api.pipedream.com/v1/connect");
this.publicKey = publicKey;
this.privateKey = privateKey;
}
private makeRequest(
method: "GET" | "POST",
path: string,
headers: object,
payload?: unknown,
) {
headers["Authorization"] = this.authorizationHeader();
return super(method, path, headers, payload);
}
}There was a problem hiding this comment.
100%, maybe after we ship all the JWT validation in catcher and Rails we can work on this next sprint
2 participants
Summary by CodeRabbit
New Features
publicKeyandsecretKeyoptional.simple-oauth2library for easier testing.ServerClientclass to ensure expected functionality.Bug Fixes
Documentation