Fix: Resolve reviewer feedback on Exporting Salesforce Leads to Google Sheets integration sample - #73
Open
minuraashen wants to merge 2 commits into
Open
Fix: Resolve reviewer feedback on Exporting Salesforce Leads to Google Sheets integration sample#73minuraashen wants to merge 2 commits into
minuraashen wants to merge 2 commits into
Conversation
Contributor
WalkthroughThis PR refactors the lead synchronization workflow by introducing normalized state handling upfront, consolidating mode-specific branching through a new dispatcher function, implementing aggregated error reporting for field validation, and adding SOQL filter sanitization for security. The timestamp utility now includes seconds precision. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ballerina-integrator/salesforce-leads-to-googlesheets/automation.bal (1)
20-29:⚠️ Potential issue | 🟡 MinorFail fast on
UPSERT_BY_EMAIL+ new spreadsheet before the empty-result return.Line 20-23 exits before Line 27-29, so this invalid configuration is reported as success whenever the query returns 0 leads. The mode check should run before the no-leads early return, ideally before querying Salesforce.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina-integrator/salesforce-leads-to-googlesheets/automation.bal` around lines 20 - 29, The UPSERT_BY_EMAIL validity check is performed after the early return on empty leadValues, causing invalid configs to be treated as success; move the isNewSpreadsheet and effectiveSyncMode check (the branch that returns error when UPSERT_BY_EMAIL is used with a new spreadsheet) so it runs before the empty-result return — ideally before the Salesforce query is executed — ensuring the validation (references: isNewSpreadsheet, effectiveSyncMode, UPSERT_BY_EMAIL) always runs and returns the error for invalid configurations regardless of lead count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ballerina-integrator/salesforce-leads-to-googlesheets/automation.bal`:
- Around line 242-252: Wrap the sequence that clears the live sheet and copies
back from the temp sheet into a transactional block so the temp sheet is
preserved on failure: move the calls involving sheetsClient->clearRange(...,
sheet.properties.title), sheetsClient->getRange(..., tempSheet.properties.title)
/ appendValues(..., tempRange.values, {sheetName: sheet.properties.title}) and
sheetsClient->removeSheet(..., tempSheet.properties.sheetId) into a do ... on
fail block (or equivalent try/catch) so that if the append from temp to live
(the appendValues using tempRange.values) fails you do not remove the temp
sheet; instead keep the temp sheet and log/propagate the error. Ensure the clear
happens only just before a guaranteed successful copy-back (or roll back by
re-appending from temp in the fail handler), and reference tempSheetName,
tempSheet, tempRange, sheet.properties.title and sheetsClient methods above to
locate and modify the code.
- Around line 131-145: The code currently iterates all sheets and calls
sheetsClient->removeSheet, wiping the whole workbook; change the replace logic
to only target the sheet named effectiveSheetName (or its sheetId) so other tabs
remain. Specifically, after creating tempSheet (tempSheetName) and appending
values, locate the existing sheet in spreadsheet.sheets whose properties.title
== effectiveSheetName (or match by known owner set), call
sheetsClient->removeSheet(spreadsheetId, thatSheet.properties.sheetId) only for
that single sheet (skip if not found or if it's the same as tempSheet), then
call sheetsClient->renameSheet(spreadsheetId, tempSheet.properties.title,
effectiveSheetName); keep existing calls to applySheetFormatting and handle
errors as before.
In `@ballerina-integrator/salesforce-leads-to-googlesheets/data_mappings.bal`:
- Around line 55-69: The current validation of fieldMapping happens inside
mapLeadToRow (using getLeadFieldMap and leadMapAsMap), which runs per lead and
too late; move that validation into main before buildSoqlQuery and before
calling salesforceClient->query so typos/unsupported fields are detected up
front. In main, obtain the canonical set of supported field names (e.g., by
calling getLeadFieldMap on a prototype or a static definition used by
getLeadFieldMap), compare fieldMapping against that set, and if any
invalidFields exist return/log a single aggregated error and avoid calling
buildSoqlQuery or salesforceClient->query; remove or keep the per-lead check in
mapLeadToRow only as a defensive fallback.
In `@ballerina-integrator/salesforce-leads-to-googlesheets/functions.bal`:
- Around line 44-57: The current dangerousKeywords check uses substring matching
on lowerFilter (derived from trimmedFilter) and therefore rejects keywords
inside quoted literals like `'Merge Labs'`; change the logic to ignore/strip
single-quoted string literals from trimmedFilter first (e.g., remove or mask
content between single quotes), then perform a case-insensitive whole-token
match against dangerousKeywords (use word-boundary regex like /\b{keyword}\b/ or
split on non-alphanumeric chars and compare tokens) instead of using includes;
update the loop that iterates dangerousKeywords and the check that returns the
error so it references the filtered/unquoted string when testing for whole-token
matches.
---
Outside diff comments:
In `@ballerina-integrator/salesforce-leads-to-googlesheets/automation.bal`:
- Around line 20-29: The UPSERT_BY_EMAIL validity check is performed after the
early return on empty leadValues, causing invalid configs to be treated as
success; move the isNewSpreadsheet and effectiveSyncMode check (the branch that
returns error when UPSERT_BY_EMAIL is used with a new spreadsheet) so it runs
before the empty-result return — ideally before the Salesforce query is executed
— ensuring the validation (references: isNewSpreadsheet, effectiveSyncMode,
UPSERT_BY_EMAIL) always runs and returns the error for invalid configurations
regardless of lead count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d40d5ca-0982-44ca-b97e-5f6d414fb843
📒 Files selected for processing (3)
ballerina-integrator/salesforce-leads-to-googlesheets/automation.balballerina-integrator/salesforce-leads-to-googlesheets/data_mappings.balballerina-integrator/salesforce-leads-to-googlesheets/functions.bal
Comment on lines
+131
to
+145
| string currentTimeStamp = check getFormattedCurrentTimeStamp(); | ||
| string tempSheetName = string `${effectiveSheetName}_temp_${currentTimeStamp}`; | ||
| sheets:Sheet tempSheet = check sheetsClient->addSheet(spreadsheetId, tempSheetName); | ||
|
|
||
| _ = check sheetsClient->appendValues(spreadsheetId, allValues, {sheetName: newSheet.properties.title}); | ||
| _ = check sheetsClient->appendValues(spreadsheetId, allValues, {sheetName: tempSheet.properties.title}); | ||
|
|
||
| check applySheetFormatting(spreadsheetId, newSheet.properties.sheetId); | ||
| check applySheetFormatting(spreadsheetId, tempSheet.properties.sheetId); | ||
|
|
||
| _ = check sheetsClient->removeSheet(spreadsheetId, tempSheet.properties.sheetId); | ||
| do { | ||
| foreach sheets:Sheet sheet in spreadsheet.sheets { | ||
| _ = check sheetsClient->removeSheet(spreadsheetId, sheet.properties.sheetId); | ||
| } | ||
|
|
||
| _ = check sheetsClient->renameSheet(spreadsheetId, tempSheet.properties.title, effectiveSheetName); | ||
| } on fail error e { |
Contributor
There was a problem hiding this comment.
FULL_REPLACE is wiping the entire workbook.
Line 140-142 removes every sheet in the spreadsheet. That deletes unrelated tabs, and when syncLeadsSplit() invokes this once per group only the last group survives. The temp-sheet swap should replace just effectiveSheetName (or a known sample-owned set), not the whole workbook.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/salesforce-leads-to-googlesheets/automation.bal` around
lines 131 - 145, The code currently iterates all sheets and calls
sheetsClient->removeSheet, wiping the whole workbook; change the replace logic
to only target the sheet named effectiveSheetName (or its sheetId) so other tabs
remain. Specifically, after creating tempSheet (tempSheetName) and appending
values, locate the existing sheet in spreadsheet.sheets whose properties.title
== effectiveSheetName (or match by known owner set), call
sheetsClient->removeSheet(spreadsheetId, thatSheet.properties.sheetId) only for
that single sheet (skip if not found or if it's the same as tempSheet), then
call sheetsClient->renameSheet(spreadsheetId, tempSheet.properties.title,
effectiveSheetName); keep existing calls to applySheetFormatting and handle
errors as before.
Comment on lines
+242
to
+252
| string tempSheetName = string `${sheet.properties.title}_temp_${check getFormattedCurrentTimeStamp()}`; | ||
| sheets:Sheet tempSheet = check sheetsClient->addSheet(spreadsheetId, tempSheetName); | ||
|
|
||
| _ = check sheetsClient->appendValues(spreadsheetId, allData, {sheetName: tempSheet.properties.title}); | ||
|
|
||
| _ = check sheetsClient->clearRange(spreadsheetId, sheet.properties.title, a1Notation = string `A:${endColumn}`); | ||
| _ = check sheetsClient->appendValues(spreadsheetId, allData, {sheetName: sheet.properties.title}); | ||
|
|
||
| check applySheetFormatting(spreadsheetId, sheet.properties.sheetId); | ||
| sheets:Range tempRange = check sheetsClient->getRange(spreadsheetId, tempSheet.properties.title, a1Notation = string `A:${endColumn}`); | ||
| _ = check sheetsClient->appendValues(spreadsheetId, tempRange.values, {sheetName: sheet.properties.title}); | ||
|
|
||
| if newLeads.length() > 0 { | ||
| _ = check sheetsClient->appendValues(spreadsheetId, newLeads, {sheetName: sheet.properties.title}); | ||
| } | ||
| _ = check sheetsClient->removeSheet(spreadsheetId, tempSheet.properties.sheetId); |
Contributor
There was a problem hiding this comment.
Make the temp-sheet copy-back in UPSERT transactional.
After Line 247 clears the live sheet, any failure in Line 249-252 leaves the original tab blank or partially restored. Wrap the clear/copy/remove sequence in do/on fail and keep the temp sheet until the copy-back succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/salesforce-leads-to-googlesheets/automation.bal` around
lines 242 - 252, Wrap the sequence that clears the live sheet and copies back
from the temp sheet into a transactional block so the temp sheet is preserved on
failure: move the calls involving sheetsClient->clearRange(...,
sheet.properties.title), sheetsClient->getRange(..., tempSheet.properties.title)
/ appendValues(..., tempRange.values, {sheetName: sheet.properties.title}) and
sheetsClient->removeSheet(..., tempSheet.properties.sheetId) into a do ... on
fail block (or equivalent try/catch) so that if the append from temp to live
(the appendValues using tempRange.values) fails you do not remove the temp
sheet; instead keep the temp sheet and log/propagate the error. Ensure the clear
happens only just before a guaranteed successful copy-back (or roll back by
re-appending from temp in the fail handler), and reference tempSheetName,
tempSheet, tempRange, sheet.properties.title and sheetsClient methods above to
locate and modify the code.
Comment on lines
+55
to
+69
| public function mapLeadToRow(Lead lead) returns SheetRow|error { | ||
| LeadFieldMap leadMap = getLeadFieldMap(lead); | ||
| map<int|string|decimal|boolean|float> leadMapAsMap = leadMap; | ||
|
|
||
| SheetRow row = from string fieldName in fieldMapping | ||
| select leadMapAsMap.hasKey(fieldName) ? | ||
| leadMapAsMap.get(fieldName) : | ||
| ""; | ||
|
|
||
| string[] invalidFields = from string fieldName in fieldMapping | ||
| where !leadMapAsMap.hasKey(fieldName) | ||
| select fieldName; | ||
|
|
||
| if invalidFields.length() > 0 { | ||
| return error(string `Invalid field name(s) in fieldMapping: ${string:'join(", ", ...invalidFields)}. Supported fields are: ${string:'join(", ", ...leadMapAsMap.keys())}`); |
Contributor
There was a problem hiding this comment.
Validate fieldMapping before the Salesforce query, not per lead.
main() builds and executes the SOQL from fieldMapping before mapLeadToRow() ever runs. That means typos still fail at salesforceClient->query(...), and valid-but-unsupported fields are never reported when the stream is empty. Move this validation ahead of buildSoqlQuery() so the aggregated error is actually reachable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/salesforce-leads-to-googlesheets/data_mappings.bal`
around lines 55 - 69, The current validation of fieldMapping happens inside
mapLeadToRow (using getLeadFieldMap and leadMapAsMap), which runs per lead and
too late; move that validation into main before buildSoqlQuery and before
calling salesforceClient->query so typos/unsupported fields are detected up
front. In main, obtain the canonical set of supported field names (e.g., by
calling getLeadFieldMap on a prototype or a static definition used by
getLeadFieldMap), compare fieldMapping against that set, and if any
invalidFields exist return/log a single aggregated error and avoid calling
buildSoqlQuery or salesforceClient->query; remove or keep the per-lead check in
mapLeadToRow only as a defensive fallback.
Comment on lines
+44
to
+57
| string lowerFilter = trimmedFilter.toLowerAscii(); | ||
|
|
||
| string[] dangerousKeywords = [ | ||
| "delete", | ||
| "insert", | ||
| "update", | ||
| "merge", | ||
| "upsert", | ||
| "undelete" | ||
| ]; | ||
|
|
||
| foreach string keyword in dangerousKeywords { | ||
| if lowerFilter.includes(keyword) { | ||
| return error(string `SOQL filter contains dangerous keyword: "${keyword}". Only SELECT queries are allowed.`); |
Contributor
There was a problem hiding this comment.
Avoid substring blacklisting across the whole filter.
Line 56 also scans quoted literals, so valid predicates like Company = 'Merge Labs' or Status = 'Updated' are rejected because they contain merge/update. Match whole tokens outside string values instead; otherwise legitimate soqlFilter values stop working.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/salesforce-leads-to-googlesheets/functions.bal` around
lines 44 - 57, The current dangerousKeywords check uses substring matching on
lowerFilter (derived from trimmedFilter) and therefore rejects keywords inside
quoted literals like `'Merge Labs'`; change the logic to ignore/strip
single-quoted string literals from trimmedFilter first (e.g., remove or mask
content between single quotes), then perform a case-insensitive whole-token
match against dangerousKeywords (use word-boundary regex like /\b{keyword}\b/ or
split on non-alphanumeric chars and compare tokens) instead of using includes;
update the loop that iterates dangerousKeywords and the check that returns the
error so it references the filtered/unquoted string when testing for whole-token
matches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Bug Fixes