Skip to content

Je/connect proxy support #15495

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Feb 5, 2025
Merged

Je/connect proxy support #15495

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e560380
+ proxy request
jsendo Jan 14, 2025
37320f8
makeProxyRequest
jsendo Jan 15, 2025
5e45ac5
remove header and body requirements
jsendo Jan 28, 2025
8b8a8b4
Merge branch 'master' into je/connect-proxy-support
jsendo Feb 3, 2025
3ec43b5
clean up
jsendo Feb 4, 2025
1e87d88
Merge branch 'master' into je/connect-proxy-support
jsendo Feb 4, 2025
f73fe79
fix linting
jsendo Feb 4, 2025
9ab1113
change the function header to be more fetch-like
jsendo Feb 5, 2025
76a8ff7
PR feedback
jsendo Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion packages/sdk/src/server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing version update and CHANGELOG

import * as oauth from "oauth4webapi";
import {
Account, BaseClient, type AppInfo, type ConnectTokenResponse,
Account, BaseClient, type AppInfo, type ConnectTokenResponse, type RequestOptions

Check failure on line 7 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Missing trailing comma
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add missing trailing comma in import statement.

Add a trailing comma after RequestOptions to comply with the project's linting rules.

-  Account, BaseClient, type AppInfo, type ConnectTokenResponse, type RequestOptions
+  Account, BaseClient, type AppInfo, type ConnectTokenResponse, type RequestOptions,
📝 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.

Suggested change
Account, BaseClient, type AppInfo, type ConnectTokenResponse, type RequestOptions
Account, BaseClient, type AppInfo, type ConnectTokenResponse, type RequestOptions,
🧰 Tools
🪛 GitHub Check: Lint Code Base

[failure] 7-7:
Missing trailing comma

🪛 ESLint

[error] 7-8: Missing trailing comma.

(comma-dangle)

🪛 GitHub Actions: Pull Request Checks

[error] 7-7: Missing trailing comma

} from "../shared/index.js";
export * from "../shared/index.js";

Expand Down Expand Up @@ -107,6 +107,19 @@
include_credentials?: boolean;
};

/**
* Parameters for the retrieval of an account from the Connect API
*/
export type MakeProxyRequestOpts = {
/**
* Whether to retrieve the account's credentials or not.
*/
method: string;
Copy link
Contributor

@bzwrk bzwrk Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should list all supported methods as a type like get | post | put | delete

headers?: Record<string, string>;
//headers: Record<string, string>;
body?: any

Check failure on line 120 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Unexpected any. Specify a different type
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix type definition issues.

The MakeProxyRequestOpts type definition has several issues:

  1. The JSDoc comment is incorrect (copied from GetAccountByIdOpts)
  2. Contains commented out code
  3. Uses any type which should be properly typed
 /**
- * Parameters for the retrieval of an account from the Connect API
+ * Options for making a proxy request through the Connect API
  */
 export type MakeProxyRequestOpts = {
   /**
-   * Whether to retrieve the account's credentials or not.
+   * The HTTP method to use for the proxy request
    */
   method: string;
+  /**
+   * Headers to include in the proxy request
+   */
   headers?: Record<string, string>;
-  //headers: Record<string, string>;
+  /**
+   * The request body
+   */
-  body?: any
+  body?: Record<string, unknown> | string | FormData | URLSearchParams | null;
 };
📝 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.

Suggested change
/**
* Parameters for the retrieval of an account from the Connect API
*/
export type MakeProxyRequestOpts = {
/**
* Whether to retrieve the account's credentials or not.
*/
method: string;
headers?: Record<string, string>;
//headers: Record<string, string>;
body?: any
};
/**
* Options for making a proxy request through the Connect API
*/
export type MakeProxyRequestOpts = {
/**
* The HTTP method to use for the proxy request
*/
method: string;
/**
* Headers to include in the proxy request
*/
headers?: Record<string, string>;
/**
* The request body
*/
body?: Record<string, unknown> | string | FormData | URLSearchParams | null;
};
🧰 Tools
🪛 GitHub Check: Lint Code Base

[failure] 120-120:
Unexpected any. Specify a different type

🪛 ESLint

[error] 120-120: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)


