search

moleculerjs / moleculer

:rocket: Progressive microservices framework for Node.js
https://moleculer.services/
MIT License
6.17k stars 587 forks source link

Params validator is tried to be serialized and fails when a Joi object is passed #309

Closed sykire closed 6 years ago

sykire commented 6 years ago

Sorry for bothering you again, I was looking to use Joi validator instead of default validator. After using the JoiValidator like in the example and setting the validation params in my actions I got this error:

gateway_1  | [2018-06-27T00:46:45.814Z] ERROR 31e01763c5e3-102/TRANSIT: Unable to send INFO packet to 'undefined' node. TypeError: Converting circular structure to JSON
gateway_1  |     at JSON.stringify (<anonymous>)
gateway_1  |     at JSONSerializer.serialize (/app/node_modules/moleculer/src/serializers/json.js:36:15)
gateway_1  |     at NatsTransporter.serialize (/app/node_modules/moleculer/src/transporters/base.js:297:33)
gateway_1  |     at Transporter.publish.resolve (/app/node_modules/moleculer/src/transporters/nats.js:211:37)
gateway_1  |     at Promise.module.exports.Promise._execute (/app/node_modules/bluebird/js/release/debuggability.js:303:9)
gateway_1  |     at Promise.module.exports.Promise._resolveFromExecutor (/app/node_modules/bluebird/js/release/promise.js:483:18)
gateway_1  |     at new Promise (/app/node_modules/bluebird/js/release/promise.js:79:10)
gateway_1  |     at NatsTransporter.publish (/app/node_modules/moleculer/src/transporters/nats.js:209:10)
gateway_1  |     at NatsTransporter.prepublish (/app/node_modules/moleculer/src/transporters/base.js:283:15)
gateway_1  |     at Transit.publish (/app/node_modules/moleculer/src/transit.js:691:18)
gateway_1  |     at sendNodeInfo.p.then (/app/node_modules/moleculer/src/transit.js:595:28)
gateway_1  |     at tryCatcher (/app/node_modules/bluebird/js/release/util.js:16:23)
gateway_1  |     at Promise.module.exports.Promise._settlePromiseFromHandler (/app/node_modules/bluebird/js/release/promise.js:512:31)
gateway_1  |     at Promise.module.exports.Promise._settlePromise (/app/node_modules/bluebird/js/release/promise.js:569:18)
gateway_1  |     at Promise.module.exports.Promise._settlePromise0 (/app/node_modules/bluebird/js/release/promise.js:614:10)
gateway_1  |     at Promise.module.exports.Promise._settlePromises (/app/node_modules/bluebird/js/release/promise.js:693:18)

It appears that the params validation object is tried to be serialized and because Joi objects are cyclic it fails.

This is not the first time I've seen problems with Joi object. There was a bug in Hoek.deepEqual when comparing objects that happens to have a joi object inside. This happens because Joi objects are cyclic. https://github.com/hapijs/hoek/issues/219

I've going to look further but at first impression it looks like the params validation object is tried to be serialized when it shouldn't.

sykire commented 6 years ago

This is where the problem begins, services rawInfo to be serialized includes actions params:

https://github.com/moleculerjs/moleculer/blob/fcfd48278e06b2a96051ecbe3fc6e57f87ff025a/src/transit.js#L596

https://github.com/moleculerjs/moleculer/blob/28a2f1f0648db352a416ed92a2afce37409fa31b/src/registry/service-catalog.js#L160

I don't know how necessary is to serialize the params validation object, but joi's objects are cyclic and can't be serialized.

Possible solutions would be:

  1. omit params from actions info. (easier)
  2. A way to specify in the validator how schemas should be serialized and deserialized. For joi there are libs to converted back and forth from jsonSchema. (harder)

P.S.: Because moleculer validation is customizable, action params typings should be any instead of Moleculer.ActionParams, or perhaps passed as a generic. Will open and issue about that later.

icebob commented 6 years ago

Hi, the action properties are public information, what is used by middlewares. So we can't just omit if it's not working. We should check the action definition object and remove cyclic elements before broker sends them to the remote nodes. I'm thinking about it because it can be fixed in v0.13 (what I would like to release this week)