refactor: Addressed issues related to integration; shopify-to-google-sheets - #81
refactor: Addressed issues related to integration; shopify-to-google-sheets#81moonlander101 wants to merge 1 commit into
Conversation
- feat: keep track of sheets with header already initialized
|
|
WalkthroughThe changes refactor Shopify order processing logic by marking multiple functions as Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ballerina-integrator/shopify-order-to-google-sheets/functions.bal (2)
124-135:⚠️ Potential issue | 🟠 MajorMake sheet creation idempotent for concurrent first writers.
This is still a check-then-create race. Two events targeting the same new sheet can both observe
"Sheet not found", then oneaddSheet()succeeds while the other returns an error and aborts that order. Please serialize creation persheetNameor treat an already-created result as success after a re-read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina-integrator/shopify-order-to-google-sheets/functions.bal` around lines 124 - 135, The current check-then-create is racy; instead of failing when sheetsClient->addSheet(...) returns an error, handle the concurrent-create case by re-reading the sheet and treating "already exists" as success or by serializing creation per sheetName. Concretely: in the branch where you call sheetsClient->addSheet(googleSheetsConfig.spreadsheetId, sheetName) and receive result is error, if the error indicates the sheet already exists (or on any addSheet error) call the same lookup you used earlier (the variable sheet from the initial check or call sheetsClient->getSheet(googleSheetsConfig.spreadsheetId, sheetName)), and if that re-read returns a valid sheets:Sheet treat the creation as successful and continue; only return the original addSheet error if the re-read still reports "Sheet not found" or another failure. Alternatively implement a per-sheet mutex keyed by sheetName to serialize calls to addSheet so only one create runs at a time.
31-92:⚠️ Potential issue | 🔴 CriticalRemove
isolatedannotation or declare configurable variables asreadonly.
applyFiltersis markedisolatedbut directly reads mutable module-level state:allowedCountryCodes,allowedCurrencies,allowedSources,allowedPaymentStatuses,allowedFulfillmentStatuses,requiredTags, andexcludedTagsare declared asconfigurable string[]inconfig.bal. Ballerina's isolation rules forbidisolatedfunctions from accessing mutable module variables without synchronization. Either remove theisolatedannotation or make these configurable variablesreadonlyto satisfy isolation constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina-integrator/shopify-order-to-google-sheets/functions.bal` around lines 31 - 92, The applyFilters function is declared isolated but reads mutable module-level configurable arrays (allowedCountryCodes, allowedCurrencies, allowedSources, allowedPaymentStatuses, allowedFulfillmentStatuses, requiredTags, excludedTags), violating Ballerina isolation rules; fix by either removing the isolated modifier from applyFilters or changing those configurable declarations in config.bal to readonly configurable (making them immutable at runtime) so isolated applyFilters can legally access them. Ensure you update either the applyFilters signature (remove isolated) or each configurable string[] declaration to readonly configurable string[] respectively.
🤖 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/shopify-order-to-google-sheets/functions.bal`:
- Around line 182-184: The check-and-record of initializedSheets must be atomic:
instead of checking initializedSheets.indexOf(sheetName) inside one lock and
pushing the sheetName in a separate lock later, acquire the lock once, check if
initializedSheets contains sheetName and if not immediately add (reserve)
sheetName to initializedSheets, then release the lock and proceed to write the
header; if header write fails remove the reservation in a locked section; update
the code that currently uses separate lock blocks around initializedSheets to
use this single reserve-then-write pattern referencing initializedSheets and
sheetName so concurrent threads cannot both initialize the same sheet.
- Line 104: The functions resolveSheetName, ensureSheetExists,
createRowFromEvent, addHeader, upsertOrderWithoutLineItems, and
upsertOrderWithLineItems are incorrectly marked isolated while they access the
module-level non-isolated final sheets:Client (sheetsClient); remove the
isolated keyword from each of those function declarations so they become normal
functions, update any callers if needed, and keep using the existing
sheetsClient instance as before.
---
Outside diff comments:
In `@ballerina-integrator/shopify-order-to-google-sheets/functions.bal`:
- Around line 124-135: The current check-then-create is racy; instead of failing
when sheetsClient->addSheet(...) returns an error, handle the concurrent-create
case by re-reading the sheet and treating "already exists" as success or by
serializing creation per sheetName. Concretely: in the branch where you call
sheetsClient->addSheet(googleSheetsConfig.spreadsheetId, sheetName) and receive
result is error, if the error indicates the sheet already exists (or on any
addSheet error) call the same lookup you used earlier (the variable sheet from
the initial check or call
sheetsClient->getSheet(googleSheetsConfig.spreadsheetId, sheetName)), and if
that re-read returns a valid sheets:Sheet treat the creation as successful and
continue; only return the original addSheet error if the re-read still reports
"Sheet not found" or another failure. Alternatively implement a per-sheet mutex
keyed by sheetName to serialize calls to addSheet so only one create runs at a
time.
- Around line 31-92: The applyFilters function is declared isolated but reads
mutable module-level configurable arrays (allowedCountryCodes,
allowedCurrencies, allowedSources, allowedPaymentStatuses,
allowedFulfillmentStatuses, requiredTags, excludedTags), violating Ballerina
isolation rules; fix by either removing the isolated modifier from applyFilters
or changing those configurable declarations in config.bal to readonly
configurable (making them immutable at runtime) so isolated applyFilters can
legally access them. Ensure you update either the applyFilters signature (remove
isolated) or each configurable string[] declaration to readonly configurable
string[] respectively.
🪄 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: 44c9a5a8-3b73-4be9-98b4-80d3e5b42b8c
📒 Files selected for processing (2)
ballerina-integrator/shopify-order-to-google-sheets/data_mappings.balballerina-integrator/shopify-order-to-google-sheets/functions.bal
| } | ||
|
|
||
| function resolveSheetName(shopify:OrderEvent event) returns string|error { | ||
| isolated function resolveSheetName(shopify:OrderEvent event) returns string|error { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected result:
# - connections.bal shows a plain final sheets:Client declaration
# - functions.bal shows the new isolated functions plus their sheetsClient call sites
echo "sheetsClient declaration:"
sed -n '1,20p' ballerina-integrator/shopify-order-to-google-sheets/connections.bal
echo
echo "isolated Sheets-facing functions:"
rg -n '^\s*isolated function (resolveSheetName|ensureSheetExists|createRowFromEvent|addHeader|upsertOrderWithoutLineItems|upsertOrderWithLineItems)\b' ballerina-integrator/shopify-order-to-google-sheets/functions.bal
echo
echo "sheetsClient call sites:"
rg -n 'sheetsClient->' ballerina-integrator/shopify-order-to-google-sheets/functions.balRepository: wso2/integration-samples
Length of output: 2804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the complete sheetsClient declaration with context
echo "=== Full sheetsClient declaration ==="
rg -A 10 'final sheets:Client sheetsClient' ballerina-integrator/shopify-order-to-google-sheets/connections.bal
echo
echo "=== Check for `@isolatedVariable` annotation ==="
rg -B 2 'final sheets:Client sheetsClient' ballerina-integrator/shopify-order-to-google-sheets/connections.bal
echo
echo "=== Check resolveSheetName function (line 104) implementation ==="
sed -n '104,120p' ballerina-integrator/shopify-order-to-google-sheets/functions.bal
echo
echo "=== Check ensureSheetExists function (line 121) implementation ==="
sed -n '121,140p' ballerina-integrator/shopify-order-to-google-sheets/functions.bal
echo
echo "=== Check createRowFromEvent function (line 141) implementation ==="
sed -n '141,180p' ballerina-integrator/shopify-order-to-google-sheets/functions.balRepository: wso2/integration-samples
Length of output: 3679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check addHeader function (line 181) implementation ==="
sed -n '181,200p' ballerina-integrator/shopify-order-to-google-sheets/functions.bal
echo
echo "=== Check upsertOrderWithoutLineItems function (line 277) implementation ==="
sed -n '277,300p' ballerina-integrator/shopify-order-to-google-sheets/functions.bal
echo
echo "=== Check upsertOrderWithLineItems function (line 301) implementation ==="
sed -n '301,330p' ballerina-integrator/shopify-order-to-google-sheets/functions.balRepository: wso2/integration-samples
Length of output: 3286
Remove isolated keyword from module-level Sheets functions—they directly access non-isolated sheetsClient.
All six functions (resolveSheetName, ensureSheetExists, createRowFromEvent, addHeader, upsertOrderWithoutLineItems, upsertOrderWithLineItems) are marked isolated but directly or transitively call methods on the module-level sheetsClient, which is declared as plain final sheets:Client without @isolatedVariable annotation. Isolated functions cannot access non-readonly module-level variables; removing the isolated keyword from these functions is required to comply with Ballerina's isolation semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/shopify-order-to-google-sheets/functions.bal` at line
104, The functions resolveSheetName, ensureSheetExists, createRowFromEvent,
addHeader, upsertOrderWithoutLineItems, and upsertOrderWithLineItems are
incorrectly marked isolated while they access the module-level non-isolated
final sheets:Client (sheetsClient); remove the isolated keyword from each of
those function declarations so they become normal functions, update any callers
if needed, and keep using the existing sheetsClient instance as before.
| lock { | ||
| if (initializedSheets.indexOf(sheetName) is int) { | ||
| return; |
There was a problem hiding this comment.
Reserve header initialization in one atomic step.
Line 183 checks the cache and Line 267 records success in separate lock blocks. Two concurrent orders for the same sheet can both miss initializedSheets and both try to write row 1, so the new guard does not actually prevent duplicate initialization under load.
Also applies to: 266-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/shopify-order-to-google-sheets/functions.bal` around
lines 182 - 184, The check-and-record of initializedSheets must be atomic:
instead of checking initializedSheets.indexOf(sheetName) inside one lock and
pushing the sheetName in a separate lock later, acquire the lock once, check if
initializedSheets contains sheetName and if not immediately add (reserve)
sheetName to initializedSheets, then release the lock and proceed to write the
header; if header write fails remove the reservation in a locked section; update
the code that currently uses separate lock blocks around initializedSheets to
use this single reserve-then-write pattern referencing initializedSheets and
sheetName so concurrent threads cannot both initialize the same sheet.
2 participants
Purpose
Some code refactors and feature changes requested in the original PR (#63).
Summary by CodeRabbit