[Common Tools] [Salesforce-Sync] Implement Salesforce Sync Service in the Common Tools Repository #1
Conversation
WalkthroughAdds a new Ballerina Salesforce-to-MySQL sync project: project manifests and gitignore, DB schema and stored procedure, types and config, Salesforce client functions, MySQL transactional DB operations, orchestration main loop and utilities, README and deployment diagram; no existing code removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as Sync Service (main)
participant Utils as Utils (process/check)
participant SF as Salesforce Client
participant DB as MySQL DB
Main->>Utils: checkSyncStatus(syncObj)
Utils->>DB: dbGetLastSyncLog(syncObj)
DB-->>Utils: last successful end_time
Utils-->>Main: upToDate? (boolean)
alt Sync required
Main->>DB: dbInsertSyncLog(PROCESSING)
DB-->>Main: logIndex
Main->>Utils: processSyncObject(syncObj)
Utils->>SF: sfQueryAccount / sfQueryOpportunity
SF-->>Utils: records
Utils->>DB: dbUpdateSync(records)
DB-->>Utils: success
Main->>DB: dbInsertSyncLog(COMPLETED, logIndex)
else Skipped or Up-to-date
Main-->>Main: skip
end
Main->>DB: dbShadowCopy()
DB-->>Main: shadow copy complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
saajidAha
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (20)
salesforce-sync/README.md (1)
1-4: Good start with minimal documentation.The README clearly identifies the service purpose and lists supported sync objects. Consider expanding with setup/usage instructions and configuration details in future updates.
salesforce-sync/documents/salesforce-sync-deployment-diagram.drawio (1)
1-1: Deployment diagram is included for operational context.The diagrams.net file provides deployment architecture visualization. Ensure the diagram remains updated as the sync service evolves.
salesforce-sync/database/sync_table_update_v1.0.1.sql (3)
1-4: Avoid column-level charset/collation specifications.MySQL Workbench typically adds charset/collation at the column level, but set character set and collation settings at the database level, and remove references to specific character sets or collations at the table or column level. This reduces complexity and prevents mismatches during replication.
Apply this diff to remove the redundant charset/collation:
-ALTER TABLE `salesforce_sync`.`sf_account` -CHANGE COLUMN `Name` `Name` VARCHAR(100) CHARACTER SET 'utf8mb4' COLLATE 'utf8mb4_bin' NOT NULL , -CHANGE COLUMN `Account_Owner_Email` `Account_Owner_Email` VARCHAR(100) NOT NULL , -CHANGE COLUMN `Account_Owner_FullName` `Account_Owner_FullName` VARCHAR(100) NOT NULL ; +ALTER TABLE `salesforce_sync`.`sf_account` +CHANGE COLUMN `Name` `Name` VARCHAR(100) NOT NULL, +CHANGE COLUMN `Account_Owner_Email` `Account_Owner_Email` VARCHAR(100) NOT NULL, +CHANGE COLUMN `Account_Owner_FullName` `Account_Owner_FullName` VARCHAR(100) NOT NULL;
6-22: Break this large migration into smaller, focused units.This migration combines sf_account alterations and 17 sf_opportunity schema changes in a single file. Each migration should execute a single unit of work on the database. This comes from the Agile software development principle of introducing small, incremental changes. Small migrations are easier to read and understand.
Consider splitting this into:
- One migration for sf_account changes.
- Separate migrations for each distinct set of sf_opportunity changes (e.g., one for CloseDate, one for ARR columns, etc.).
This improves readability, makes rollbacks safer, and aligns with industry best practices.
9-13: Inconsistent default value formats across migrations.Lines 9–13 use numeric defaults (
0.00), while v1.0.4, v1.0.9, and v1.0.11 use string defaults ('0.00'). Line 5 usestinyint DEFAULT '0'(string for numeric type). Standardize on one convention:-ADD COLUMN `Cloud_ARR_Opportunity__c` DECIMAL(16,2) NULL DEFAULT 0.00 AFTER `OB_Delayed_ARR__c`, -ADD COLUMN `IAM_BU_ARR_Opportunity__c` DECIMAL(16,2) NULL DEFAULT 0.00 AFTER `Cloud_ARR_Opportunity__c`, +ADD COLUMN `Cloud_ARR_Opportunity__c` DECIMAL(16,2) NULL DEFAULT '0.00' AFTER `OB_Delayed_ARR__c`, +ADD COLUMN `IAM_BU_ARR_Opportunity__c` DECIMAL(16,2) NULL DEFAULT '0.00' AFTER `Cloud_ARR_Opportunity__c`,salesforce-sync/database/sync_table_update_v1.0.2.sql (1)
1-15: Shadow-copy procedure is correct but consider schema-safety and operational behaviorThe procedure does what it says, but two aspects are worth double‑checking:
INSERT INTO arr_sf_account SELECT * FROM sf_account(and the opportunity equivalent) rely onCREATE ... LIKE+SELECT *. That’s safe today, but an explicit column list is more robust if future schema changes introduce computed/virtual columns or different column ordering.DROP TABLE/CREATE TABLEforarr_sf_*on every run will drop and recreate indexes and can briefly remove the tables. If these tables are queried concurrently, or the data set is large, you may want to considerTRUNCATE arr_sf_*; INSERT ...instead (if you don't need to change the table definition each run).salesforce-sync/database/sync_table_update_v1.0.10.sql (1)
1-8: TRUNCATE + new DECIMAL columns – confirm data-loss and nullability expectationsThis migration looks structurally fine, but please confirm two things:
TRUNCATE salesforce_sync.sf_opportunityirreversibly wipes all existing opportunities in this table. That’s appropriate only if you are guaranteed to fully repopulate from Salesforce on deployment (and no local‑only data lives here).- New columns are
NULL DEFAULT '0.00'. If you conceptually treat missing values as zero, consider making themNOT NULL DEFAULT 0.00to simplify downstream logic and avoid needing to handleNULLin queries and application code.salesforce-sync/main.bal (3)
10-41: Tighten typing betweenSyncObjectandgetSyncConfig/checkSyncStatusThe orchestration loop is clear, but the type boundary is a bit loose:
supportedSyncObjsisSyncObject[], and you iterate asforeach SyncObject syncObject in supportedSyncObjs.getSyncConfigis declared asfunction getSyncConfig(string syncObj)and matches onACCOUNT/OPPORTUNITY.For better type-safety and IDE support, consider changing
getSyncConfig(and any similar helpers) to acceptSyncObjectinstead ofstring, and keep thematchonACCOUNT/OPPORTUNITYin that type. That removes any chance of passing arbitrary strings and aligns the API with howmainandexecuteSyncalready use the enum.
42-53: Shadow-copy runs even when per-object sync fails – verify intended behavior
isSyncExecutedis set totrueimmediately afterexecuteSync(syncObject);and is never reset. SinceexecuteSynchandles errors internally and does not propagate them,isSyncExecutedbecomestrueeven if the sync logic for an object failed and only logged/markedFAILED.That means:
dbShadowCopy()is invoked whenever at least one object attempted to sync, not only when all attempted syncs completed successfully.If the shadow tables (
arr_sf_*) are intended as a “last good snapshot”, you may want to:
- Make
executeSyncreturn abooleanorerror?indicating success, and- Only set
isSyncExecuted(or calldbShadowCopy) when all required syncs succeed.Otherwise, please confirm that taking a shadow copy after a failed sync is acceptable for your downstream consumers.
55-80: executeSync error handling and logging look solid (minor nit only)The
do/on failpattern,dbCheckProcessingguard, and logging to the sync log table form a good unit of work. The only minor nit is the comment typo (whehterat Line 61), which you can fix at your convenience.salesforce-sync/sync_config.bal (1)
17-109: AligngetSyncConfigparameter withSyncObjectenum and central callers
getSyncConfigis currently declared as:function getSyncConfig(string syncObj) returns SfSyncConf|error { match syncObj { ACCOUNT => { ... } OPPORTUNITY => { ... } _ => error ... } }Given that callers (e.g.,
mainandexecuteSync) already work with theSyncObjectenum, consider:
- Changing the signature to
function getSyncConfig(SyncObject syncObj) returns SfSyncConf|error.- Keeping the
matcharms asACCOUNTandOPPORTUNITY(now clearly enum members).- Letting any stringly-typed callers convert explicitly if needed, instead of the core API taking a plain
string.This improves type-safety and keeps the set of valid values tightly bound to the enum.
salesforce-sync/types.bal (1)
122-232: Verify non-nullable decimal fields inSFOpportunitySyncRecordare always populatedThe
SFOpportunitySyncRecorddefinition closely matches the OPPORTUNITY SOQL, which is great. One thing to confirm:
- Several currency fields are declared as non-nullable
decimal(e.g.,ARR__c,IAM_ARR__c,APIM_ARR__c,Integration_ARR__c,Open_Banking_ARR__c,Delayed_ARR__c,IAM_Delayed_ARR__c,APIM_Delayed_ARR__c,Integration_Delayed__c,CL_ARR_Today__c), while others aredecimal?.If Salesforce can ever return
nullfor any of the non-nullable ones (e.g., for older or partially filled opportunities),result.cloneWithType()insf_functions.balwill fail for those records.Please verify that these fields are guaranteed non-null in your org. If not, it would be safer to mark them as
decimal?and adjust downstream code accordingly.salesforce-sync/sf_functions.bal (2)
35-53: Simplify result handling insfQueryAccountYou already use
check salesforceEP->query(...), so the outererrorcase is handled at that point. Given that:stream<SFAccountSyncRecord, error?>|error resultStream = check salesforceEP->query(syncConfig.soqlQuery); if resultStream is error { error err = resultStream; return handleSalesforceError(err); }is effectively dead code. Consider:
stream<SFAccountSyncRecord, error?> resultStream = check salesforceEP->query(syncConfig.soqlQuery); SFAccountSyncRecord[] sfRecords = check from var result in resultStream select check result.cloneWithType();and let any per-record stream errors propagate via
checkas you do now. This reduces branching and keeps error-handling clearer.
55-73: Apply the same simplification pattern tosfQueryOpportunityThe same comment as for
sfQueryAccountapplies here: once youcheckthe result ofsalesforceEP->query, you no longer needstream|erroror theif resultStream is errorguard. Typing it as a plainstream<SFOpportunitySyncRecord, error?>and relying oncheckin the comprehension keeps the function lean and idiomatic.salesforce-sync/utils.bal (2)
17-30: Tighten sync object typing / match coverage inprocessSyncObject
syncObjis astring, but thematchonly coversACCOUNTandOPPORTUNITY. It would be safer/clearer to either:
- Type
syncObjas theSyncObjectenum (or equivalent constrained type), or- Add a default (
_) branch that logs and returns an error for unsupported values.This avoids silent no-ops or future mismatch if new sync object types are introduced.
38-63: Clarify time handling and debug label incheckSyncStatusTwo minor points here:
timeLastSuccessis initialized with[0]and later used inutcDiffSeconds/utcAddSeconds. That works as a "epoch" sentinel, but it’s a bit opaque and tightly couples to the currenttime:Utcrepresentation. A more explicit approach (e.g., deriving a proper epochUtcvia an API helper) would be clearer and more resilient to library evolution.The debug messages are labeled
[cacheSyncStatus()]while the function ischeckSyncStatus. This can confuse log readers.Consider:
- Constructing
timeLastSuccessvia a dedicated helper (or clearly documenting the sentinel usage), and- Updating the debug labels to
[checkSyncStatus()].salesforce-sync/db_functions.bal (3)
123-146: Prefer higher‑level time APIs instead of manualUtctuple arithmetic
hourAgoTimeis built via:time:Utc hourAgoTime = [time:utcNow()[0] - (60 * 60)];This relies on the internal shape of
time:Utcand manual element indexing, which is brittle and harder to read. The intent (“one hour ago”) is clearer and more robust if expressed via the time module’s helpers, e.g.:- time:Utc hourAgoTime = [time:utcNow()[0] - (60 * 60)]; + time:Utc hourAgoTime = time:utcAddSeconds(time:utcNow(), -3600);It also keeps the representation details fully encapsulated in the time module.
200-209: Correct function name indbPrepareInitQueryerror messageThe error message currently mentions
dbPrepareBatchQuery()even though this isdbPrepareInitQuery(), which can mislead debugging:return error(string `Invalid records type: [dbPrepareBatchQuery()] Batch SQL query for passed in record type` + string `(${(typeof records).toString()}) not defined.`);Consider updating the label to reflect the actual function:
- return error(string `Invalid records type: [dbPrepareBatchQuery()] Batch SQL query for passed in record type` + return error(string `Invalid records type: [dbPrepareInitQuery()] Init SQL query for passed-in record type` + string `(${(typeof records).toString()}) not defined.`);This will make log/error messages much easier to interpret.
216-436: Confirm omission ofOB_Delayed_ARR__cfrom opportunity sync mappingsThe
sf_opportunitytable schema (seesalesforce-sync/database/sync_table_creation.sql) includes anOB_Delayed_ARR__ccolumn, butdbPrepareBatchQuery’s insert/update templates forSFOpportunitySyncRecord[]do not set or update this field.If the Salesforce records expose
OB_Delayed_ARR__cand it’s important for downstream reporting, you may want to include it in both theINSERTcolumn list/values and theON DUPLICATE KEY UPDATEclause. If it’s intentionally unused, a short comment indicating that would help future maintainers.salesforce-sync/database/sync_table_creation.sql (1)
95-105: Shadow copy procedure assumes column parity viaSELECT *
arr_shadow_copyusesCREATE TABLE ... LIKEfollowed byINSERT ... SELECT *:CREATE TABLE IF NOT EXISTS arr_sf_account LIKE sf_account; INSERT INTO arr_sf_account SELECT * FROM sf_account; ... CREATE TABLE IF NOT EXISTS arr_sf_opportunity LIKE sf_opportunity; INSERT INTO arr_sf_opportunity SELECT * FROM sf_opportunity;This is fine as long as:
- The shadow tables are always regenerated from the live tables (as here), and
- Future schema changes keep the source tables compatible.
If you ever need additional columns or different defaults in the shadow tables, switching to an explicit column list would be safer. For now, a brief comment stating that
arr_sf_*are intended to be exact structural clones ofsf_*would clarify the design intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
salesforce-sync/.gitignore(1 hunks)salesforce-sync/Ballerina.toml(1 hunks)salesforce-sync/CHANGELOG.md(1 hunks)salesforce-sync/Dependencies.toml(1 hunks)salesforce-sync/README.md(1 hunks)salesforce-sync/database/sync_table_creation.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.1.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.10.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.11.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.12.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.13.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.14.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.2.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.4.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.5.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.6.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.7.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.8.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.9.sql(1 hunks)salesforce-sync/db_functions.bal(1 hunks)salesforce-sync/documents/salesforce-sync-deployment-diagram.drawio(1 hunks)salesforce-sync/main.bal(1 hunks)salesforce-sync/sf_functions.bal(1 hunks)salesforce-sync/sync_config.bal(1 hunks)salesforce-sync/types.bal(1 hunks)salesforce-sync/utils.bal(1 hunks)
🧰 Additional context used
🪛 LanguageTool
salesforce-sync/CHANGELOG.md
[style] ~68-~68: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ration record name -> DatabaseConf. - Renamed [types] ConfChoreoApp configuration r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~69-~69: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...iguration record name -> ChoreoApp. - Renamed [types] ConfErrorEmail configuration ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~76-~76: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t to fetch sync status information. - Removed [types] SyncInfo sync info record. ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~77-~77: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...types] SyncInfo sync info record. - Removed [types] SyncInfoItem sync info item r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~78-~78: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...yncInfoItemsync info item record. - Removed [types]SyncInfoResponse` info respons...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~79-~79: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...InfoResponseinfo response record. - Removed [types]ServiceErrorDetail` service er...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~80-~80: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tailservice error details record. - Removed [types]BadRequestError 400: Bad Req...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~81-~81: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...400: Bad Request response record. - Removed [types] InternalServerError `500: Int...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~82-~82: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rnal Server Errorresponse record. - Removed [utils]ackCaller()` HTTP caller ackno...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...HTTP caller acknowledge function. - Removed [utils]generateInfoPayload()` info pa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~84-~84: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...()` info payload generation function. - Removed the redundant caching functionalities w...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... functionalities with the new flow. - Removed [types] CacheKeys cache key enum. -...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...[types] CacheKeys cache key enum. - Removed [types] ServiceStatus service status ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~87-~87: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eStatusservice status cache enum. - Removed [utils]cacheInit()` cache initialisat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t()cache initialisation function. - Removed [utils]cacheReset()` cache reset func...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~89-~89: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cacheReset()cache reset function. - Removed [utils]cacheSyncBegin()` sync begin c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~90-~90: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Begin()sync begin cache function. - Removed [utils]cacheSyncEnd()` sync end cache...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~91-~91: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...heSyncEnd()` sync end cache function. - Removed failure email notification functionalit...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~92-~92: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...email notification functionalities. - Removed [types] ChoreoApp Choreo OAuth2 app c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eo OAuth2 app configuration record. - Removed [types] ErrorEmail error email config...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~94-~94: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...error email configuration record. - Removed [utils]sendErrorEmail()` error notifi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~120-~120: Ensure spelling is correct
Context: ...le)) - Renamed [types] ConfSalesforce Saleforce configuration record -> `ConfAuthSalesf...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~134-~134: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... annotation to Azure database client. - Added [sf_functions] display annotation to ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~143-~143: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...p:BadRequest->BadRequestError. - Replaced http:InternalServerError->Internal...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~148-~148: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Detailfor capturing service errors. - Added [types]BadRequestError` for returning...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~149-~149: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eturning bad request (400) responses. - Added [types] InternalServerError for retur...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~150-~150: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nternal server error (500) responses. - Added [types] SyncInfoResponse for returnin...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~182-~182: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eryResult()-> newquery()` method. - Updated [sf_functions] individual Salesforce st...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~183-~183: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... record type configurable sfConfig. - Updated [db_functions] individual database stri...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~184-~184: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ecord type configurable azureMySql. - Updated [records] ConfErrorEmail record field...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~185-~185: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...all capital snake-case to camel-case. - Updated [util] error email configuration name f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~186-~186: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e from ERR_EMAIL -> errEmailConf. - Updated [service] service port configuration na...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~187-~187: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...from SERVICE_PORT -> servicePort. - Updated [service] running mode check configurat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~188-~188: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tion name from IS_PROD -> isProd. - Updated [util] sync status check function name ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~189-~189: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...SyncStatus()->checkSyncStatus(). - Updated [util] cacheSyncEnd()` function to ret...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~194-~194: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...o capture Salesforce response errors. - Added [records] ConfDatabase configuration ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~195-~195: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ration record for Azure MySQL client. - Added [sync_config] sync interval period conf...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~202-~202: Ensure spelling is correct
Context: ...lean configurable to dynamically handle enviroment based varibales/logic. ## Version: 1.0.4...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~202-~202: Ensure spelling is correct
Context: ... to dynamically handle enviroment based varibales/logic. ## Version: 1.0.4 ### New - Adde...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~212-~212: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...any active syncs are being processed. - Added 400:Bad Request error response in `/{...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~213-~213: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...viding unsupported sync object types. - Added [ackCaller()] util function to ackowled...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~213-~213: Ensure spelling is correct
Context: ... - Added [ackCaller()] util function to ackowledge the service caller. _(added to avoid ME...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~222-~222: Ensure spelling is correct
Context: ... removed from the cache and dynamically fetcehd from the database. - Updated [cacheInit...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~224-~224: Use a hyphen to join words.
Context: ...tatus()] util function by using database fetched sync times for comparison, inste...
(QB_NEW_EN_HYPHEN)
[style] ~225-~225: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sing (now removed) cached sync times. - Updated [cacheSyncStatus()] util function by re...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~226-~226: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...(now removed) cached sync start time. - Updated [cacheSyncEnd()] util function by remov...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~227-~227: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ow removed) cached sync success time. - Updated [cacheSyncEnd()] util function by remov...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~276-~276: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...o insert/update Salesforce sync data. - Added database functions to maintain sync sta...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~277-~277: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...(processing, completed, failed) logs. - Added record types for each sync object and e...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~278-~278: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...service/database maintained statuses. - Added caching functions to cache/queue concur...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~279-~279: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...to cache/queue concurrent sync calls. - Added caching functions to maintain sync stat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (14)
salesforce-sync/.gitignore (1)
1-4: Appropriate ignore patterns for Ballerina projects.The configuration and build output patterns are standard and correct.
salesforce-sync/Ballerina.toml (1)
1-8: Package metadata is properly configured.Version alignment with changelog and appropriate observability settings for a production service.
salesforce-sync/database/sync_table_update_v1.0.5.sql (1)
1-5: Migration follows standard pattern for schema evolution.The TRUNCATE-then-ALTER approach is appropriate for adding columns to the Opportunity sync table. Column definition (DECIMAL(16,2)) aligns with changelog.
salesforce-sync/Dependencies.toml (1)
1-4: Auto-generated dependencies manifest is properly marked.This file should not be hand-edited as noted in the header. The dependency graph includes all necessary packages for Salesforce sync, MySQL operations, and observability.
salesforce-sync/database/sync_table_update_v1.0.6.sql (2)
1-1: Verify migration date accuracy.The comment shows "29/01/2024" but the PR is dated November 14, 2025. If this is a historical migration being included in the codebase migration, the date is acceptable. However, if this migration was recently created, the year should be updated to 2025 for consistency with the codebase timeline.
Confirm whether this migration is from the historical codebase (Jan 2024) or needs its date updated to reflect the current migration effort (2025).
3-5: Migration follows standard pattern for schema evolution.The TRUNCATE-then-ALTER approach is appropriate for adding columns to the Account sync table. Column definition (varchar(100)) aligns with existing Account schema patterns.
salesforce-sync/database/sync_table_update_v1.0.1.sql (1)
14-22: Risky column type conversions with existing data.The
CHANGE COLUMNoperations (lines 14–22) alter existing ARR columns from their prior type/constraints toDECIMAL(16,2) NOT NULL DEFAULT 0.00. If these columns already contain data, this could fail or truncate values. Ensure compatibility by:
- Testing against a production data snapshot first.
- Adding a pre-check or validation step to confirm no data loss.
- If any columns contain NULL values, populate them before adding
NOT NULLconstraint.Please verify these columns currently exist and contain no incompatible data types or NULL values that would fail this migration.
salesforce-sync/database/sync_table_update_v1.0.12.sql (1)
1-11: Second TRUNCATE onsf_opportunity– align with sync behavior and type expectationsAs with v1.0.10, this migration is structurally sound but:
TRUNCATE sf_opportunityagain assumes the table is purely a cache of Salesforce data. Please confirm that the deployment/runbook explicitly expects full repopulation and that no downstream consumer depends on historical rows surviving this migration.- The new columns (
Subs_*,ARR_Cloud_ARR__c,IAM_ARR_AND_Cloud__c,APIM_ARR_Cloud__c,Integration_ARR_AND_Cloud__c,Direct_Channel__c,Forecast_Type1__c) match the fields inSFOpportunitySyncRecordand in the OPPORTUNITY SOQL, which is good. Just ensure DB nullability (NULL DEFAULT 0.00for decimals) aligns with how the app treats these fields (nullable vs required).salesforce-sync/sync_config.bal (1)
8-10: Config defaults look reasonable
accountSyncandopportunitySyncdefaults (enabled with 4-hour period) are a sensible starting point and match the intent of periodic sync. No issues here.salesforce-sync/types.bal (4)
10-43: Enums and sync config record types look consistent
SyncObject,LogStatus,SfSyncConf, andSfSyncMinimalConfare well-structured and match their usage in the rest of the codebase. No issues here.
50-89: Auth and DB config models are straightforward – just ensure secrets handling practices
SalesforceAuthConfandDatabaseConfare simple and clear. From a code perspective this is fine; just make sure in deployment that:
refreshToken,clientId,clientSecret, and DB credentials are provided via secure config sources (env/secret storage) and never logged.
92-121: Account sync record matches the Account SOQL shape
SFAccountSyncRecordfields (includingOwner { Email, Name }) line up with the Account SOQL ingetSyncConfig, socloneWithTypeshould be happy as long as Salesforce actually returns those fields. Looks good.
234-239: RecordType wrapper type is aligned with SOQL usage
SFRecordTypewith nullableNamematchesRecordType.Namein the OPPORTUNITY query, and usingSFRecordType? RecordType;inSFOpportunitySyncRecordis a good fit. No changes needed.salesforce-sync/sf_functions.bal (1)
12-33: Salesforce client configuration looks goodThe
salesforce:Clientinitialization with configurable base URL/auth, sensible timeout, retry on 500/503, and disabled HTTP/1.1 keepalive is a solid setup for a cron-style sync service. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
salesforce-sync/CHANGELOG.md (1)
202-202: Add hyphen to compound adjective:environment-based.Line 202 should use a hyphenated compound adjective to follow standard English grammar conventions.
- Added `IS_PROD` boolean configurable to dynamically handle environment based variables/logic. + Added `IS_PROD` boolean configurable to dynamically handle environment-based variables/logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
salesforce-sync/CHANGELOG.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.210Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
🪛 LanguageTool
salesforce-sync/CHANGELOG.md
[style] ~68-~68: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ration record name -> DatabaseConf. - Renamed [types] ConfChoreoApp configuration r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~69-~69: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...iguration record name -> ChoreoApp. - Renamed [types] ConfErrorEmail configuration ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~76-~76: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t to fetch sync status information. - Removed [types] SyncInfo sync info record. ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~77-~77: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...types] SyncInfo sync info record. - Removed [types] SyncInfoItem sync info item r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~78-~78: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...yncInfoItemsync info item record. - Removed [types]SyncInfoResponse` info respons...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~79-~79: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...InfoResponseinfo response record. - Removed [types]ServiceErrorDetail` service er...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~80-~80: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tailservice error details record. - Removed [types]BadRequestError 400: Bad Req...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~81-~81: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...400: Bad Request response record. - Removed [types] InternalServerError `500: Int...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~82-~82: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rnal Server Errorresponse record. - Removed [utils]ackCaller()` HTTP caller ackno...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...HTTP caller acknowledge function. - Removed [utils]generateInfoPayload()` info pa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~84-~84: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...()` info payload generation function. - Removed the redundant caching functionalities w...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... functionalities with the new flow. - Removed [types] CacheKeys cache key enum. -...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...[types] CacheKeys cache key enum. - Removed [types] ServiceStatus service status ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~87-~87: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eStatusservice status cache enum. - Removed [utils]cacheInit()` cache initialisat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t()cache initialisation function. - Removed [utils]cacheReset()` cache reset func...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~89-~89: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cacheReset()cache reset function. - Removed [utils]cacheSyncBegin()` sync begin c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~90-~90: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Begin()sync begin cache function. - Removed [utils]cacheSyncEnd()` sync end cache...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~91-~91: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...heSyncEnd()` sync end cache function. - Removed failure email notification functionalit...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~92-~92: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...email notification functionalities. - Removed [types] ChoreoApp Choreo OAuth2 app c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eo OAuth2 app configuration record. - Removed [types] ErrorEmail error email config...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~94-~94: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...error email configuration record. - Removed [utils]sendErrorEmail()` error notifi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~134-~134: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... annotation to Azure database client. - Added [sf_functions] display annotation to ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~143-~143: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...p:BadRequest->BadRequestError. - Replaced http:InternalServerError->Internal...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~148-~148: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Detailfor capturing service errors. - Added [types]BadRequestError` for returning...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~149-~149: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eturning bad request (400) responses. - Added [types] InternalServerError for retur...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~150-~150: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nternal server error (500) responses. - Added [types] SyncInfoResponse for returnin...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~182-~182: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eryResult()-> newquery()` method. - Updated [sf_functions] individual Salesforce st...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~183-~183: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... record type configurable sfConfig. - Updated [db_functions] individual database stri...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~184-~184: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ecord type configurable azureMySql. - Updated [records] ConfErrorEmail record field...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~185-~185: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...all capital snake-case to camel-case. - Updated [util] error email configuration name f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~186-~186: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e from ERR_EMAIL -> errEmailConf. - Updated [service] service port configuration na...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~187-~187: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...from SERVICE_PORT -> servicePort. - Updated [service] running mode check configurat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~188-~188: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tion name from IS_PROD -> isProd. - Updated [util] sync status check function name ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~189-~189: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...SyncStatus()->checkSyncStatus(). - Updated [util] cacheSyncEnd()` function to ret...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~194-~194: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...o capture Salesforce response errors. - Added [records] ConfDatabase configuration ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~195-~195: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ration record for Azure MySQL client. - Added [sync_config] sync interval period conf...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~202-~202: Use a hyphen to join words.
Context: ...urable to dynamically handle environment based variables/logic. ## Version: 1.0.4...
(QB_NEW_EN_HYPHEN)
[style] ~212-~212: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...any active syncs are being processed. - Added 400:Bad Request error response in `/{...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~213-~213: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...viding unsupported sync object types. - Added [ackCaller()] util function to acknowle...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~225-~225: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sing (now removed) cached sync times. - Updated [cacheSyncStatus()] util function by re...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~226-~226: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...(now removed) cached sync start time. - Updated [cacheSyncEnd()] util function by remov...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~227-~227: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ow removed) cached sync success time. - Updated [cacheSyncEnd()] util function by remov...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~276-~276: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...o insert/update Salesforce sync data. - Added database functions to maintain sync sta...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~277-~277: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...(processing, completed, failed) logs. - Added record types for each sync object and e...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~278-~278: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...service/database maintained statuses. - Added caching functions to cache/queue concur...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~279-~279: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...to cache/queue concurrent sync calls. - Added caching functions to maintain sync stat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
salesforce-sync/main.bal (1)
66-89: Parallel instance guard is best-effort only (race window remains)
executeSync()checksdbCheckProcessing()before inserting the newPROCESSINGlog. In a multi-instance deployment, two instances can still pass the check concurrently and both insertPROCESSINGlogs, so this does not fully prevent overlapping runs across nodes.If you want stronger guarantees, consider making the “no-active-sync” check atomic with log creation (e.g., single-row lock,
INSERT ... WHERE NOT EXISTS, or a DB-side mutex pattern) so only one instance can transition from “no active log” to “PROCESSING” at a time.salesforce-sync/sf_functions.bal (2)
47-61: Remove redundantcheckvsis errorhandling aroundsalesforceEP->queryIn
sfQueryAccount, you both:
- use
check salesforceEP->query(...), which will propagate anyerror, and- declare
resultStreamasstream<...>|errorand testif resultStream is error.Because of
check,resultStreamwill never be anerrorat runtime, so thehandleSalesforceErrorpath is effectively dead code.To keep the centralized error mapping via
handleSalesforceError, you can drop thecheckand rely solely on the explicit error branch:-function sfQueryAccount(SfSyncConf syncConfig) returns SFAccountSyncRecord[]|error { +function sfQueryAccount(SfSyncConf syncConfig) returns SFAccountSyncRecord[]|error { log:printDebug(string `Querying ${ACCOUNT} object on Salesforce...`); - stream<SFAccountSyncRecord, error?>|error resultStream = check salesforceEP->query(syncConfig.soqlQuery); - if resultStream is error { - error err = resultStream; - return handleSalesforceError(err); - } + stream<SFAccountSyncRecord, error?>|error resultStream = + salesforceEP->query(syncConfig.soqlQuery); + if resultStream is error { + return handleSalesforceError(resultStream); + } - SFAccountSyncRecord[] sfRecords = - check from var result in resultStream - select check result.cloneWithType(); + SFAccountSyncRecord[] sfRecords = check from var result in resultStream + select check result.cloneWithType(); log:printDebug(string `SUCCESS: Received ${sfRecords.length()} records from Salesforce.`); return sfRecords; }Same pattern applies to
sfQueryOpportunity.
67-81: Mirror the same streamlined pattern for opportunity queriesOnce you update
sfQueryAccount, please mirror the same error-handling and streaming pattern insfQueryOpportunityfor consistency (no double-handling oferror, samehandleSalesforceErrorusage).salesforce-sync/db_functions.bal (1)
203-217: Minor: error message indbPrepareInitQueryreferences the wrong functionThe fallback error message mentions
[dbPrepareBatchQuery()]even though it’s emitted fromdbPrepareInitQuery. Not a functional issue, but can be confusing when debugging.Consider updating the text to reference
dbPrepareInitQuery()instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
salesforce-sync/db_functions.bal(1 hunks)salesforce-sync/main.bal(1 hunks)salesforce-sync/sf_functions.bal(1 hunks)salesforce-sync/sync_config.bal(1 hunks)salesforce-sync/types.bal(1 hunks)salesforce-sync/utils.bal(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- salesforce-sync/sync_config.bal
- salesforce-sync/utils.bal
- salesforce-sync/types.bal
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.210Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.783Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
📚 Learning: 2025-11-14T05:14:35.783Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.783Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Applied to files:
salesforce-sync/main.balsalesforce-sync/sf_functions.balsalesforce-sync/db_functions.bal
📚 Learning: 2025-11-14T05:08:35.210Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.210Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
Applied to files:
salesforce-sync/main.balsalesforce-sync/db_functions.bal
🔇 Additional comments (7)
salesforce-sync/main.bal (1)
30-61: Overall sync orchestration inmain()looks solidSequential per-object processing with config gating, up-to-date checks, and a single
dbShadowCopy()call guarded byisSyncExecutedis straightforward and matches the intended sync semantics. Thedo/on failblock provides a clear central error log path without leaking errors to the runtime.salesforce-sync/sf_functions.bal (1)
20-41: Salesforce client configuration is clear and idiomaticTop-level
salesforce:Clientconfiguration (base URL, auth, timeout, retry on 500/503, HTTP/1.1) is straightforward and should be easy to operate/debug.salesforce-sync/db_functions.bal (5)
31-42: DB client configuration looks appropriate for Azure MySQL
dbClientsetup with explicit host/user/password, UTCserverTimezone, andSSL_PREFERREDis sane for a cloud MySQL deployment and matches typical connector usage.
48-69: Batch sync logic indbUpdateSyncis consistentThe batch-count calculation and
int:range(0, batchCount, 1)loop work together to produce contiguous, non-overlapping slices, anddbPrepareInitQuery/dbPrepareBatchQueryseparation keeps the SQL generation clear.
80-123: Sync log insert/update flow is clear and robust
dbInsertSyncLogcleanly separates PROCESSING vs COMPLETED/FAILED behaviour, validateslogIndexfor end logs, and returns the insert ID for the start log, which is then reused for completion/failure. This matches howexecuteSyncinmain.baluses it.
162-191:dbGetLastSyncLogstatus guard and result handling look goodRestricting to
[COMPLETED, FAILED]and explicitly handlingsql:NoRowsErrorvs a realDBSyncLogTimesrow vs othererrortypes makes the behaviour of “no matching log vs error” very clear to the caller.
224-444: Account/opportunity batch query builders are consistent with the schemaThe
dbPrepareBatchQueryimplementations forSFAccountSyncRecord[]andSFOpportunitySyncRecord[]produce parameterizedINSERT ... ON DUPLICATE KEY UPDATEstatements that setIsInSF = 1on upsert, which aligns with the initialIsInSF = 0reset and the “mirror cache” semantics from the migrations.
saajidAha
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
salesforce-sync/types.bal (3)
37-50: Consider adding validation for periodInHours.The
periodInHoursfield in bothSfSyncConfandSfSyncMinimalConfuses thedecimaltype without constraints. Consider validating that this value is positive when the configuration is loaded to prevent invalid sync intervals.
122-125: Consider extracting the Owner record type to reduce duplication.The nested Owner record structure (with
Namefields) is duplicated betweenSFAccountSyncRecord(lines 122-125) andSFOpportunitySyncRecord(lines 233-236). Consider extracting this into a shared type:+# Salesforce owner details +# +# + Email - Owner email address +# + Name - Owner name +type SFOwner record { + string Email; + string Name; +}; + # [Salesforce] Account sync query record. # # + Id - Account ID # + Name - Account name ... # + Owner - Account owner details type SFAccountSyncRecord record { string Id; string Name; ... - record { - string Email; - string Name; - } Owner; + SFOwner Owner; };Apply the same change to
SFOpportunitySyncRecord.Also applies to: 233-236
182-238: Large record type may affect maintainability.
SFOpportunitySyncRecordcontains approximately 55 fields spanning over 100 lines. While this comprehensively maps the Salesforce Opportunity object, consider whether all fields are actively used. If subsets of fields are accessed in different contexts, you might benefit from creating focused projections or subsets of this type for specific use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
salesforce-sync/database/sync_table_update_v1.0.4.sql(1 hunks)salesforce-sync/database/sync_table_update_v1.0.5.sql(1 hunks)salesforce-sync/types.bal(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- salesforce-sync/database/sync_table_update_v1.0.4.sql
- salesforce-sync/database/sync_table_update_v1.0.5.sql
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.783Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.210Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
📚 Learning: 2025-11-14T05:14:35.783Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.783Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Applied to files:
salesforce-sync/types.bal
🔇 Additional comments (3)
salesforce-sync/types.bal (3)
19-29: LGTM! Well-defined enums.The enum definitions are clear and appropriately scoped for the sync service. Using string values for LogStatus is a good design choice for database persistence.
78-96: LGTM! Clean utility types.The error and logging types are well-defined with appropriate field types, particularly the use of
time:Utcfor timestamps.
58-76: No issues found; secure handling is properly implemented.Verification confirms that sensitive credentials in both
SalesforceAuthConfandDatabaseConfare handled securely throughout the codebase:
- Not logged in plaintext: All log statements (lines in utils.bal, sf_functions.bal, db_functions.bal, main.bal) only log non-sensitive context (object names, counts, timestamps). Sensitive fields never appear in logs.
- Loaded from secure sources: Both configs use Ballerina's
configurablepattern with external bindings (environment variables or config files), not hardcoded values.- Not exposed in error messages: Error handling passes only error messages and stack traces to logging—credential fields are never included.
Credentials are passed only to official client libraries (
salesforce:Client,mysql:Client), which handle them securely internally.
|
Let's rename the base directory to |
Co-authored-by: Suhan Dharmasuriya <suhand@users.noreply.github.com>
Co-authored-by: Suhan Dharmasuriya <suhand@users.noreply.github.com>
Co-authored-by: Suhan Dharmasuriya <suhand@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
salesforce-sync/main.bal (1)
24-24: Remove the owner comment per previous review feedback.The owner line should be removed as requested in the previous review.
Apply this diff:
# Sync Salesforce objects to a managed database. # -# Owner: YUK7HI # Version: 1.0.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
salesforce-sync/main.bal(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.783Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.210Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
📚 Learning: 2025-11-14T05:14:35.783Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.783Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Applied to files:
salesforce-sync/main.bal
📚 Learning: 2025-11-14T05:08:35.210Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.210Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
Applied to files:
salesforce-sync/main.bal
Co-authored-by: Suhan Dharmasuriya <suhand@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
salesforce-to-mysql-data-sync/db_functions.bal (1)
132-133: Fixtime:Utcconstruction (type mismatch).As previously noted, line 132 constructs
hourAgoUtcas a single-element tuple[int], buttime:Utcis defined asreadonly & [int, decimal](a 2-element tuple). This will fail type checking.Use
time:utcAddSecondsto properly compute the time one hour ago:- time:Utc hourAgoUtc = [time:utcNow()[0] - (60 * 60)]; - time:Civil hourAgoTime = time:utcToCivil(hourAgoUtc); + time:Utc now = time:utcNow(); + time:Utc hourAgoUtc = time:utcAddSeconds(now, -3600.0); + time:Civil hourAgoTime = time:utcToCivil(hourAgoUtc);
🧹 Nitpick comments (2)
salesforce-to-mysql-data-sync/db_functions.bal (2)
38-40: Consider using stricter SSL mode for production environments.The MySQL client uses
SSL_PREFERRED, which will use SSL if available but falls back to unencrypted connections. For production Azure MySQL deployments, consider usingmysql:SSL_REQUIREDto enforce encrypted connections and prevent fallback to unencrypted traffic.Apply this diff if stricter SSL enforcement is desired:
options = { serverTimezone: "UTC", ssl: { - mode: mysql:SSL_PREFERRED + mode: mysql:SSL_REQUIRED } }
51-51: Consider tracking the transaction optimization task.The TODO comment indicates that transactions should be enabled once database connectivity is optimized. This could impact data consistency if the process fails mid-batch.
Would you like me to open a new issue to track this optimization task and ensure it's addressed in a future iteration?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
salesforce-to-mysql-data-sync/Ballerina.toml(1 hunks)salesforce-to-mysql-data-sync/Dependencies.toml(1 hunks)salesforce-to-mysql-data-sync/db_functions.bal(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.791Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.231Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
📚 Learning: 2025-11-14T05:14:35.791Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.791Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Applied to files:
salesforce-to-mysql-data-sync/db_functions.bal
📚 Learning: 2025-11-14T05:08:35.231Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.231Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
Applied to files:
salesforce-to-mysql-data-sync/db_functions.bal
🔇 Additional comments (5)
salesforce-to-mysql-data-sync/db_functions.bal (4)
79-124: LGTM!The function correctly handles all log status cases with appropriate validation. The check for invalid logIndex (line 105-107) prevents updates with an uninitialized index, and the match expression comprehensively covers all status types.
161-190: LGTM!The function correctly validates that only completed or failed logs can be retrieved (lines 163-165) and handles all result cases appropriately, including the no-rows scenario.
195-214: LGTM!Both functions are well-implemented.
dbShadowCopyproperly closes the procedure result (line 198), anddbPrepareInitQueryuses type-safe dispatch with clear error handling for unsupported record types.
220-440: LGTM!The batch query generation is well-implemented with proper parameterization to prevent SQL injection. The use of
ON DUPLICATE KEY UPDATE(lines 252-266, 381-435) ensures idempotent upsert behavior, and theIsInSF = 1flag correctly marks synced records. Optional chaining forRecordType?.Name(lines 336, 391) appropriately handles nullable fields.salesforce-to-mysql-data-sync/Ballerina.toml (1)
5-5: No update required—version is current.Verification confirms that
2201.13.0matches the latest available Ballerina release. The project is already on the most current version and does not need updating.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
salesforce-to-mysql-data-sync/db_functions.bal (1)
125-149: CRITICAL: time:Utc construction is still incorrect despite past review.Line 127 creates a single-element tuple
[time:utcNow()[0] - (60 * 60)], buttime:Utcrequires a two-element tuple[int, decimal]representing seconds and fractional seconds from the epoch. This will fail type checking.The past review comment indicated this was addressed in commits 257c47c to 5bd8063, but the incorrect code is still present.
Apply this fix:
- time:Utc hourAgoUtc = [time:utcNow()[0] - (60 * 60)]; + time:Utc hourAgoUtc = time:utcAddSeconds(time:utcNow(), -3600.0);
🧹 Nitpick comments (1)
salesforce-to-mysql-data-sync/sf_functions.bal (1)
43-77: Consider extracting common query pattern.Both
sfQueryAccountandsfQueryOpportunityfollow identical logic (debug log → query → error handling → stream-to-array → success log). You could extract a generic helper:function sfQueryGeneric<T>(string objectName, string soqlQuery) returns T[]|error { log:printDebug(string `Querying ${objectName} object on Salesforce...`); stream<T, error?>|error resultStream = check salesforceEP->query(soqlQuery); if resultStream is error { return handleSalesforceError(resultStream); } T[] sfRecords = check from var result in resultStream select check result.cloneWithType(); log:printDebug(string `SUCCESS: Received ${sfRecords.length()} records from Salesforce.`); return sfRecords; }However, the current approach provides stronger type safety at compile time, so this refactor is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
salesforce-to-mysql-data-sync/database/sync_table_creation.sql(1 hunks)salesforce-to-mysql-data-sync/db_functions.bal(1 hunks)salesforce-to-mysql-data-sync/sf_functions.bal(1 hunks)salesforce-to-mysql-data-sync/sync_config.bal(1 hunks)salesforce-to-mysql-data-sync/types.bal(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- salesforce-to-mysql-data-sync/database/sync_table_creation.sql
- salesforce-to-mysql-data-sync/sync_config.bal
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.791Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.231Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
📚 Learning: 2025-11-14T05:14:35.791Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.791Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Applied to files:
salesforce-to-mysql-data-sync/sf_functions.balsalesforce-to-mysql-data-sync/db_functions.bal
📚 Learning: 2025-11-14T05:08:35.231Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.231Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
Applied to files:
salesforce-to-mysql-data-sync/db_functions.bal
🔇 Additional comments (13)
salesforce-to-mysql-data-sync/sf_functions.bal (2)
20-21: LGTM! Configurable variables properly defined.The required configurable pattern ensures Salesforce credentials and base URL must be provided at deployment, preventing runtime failures from missing configuration.
28-37: LGTM! Reasonable timeout and retry configuration.The 300-second timeout accommodates large SOQL queries, and the retry policy (3 attempts, 5-second interval) on 500/503 errors handles transient failures appropriately. Since SOQL queries are idempotent read operations, retries are safe.
salesforce-to-mysql-data-sync/db_functions.bal (6)
22-42: LGTM! Database configuration and client initialization are well-structured.The MySQL client configuration correctly sets UTC timezone to avoid DST issues and enables SSL with
SSL_PREFERREDmode for secure connections when available.
48-64: LGTM! Transaction ensures atomic sync operations.The transaction wraps both the initialization query (setting
IsInSF = 0) and batch inserts/updates, ensuring atomicity. The pattern of resettingIsInSFbefore sync allows detection of records removed from Salesforce. Error handling properly logs and returns errors while rolling back on failure.
74-119: LGTM! Log insertion/update logic is well-structured.The function correctly handles different log statuses:
PROCESSING: Inserts a new log entry and returns the insert IDCOMPLETED/FAILED: Updates the existing log entry usinglogIndexThe validation at line 100-102 properly prevents invalid updates when
logIndex = -1.
156-185: LGTM! Last sync log retrieval is properly implemented.The function correctly:
- Validates that only
COMPLETEDorFAILEDstatuses can be queried (lines 158-160)- Uses
ORDER BY id DESC LIMIT 1to fetch the most recent log- Handles the
NoRowsErrorcase by returningnil- Provides type-safe results via
DBSyncLogTimes
190-194: LGTM! Shadow copy procedure call is correctly implemented.The function properly calls the
arr_shadow_copy()stored procedure and closes the result handle to prevent resource leaks.
200-279: LGTM! Query generation functions handle both record types correctly.The functions properly:
- Use type-based dispatch to generate appropriate SQL for Account and Opportunity records
- Implement upsert behavior via
INSERT ... ON DUPLICATE KEY UPDATE- Set
IsInSF = 1during insert/update to mark records present in Salesforce- Return descriptive errors for unsupported record types
The pattern aligns with the sync strategy where
IsInSFflags indicate which records are currently in Salesforce.Based on learnings
salesforce-to-mysql-data-sync/types.bal (5)
18-29: LGTM! Enum definitions are clear and appropriate.The
SyncObjectandLogStatusenums provide type-safe constants for sync operations and log states.
31-47: LGTM! Sync configuration records are well-structured.The distinction between
SfSyncConf(with explicit SOQL query) andSfSyncMinimalConf(without query) provides flexibility for different configuration patterns. UsingdecimalforperiodInHoursallows fractional hour intervals.
49-71: LGTM! Configuration records properly structured for sensitive credentials.Both
SalesforceAuthConfandDatabaseConfare marked with[Configurable]in their documentation and use closed record syntax. These records are correctly used with Ballerina'sconfigurablekeyword insf_functions.balanddb_functions.bal, ensuring credentials are provided at runtime via configuration files or environment variables rather than hardcoded.
73-89: LGTM! Utility and database records properly defined.The
DBSyncLogTimesrecord correctly usestime:Utctype for timestamp fields, enabling proper time arithmetic and timezone handling.
91-129: LGTM! Salesforce sync records are appropriately defined.The record definitions are clean and straightforward:
- Optional fields (
BillingCountry?,ShippingCountry?) correctly use the?marker- Inline
Ownernested records provide clear structure- Field types align with Salesforce data types
The past review comments about missing optional markers on ARR and boolean fields no longer apply, as those fields have been removed from these simplified record definitions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
salesforce-to-mysql-data-sync/db_functions.bal (1)
127-128: Thetime:Utcconstruction is still incorrect.Line 127 creates a single-element tuple, but
time:Utcis defined asreadonly & [int, decimal](a 2-element tuple containing both seconds and fractional seconds). This will fail type checking.Although a previous review comment indicated this was addressed, the current code still shows the problematic construction.
Apply this diff to fix the time calculation:
- time:Utc hourAgoUtc = [time:utcNow()[0] - (60 * 60)]; - time:Civil hourAgoTime = time:utcToCivil(hourAgoUtc); + time:Utc now = time:utcNow(); + time:Utc hourAgoUtc = time:utcAddSeconds(now, -3600.0); + time:Civil hourAgoTime = time:utcToCivil(hourAgoUtc);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
salesforce-to-mysql-data-sync/.gitignore(1 hunks)salesforce-to-mysql-data-sync/db_functions.bal(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- salesforce-to-mysql-data-sync/.gitignore
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.231Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.791Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
📚 Learning: 2025-11-14T05:14:35.791Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_update_v1.0.14.sql:1-4
Timestamp: 2025-11-14T05:14:35.791Z
Learning: In the salesforce-sync project, TRUNCATE statements in migration files (e.g., salesforce-sync/database/sync_table_update_v*.sql) are intentional and follow a common pattern for the sync process. The database tables (sf_opportunity, sf_account) act as a cache/mirror of Salesforce data that can be fully reloaded from the Salesforce API, so data loss from TRUNCATE is not critical.
Applied to files:
salesforce-to-mysql-data-sync/db_functions.bal
📚 Learning: 2025-11-14T05:08:35.231Z
Learnt from: saajidAha
Repo: wso2-open-operations/common-tools PR: 1
File: salesforce-sync/database/sync_table_creation.sql:3-5
Timestamp: 2025-11-14T05:08:35.231Z
Learning: The file salesforce-sync/database/sync_table_creation.sql with DROP DATABASE IF EXISTS is intentionally destructive and only used for initial setup in local environments, not for existing running systems.
Applied to files:
salesforce-to-mysql-data-sync/db_functions.bal
🔇 Additional comments (5)
salesforce-to-mysql-data-sync/db_functions.bal (5)
44-64: Transaction implementation looks solid.The function correctly uses a transaction block with proper error handling. All queries are parameterized, which prevents SQL injection vulnerabilities.
66-119: Well-structured log management with proper validation.The pattern matching on
LogStatusis clean, and the validation ensuringlogIndex != -1for update operations prevents invalid updates. Good defensive programming.
151-185: Proper validation and error handling for log retrieval.The status validation and handling of the
NoRowsErrorcase are both correct. The query is properly parameterized and the separate variable assignment improves readability for the multi-line query.
187-194: LGTM!The stored procedure call is correct and the result is properly closed to prevent resource leaks.
211-279: Excellent use of parameterized queries and query expressions.The upsert pattern with
ON DUPLICATE KEY UPDATEis well-implemented, and the query expressions make the batch generation clean and maintainable. All queries are properly parameterized, preventing SQL injection.
4 participants
This PR introduces the salesforce-sync service, which synchronizes Salesforce Account and Opportunity objects with a specified managed SQL database.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.