From 71f9e379e1466838fe7c9a975b7ae7b6638f6d8d Mon Sep 17 00:00:00 2001
From: Tyler Gregg <greggt@amazon.com>
Date: Fri, 11 Oct 2024 16:35:24 -0700
Subject: [PATCH] Fixes #963 by properly setting the cursor's checkpoint after
 reading tagged values in an expression group.

---
 .../com/amazon/ion/impl/IonCursorBinary.java  | 19 +++++++++++++++++++
 ...onReaderContinuableTopLevelBinaryTest.java |  8 ++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java
index ce9a845bc..889605c8f 100644
--- a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java
+++ b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java
@@ -3349,6 +3349,23 @@ private boolean fillArgumentGroupPage(ArgumentGroupMarker group) {
         return false;
     }
 
+    /**
+     * Sets the checkpoint based on whether a scalar or container header has just been read. It is up to the caller
+     * to ensure that the cursor is positioned immediately after a value header.
+     */
+    private void setCheckpointAfterValueHeader() {
+        switch (event) {
+            case START_SCALAR:
+                setCheckpoint(CheckpointLocation.AFTER_SCALAR_HEADER);
+                break;
+            case START_CONTAINER:
+                setCheckpoint(CheckpointLocation.AFTER_CONTAINER_HEADER);
+                break;
+            default:
+                throw new IllegalStateException();
+        }
+    }
+
     /**
      * Positions the cursor on the next value in the tagged group. Upon return, the value will be filled and
      * `valueMarker` set to the value's start and end indices.
@@ -3381,6 +3398,7 @@ private Event nextGroupedTaggedValue(ArgumentGroupMarker group) {
                     return event;
                 }
                 isUserValue = uncheckedReadHeader(b, false, valueMarker);
+                setCheckpointAfterValueHeader();
             }
         } else {
             if (peekIndex == group.pageEndIndex) {
@@ -3392,6 +3410,7 @@ private Event nextGroupedTaggedValue(ArgumentGroupMarker group) {
                 return event;
             }
             isUserValue = uncheckedReadHeader(buffer[(int)(peekIndex++)] & SINGLE_BYTE_MASK, false, valueMarker);
+            setCheckpointAfterValueHeader();
         }
         valueTid = valueMarker.typeId;
         if (!isUserValue) {
diff --git a/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java
index fab606f4d..9655eeb60 100644
--- a/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java
+++ b/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java
@@ -5968,13 +5968,12 @@ public void invokeValuesUsingSystemMacroOpcode(String bytes) throws Exception {
     })
     public void invokeValuesWithExpressionGroupsUsingSystemMacroOpcode(String bytes) throws Exception {
         invokeValuesUsingSystemMacroOpcodeHelper(true, bytes);
-        // TODO: Determine why the checkpoint in the cursor isn't being set correctly before calling slowFillValue()
-        //       specifically while reading from a length-prefixed expression group
-        //       See https://github.com/amazon-ion/ion-java/issues/963
-        // invokeValuesUsingSystemMacroOpcodeHelper(false, bytes);
+        invokeValuesUsingSystemMacroOpcodeHelper(false, bytes);
     }
 
     private void invokeValuesUsingSystemMacroOpcodeHelper(boolean constructFromBytes, String bytes) throws Exception {
+        // Reset the byte counter between runs since this test utility method is called multiple times per test.
+        byteCounter.set(0);
         byte[] input = withIvm(1, hexStringToByteArray(cleanCommentedHexBytes(bytes)));
         readerBuilder = readerBuilder.withIncrementalReadingEnabled(false);
         reader = readerFor(readerBuilder, constructFromBytes, input);
@@ -5983,6 +5982,7 @@ private void invokeValuesUsingSystemMacroOpcodeHelper(boolean constructFromBytes
             next(IonType.INT), intValue(1),
             next(null)
         );
+        closeAndCount();
     }
 
     // TODO Ion 1.1 symbol tables with all kinds of annotation encodings (opcodes E4 - E9, inline and SID)