Fix Purchase.synchronizeReceipts re-submitting same receipt and firing callback multiple times

CodenameOne/src/com/codename1/payment/Purchase.java

@@ -52,6 +52,7 @@ public abstract class Purchase {
     private static final String RECEIPTS_KEY = "CN1SubscriptionsData.dat";
     private static final String RECEIPTS_REFRESH_TIME_KEY = "CN1SubscriptionsDataRefreshTime.dat";
     private static final String PENDING_PURCHASE_KEY = "PendingPurchases.dat";
+    private static final String PROCESSED_PURCHASE_KEY = "ProcessedPurchases.dat";
     private static final Object PENDING_PURCHASE_LOCK = new Object();
     private static final Object synchronizationLock = new Object();
     private static final Object receiptsLock = new Object();
@@ -458,39 +459,99 @@ public List<Receipt> getPendingPurchases() {
         }
     }
 
-    /// Adds a receipt to be pushed to the server.
+    /// Adds a receipt to be pushed to the server.  Receipts whose
+    /// `transactionId` has already been successfully submitted to the
+    /// `ReceiptStore` (recorded in the persistent processed set) or is
+    /// already sitting in the pending queue are silently dropped.  This
+    /// closes the abstraction so the user's `ReceiptStore.submitReceipt`
+    /// is invoked at most once per `transactionId` across the lifetime of
+    /// the install, regardless of platform-level redelivery quirks
+    /// (e.g. iOS StoreKit redelivering an unfinished transaction across
+    /// app sessions, or sandbox subscription renewals).
     ///
     /// #### Parameters
     ///
     /// - `receipt`: the receipt
     private void addPendingPurchase(Receipt receipt) {
         synchronized (PENDING_PURCHASE_LOCK) {
             Storage s = Storage.getInstance();
-            List<Receipt> pendingPurchases = getPendingPurchases();
-            pendingPurchases.add(receipt);
-            s.writeObject(PENDING_PURCHASE_KEY, pendingPurchases);
+            String txId = receipt.getTransactionId();
+            if (txId != null) {
+                if (getProcessedTransactionIds().contains(txId)) {
+                    return;
+                }
+                List<Receipt> pendingPurchases = getPendingPurchases();
+                for (Receipt r : pendingPurchases) {
+                    if (txId.equals(r.getTransactionId())) {
+                        return;
+                    }
+                }
+                pendingPurchases.add(receipt);
+                s.writeObject(PENDING_PURCHASE_KEY, pendingPurchases);
+            } else {
+                // Receipts without a transactionId can't be tracked in the
+                // processed set; fall back to enqueueing and let the
+                // synchronize path drain them.  receiptsMatch handles
+                // removal correctly when transactionId is null on both
+                // sides.
+                List<Receipt> pendingPurchases = getPendingPurchases();
+                pendingPurchases.add(receipt);
+                s.writeObject(PENDING_PURCHASE_KEY, pendingPurchases);
+            }
         }
     }
 
-    /// Removes a receipt from pending purchases.
+    /// Returns the persistent list of transactionIds that have already
+    /// been successfully submitted to the `ReceiptStore`.  The caller
+    /// must hold `PENDING_PURCHASE_LOCK`.
+    @SuppressWarnings("unchecked")
+    private List<String> getProcessedTransactionIds() {
+        Storage s = Storage.getInstance();
+        if (s.exists(PROCESSED_PURCHASE_KEY)) {
+            return (List<String>) s.readObject(PROCESSED_PURCHASE_KEY);
+        }
+        return new ArrayList<String>();
+    }
+
+    /// Records that the receipt with the given transactionId has been
+    /// successfully submitted to the `ReceiptStore`, so future
+    /// `addPendingPurchase` calls with the same transactionId skip the
+    /// enqueue.
+    private void recordProcessedTransactionId(String txId) {
+        if (txId == null) {
+            return;
+        }
+        synchronized (PENDING_PURCHASE_LOCK) {
+            List<String> processed = getProcessedTransactionIds();
+            if (!processed.contains(txId)) {
+                processed.add(txId);
+                Storage.getInstance().writeObject(PROCESSED_PURCHASE_KEY, processed);
+            }
+        }
+    }
+
+    /// Removes a receipt from pending purchases.  Receipts with a non-null
+    /// `transactionId` are matched on `transactionId`; receipts whose
+    /// `transactionId` is null fall back to matching on `sku`, `storeCode`,
+    /// `purchaseDate` and `orderData` so that the receipt is still removed
+    /// from the queue and `synchronizeReceipts` doesn't re-submit it forever.
     ///
     /// #### Parameters
     ///
