OpenAPI: errors under nodejs 10

[muxator]

When executing under Nodejs 10 (the minimum currently supported LTS version), the backend test (cd src ; npm test) cause an error in the server log.

Etherpad version: e625168 (current develop).
Tested under:

• linux, nodejs 12.13.1 <-- works
• linux, nodejs 10.14.1 <-- error

This is the relevant snippet:

[2020-04-05 02:50:47.813] [INFO] API - REQUEST, v1.2.14:setHTML, {"apikey":"XXXX","padID":"pWvFw","html":"<div><b>Hello HTML</title></head></div>"}
[2020-04-05 02:50:47.815] [WARN] ImportHtml - HTML was not properly formed TypeError: Cannot read property 'tagName' of undefined
    at Object.nodeTagName (<BASE>/src/static/js/contentcollector.js:50:16)
    at isBlockElement (<BASE>/src/static/js/contentcollector.js:97:31)
    at Object.cc.collectContent (<BASE>/src/static/js/contentcollector.js:395:19)
    at Object.exports.setPadHTML (<BASE>/src/node/utils/ImportHtml.js:41:8)
    at Object.exports.setHTML (<BASE>/src/node/db/API.js:290:16)
    at process._tickCallback (internal/process/next_tick.js:68:7)
TypeError: Cannot read property 'tagName' of undefined
    at Object.nodeTagName (<BASE>/src/static/js/contentcollector.js:50:16)
    at isBlockElement (<BASE>/src/static/js/contentcollector.js:97:31)
    at Object.cc.collectContent (<BASE>/src/static/js/contentcollector.js:395:19)
    at Object.exports.setPadHTML (<BASE>/src/node/utils/ImportHtml.js:41:8)
    at Object.exports.setHTML (<BASE>/src/node/db/API.js:290:16)
    at process._tickCallback (internal/process/next_tick.js:68:7)
[2020-04-05 02:50:47.818] [ERROR] console - (node:52817) [DEP0079] DeprecationWarning: Custom inspection function on Objects via .inspect() is deprecated

I noticed this when trying to run jshint, and it complained that something was only supported on ECMAScript 9 (I had to enable esversion: 9 in .jshintrc).

More details later.

@anttiviljami: can you please investigate? Thanks

Etherpad NodeJS support:

Etherpad 1.8.0 supports node 8 (see the startup logs) and the documentation.
Etherpad 1.8.3 will require node 10 (and is required to support it). See #3650. The next version will be called 1.8.3 (bad milestone name)

[anttiviljami]

Taking a look now

[anttiviljami]

So the test that triggers this error is this one:

etherpad-lite/tests/backend/specs/api/pad.js

Lines 544 to 558 in e625168

	describe('setHTML', function(){
	it('Sets the HTML of a Pad attempting to pass ugly HTML', function(done) {
	var html = "<div><b>Hello HTML</title></head></div>";
	api.post(endPoint('setHTML'))
	.send({
	"padID": testPadId,
	"html": html,
	})
	.expect(function(res){
	if(res.body.code !== 1) throw new Error("Allowing crappy HTML to be imported")
	})
	.expect('Content-Type', /json/)
	.expect(200, done)
	});
	})

It deliberately tries to set incorrect HTML, so the warning message is actually correct behaviour.

[WARN] ImportHtml - HTML was not properly formed TypeError: Cannot read property 'tagName' of undefined

With Node 10 however it is also accompanied with an error-level deprecation message about using custom .inspect() methods.

[ERROR] console - (node:52817) [DEP0079] DeprecationWarning: Custom inspection function on Objects via .inspect() is deprecated

Investigating further, this seems to happen when trying to inspect an error generated by the customError module found here: https://github.com/ether/etherpad-lite/blob/e6251687bf6f0d773ef5cd8883465d67e18f4816/src/node/utils/customError.js

Rewriting the customError module using modern class syntax fixes the issue for node 10 and above.

[anttiviljami]

See PR #3841

[anttiviljami]

Also noticed that the version of log4js we're using is absolutely ancient. This causes custom error objects logged with log4js to look like this: { inspect: [Function: inspect] }.

This behaviour, fixed in newer versions of log4js is what the deprecation warning on node 10 was about.

I would create another PR to upgrade log4js but looks like we're using an old configuration format for it, which makes things a bit more involved.

Not sure what to do with that. Waiting for feedback.

This issue doesn't really seem to have anything to do with introducing openapi-backend. We just notice it now that errors are properly logged from the API.

[muxator]

See PR #3841
[...]
This issue doesn't really seem to have anything to do with introducing openapi-backend. We just notice it now that errors are properly logged from the API.

I see, this is only uncovered by a test.

Thanks for the PR. I will go with that one, for now, because if the log4j configuration format changes in modern versions (and rightly so), we have at least to think about what to do with backward compatibility, and I cannot ask you to do this now.

I agree that the way forward is getting rid of the ancient modules. The migration to OpenAPI was one of the most important moves. :)

I am going to look at #3841 and incorporate it in 1.8.3.

Thanks

[muxator]

For tracking the (distinct) issue of the very old log4js version I've opened #3843.

[JohnMcLear]

Hah whoops :D Apols @anttiviljami