Fix #598: Separate END_OBJECT return from parser close in ProtobufParser (#626)

cowtowncoder · web-flow · commit 4b08f7351225 · 2025-10-26T17:34:52.000-07:00
Previously, ProtobufParser would call close() while simultaneously returning
END_OBJECT at the top level, causing the parser to be in a closed state
immediately after returning the final token. This created incompatibility
with jackson-core issue #1438.

Solution:
- Added new parser state STATE_ROOT_END to separate the two actions
- Parser now returns END_OBJECT in STATE_ROOT_END state
- Parser only closes on the subsequent nextToken() call
- Updated all methods: nextToken(), nextName(), nextName(SerializableString),
  nextNameMatch(), and _skipUnknownField()
- Clear current name when transitioning to STATE_ROOT_END

Tests:
- Added ParserStateEndTest with 3 test methods verifying correct behavior
diff --git a/protobuf/src/main/java/tools/jackson/dataformat/protobuf/ProtobufParser.java b/protobuf/src/main/java/tools/jackson/dataformat/protobuf/ProtobufParser.java
@@ -54,9 +54,14 @@ public class ProtobufParser extends ParserMinimalBase
     // State after either reaching end-of-input, or getting explicitly closed
     private final static int STATE_CLOSED = 12;
 
+    // State after returning END_OBJECT for root level, before closing
+    // (added for issue #598 to separate END_OBJECT return from close())
+    //
+    // @since 3.1
+    private final static int STATE_ROOT_END = 13;
+
     private final static int[] UTF8_UNIT_CODES = ProtobufUtil.sUtf8UnitLengths;
 
-    // @since 2.14
     protected final static JacksonFeatureSet<StreamReadCapability> PROTOBUF_READ_CAPABILITIES
         = DEFAULT_READ_CAPABILITIES.with(StreamReadCapability.EXACT_FLOATS);
 
@@ -539,7 +544,8 @@ public JsonToken nextToken() throws JacksonException
             // end-of-input?
             if (_inputPtr >= _inputEnd) {
                 if (!loadMore()) {
-                    close();
+                    _state = STATE_ROOT_END;
+                    _streamReadContext.setCurrentName(null);
                     return _updateToken(JsonToken.END_OBJECT);
                 }
             }
@@ -634,9 +640,14 @@ public JsonToken nextToken() throws JacksonException
             return _updateToken(_readNextValue(_currentField.type, STATE_NESTED_KEY));
 
         case STATE_MESSAGE_END: // occurs if we end with array
-            close(); // sets state to STATE_CLOSED
+            _state = STATE_ROOT_END;
+            _streamReadContext.setCurrentName(null);
             return _updateToken(JsonToken.END_OBJECT);
 
+        case STATE_ROOT_END: // returned END_OBJECT, now close on next call
+            close(); // sets state to STATE_CLOSED
+            return null;
+
         case STATE_CLOSED:
             return null;
 
@@ -913,7 +924,8 @@ private JsonToken _skipUnknownField(int tag, int wireType, boolean nestedField)
                 }
             } else if (_inputPtr >= _inputEnd) {
                 if (!loadMore()) {
-                    close();
+                    _state = STATE_ROOT_END;
+                    _streamReadContext.setCurrentName(null);
                     return _updateToken(JsonToken.END_OBJECT);
                 }
             }
@@ -976,7 +988,8 @@ public String nextName() throws JacksonException
         if (_state == STATE_ROOT_KEY) {
             if (_inputPtr >= _inputEnd) {
                 if (!loadMore()) {
-                    close();
+                    _state = STATE_ROOT_END;
+                    _streamReadContext.setCurrentName(null);
                     _updateToken(JsonToken.END_OBJECT);
                     return null;
                 }
@@ -1051,7 +1064,8 @@ public String nextName() throws JacksonException
             return name;
         }
         if (_state == STATE_MESSAGE_END) {
-            close(); // sets state to STATE_CLOSED
+            _state = STATE_ROOT_END;
+            _streamReadContext.setCurrentName(null);
             _updateToken(JsonToken.END_OBJECT);
             return null;
         }
@@ -1064,7 +1078,8 @@ public boolean nextName(SerializableString sstr) throws JacksonException
         if (_state == STATE_ROOT_KEY) {
             if (_inputPtr >= _inputEnd) {
                 if (!loadMore()) {
-                    close();
+                    _state = STATE_ROOT_END;
+                    _streamReadContext.setCurrentName(null);
                     _updateToken(JsonToken.END_OBJECT);
                     return false;
                 }
@@ -1137,7 +1152,8 @@ public boolean nextName(SerializableString sstr) throws JacksonException
             return name.equals(sstr.getValue());
         }
         if (_state == STATE_MESSAGE_END) {
-            close(); // sets state to STATE_CLOSED
+            _state = STATE_ROOT_END;
+            _streamReadContext.setCurrentName(null);
             _updateToken(JsonToken.END_OBJECT);
             return false;
         }
@@ -1150,7 +1166,8 @@ public int nextNameMatch(PropertyNameMatcher matcher) throws JacksonException
         if (_state == STATE_ROOT_KEY) {
             if (_inputPtr >= _inputEnd) {
                 if (!loadMore()) {
-                    close();
+                    _state = STATE_ROOT_END;
+                    _streamReadContext.setCurrentName(null);
                     _updateToken(JsonToken.END_OBJECT);
                     return PropertyNameMatcher.MATCH_END_OBJECT;
                 }
@@ -1227,7 +1244,8 @@ public int nextNameMatch(PropertyNameMatcher matcher) throws JacksonException
             return matcher.matchName(name);
         }
         if (_state == STATE_MESSAGE_END) {
-            close(); // sets state to STATE_CLOSED
+            _state = STATE_ROOT_END;
+            _streamReadContext.setCurrentName(null);
             _updateToken(JsonToken.END_OBJECT);
             return PropertyNameMatcher.MATCH_END_OBJECT;
         }
diff --git a/protobuf/src/test/java/tools/jackson/dataformat/protobuf/ParserStateEndTest.java b/protobuf/src/test/java/tools/jackson/dataformat/protobuf/ParserStateEndTest.java
@@ -0,0 +1,150 @@
+package tools.jackson.dataformat.protobuf;
+
+import org.junit.jupiter.api.Test;
+
+import tools.jackson.core.*;
+
+import tools.jackson.dataformat.protobuf.schema.ProtobufSchema;
+import tools.jackson.dataformat.protobuf.schema.ProtobufSchemaLoader;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Test for issue #598: Protobuf parser state handling wrong for implicit close (END_OBJECT)
+ */
+public class ParserStateEndTest extends ProtobufTestBase
+{
+    private final ProtobufMapper MAPPER = newObjectMapper();
+
+    /**
+     * Test that verifies the parser properly handles the end-of-input state.
+     * The parser should NOT be closed when returning the final END_OBJECT token;
+     * it should only be closed on the subsequent nextToken() call.
+     */
+    @Test
+    public void testParserStateAtEndObject() throws Exception
+    {
+        // Use a simple Point schema
+        ProtobufSchema schema = ProtobufSchemaLoader.std.parse(PROTOC_POINT);
+
+        // Create test data
+        Point input = new Point(42, 13);
+        byte[] bytes = MAPPER.writerFor(Point.class)
+                .with(schema)
+                .writeValueAsBytes(input);
+
+        // Parse with streaming parser
+        try (JsonParser p = MAPPER.reader()
+                .with(schema)
+                .createParser(bytes)) {
+            assertToken(JsonToken.START_OBJECT, p.nextToken());
+    
+            // First field: "x"
+            assertToken(JsonToken.PROPERTY_NAME, p.nextToken());
+            assertEquals("x", p.currentName());
+    
+            assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken());
+            assertEquals(42, p.getIntValue());
+    
+            // Second field: "y"
+            assertToken(JsonToken.PROPERTY_NAME, p.nextToken());
+            assertEquals("y", p.currentName());
+    
+            assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken());
+            assertEquals(13, p.getIntValue());
+    
+            // END_OBJECT - This is the critical test
+            assertToken(JsonToken.END_OBJECT, p.nextToken());
+    
+            // THIS IS THE KEY ASSERTION: Parser should NOT be closed yet
+            // The parser should only be closed on the NEXT nextToken() call
+            assertFalse(p.isClosed(),
+                "Parser should NOT be closed immediately after returning END_OBJECT");
+    
+            // Verify currentToken() returns END_OBJECT (not null)
+            assertEquals(JsonToken.END_OBJECT, p.currentToken(),
+                "currentToken() should return END_OBJECT, not null");
+    
+            // Now the next token should be null AND close the parser
+            assertNull(p.nextToken(), "After END_OBJECT, nextToken() should return null");
+            assertTrue(p.isClosed(), "Parser should be closed after nextToken() returns null");
+        }
+    }
+
+    /**
+     * Similar test but using nextName() optimization
+     */
+    @Test
+    public void testParserStateAtEndObjectWithNextName() throws Exception
+    {
+        ProtobufSchema schema = ProtobufSchemaLoader.std.parse(PROTOC_POINT);
+
+        Point input = new Point(42, 13);
+        byte[] bytes = MAPPER.writerFor(Point.class)
+                .with(schema)
+                .writeValueAsBytes(input);
+        
+        try (JsonParser p = MAPPER.reader()
+                .with(schema)
+                .createParser(bytes)) {
+            assertToken(JsonToken.START_OBJECT, p.nextToken());
+    
+            // Use nextName() for field access
+            assertEquals("x", p.nextName());
+            assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken());
+    
+            assertEquals("y", p.nextName());
+            assertToken(JsonToken.VALUE_NUMBER_INT, p.nextToken());
+    
+            // Should get null from nextName() at end
+            assertNull(p.nextName());
+    
+            // Current token should be END_OBJECT
+            assertEquals(JsonToken.END_OBJECT, p.currentToken(),
+                "currentToken() should return END_OBJECT after nextName() returns null");
+    
+            // Parser should NOT be closed yet
+            assertFalse(p.isClosed(),
+                "Parser should NOT be closed when currentToken is END_OBJECT");
+    
+            // Next token should be null and close parser
+            assertNull(p.nextToken());
+            assertTrue(p.isClosed());
+        }
+    }
+
+    /**
+     * Test with empty message (no fields)
+     */
+    @Test
+    public void testParserStateWithEmptyMessage() throws Exception
+    {
+        final String PROTOC_EMPTY = "message Empty {}\n";
+        ProtobufSchema schema = ProtobufSchemaLoader.std.parse(PROTOC_EMPTY);
+
+        // Empty message = just START_OBJECT, END_OBJECT
+        byte[] bytes = MAPPER.writer()
+                .with(schema)
+                .writeValueAsBytes(new Object());
+
+        try (JsonParser p = MAPPER.reader()
+                .with(schema)
+                .createParser(bytes)) {
+            // START_OBJECT
+            assertToken(JsonToken.START_OBJECT, p.nextToken());
+            assertFalse(p.isClosed());
+    
+            // END_OBJECT immediately
+            assertToken(JsonToken.END_OBJECT, p.nextToken());
+    
+            // Parser should NOT be closed yet
+            assertFalse(p.isClosed(),
+                "Parser should NOT be closed immediately after END_OBJECT");
+            assertEquals(JsonToken.END_OBJECT, p.currentToken());
+    
+            // Next token closes
+            assertNull(p.nextToken());
+            assertTrue(p.isClosed());
+        }
+    }
+}
diff --git a/release-notes/VERSION b/release-notes/VERSION
@@ -16,6 +16,8 @@ implementations)
 
 3.1.0 (not yet released)
 
+#598: (protobuf) Protobuf parser state handling wrong for implicit close (END_OBJECT)
+ (fix by @cowtowncoder, w/ Claude code)
 #619: (avro, cbor, ion, smile) Add `isEnabled()` methods for format-specific features
   (like `CBORReadFeature` and `CBORWriteFeature`) to mappers
  (requested by Andy W)
