From f54f4affddadba886f3295af3b9b98644b921397 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Wed, 28 Oct 2026 09:20:09 +0100 Subject: [PATCH] THRIFT-3669: Keep the lib/js TJSONProtocol read stack aligned after empty lists Client: js readListBegin pushes the first element of a list onto the read stack, and readListEnd popped a single entry before it pushed the next element of the enclosing list. After a non-empty list that entry is the list itself. An empty list has no first element and leaves an undefined entry above itself, so the single pop left the emptied list on the stack, and the next element went on top of it. The stack stayed out of step from there, and the elements of the enclosing list that followed were lost or read from the wrong place: [[],[],[1,2]] came back as [[],[],[]]. Sets end through readListEnd and were affected the same way. readListEnd now pops back to the enclosing container before it pushes the next element, as the Node.js library does since THRIFT-6409. test-nested-empty-lists.html reads nested lists, a list of sets and a map of nested lists with empty lists before, between and after non-empty ones, and round-trips list> and list>> through the code generated from JsDeepConstructorTest.thrift. Gruntfile.js runs it with the other QUnit pages. A comment in container_size_limit.test.js that described the old behaviour is updated. Co-Authored-By: Claude Opus 5.5 --- lib/js/Gruntfile.js | 10 ++ lib/js/src/thrift.js | 5 +- lib/js/test/container_size_limit.test.js | 3 +- lib/js/test/test-nested-empty-lists.html | 45 +++++++ lib/js/test/test-nested-empty-lists.js | 161 +++++++++++++++++++++++ 5 files changed, 221 insertions(+), 3 deletions(-) create mode 100644 lib/js/test/test-nested-empty-lists.html create mode 100644 lib/js/test/test-nested-empty-lists.js diff --git a/lib/js/Gruntfile.js b/lib/js/Gruntfile.js index a18d090fdfd..322d84be1bb 100644 --- a/lib/js/Gruntfile.js +++ b/lib/js/Gruntfile.js @@ -202,6 +202,15 @@ module.exports = function (grunt) { }, }, }, + ThriftJS_NestedEmptyLists: { + options: { + urls: ["http://localhost:8089/test-nested-empty-lists.html"], + puppeteer: { + headless: true, + args: ["--no-sandbox"], + }, + }, + }, ThriftWS: { options: { urls: ["http://localhost:8089/testws.html"], @@ -388,6 +397,7 @@ module.exports = function (grunt) { "qunit:ThriftJS_Int64", "qunit:ThriftJS_SkipDepth", "qunit:ThriftJS_NegativeSize", + "qunit:ThriftJS_NestedEmptyLists", "shell:ThriftTestServer:kill", "shell:ThriftTestServer_TLS:kill", "shell:ThriftTestServerES6:kill", diff --git a/lib/js/src/thrift.js b/lib/js/src/thrift.js index 824357b98e2..fd35b9c3a28 100644 --- a/lib/js/src/thrift.js +++ b/lib/js/src/thrift.js @@ -1329,7 +1329,10 @@ Thrift.Protocol.prototype = { readListEnd: function() { var pos = this.rpos.pop() - 2; var st = this.rstack; - st.pop(); + // Empty lists leave an undefined child and the emptied list on the stack. + while (st.length > pos + 1) { + st.pop(); + } if (st instanceof Array && st.length > pos && st[pos].length > 0) { st.push(st[pos].shift()); } diff --git a/lib/js/test/container_size_limit.test.js b/lib/js/test/container_size_limit.test.js index d9e47cea137..f6a64b6dab0 100644 --- a/lib/js/test/container_size_limit.test.js +++ b/lib/js/test/container_size_limit.test.js @@ -111,8 +111,7 @@ const results = vm.runInContext( list3: run(function () { return proto1(['i32', 3, 1, 2, 3]).readListBegin().size; }), map2: run(function () { return proto1(['i32', 'i32', 2, {'1':2,'3':4}]).readMapBegin().size; }), emptyList: run(function () { return proto1(['i32', 0]).readListBegin().size; }), - // No size at all: readListEnd can leave an emptied list behind, and that - // reads as empty rather than being rejected. + // No size at all reads as empty rather than being rejected. sizeless: run(function () { proto1(['i32']).readListBegin(); return 'ok'; }) }; }) diff --git a/lib/js/test/test-nested-empty-lists.html b/lib/js/test/test-nested-empty-lists.html new file mode 100644 index 00000000000..a9f89d3ab79 --- /dev/null +++ b/lib/js/test/test-nested-empty-lists.html @@ -0,0 +1,45 @@ + + + + + + Thrift Javascript Bindings: Nested Empty Lists Test + + + + + + + + + + + + + + + +

Thrift Javascript Bindings: Nested Empty Lists Test

+

+
+

+
+ + diff --git a/lib/js/test/test-nested-empty-lists.js b/lib/js/test/test-nested-empty-lists.js new file mode 100644 index 00000000000..2cdfde2cefd --- /dev/null +++ b/lib/js/test/test-nested-empty-lists.js @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// The JSON protocol reads from the parsed message through a stack. An empty +// list leaves an undefined element and the emptied list on it, and readListEnd +// has to remove both before it pushes the next element of the enclosing list, +// or the elements that follow are read from the wrong place. + +QUnit.module('JSON protocol nested empty lists'); + +(function() { + // The wire form of a list nested "depth" levels deep, with i32 elements. + function encodeList(values, depth) { + var wire = [depth === 1 ? 'i32' : 'lst', values.length]; + values.forEach(function(value) { + wire.push(depth === 1 ? value : encodeList(value, depth - 1)); + }); + return wire; + } + + // Reads a list the way the generated code does. + function readList(protocol, depth) { + var size = protocol.readListBegin().size || 0; + var values = []; + for (var i = 0; i < size; i++) { + values.push(depth === 1 ? protocol.readI32().value : readList(protocol, depth - 1)); + } + protocol.readListEnd(); + return values; + } + + // Parses a message whose struct holds the one field given, and reads that + // field's value with readValue. + function readField(field, readValue) { + var transport = new Thrift.Transport('/service'); + transport.setRecvBuffer(JSON.stringify([1, 'm', Thrift.MessageType.CALL, 0, {'1': field}])); + var protocol = new Thrift.Protocol(transport); + protocol.readMessageBegin(); + protocol.readStructBegin(); + protocol.readFieldBegin(); + var value = readValue(protocol); + protocol.readFieldEnd(); + protocol.readStructEnd(); + protocol.readMessageEnd(); + return value; + } + + [ + {depth: 2, value: [[], [], [1, 2]]}, + {depth: 2, value: [[1], [], [], [2]]}, + {depth: 3, value: [[[1, 2], []], [], [[3]]]} + ].forEach(function(tc) { + QUnit.test('list ' + JSON.stringify(tc.value), function(assert) { + var read = readField({lst: encodeList(tc.value, tc.depth)}, function(protocol) { + return readList(protocol, tc.depth); + }); + assert.deepEqual(read, tc.value, JSON.stringify(tc.value)); + }); + }); + + // readSetEnd ends a set the way readListEnd ends a list. + QUnit.test('list of sets [[],[],[5,6]]', function(assert) { + var value = [[], [], [5, 6]]; + var read = readField({lst: ['set', 3, ['i32', 0], ['i32', 0], ['i32', 2, 5, 6]]}, function(protocol) { + var size = protocol.readListBegin().size || 0; + var sets = []; + for (var i = 0; i < size; i++) { + var setSize = protocol.readSetBegin().size || 0; + var set = []; + for (var j = 0; j < setSize; j++) { + set.push(protocol.readI32().value); + } + protocol.readSetEnd(); + sets.push(set); + } + protocol.readListEnd(); + return sets; + }); + assert.deepEqual(read, value, JSON.stringify(value)); + }); + + // The generated map reader pops one leftover stack entry before every key + // but the first; the value [[],[]] used to leave two. + QUnit.test('map values {"0":[[],[]],"1":[[7]]}', function(assert) { + var value = {'0': [[], []], '1': [[7]]}; + var entries = {'0': encodeList(value['0'], 2), '1': encodeList(value['1'], 2)}; + var read = readField({map: ['i32', 'lst', 2, entries]}, function(protocol) { + var size = protocol.readMapBegin().size || 0; + var map = {}; + for (var i = 0; i < size; i++) { + if (i > 0) { + if (protocol.rstack.length > protocol.rpos[protocol.rpos.length - 1] + 1) { + protocol.rstack.pop(); + } + } + var key = protocol.readI32().value; + map[key] = readList(protocol, 2); + } + protocol.readMapEnd(); + return map; + }); + assert.deepEqual(read, value, JSON.stringify(value)); + }); +})(); + +// list>, the type of THRIFT-3669, and a list nested one level +// deeper, round-tripped through the code generated from +// JsDeepConstructorTest.thrift. +QUnit.module('Generated nested lists with empty lists'); + +(function() { + function roundTrip(obj, Type) { + var transport = new Thrift.Transport('/service'); + var protocol = new Thrift.Protocol(transport); + protocol.writeMessageBegin('', 0, 0); + obj[Symbol.for('write')](protocol); + protocol.writeMessageEnd(); + + var serialized = transport.send_buf; + transport = new Thrift.Transport('/service'); + transport.setRecvBuffer(serialized); + protocol = new Thrift.Protocol(transport); + protocol.readMessageBegin(); + var received = new Type(); + received[Symbol.for('read')](protocol); + protocol.readMessageEnd(); + return received; + } + + QUnit.test('list>', function(assert) { + [ + [[], [], ['one', 'two']], + [['one'], [], ['two'], ['three']] + ].forEach(function(value) { + var received = roundTrip(new Complex({list_of_list_field: value}), Complex); + assert.deepEqual(received.list_of_list_field, value, JSON.stringify(value)); + }); + }); + + QUnit.test('list>>', function(assert) { + var value = [[['one', 'two'], []], [], [['three']]]; + var received = roundTrip(new Complex({list_of_list_of_list_field: value}), Complex); + assert.deepEqual(received.list_of_list_of_list_field, value, JSON.stringify(value)); + }); +})();