/**
* Creates a new instance of BackendClient with the provided options.
*
Expand Down Expand Up @@ -374,4 +387,34 @@
method: "GET",
});
}

/**
* Makes a proxy request to a URL with the specified query parameters and options.
*
* @returns A promise resolving to the response from the downstream service
*/
public makeProxyRequest(url: string, query: any, opts: MakeProxyRequestOpts): Promise<any> {

Check failure on line 396 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Unexpected any. Specify a different type

Check failure on line 396 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Unexpected any. Specify a different type
const url64 = btoa(url).replace(/\+/g, "-")
.replace(/\//g, "_")

Check failure on line 398 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Expected indentation of 6 spaces but found 4
.replace(/=+$/, "");

Check failure on line 399 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Expected indentation of 6 spaces but found 4

const headers = opts.headers || {};

const newHeaders = Object.keys(headers).reduce<{ [key: string]: string }>((acc, key) => {
acc[`x-pd-proxy-${key}`] = headers[key];
return acc;
}, {});

const newOpts: RequestOptions = {
method: opts.method,
headers: newHeaders,
params: query,
}

if (opts.body) {
newOpts.body = opts.body
}

return this.makeConnectRequest(`/proxy/${url64}`, newOpts);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety and error handling in proxy request implementation.

The implementation should:

  1. Use proper types instead of any
  2. Add input validation
  3. Handle potential base64 encoding errors
  4. Fix indentation
-  public makeProxyRequest(url: string, query: any, opts: MakeProxyRequestOpts): Promise<any> {
+  public makeProxyRequest(
+    url: string,
+    query: Record<string, string | number | boolean | null>,
+    opts: MakeProxyRequestOpts
+  ): Promise<unknown> {
+    if (!url?.trim()) {
+      throw new Error("URL is required");
+    }

+    let url64: string;
+    try {
       url64 = btoa(url).replace(/\+/g, "-")
-    .replace(/\//g, "_")
-    .replace(/=+$/, "");
+        .replace(/\//g, "_")
+        .replace(/=+$/, "");
+    } catch (error) {
+      throw new Error(`Failed to encode URL: ${error.message}`);
+    }

     const headers = opts.headers || {};

     const newHeaders = Object.keys(headers).reduce<{ [key: string]: string }>((acc, key) => {
       acc[`x-pd-proxy-${key}`] = headers[key];
       return acc;
     }, {});

     const newOpts: RequestOptions = {
       method: opts.method,
       headers: newHeaders,
       params: query,
     }

     if (opts.body) {
       newOpts.body = opts.body
     }

     return this.makeConnectRequest(`/proxy/${url64}`, newOpts);
   }
📝 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.

Suggested change
public makeProxyRequest(url: string, query: any, opts: MakeProxyRequestOpts): Promise<any> {
const url64 = btoa(url).replace(/\+/g, "-")
.replace(/\//g, "_")
.replace(/=+$/, "");
const headers = opts.headers || {};
const newHeaders = Object.keys(headers).reduce<{ [key: string]: string }>((acc, key) => {
acc[`x-pd-proxy-${key}`] = headers[key];
return acc;
}, {});
const newOpts: RequestOptions = {
method: opts.method,
headers: newHeaders,
params: query,
}
if (opts.body) {
newOpts.body = opts.body
}
return this.makeConnectRequest(`/proxy/${url64}`, newOpts);
}
public makeProxyRequest(
url: string,
query: Record<string, string | number | boolean | null>,
opts: MakeProxyRequestOpts
): Promise<unknown> {
if (!url?.trim()) {
throw new Error("URL is required");
}
let url64: string;
try {
url64 = btoa(url).replace(/\+/g, "-")
.replace(/\//g, "_")
.replace(/=+$/, "");
} catch (error: any) {
throw new Error(`Failed to encode URL: ${error.message}`);
}
const headers = opts.headers || {};
const newHeaders = Object.keys(headers).reduce<{ [key: string]: string }>((acc, key) => {
acc[`x-pd-proxy-${key}`] = headers[key];
return acc;
}, {});
const newOpts: RequestOptions = {
method: opts.method,
headers: newHeaders,
params: query,
}
if (opts.body) {
newOpts.body = opts.body;
}
return this.makeConnectRequest(`/proxy/${url64}`, newOpts);
}
🧰 Tools
🪛 GitHub Check: Lint Code Base

[failure] 396-396:
Unexpected any. Specify a different type

[failure] 396-396:
Unexpected any. Specify a different type

[failure] 398-398:
Expected indentation of 6 spaces but found 4

[failure] 399-399:
Expected indentation of 6 spaces but found 4

🪛 ESLint

[error] 396-396: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)

[error] 396-396: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)

[error] 398-398: Expected indentation of 6 spaces but found 4.

(indent)

[error] 399-399: Expected indentation of 6 spaces but found 4.

(indent)

}
2 changes: 1 addition & 1 deletion packages/sdk/src/shared/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,8 +995,8 @@ export abstract class BaseClient {
};

return this.makeRequest(path, {
headers,
...opts,
headers,
});
}

Expand Down
Loading