chore: Strict schema for datasource and action APIs#34366
Conversation
WalkthroughThe recent updates mainly involve refining the payloads for API requests in two key files: Changes
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/api/ActionAPI.tsx (2 hunks)
- app/client/src/api/DatasourcesApi.ts (4 hunks)
Additional comments not posted (4)
app/client/src/api/DatasourcesApi.ts (2)
37-66: Consider simplifying the payload manipulation increateDatasource.
[REFACTOR_SUGGESTion]
The payload manipulation in thecreateDatasourcemethod seems overly complex and repetitive, especially with the nested object and undefined assignments. Consider using a helper function to clean this up, which can be reused across different methods where similar manipulations are needed.+ function cleanPayload(payload) { + const cleanedPayload = {...payload}; + Object.entries(payload).forEach(([key, value]) => { + if (typeof value === 'object' && value !== null) { + cleanedPayload[key] = cleanPayload(value); + } else { + cleanedPayload[key] = undefined; + } + }); + return cleanedPayload; + } - // existing code block + const cleanedConfig = cleanPayload(datasourceConfig); + return API.post(DatasourcesApi.url, cleanedConfig);
76-95: Ensure that thetestDatasourcemethod handles potential null values safely.The method manipulates the
datasourceConfigurationand its nestedconnectionandsslobjects without null checks. This could lead to runtime errors if these are not defined. Verify that the upstream code ensures these objects are always defined or add necessary null checks.#!/bin/bash # Description: Verify upstream null checks for datasourceConfiguration. # Test: Search for the method usage. Expect: Proper null checks or guarantees that the object is defined. rg --type typescript $'DatasourcesApi.testDatasource'app/client/src/api/ActionAPI.tsx (2)
135-146: Validate the addition of undefined properties increateAction.The method now includes additional properties (
isValid,entityReferenceType,new) set toundefined. It's crucial to confirm that these additions align with the intended schema changes and are handled appropriately by the server.#!/bin/bash # Description: Verify server schema handling for new undefined properties. # Test: Search for server-side handling of new properties. Expect: Proper handling or ignoring of undefined properties. rg --type typescript $'createAction'
183-184: Check the consistency of changes inupdateAction.The
updateActionmethod also manipulates thedatasourceobject like thecreateActionmethod, but it appears to handle fewer properties. Verify that all necessary properties are being handled consistently between these methods.#!/bin/bash # Description: Verify consistency between createAction and updateAction in handling datasource properties. # Test: Compare the handling of properties in both methods. Expect: Consistent handling of properties. rg --type typescript $'ActionAPI.createAction|ActionAPI.updateAction'
Currently, if the incoming request body contains fields that aren't recognized by the backend, we just ignore them, and deserialize what we _can_. This has lead to subtle bugs and wasted a lot of developer time in the past, and added little value in return. Most recently, with application creation, now fixed in #33722. Another one, for action creation, client sends this in `eventData` field of the request body:  But the class defined for `eventData` is this:  Clearly isn't working. This PR enabled the `FAIL_ON_UNKNOWN_PROPERTIES` setting only for the deserialization of HTTP request payloads. It shouldn't have impact on other deserializations like those for Git, and those that load resource files. Primarily to limit the scope. This should also bring in some much-needed type strictness to the client as well. So far, server has been lax in accepting just any fields in the incoming request body, so client was able to afford being lax about the object that was sent up. We're enabling this restriction only for CE currently. Will be opening a similar PR on EE and once all tests pass there, we enable for EE as well. Depends on #33728 #33730 #33731 #34366 #34405 #34446 #34473 #34506 Run No. 1 > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9685619413> > Commit: 04e225f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9685619413&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` **/test all** <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9690185000> > Commit: 29da8b4 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9690185000&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added `certificateType` property to datasources for enhanced SSL configuration options. - **Enhancements** - Improved JSON deserialization behavior for better data handling and flexibility. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Part of appsmithorg#33724. This is an effort to harden the server in terms of what request payloads are acceptable. **/test all** <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced API request payload handling for creating actions to include additional properties, improving data integrity and server-side validation. - **Improvements** - Refined data manipulation and payload structures in datasource-related API requests for better compatibility with server requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai --><!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > 🔴 🔴 🔴 Some tests have failed. > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9626214547> > Commit: fe5db45 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9626214547&attempt=2&selectiontype=test&testsstatus=failed&specsstatus=fail" target="_blank">Cypress dashboard</a>. > Tags: `` > The following are new failures, please fix them before merging the PR: <ol> > <li>cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor3_spec.ts > <li>cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_1_spec.js > <li>cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_2_spec.js </ol> > <a href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master" target="_blank">List of identified flaky tests</a>. <!-- end of auto-generated comment: Cypress test results -->
Currently, if the incoming request body contains fields that aren't recognized by the backend, we just ignore them, and deserialize what we _can_. This has lead to subtle bugs and wasted a lot of developer time in the past, and added little value in return. Most recently, with application creation, now fixed in appsmithorg#33722. Another one, for action creation, client sends this in `eventData` field of the request body:  But the class defined for `eventData` is this:  Clearly isn't working. This PR enabled the `FAIL_ON_UNKNOWN_PROPERTIES` setting only for the deserialization of HTTP request payloads. It shouldn't have impact on other deserializations like those for Git, and those that load resource files. Primarily to limit the scope. This should also bring in some much-needed type strictness to the client as well. So far, server has been lax in accepting just any fields in the incoming request body, so client was able to afford being lax about the object that was sent up. We're enabling this restriction only for CE currently. Will be opening a similar PR on EE and once all tests pass there, we enable for EE as well. Depends on appsmithorg#33728 appsmithorg#33730 appsmithorg#33731 appsmithorg#34366 appsmithorg#34405 appsmithorg#34446 appsmithorg#34473 appsmithorg#34506 Run No. 1 > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9685619413> > Commit: 04e225f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9685619413&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` **/test all** <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9690185000> > Commit: 29da8b4 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9690185000&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added `certificateType` property to datasources for enhanced SSL configuration options. - **Enhancements** - Improved JSON deserialization behavior for better data handling and flexibility. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Part of #33724. This is an effort to harden the server in terms of what request payloads are acceptable.
/test all
Summary by CodeRabbit
New Features
Improvements
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9626214547
Commit: fe5db45
Cypress dashboard.
Tags: ``
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor3_spec.ts
- cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_1_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_2_spec.js
List of identified flaky tests.