Skip to content

refactor: Addressed issues related to integration; shopify-to-google-sheets #81

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
moonlander101 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: Addressed issues related to integration; shopify-to-google-sheets #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 24 additions & 46 deletions ballerina-integrator/shopify-order-to-google-sheets/data_mappings.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,78 +4,56 @@ import ballerina/time;
# Formats a date string according to the configured date format
# + dateString - The input date string (RFC 3339 format from Shopify)
# + return - Formatted date string or original if formatting fails/not configured
function formatDate(string? dateString) returns string {
isolated function formatDate(string? dateString) returns string {
if dateString is () {
return "";
}
if dateFormat == "default" || dateString.trim() == "" {
return dateString;
}

time:Civil|time:Error civilResult = time:civilFromString(dateString);
if civilResult is time:Error {
return dateString;
}

time:Civil civil = civilResult;

if dateFormat == "iso8601" {
string|time:Error formatted = time:civilToString(civil);
if formatted is string {
return formatted;
match dateFormat {
"iso8601" => {
string|time:Error formatted = time:civilToString(civilResult);
if formatted is string {
return formatted;
}
}
} else if dateFormat == "rfc5322" {
string|time:Error formatted = time:civilToEmailString(civil, time:PREFER_ZONE_OFFSET);
if formatted is string {
return formatted;
"rfc5322" => {
string|time:Error formatted = time:civilToEmailString(civilResult, time:PREFER_ZONE_OFFSET);
if formatted is string {
return formatted;
}
}
}

return dateString;
}

# Helper function to extract discount codes
# + codes - Array of discount code objects from Shopify order
# + return - Comma-separated string of discount codes or empty string if none
function getDiscountCodes(shopify:DiscountCode[]? codes) returns string {
isolated function getDiscountCodes(shopify:DiscountCode[]? codes) returns string {
if codes is () || codes.length() == 0 {
return "";
}
string[] codeList = from var code in codes select code?.code ?: "";
return string:'join(", ", ...codeList);
}
string[] codeList = from var code in codes let string c = code?.code ?: "" where c != "" select c;

# Helper function to extract shipping method
# + lines - Array of shipping line objects from Shopify order
# + return - Shipping method title or empty string if not available
function getShippingMethod(shopify:ShippingLine[]? lines) returns string {
if lines is () || lines.length() == 0 {
return "";
}
return lines[0]?.title ?: "";
return string:'join(", ", ...codeList);
}

# Helper function to get shipping price
# Helper function to retrieve the first shipping line
# + lines - Array of shipping line objects from Shopify order
# + return - Shipping price as string or "0.00" if not available
function getShippingPrice(shopify:ShippingLine[]? lines) returns string {
# + return - The first ShippingLine record, or nil if not available
isolated function getFirstShippingLine(shopify:ShippingLine[]? lines) returns shopify:ShippingLine? {
if lines is () || lines.length() == 0 {
return "0.00";
}
return lines[0]?.price ?: "0.00";
}

# Helper function to get customer ID
# + customerId - Customer ID from Shopify order (nullable)
# + return - Customer ID as string or empty string if not available
function getCustomerId(int? customerId) returns string {
if customerId is () {
return "";
return ();
}
return customerId.toString();
return lines[0];
}

function eventToRowData(shopify:OrderEvent event) returns (int|string|decimal)[] => [
isolated function eventToRowData(shopify:OrderEvent event) returns (int|string|decimal)[] => [
// Order Identifiers
event?.id.toString(),
event?.order_number ?: "",
Expand All @@ -87,7 +65,7 @@ function eventToRowData(shopify:OrderEvent event) returns (int|string|decimal)[]
event?.subtotal_price ?: "0.00",
event?.total_tax ?: "0.00",
event?.total_discounts ?: "0.00",
getShippingPrice(event?.shipping_lines),
getFirstShippingLine(event?.shipping_lines)?.price ?: "0.00",
event?.total_line_items_price ?: "0.00",
event?.currency ?: "",

Expand All @@ -102,7 +80,7 @@ function eventToRowData(shopify:OrderEvent event) returns (int|string|decimal)[]
formatDate(event?.cancelled_at),

// Customer Information
getCustomerId(event?.customer?.id),
event?.customer?.id ?: "",
event?.email ?: "",
event?.customer?.first_name ?: "",
event?.customer?.last_name ?: "",
Expand All @@ -120,7 +98,7 @@ function eventToRowData(shopify:OrderEvent event) returns (int|string|decimal)[]
event?.shipping_address?.country ?: "",
event?.shipping_address?.country_code ?: "",
event?.shipping_address?.phone ?: "",
getShippingMethod(event?.shipping_lines),
getFirstShippingLine(event?.shipping_lines)?.title ?: "",

// Billing Address
event?.billing_address?.first_name ?: "",
Expand Down
88 changes: 46 additions & 42 deletions ballerina-integrator/shopify-order-to-google-sheets/functions.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,32 @@ import ballerina/time;
import ballerinax/googleapis.sheets;
import ballerinax/trigger.shopify;

isolated string[] initializedSheets = [];

# Extracts line item details and creates a row for each line item
# + rowValues - The base row values from the order event
# + lineItems - The list of line items from the order event
# + return - A list of rows with line item details included
isolated function expandLineItems((int|string|decimal)[] rowValues, shopify:LineItem[] lineItems) returns (int|string|decimal)[][] {
(int|string|decimal)[][] allRows = [];
foreach var item in lineItems {
(int|string|decimal)[] lineItemValues = rowValues.clone();
lineItemValues.push(item?.title ?: "");
lineItemValues.push(item?.sku ?: "");
lineItemValues.push(item?.variant_title ?: "");
lineItemValues.push(item?.quantity ?: 0);
lineItemValues.push(item?.price ?: "0.00");
lineItemValues.push(item?.product_id.toString());
lineItemValues.push(item?.variant_id.toString());
allRows.push(lineItemValues);
}
return allRows;
}

# Apply filters to determine if order should be processed
# + event - The order event
# + return - True if order should be filtered out (skipped), false if it should be processed
function applyFilters(shopify:OrderEvent event) returns boolean {
isolated function applyFilters(shopify:OrderEvent event) returns boolean {
string orderNumber = event?.order_number.toString();

if allowedCountryCodes.length() > 0 {
Expand Down Expand Up @@ -51,21 +73,17 @@ function applyFilters(shopify:OrderEvent event) returns boolean {

string orderTags = event?.tags ?: "";
string:RegExp commaPattern = re `,`;
string[] orderTagList = orderTags == "" ? [] : from string tag in commaPattern.split(orderTags) select tag.trim();

string[] tempTagList = orderTags == "" ? [] : from string tag in commaPattern.split(orderTags) select tag.trim();
final string[] & readonly orderTagList = tempTagList.cloneReadOnly();

if requiredTags.length() > 0 {
if orderTagList.length() == 0 {
log:printWarn(string `Filter failed for order ${orderNumber}: No tags found, but required tags are configured`);
return true;
}
boolean hasRequiredTag = false;
foreach string requiredTag in requiredTags {
if orderTagList.indexOf(requiredTag) !is () {
hasRequiredTag = true;
break;
}
}
if !hasRequiredTag {

if !requiredTags.some(isolated function(string tag) returns boolean => orderTagList.indexOf(tag) is int) {
log:printWarn(string `Filter failed for order ${orderNumber}: Order tags '${orderTags}' do not contain any required tags`);
return true;
}
Expand All @@ -83,7 +101,7 @@ function applyFilters(shopify:OrderEvent event) returns boolean {
return false;
}

function resolveSheetName(shopify:OrderEvent event) returns string|error {
isolated function resolveSheetName(shopify:OrderEvent event) returns string|error {

@coderabbitai coderabbitai Bot Mar 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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.bal

Repository: 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.bal

Repository: 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.bal

Repository: 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.

if !groupByMonth {
check ensureSheetExists(sheetName);
return sheetName;
Expand All @@ -100,7 +118,7 @@ function resolveSheetName(shopify:OrderEvent event) returns string|error {
return name;
}

function ensureSheetExists(string sheetName) returns error? {
isolated function ensureSheetExists(string sheetName) returns error? {
sheets:Sheet|error sheet = sheetsClient->getSheetByName(googleSheetsConfig.spreadsheetId, sheetName);

if sheet is error {
Expand All @@ -120,7 +138,7 @@ function ensureSheetExists(string sheetName) returns error? {
# create row from order event
# + event - The order event
# + return - Error if operation fails
function createRowFromEvent(shopify:OrderEvent event) returns error? {
isolated function createRowFromEvent(shopify:OrderEvent event) returns error? {
log:printDebug(string `Received order event: ${event?.order_number.toString()}`);
boolean shouldFilter = applyFilters(event);
if shouldFilter {
Expand All @@ -129,6 +147,7 @@ function createRowFromEvent(shopify:OrderEvent event) returns error? {

string sheetName = check resolveSheetName(event);
check addHeader(sheetName);

(int|string|decimal)[] rowValues = eventToRowData(event);
sheets:A1Range a1Range = {
sheetName: sheetName,
Expand All @@ -141,19 +160,7 @@ function createRowFromEvent(shopify:OrderEvent event) returns error? {
var lineItems = event?.line_items;

if !(lineItems is ()) && lineItems.length() > 0 {
(int|string|decimal)[][] allRows = [];
foreach var item in lineItems {
(int|string|decimal)[] lineItemValues = rowValues.clone();
lineItemValues.push(item?.title ?: "");
lineItemValues.push(item?.sku ?: "");
lineItemValues.push(item?.variant_title ?: "");
lineItemValues.push(item?.quantity ?: 0);
lineItemValues.push(item?.price ?: "0.00");
lineItemValues.push(item?.product_id ?: "");
lineItemValues.push(item?.variant_id ?: "");
allRows.push(lineItemValues);
}

(int|string|decimal)[][] allRows = expandLineItems(rowValues, lineItems);
_ = check sheetsClient->appendValues(googleSheetsConfig.spreadsheetId, allRows, a1Range);
return;
}
Expand All @@ -166,14 +173,18 @@ function createRowFromEvent(shopify:OrderEvent event) returns error? {
check upsertOrderWithoutLineItems(event, rowValues, a1Range, sheetName);
}
}

return;
}

# Adds the header row for an empty sheet
# + sheetName - Target sheet name
# + return - Error if operation fails
function addHeader(string sheetName) returns error? {
isolated function addHeader(string sheetName) returns error? {
lock {
if (initializedSheets.indexOf(sheetName) is int) {
return;
Comment on lines +182 to +184

@coderabbitai coderabbitai Bot Mar 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}
}

sheets:Row firstRow = check sheetsClient->getRow(googleSheetsConfig.spreadsheetId, sheetName, 1);
(int|string|decimal)[] firstRowValues = firstRow.values;

Expand Down Expand Up @@ -252,6 +263,9 @@ function addHeader(string sheetName) returns error? {
}
check sheetsClient->createOrUpdateRow(googleSheetsConfig.spreadsheetId, sheetName, 1, headerRow);
}
lock {
initializedSheets.push(sheetName);
}
}

# Upsert order without line items (single row per order)
Expand All @@ -260,7 +274,7 @@ function addHeader(string sheetName) returns error? {
# + a1Range - The A1 range for appending
# + sheetName - Target sheet name
# + return - Error if operation fails
function upsertOrderWithoutLineItems(shopify:OrderEvent event, (int|string|decimal)[] rowValues, sheets:A1Range a1Range, string sheetName) returns error? {
isolated function upsertOrderWithoutLineItems(shopify:OrderEvent event, (int|string|decimal)[] rowValues, sheets:A1Range a1Range, string sheetName) returns error? {
sheets:Column orderNumberRowData = check sheetsClient->getColumn(googleSheetsConfig.spreadsheetId, sheetName, "B");
(int|string|decimal)[] orderNums = orderNumberRowData.values;

Expand All @@ -284,7 +298,7 @@ function upsertOrderWithoutLineItems(shopify:OrderEvent event, (int|string|decim
# + a1Range - The A1 range for appending
# + sheetName - Target sheet name
# + return - Error if operation fails
function upsertOrderWithLineItems(shopify:OrderEvent event, (int|string|decimal)[] rowValues, sheets:A1Range a1Range, string sheetName) returns error? {
isolated function upsertOrderWithLineItems(shopify:OrderEvent event, (int|string|decimal)[] rowValues, sheets:A1Range a1Range, string sheetName) returns error? {
sheets:Column orderNumberRowData = check sheetsClient->getColumn(googleSheetsConfig.spreadsheetId, sheetName, "B");
(int|string|decimal)[] orderNums = orderNumberRowData.values;

Expand All @@ -300,17 +314,7 @@ function upsertOrderWithLineItems(shopify:OrderEvent event, (int|string|decimal)
var lineItems = event?.line_items;
(int|string|decimal)[][] allRows = [];
if !(lineItems is ()) && lineItems.length() > 0 {
foreach var item in lineItems {
(int|string|decimal)[] lineItemValues = rowValues.clone();
lineItemValues.push(item?.title ?: "");
lineItemValues.push(item?.sku ?: "");
lineItemValues.push(item?.variant_title ?: "");
lineItemValues.push(item?.quantity ?: 0);
lineItemValues.push(item?.price ?: "0.00");
lineItemValues.push(item?.product_id ?: "");
lineItemValues.push(item?.variant_id ?: "");
allRows.push(lineItemValues);
}
allRows = expandLineItems(rowValues, lineItems);
} else {
// No line items, just use base row
allRows.push(rowValues);
Expand Down