Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actions in services with ProtoBuf and NATS cannot return 0 and false. #511

Closed
4 tasks done
alvaroinckot opened this issue Apr 3, 2019 · 12 comments
Closed
4 tasks done

Comments

@alvaroinckot
Copy link
Contributor

alvaroinckot commented Apr 3, 2019

Prerequisites

Please answer the following questions for yourself before submitting an issue.

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed
  • I'm reporting the issue to the correct repository

Expected Behavior

Be able to receive 0 and false as a result of some action in a service using NATS and Protobuf.

Current Behavior

0 and false are received as undefined.

Failure Information

Protobuf serialize or deserialize the message incorrectly and turns 0 and false results into undefined

Steps to Reproduce

  1. step 1
  2. step 2
  3. you get it...

Reproduce code snippet

const broker = new ServiceBroker({
    transporter: "NATS",
    serializer: 'ProtoBuf'
});

broker.createService({
    name: "greeter",
   version: 1,
    actions: {
		willReturnFalse() {
			return false;
		},

		willReturnZero() {
			return 0;
		}
});

describe("Test 'greeter' service", () => {

	const broker = new ServiceBroker({ transporter: 'nats://localhost:4222', serializer: 'ProtoBuf' });

	beforeAll(async () => {
		await broker.start();
		await broker.waitForServices([{ name: 'greeter', version: 1 }]);
	});

	afterAll(() => broker.stop());

	describe("Test 'greeter.willReturnFalse' action", () => {
		it("should return false", () => {
			expect(broker.call("v1.greeter.willReturnFalse")).resolves.toBe(false);
		});
	});


	describe("Test 'greeter.willReturnFalse' action", () => {
		it("should return 0", () => {
			expect(broker.call("v1.greeter.willReturnZero")).resolves.toBe(0);
		});
	});
});

// both tests will fail receiving undefined

EXAMPLE READY TO RUN:

https://github.com/alvarocantador/moleculer-zero-and-false-bug

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

  • Moleculer version: 0.13.8
  • NodeJS version: 10.15.3
  • Operating System: MacOS

Failure Logs


@icebob
Copy link
Member

icebob commented Apr 4, 2019

@AndreMaz could you help with it?

@lucduong
Copy link
Sponsor Contributor

lucduong commented Apr 4, 2019

@hoanganh25991 Could you check once?

@hoanganh25991
Copy link

hoanganh25991 commented Apr 5, 2019

Hi @alvarocantador ,
I've tested ProtoBuf on NATS, the serializer work as expected
image

In my test, I only use moleculer-cli to init minimal project template.
I guess your API service make change on value.

Example ready to run:
https://github.com/hoanganh25991/protobuf-serialize-false-zero

@alvaroinckot
Copy link
Contributor Author

alvaroinckot commented Apr 5, 2019

@icebob @hoanganh25991 In your case, the greeter is a local service and its work as expected. Try to run dev in a shell and call it in another repl, you will see undefined.

I have the bug example here: https://github.com/alvarocantador/moleculer-zero-and-false-bug

In this case I am using the moleculer-runner inside of a docker image, and via NATS in another broker, I am trying to call the willReturnFalse and willReturnZero actions.

@alvaroinckot
Copy link
Contributor Author

alvaroinckot commented Apr 5, 2019

Only in the npm run dev shell:
image

Now with an REPL:
image

  • WITH A RUNNER

@hoanganh25991
Copy link

Hi @alvarocantador ,

By default, broker config "prefer local", which lead to call without send through NATS.
I see your issue when add another repl.

I digging in transporter & serializer code to figure out.

@hoanganh25991
Copy link

Protobuf serialize flow:

  • Convert data to string
  • Build up buffer from that string
  • Encode with proto/packets.proto.js before send out

I think the if check falsy value on obj.data in serializer/base.js cause the problem:

  • JSON.stringify never hit
  • cause Buffer.from never hit
  • cause deserialize return undefined

As what I test, remove the falsy check, response return false, 0 as expected

https://github.com/moleculerjs/moleculer/blob/master/src/serializers/base.js#L98

image

Protobuf Serializer

https://github.com/moleculerjs/moleculer/blob/master/src/serializers/protobuf.js

Serialize

base.js > serializeCustomFields
Falsy value not converted to string

case P.PACKET_RESPONSE: {
  obj.meta = JSON.stringify(obj.meta);
  if (obj.data) {
    if (!obj.stream) {
      obj.data = JSON.stringify(obj.data);
    }
  }
}

protobuf.js > serializeCustomFields
Buffer not built up

case P.PACKET_RESPONSE: {
  if (obj.data && !obj.stream)
    obj.data = Buffer.from(obj.data);
  break;
}

Deserialize

protobuf.js > deserializeCustomFields
No data key on obj, return undefined

case P.PACKET_RESPONSE: {
  if (obj.data && !obj.stream) {
    if (obj.data.length)
      obj.data = obj.data.toString("utf8");
    else 
      obj.data = undefined; 
  }
  break;
}

@icebob
Copy link
Member

icebob commented Apr 6, 2019

@hoanganh25991 good catch. Thank you, I'm fixing.

@icebob icebob closed this as completed in 74dc8a6 Apr 6, 2019
@icebob
Copy link
Member

icebob commented Apr 6, 2019

I've fixed. @hoanganh25991 @alvarocantador could you check it to install master with npm i moleculerjs/moleculer?

@hoanganh25991
Copy link

hoanganh25991 commented Apr 7, 2019

Hi @icebob ,

I've tested your fix in "moleculerjs/moleculer" package, now Protobuf serialize correctly on false, 0 case. However, it return as "undefined" on null case.

protobuf-serialize-false-zero-2019-04-07 14-32-45

As my test above, on "greeter.willReturnNull", Protobuf deserialize as "undefined".

Test example ready to run:
https://github.com/hoanganh25991/protobuf-serialize-false-zero

@icebob
Copy link
Member

icebob commented Apr 7, 2019

Thanks. I'm checking...

@icebob icebob reopened this Apr 7, 2019
@icebob
Copy link
Member

icebob commented Apr 7, 2019

I've fixed it.

@icebob icebob closed this as completed Apr 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants