chore: Fix datasource creation throwing 400 #34588
Conversation
WalkthroughThis update to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DatasourcesApi
Client->>DatasourcesApi: call cleanAuthenticationObject(authentication)
DatasourcesApi->>DatasourcesApi: Determine authenticationType
alt dbAuth, oAuth2, basic, apiKey, bearerToken, snowflakeKeyPairAuth
DatasourcesApi->>DatasourcesApi: Sanitize authentication data
end
DatasourcesApi-->>Client: Return sanitized authentication object
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9713514957. |
|
Deploy-Preview-URL: https://ce-34588.dp.appsmith.com |
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)
- app/client/src/api/DatasourcesApi.ts (2 hunks)
Additional context used
Biome
app/client/src/api/DatasourcesApi.ts
[error] 24-240: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 53-53: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (2)
app/client/src/api/DatasourcesApi.ts (2)
Line range hint
24-240: Consider refactoring to a simple module.The class
DatasourcesApionly contains static methods, which can be a sign that a class structure is not necessary. Consider refactoring to a simple module with functions to simplify the structure and enhance modularity.
[REFACTOR_SUGGESTion]- class DatasourcesApi { - static url = "v1/datasources"; - // static methods - } + const DatasourcesApiUrl = "v1/datasources"; + // Export functions directly
73-130: Review ofcleanAuthenticationObjectmethod.The method appears to be well-implemented with a clear switch-case structure for different authentication types. However, ensure that all fields are necessary and securely handled, especially sensitive fields like passwords and keys.
Consider adding more comments to explain the rationale behind the choices of fields to retain for each authentication type, enhancing maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
app/client/src/api/DatasourcesApi.ts (1)
Line range hint
24-240: Consider refactoring the class into a module.The class
DatasourcesApionly contains static methods, which might be better structured as a module. This can enhance clarity and reduce the overhead associated with class syntax.Consider converting
DatasourcesApiinto a module with exported functions. This approach can simplify the structure and usage of these API methods.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/api/DatasourcesApi.ts (2 hunks)
Additional context used
Biome
app/client/src/api/DatasourcesApi.ts
[error] 24-240: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| static cleanAuthenticationObject(authentication: any): any { | ||
| if (!authentication) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const clean: any = { | ||
| authenticationType: authentication.authenticationType ?? "dbAuth", | ||
| }; | ||
|
|
||
| switch (clean.authenticationType) { | ||
| case "dbAuth": | ||
| clean.authType = authentication.authType; | ||
| clean.username = authentication.username; | ||
| clean.password = authentication.password; | ||
| clean.databaseName = authentication.databaseName; | ||
| break; | ||
| case "oAuth2": | ||
| clean.grantType = authentication.grantType; | ||
| clean.isTokenHeader = authentication.isTokenHeader; | ||
| clean.isAuthorizationHeader = authentication.isAuthorizationHeader; | ||
| clean.clientId = authentication.clientId; | ||
| clean.clientSecret = authentication.clientSecret; | ||
| clean.authorizationUrl = authentication.authorizationUrl; | ||
| clean.expiresIn = authentication.expiresIn; | ||
| clean.accessTokenUrl = authentication.accessTokenUrl; | ||
| clean.scopeString = authentication.scopeString; | ||
| clean.scope = authentication.scope; | ||
| clean.sendScopeWithRefreshToken = | ||
| authentication.sendScopeWithRefreshToken; | ||
| clean.refreshTokenClientCredentialsLocation = | ||
| authentication.refreshTokenClientCredentialsLocation; | ||
| clean.headerPrefix = authentication.headerPrefix; | ||
| clean.customTokenParameters = authentication.customTokenParameters; | ||
| clean.audience = authentication.audience; | ||
| clean.resource = authentication.resource; | ||
| clean.useSelfSignedCert = authentication.useSelfSignedCert; | ||
| break; | ||
| case "basic": | ||
| clean.username = authentication.username; | ||
| clean.password = authentication.password; | ||
| break; | ||
| case "apiKey": | ||
| clean.addTo = authentication.addTo; | ||
| clean.label = authentication.label; | ||
| clean.headerPrefix = authentication.headerPrefix; | ||
| clean.value = authentication.value; | ||
| break; | ||
| case "bearerToken": | ||
| clean.bearerToken = authentication.bearerToken; | ||
| break; | ||
| case "snowflakeKeyPairAuth": | ||
| clean.username = authentication.username; | ||
| clean.privateKey = authentication.privateKey; | ||
| clean.passphrase = authentication.passphrase; | ||
| } | ||
|
|
||
| return clean; | ||
| } |
There was a problem hiding this comment.
Refactor suggestion for cleanAuthenticationObject method.
This method effectively handles various authentication types by cleaning unnecessary fields. However, it can be optimized by using a mapping strategy instead of a large switch-case block, which can improve maintainability and readability.
- switch (clean.authenticationType) {
- case "dbAuth":
- clean.authType = authentication.authType;
- clean.username = authentication.username;
- ...
- case "oAuth2":
- clean.grantType = authentication.grantType;
- ...
- }
+ const authFieldMap = {
+ "dbAuth": ["authType", "username", "password", "databaseName"],
+ "oAuth2": ["grantType", "isTokenHeader", "isAuthorizationHeader", "clientId", "clientSecret", "authorizationUrl", "expiresIn", "accessTokenUrl", "scopeString", "scope", "sendScopeWithRefreshToken", "refreshTokenClientCredentialsLocation", "headerPrefix", "customTokenParameters", "audience", "resource", "useSelfSignedCert"],
+ "basic": ["username", "password"],
+ ...
+ };
+ const fields = authFieldMap[clean.authenticationType] || [];
+ fields.forEach(field => {
+ clean[field] = authentication[field];
+ });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.
| static cleanAuthenticationObject(authentication: any): any { | |
| if (!authentication) { | |
| return undefined; | |
| } | |
| const clean: any = { | |
| authenticationType: authentication.authenticationType ?? "dbAuth", | |
| }; | |
| switch (clean.authenticationType) { | |
| case "dbAuth": | |
| clean.authType = authentication.authType; | |
| clean.username = authentication.username; | |
| clean.password = authentication.password; | |
| clean.databaseName = authentication.databaseName; | |
| break; | |
| case "oAuth2": | |
| clean.grantType = authentication.grantType; | |
| clean.isTokenHeader = authentication.isTokenHeader; | |
| clean.isAuthorizationHeader = authentication.isAuthorizationHeader; | |
| clean.clientId = authentication.clientId; | |
| clean.clientSecret = authentication.clientSecret; | |
| clean.authorizationUrl = authentication.authorizationUrl; | |
| clean.expiresIn = authentication.expiresIn; | |
| clean.accessTokenUrl = authentication.accessTokenUrl; | |
| clean.scopeString = authentication.scopeString; | |
| clean.scope = authentication.scope; | |
| clean.sendScopeWithRefreshToken = | |
| authentication.sendScopeWithRefreshToken; | |
| clean.refreshTokenClientCredentialsLocation = | |
| authentication.refreshTokenClientCredentialsLocation; | |
| clean.headerPrefix = authentication.headerPrefix; | |
| clean.customTokenParameters = authentication.customTokenParameters; | |
| clean.audience = authentication.audience; | |
| clean.resource = authentication.resource; | |
| clean.useSelfSignedCert = authentication.useSelfSignedCert; | |
| break; | |
| case "basic": | |
| clean.username = authentication.username; | |
| clean.password = authentication.password; | |
| break; | |
| case "apiKey": | |
| clean.addTo = authentication.addTo; | |
| clean.label = authentication.label; | |
| clean.headerPrefix = authentication.headerPrefix; | |
| clean.value = authentication.value; | |
| break; | |
| case "bearerToken": | |
| clean.bearerToken = authentication.bearerToken; | |
| break; | |
| case "snowflakeKeyPairAuth": | |
| clean.username = authentication.username; | |
| clean.privateKey = authentication.privateKey; | |
| clean.passphrase = authentication.passphrase; | |
| } | |
| return clean; | |
| } | |
| static cleanAuthenticationObject(authentication: any): any { | |
| if (!authentication) { | |
| return undefined; | |
| } | |
| const clean: any = { | |
| authenticationType: authentication.authenticationType ?? "dbAuth", | |
| }; | |
| const authFieldMap = { | |
| "dbAuth": ["authType", "username", "password", "databaseName"], | |
| "oAuth2": ["grantType", "isTokenHeader", "isAuthorizationHeader", "clientId", "clientSecret", "authorizationUrl", "expiresIn", "accessTokenUrl", "scopeString", "scope", "sendScopeWithRefreshToken", "refreshTokenClientCredentialsLocation", "headerPrefix", "customTokenParameters", "audience", "resource", "useSelfSignedCert"], | |
| "basic": ["username", "password"], | |
| "apiKey": ["addTo", "label", "headerPrefix", "value"], | |
| "bearerToken": ["bearerToken"], | |
| "snowflakeKeyPairAuth": ["username", "privateKey", "passphrase"], | |
| }; | |
| const fields = authFieldMap[clean.authenticationType] || []; | |
| fields.forEach(field => { | |
| clean[field] = authentication[field]; | |
| }); | |
| return clean; | |
| } |
There was a problem hiding this comment.
I'm not very certain this "improves" code readability, and especially maintainability (refactoring becomes impossible), so I'd rather not do this.
There was a problem hiding this comment.
@sharat87, understood. We'll retain the current implementation. Thank you for the feedback.
| @@ -67,6 +70,65 @@ class DatasourcesApi extends API { | |||
| return API.post(DatasourcesApi.url, datasourceConfig); | |||
| } | |||
|
|
|||
| static cleanAuthenticationObject(authentication: any): any { | |||
There was a problem hiding this comment.
Is the usage of any avoidable here? Ideally would like to have as much type safety at compile time as possible.
There was a problem hiding this comment.
I tried looking into it to setup a type, but, in my opinion, the type definitions to represent this just aren't there today. It's one of the reasons it was so hard to identify what fields are being sent to the server, because more often than not, the type of the request body wasn't an accurate representation of the runtime object fields.
I've raised it in the Slack group I just added you to as well. I'm considering tackling this in small pieces after I'm done with API hardening on the server side.
But for now, I don't have better options here, without blowing this PR up in size that is. Is that okay?
There was a problem hiding this comment.
Hmm ... OK. I'm approving this for now. In the future, I think we can potentially copy over the object structures from the server to construct the types where they don't exist on the client.
There was a problem hiding this comment.
Oh indeed. I've been thinking about generating an Api.ts with methods the same names as backend controller classes, full with type definitions for query params, request body, and response body. Almost like gRPC (I think). We'll get there. Right after I'm done hardening the server a bit more, I'll start biting off at the client. 🙂
The recent changes to deny any requests that have unrecognized fields in the body, is making the datasource creation API fail. This is because we are sending extra fields that shouldn't be sent. [Slack conversation](https://theappsmith.slack.com/archives/CPQNLFHTN/p1719577339838649?thread_ts=1719466294.012589&cid=CPQNLFHTN). **/test sanity datasource** <!-- This is an auto-generated comment: Cypress test results --> > [!WARNING] > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9714799545> > Commit: 85495fb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9714799545&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.Datasource` > It seems like **no tests ran** 😔. We are not able to recognize it, please check <a href="https://github.com/appsmithorg/appsmith/actions/runs/9714799545" target="_blank">workflow here</a>. <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced data security by adding a method to sanitize and retain only necessary authentication information for various authentication types. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The recent changes to deny any requests that have unrecognized fields in the body, is making the datasource creation API fail. This is because we are sending extra fields that shouldn't be sent.
Slack conversation.
/test sanity datasource
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9714799545
Commit: 85495fb
Cypress dashboard.
Tags:
@tag.Sanity, @tag.DatasourceIt seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Summary by CodeRabbit