-    /// - `transactionId`: the transaction id
+    /// - `target`: the receipt to remove
     ///
     /// #### Returns
     ///
-    /// the removed receipt
-    private Receipt removePendingPurchase(String transactionId) {
+    /// the removed receipt, or null if no matching receipt was found
+    private Receipt removePendingPurchase(Receipt target) {
         synchronized (PENDING_PURCHASE_LOCK) {
             Storage s = Storage.getInstance();
             List<Receipt> pendingPurchases = getPendingPurchases();
             Receipt found = null;
             for (Receipt r : pendingPurchases) {
-                if (r.getTransactionId() != null && r.getTransactionId().equals(transactionId)) {
+                if (receiptsMatch(r, target)) {
                     found = r;
                     break;
-
                 }
             }
             if (found != null) {
@@ -503,6 +564,25 @@ private Receipt removePendingPurchase(String transactionId) {
         }
     }
 
+    private static boolean receiptsMatch(Receipt a, Receipt b) {
+        String aTx = a.getTransactionId();
+        String bTx = b.getTransactionId();
+        if (aTx != null && bTx != null) {
+            return aTx.equals(bTx);
+        }
+        if (aTx != null || bTx != null) {
+            return false;
+        }
+        return nullSafeEquals(a.getSku(), b.getSku())
+                && nullSafeEquals(a.getStoreCode(), b.getStoreCode())
+                && nullSafeEquals(a.getPurchaseDate(), b.getPurchaseDate())
+                && nullSafeEquals(a.getOrderData(), b.getOrderData());
+    }
+
+    private static boolean nullSafeEquals(Object a, Object b) {
+        return a == null ? b == null : a.equals(b);
+    }
+
     public final void synchronizeReceipts() {
         if (syncInProgress) {
             return;
@@ -552,19 +632,30 @@ public final void synchronizeReceipts(final long ifOlderThanMs, final SuccessCal
             if (!pending.isEmpty() && receiptStore != null) {
 
                 final Receipt receipt = pending.get(0);
-                receiptStore.submitReceipt(pending.get(0), new SuccessCallback<Boolean>() {
+                receiptStore.submitReceipt(receipt, new SuccessCallback<Boolean>() {
 
                     @Override
                     public void onSucess(Boolean submitSucceeded) {
+                        // Reset syncInProgress before doing any work that can
+                        // throw so that a failure here doesn't permanently
+                        // wedge synchronizeReceipts in the "in progress"
+                        // state for the rest of the app's lifetime.
+                        syncInProgress = false;
                         if (submitSucceeded) {
-                            removePendingPurchase(receipt.getTransactionId());
-                            syncInProgress = false;
-
-                            // If the submit succeeded we need to refetch
-                            // so we set this to zero here.
-                            synchronizeReceipts(0, callback);
+                            // Record the transactionId before removing the
+                            // receipt from pending so a parallel
+                            // postReceipt re-enqueue (e.g. iOS redelivery)
+                            // is dropped by addPendingPurchase rather than
+                            // sneaking back into the queue.
+                            recordProcessedTransactionId(receipt.getTransactionId());
+                            removePendingPurchase(receipt);
+                            // Continue draining the queue.  The original
+                            // callback is already registered in
+                            // synchronizeReceiptsCallbacks; passing null here
+                            // avoids registering it again and firing it once
+                            // per drained receipt.
+                            synchronizeReceipts(0, null);
                         } else {
-                            syncInProgress = false;
                             fireSynchronizeReceiptsCallbacks(false);
                         }
                     }

maven/core-unittests/src/test/java/com/codename1/payment/PurchaseTest.java

@@ -24,6 +24,7 @@ class PurchaseTest extends UITestBase {
     private static final String RECEIPTS_STORAGE = "CN1SubscriptionsData.dat";
     private static final String RECEIPTS_REFRESH_STORAGE = "CN1SubscriptionsDataRefreshTime.dat";
     private static final String PENDING_STORAGE = "PendingPurchases.dat";
+    private static final String PROCESSED_STORAGE = "ProcessedPurchases.dat";
 
     private TestPurchase purchase;
 
@@ -56,6 +57,7 @@ private void resetPurchaseState() {
         storage.deleteStorageFile(RECEIPTS_STORAGE);
         storage.deleteStorageFile(RECEIPTS_REFRESH_STORAGE);
         storage.deleteStorageFile(PENDING_STORAGE);
+        storage.deleteStorageFile(PROCESSED_STORAGE);
         ReceiptStore clearingStore = new ReceiptStore() {
             public void fetchReceipts(SuccessCallback<Receipt[]> callback) {
                 callback.onSucess(new Receipt[0]);
@@ -243,6 +245,116 @@ void testSynchronizeReceiptsSyncReturnsFalseWhenSubmitFails() {
         assertEquals("silver", pending.get(0).getSku());
     }
 
+    @EdtTest
+    void testSynchronizeReceiptsSyncDrainsMultiplePendingReceipts() {
+        List<Receipt> pending = new ArrayList<Receipt>();
+        pending.add(createReceipt("gold", new Date(1000L), new Date(5000L)));
+        pending.add(createReceipt("silver", new Date(2000L), new Date(6000L)));
+        pending.add(createReceipt("bronze", new Date(3000L), new Date(7000L)));
+        Storage.getInstance().writeObject(PENDING_STORAGE, pending);
+
+        TestReceiptStore store = new TestReceiptStore();
+        purchase.setReceiptStore(store);
+
+        boolean success = purchase.synchronizeReceiptsSync(0);
+        assertTrue(success);
+        assertEquals(3, store.getSubmittedReceipts().size(),
+                "Each pending receipt should be submitted exactly once");
+        assertTrue(purchase.getPendingPurchases().isEmpty());
+    }
+
+    @EdtTest
+    void testSynchronizeReceiptsCallbackFiresOnceWhenDrainingMultiplePendingReceipts() {
+        List<Receipt> pending = new ArrayList<Receipt>();
+        pending.add(createReceipt("gold", new Date(1000L), new Date(5000L)));
+        pending.add(createReceipt("silver", new Date(2000L), new Date(6000L)));
+        pending.add(createReceipt("bronze", new Date(3000L), new Date(7000L)));
+        Storage.getInstance().writeObject(PENDING_STORAGE, pending);
+
+        TestReceiptStore store = new TestReceiptStore();
+        purchase.setReceiptStore(store);
+
+        final int[] callCount = new int[1];
+        final boolean[] result = new boolean[1];
+        purchase.synchronizeReceipts(0, new SuccessCallback<Boolean>() {
+            public void onSucess(Boolean value) {
+                callCount[0]++;
+                result[0] = value;
+            }
+        });
+        flushSerialCalls();
+
+        assertEquals(1, callCount[0],
+                "Synchronize callback must fire exactly once, not once per drained receipt");
+        assertTrue(result[0]);
+        assertEquals(3, store.getSubmittedReceipts().size());
+    }
+
+    @FormTest
+    void testPostReceiptSkipsReceiptThatWasAlreadySuccessfullySubmitted() {
+        // Simulate iOS StoreKit redelivering a transaction across app
+        // sessions: the same transactionId arrives via postReceipt after
+        // it was already successfully submitted in a prior cycle.
+        TestReceiptStore store = new TestReceiptStore();
+        purchase.setReceiptStore(store);
+
+        long purchaseTime = System.currentTimeMillis();
+        Purchase.postReceipt(Receipt.STORE_CODE_ITUNES, "pro", "tx-redelivery", purchaseTime, "order-1");
+        flushSerialCalls();
+
+        assertEquals(1, store.getSubmittedReceipts().size());
+        assertTrue(purchase.getPendingPurchases().isEmpty());
+
+        // Same transactionId arrives again (e.g. iOS redelivery on the
+        // next app launch).  Framework should drop it before it reaches
+        // the pending queue, so submitReceipt is not invoked a second
+        // time.
+        Purchase.postReceipt(Receipt.STORE_CODE_ITUNES, "pro", "tx-redelivery", purchaseTime, "order-1");
+        flushSerialCalls();
+
+        assertEquals(1, store.getSubmittedReceipts().size(),
+                "Receipt with already-processed transactionId must not be re-submitted");
+        assertTrue(purchase.getPendingPurchases().isEmpty());
+    }
+
+    @FormTest
+    void testPostReceiptSkipsDuplicateTransactionIdAlreadyPending() {
+        // Same transactionId queued twice before any sync happens (e.g.
+        // native layer fires postReceipt twice for one transaction).
+        // The second add must be silently dropped so the receipt is only
+        // submitted once when sync runs.
+        long purchaseTime = System.currentTimeMillis();
+        Purchase.postReceipt(Receipt.STORE_CODE_ITUNES, "pro", "tx-dupe", purchaseTime, "order-1");
+        Purchase.postReceipt(Receipt.STORE_CODE_ITUNES, "pro", "tx-dupe", purchaseTime, "order-1");
+        flushSerialCalls();
+
+        List<Receipt> pending = purchase.getPendingPurchases();
+        assertEquals(1, pending.size(),
+                "Duplicate transactionId enqueued before sync should be dropped at addPendingPurchase");
+    }
+
+    @EdtTest
+    void testSynchronizeReceiptsDoesNotInfinitelyResubmitReceiptWithNullTransactionId() {
+        // A receipt with a null transactionId must still be removable from the
+        // pending queue.  Otherwise synchronizeReceipts recurses forever,
+        // resubmitting the same receipt to the ReceiptStore on every iteration.
+        Receipt nullTxReceipt = createReceipt("orphan", new Date(1000L), new Date(5000L));
+        nullTxReceipt.setTransactionId(null);
+        List<Receipt> pending = new ArrayList<Receipt>();
+        pending.add(nullTxReceipt);
+        Storage.getInstance().writeObject(PENDING_STORAGE, pending);
+
+        CountingReceiptStore store = new CountingReceiptStore(5);
+        purchase.setReceiptStore(store);
+
+        boolean success = purchase.synchronizeReceiptsSync(0);
+        assertTrue(success);
+        assertEquals(1, store.getSubmittedReceipts().size(),
+                "Receipt with null transactionId should be submitted exactly once, not in a loop");
+        assertTrue(purchase.getPendingPurchases().isEmpty(),
+                "Receipt with null transactionId should be removed from pending queue after successful submit");
+    }
+
     @Test
     void testSynchronizeReceiptsSyncWaitsForAsyncFetch() {
         final Receipt asyncReceipt = createReceipt("async", new Date(1000L), new Date(5000L));
@@ -301,6 +413,35 @@ public void submitReceipt(Receipt receipt, SuccessCallback<Boolean> callback) {
         }
     }
 
+    /// Receipt store that refuses further submissions after a hard cap is hit
+    /// so a regression of the null-transactionId loop bug fails the test
+    /// instead of hanging it.
+    private static class CountingReceiptStore implements ReceiptStore {
+        private final List<Receipt> submitted = new ArrayList<Receipt>();
+        private final int maxSubmits;
+
+        CountingReceiptStore(int maxSubmits) {
+            this.maxSubmits = maxSubmits;
+        }
+
+        List<Receipt> getSubmittedReceipts() {
+            return new ArrayList<Receipt>(submitted);
+        }
+
+        public void fetchReceipts(SuccessCallback<Receipt[]> callback) {
+            callback.onSucess(new Receipt[0]);
+        }
+
+        public void submitReceipt(Receipt receipt, SuccessCallback<Boolean> callback) {
+            submitted.add(receipt);
+            if (submitted.size() > maxSubmits) {
+                throw new AssertionError("submitReceipt invoked more than " + maxSubmits
+                        + " times; pending receipt is being resubmitted in a loop");
+            }
+            callback.onSucess(Boolean.TRUE);
+        }
+    }
+
     private static class TestPurchase extends Purchase {
         private final List<String> recordedPurchases = new ArrayList<String>();
         private final List<String> recordedPromotionalPurchases = new ArrayList<String>();