feat: enable soql by default #4874
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Salesforce SDK and transform refactoring src/v0/destinations/salesforce/transform.js, src/v0/destinations/salesforce/utils.js |
Removed workspace-based conditional branching and isWorkspaceSupportedForSoql checks. Simplified function signatures by removing metadata parameter from getSalesforceIdFromPayload, processIdentify, and processSingleMessage. Consolidated ID lookup to always use Salesforce SDK path, eliminating HTTP fallback logic in getSalesforceIdForRecord and getSalesforceIdForLead. SDK initialization now unconditional using token and instanceUrl from authorizationData. |
Test updates src/v0/destinations/salesforce/utils.test.js |
Updated test descriptions and assertions to reflect SDK-only path. Replaced workspace-support-based test scenarios with cases covering no results, multiple results, and error handling. Added tests for Lead vs. Contact determination based on IsConverted and useContactId flags. Removed predefined workspace fixtures in favor of in-test configuration. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~30–40 minutes
Areas requiring extra attention:
- Verify all internal and external call sites have been updated to pass only
{ message, destination }instead of includingmetadata - Ensure the SDK-only path in
getSalesforceIdForRecordandgetSalesforceIdForLeadhandles all error scenarios and edge cases previously managed by the HTTP fallback - Confirm test assertions accurately validate query construction and error handling across all new scenarios (no results, multiple results, conversion flags)
- Check that removed stats instrumentation doesn't impact observability or monitoring
Possibly related PRs
- fix: handle refresh scenario for the sdk response #4791: Adds session-expired/REFRESH_TOKEN retry handling in SDK error handlers for the same Salesforce lookup logic, complementing this PR's consolidation to SDK-only paths
- feat: update salesforce to soql #4771: Introduces the workspace-SOQL branching logic that this PR removes; provides historical context for understanding the refactoring
- chore(release): pull main into develop post release v1.113.1 #4798: Modifies the same Salesforce SDK lookup helpers and test coverage for
getSalesforceId*UsingSdkpaths
Suggested reviewers
- chandumlg
- achettyiitr
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'feat: enable soql by default' is partially related to the changeset. While the PR does consolidate SOQL usage, the actual changes remove workspace-specific SOQL checks and make SDK-based lookups mandatory (removing HTTP fallback), rather than enabling SOQL for the first time. The title could be more precise about this consolidation. |
| Description check | ✅ Passed | The PR description comprehensively addresses the template requirements. It explains the changes (removing HTTP lookups, consolidating to SDK), provides related Linear task reference (INT-XXX), details refactored functions and test updates, and includes both checklists. All template sections are addressed. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat.enable-soql-default
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/v0/destinations/salesforce/utils.js (3)
323-325: Potential SOQL injection vulnerability via unescaped identifier value.The
identifierValueis directly interpolated into the SOQL query string without escaping single quotes. IfidentifierValuecontains a single quote (e.g.,O'Brien), the query will be malformed and could potentially be exploited.Consider escaping single quotes by replacing
'with\'before interpolation:Suggested fix
+const escapeSoqlValue = (value) => String(value).replace(/'/g, "\\'"); + async function getSalesforceIdForRecordUsingSdk( salesforceSdk, objectType, identifierType, identifierValue, ) { let queryResponse; try { + const escapedValue = escapeSoqlValue(identifierValue); queryResponse = await salesforceSdk.query( - `SELECT Id FROM ${objectType} WHERE ${identifierType} = '${identifierValue}'`, + `SELECT Id FROM ${objectType} WHERE ${identifierType} = '${escapedValue}'`, );
395-397: Same SOQL injection risk with email parameter.The
Suggested fix
async function getSalesforceIdForLeadUsingSdk(salesforceSdk, email, destination) { let queryResponse; try { + const escapedEmail = String(email).replace(/'/g, "\\'"); queryResponse = await salesforceSdk.query( - `SELECT Id, IsConverted, ConvertedContactId, IsDeleted FROM Lead WHERE Email = '${email}'`, + `SELECT Id, IsConverted, ConvertedContactId, IsDeleted FROM Lead WHERE Email = '${escapedEmail}'`, );
248-253:isWorkspaceSupportedForSoqlis unused production code and should be removed.The function is exported at line 547 but is not called from any production code. It only appears in the definition and export, with no usages elsewhere in the codebase outside of test files. Since workspace-based SOQL checks are being removed per the PR summary, remove this function and its associated test cases. Also remove the
DEST_SALESFORCE_SOQL_SUPPORTED_WORKSPACE_IDSenvironment variable dependency if it's no longer needed elsewhere.src/v0/destinations/salesforce/utils.test.js (1)
573-589: This test demonstrates the SOQL injection issue.The test shows that a value containing a single quote (
test'value) produces a malformed query:'test'value'. This would likely cause a SOQL syntax error at runtime. Once the escaping fix is applied inutils.js, this test should be updated to expect the escaped value:- expect(mockSalesforceSdk.query).toHaveBeenCalledWith( - "SELECT Id FROM Account WHERE External_ID__c = 'test'value'", - ); + expect(mockSalesforceSdk.query).toHaveBeenCalledWith( + "SELECT Id FROM Account WHERE External_ID__c = 'test\\'value'", + );
🧹 Nitpick comments (2)
src/v0/destinations/salesforce/utils.test.js (1)
41-76: Tests forisWorkspaceSupportedForSoqlmay be testing dead code.If
isWorkspaceSupportedForSoqlis removed fromutils.jsas suggested, these tests should also be removed. If the function is kept for backward compatibility, consider adding a comment explaining why.src/v0/destinations/salesforce/utils.js (1)
265-305: HTTP-based lookup functions are exported but not used by their public entry points.
getSalesforceIdForRecordUsingHttpandgetSalesforceIdForLeadUsingHttpare defined and exported but the public entry points (getSalesforceIdForRecord,getSalesforceIdForLead) now exclusively call their SDK-based counterparts. Consider documenting these functions if they're intentionally maintained for backward compatibility or external SDK consumers. Otherwise, evaluate removing them to reduce maintenance burden and improve code clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/v0/destinations/salesforce/transform.js(6 hunks)src/v0/destinations/salesforce/utils.js(2 hunks)src/v0/destinations/salesforce/utils.test.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
Focus on ESLint errors (max 3) and warnings (max 5).
Files:
src/v0/destinations/salesforce/utils.test.jssrc/v0/destinations/salesforce/utils.jssrc/v0/destinations/salesforce/transform.js
🧠 Learnings (2)
📚 Learning: 2025-12-12T12:38:43.404Z
Learnt from: maheshkutty
Repo: rudderlabs/rudder-transformer PR: 4855
File: src/cdk/v2/destinations/zoho/utils.ts:412-433
Timestamp: 2025-12-12T12:38:43.404Z
Learning: In the Zoho destination (src/cdk/v2/destinations/zoho/), identifier values in `message.identifiers` will not contain object data types; they are expected to be scalar values (string, number, boolean) or arrays of scalars, so additional type guards against objects are not needed.
Applied to files:
src/v0/destinations/salesforce/transform.js
📚 Learning: 2025-12-05T08:37:16.001Z
Learnt from: maheshkutty
Repo: rudderlabs/rudder-transformer PR: 4846
File: src/cdk/v2/destinations/zoho/transformRecordV2.ts:286-286
Timestamp: 2025-12-05T08:37:16.001Z
Learning: In src/cdk/v2/destinations/zoho/transformRecordV2.ts, the `identifiers` property is optional in the Message type (identifiers?: Record<string, unknown>), so checking both `!identifiers` and `isEmptyObject(identifiers)` is necessary to handle the case where identifiers is undefined before passing it to isEmptyObject.
Applied to files:
src/v0/destinations/salesforce/transform.js
🧬 Code graph analysis (2)
src/v0/destinations/salesforce/utils.test.js (1)
src/v0/destinations/salesforce/transform.js (3)
stateInfo(217-217)stateInfo(260-263)destination(194-194)
src/v0/destinations/salesforce/utils.js (1)
src/v0/destinations/salesforce/transform.js (4)
stateInfo(217-217)stateInfo(260-263)destination(194-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Code Coverage
- GitHub Check: check-health
- GitHub Check: Check for formatting & lint errors
- GitHub Check: test_and_publish
- GitHub Check: UT Tests
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
src/v0/destinations/salesforce/utils.js (2)
371-383: SDK-only delegation looks correct.The simplified function signature accepting
{ objectType, identifierType, identifierValue, stateInfo }and delegating togetSalesforceIdForRecordUsingSdkis clean and appropriate for the SDK-only flow.
531-533: SDK-only flow for lead lookup is correctly implemented.The function now exclusively uses
getSalesforceIdForLeadUsingSdk, properly passing the SDK instance fromstateInfo. The simplified signature aligns with the PR objective.src/v0/destinations/salesforce/transform.js (4)
254-263: SDK initialization change looks correct.The unconditional initialization of
SalesforceSDK.SalesforcewithtokenandinstanceUrlfromauthorizationDataproperly implements the SDK-only flow. ThestateInfoobject cleanly bundlesauthInfoandsalesforceSdkfor downstream functions.
284-299: Good error handling for SDK initialization in router path.The try-catch block around SDK creation properly handles initialization failures and returns appropriate error responses. This matches the pattern used for authorization errors above.
119-119: Function signatures consistently updated.The simplified signatures removing
metadataparameter are consistently applied acrossgetSalesforceIdFromPayload,processIdentify, andprocessSingleMessage. This aligns with the SDK-only approach where metadata is no longer needed for workspace checks.Also applies to: 193-193, 240-240
156-161: VerifystateInfois passed correctly togetSalesforceIdForRecord.The call passes
stateInfoas a property in the options object, which matches the updated function signature inutils.js. This looks correct.src/v0/destinations/salesforce/utils.test.js (4)
608-631: SDK-only test correctly verifies behavior.The test properly verifies that
getSalesforceIdForRecorduses the SDK to query Salesforce and thathandleHttpRequestis not called. This aligns with the PR objective of SDK-only lookups.
633-674: Good coverage for no-records and multiple-records scenarios.These tests properly verify the SDK-only behavior for edge cases: returning
undefinedwhen no record is found and throwingNetworkInstrumentationErrorwhen multiple records are found. The assertions confirm the SDK is called appropriately.
1387-1419: Lead lookup SDK-only test is comprehensive.The test verifies that
getSalesforceIdForLeaduses the SDK with the correct SOQL query and that HTTP is not called. The expected query format and return structure are properly validated.
1421-1456: Good test coverage for lead conversion and multiple leads scenarios.Tests properly cover:
- Lead converted with
useContactId: truereturning Contact type- No lead found returning undefined
- Multiple leads throwing
NetworkInstrumentationErrorThese align with the SDK-based behavior expected after the refactor.
Also applies to: 1458-1500
|
2 participants
What are the changes introduced in this PR?
This pull request simplifies the Salesforce destination integration by removing legacy HTTP-based lookup logic and consolidating all Salesforce ID lookups to use the Salesforce SDK. As a result, related code paths, parameters, and tests have been cleaned up to reflect this unified approach.
Refactor: Remove legacy HTTP lookup and workspace SOQL support checks
isWorkspaceSupportedForSoqlutility and all references to it. [1] [2] [3] [4] [5]API and parameter cleanup
getSalesforceIdFromPayload,processIdentify,processSingleMessage,getSalesforceIdForRecord, andgetSalesforceIdForLeadhave been refactored to remove unusedmetadataparameters, as they are no longer needed for SOQL support checks or HTTP lookups. [1] [2] [3] [4] [5] [6]Test suite updates
Minor dependency cleanup
statshave been removed fromutils.jssince instrumentation for HTTP lookups is no longer needed.These changes streamline the Salesforce integration codebase, making it easier to maintain and reducing the potential for inconsistent behavior between SDK and HTTP lookup paths.
What is the related Linear task?
Resolves INT-XXX
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new generic utility introduced or modified. Please explain the changes.
N/A
Any technical or performance related pointers to consider with the change?
N/A
@coderabbitai review
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added in new readability format?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.