Skip to content

Commit 6f5b5ad

Browse files
Jens-Gclaude
andcommitted
THRIFT-6418: Give the browser TApplicationException the symbol-keyed read and write methods
Client: js Generated JavaScript code reads and writes structs through the keys Symbol.for("read") and Symbol.for("write"). Since 5fc920d (0.21.0) the generated client also reads an EXCEPTION reply that way, calling x[Symbol.for("read")](input) on a new Thrift.TApplicationException. lib/nodejs gives its TApplicationException both keys, but lib/js did not, so a browser client threw "TypeError: x[Symbol.for(...)] is not a function" instead of the TApplicationException the server sent. The two keys now point at the existing read() and write(), as in lib/nodejs. The ant JSLint check in lib/js/test/build.xml reads thrift.js as ES3, where "for" cannot be a property name, so it now runs with the es5 option. The test that should have caught this could not. test_handler.js took TException from the top level of the Node.js library, where it is undefined, so testException("TException") failed inside the test server with HTTP 500, and the TException test in test.js had been changed to accept any error. The handler now uses Thrift.TException, and the test expects the TApplicationException that the server sends for an undeclared exception. A second test decodes an EXCEPTION reply through the generated client without a server. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 5cba029 commit 6f5b5ad

4 files changed

Lines changed: 36 additions & 11 deletions

File tree

‎lib/js/src/thrift.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,12 @@ Thrift.TApplicationException.prototype.write = function(output) {
263263
output.writeStructEnd();
264264
};
265265

266+
// Generated code reads and writes every struct through these two keys, which a
267+
// field named "read" or "write" cannot hide, and it reads an EXCEPTION reply into
268+
// a TApplicationException the same way.
269+
Thrift.TApplicationException.prototype[Symbol.for('read')] = Thrift.TApplicationException.prototype.read;
270+
Thrift.TApplicationException.prototype[Symbol.for('write')] = Thrift.TApplicationException.prototype.write;
271+
266272
/**
267273
* Returns the application exception code set on the exception.
268274
* @readonly

‎lib/js/test/build.xml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,8 @@
182182
the following options would probably make sense in the future:
183183
browser,undef,eqeqeq,plusplus,bitwise,regexp,strict,newcap,immed
184184
-->
185-
<jsl:jslint options="evil,forin,browser,bitwise,regexp,newcap,immed" encoding="UTF-8">
185+
<!-- es5: thrift.js uses Symbol.for(), and only ES5 allows "for" as a property name -->
186+
<jsl:jslint options="evil,forin,browser,bitwise,regexp,newcap,immed,es5" encoding="UTF-8">
186187
<formatter type="plain" />
187188
<fileset dir="../src" includes="thrift.js" />
188189

‎lib/js/test/test.js‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -315,16 +315,34 @@ QUnit.module('Exception');
315315
});
316316

317317
QUnit.test('TException', function(assert) {
318-
//ThriftTest does not list TException as a legal exception so it will
319-
// generate an exception on the server that does not propagate back to
320-
// the client. This test has been modified to equate to "no exception"
318+
// ThriftTest does not declare TException, so the server answers with an
319+
// EXCEPTION reply, which the client raises as a TApplicationException.
321320
assert.expect(1);
322-
try {
323-
client.testException('TException');
324-
} catch (e) {
325-
//assert.ok(false);
326-
}
327-
assert.ok(true);
321+
assert.throws(
322+
function() { client.testException('TException'); },
323+
function(e) {
324+
return e instanceof Thrift.TApplicationException &&
325+
e.message === 'TException' &&
326+
e.code === Thrift.TApplicationExceptionType.UNKNOWN;
327+
},
328+
'testException("TException") raises a TApplicationException'
329+
);
330+
});
331+
332+
QUnit.test('EXCEPTION reply without a server', function(assert) {
333+
const offlineTransport = new Thrift.Transport('/service');
334+
const offlineClient = new ThriftTest.ThriftTestClient(new Thrift.Protocol(offlineTransport));
335+
offlineTransport.setRecvBuffer('[1,"testVoid",3,0,{"1":{"str":"boom"},"2":{"i32":6}}]');
336+
assert.expect(1);
337+
assert.throws(
338+
function() { offlineClient.recv_testVoid(); },
339+
function(e) {
340+
return e instanceof Thrift.TApplicationException &&
341+
e.message === 'boom' &&
342+
e.code === Thrift.TApplicationExceptionType.INTERNAL_ERROR;
343+
},
344+
'recv_testVoid raises the TApplicationException of an EXCEPTION reply'
345+
);
328346
});
329347

330348

‎lib/js/test/test_handler.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
const es6Mode = process.argv.includes('--es6');
2424
const genFolder = es6Mode ? 'gen-nodejs-es6' : 'gen-nodejs';
2525
const ttypes = require(`./${genFolder}/ThriftTest_types`);
26-
const TException = require('../../nodejs/lib/thrift').TException;
26+
const TException = require('../../nodejs/lib/thrift').Thrift.TException;
2727
const Int64 = require('node-int64');
2828

2929
exports.ThriftTestHandler = {

0 commit comments

Comments
 (0)