I'm not able to set headers for NATS response

Dzixxx commented 2 years ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

I want to send back from my method decorated with MessagePattern/EventPattern data with NATS headers and turns out that they're not passed in any way.

I tried with setting

@MessagePattern('abc')
method(@Ctx() context: NatsContext) {
  ctx.getHeaders().set('response-header', 'abc')
  return 'my data';
}

with hope that they will be applied but with no luck - they're undefined 😂

Minimum reproduction code

https://github.com/Dzixxx/nestjs-typescript-starter-we4m7w

Steps to reproduce

No response

Expected behavior

I would expect that @Ctx() context: NatsContext will have method setResponseHeader and in ServerNats.getPublisher method it will apply them to packet and default serializer will handle it:

// line 26 of nats-record.serializer.ts
: new NatsRecordBuilder(packet?.data).build()

// new line 26 of nats-record.serializer.ts
:  new NatsRecordBuilder(packet?.data).setHeaders(packet?.headers).build()

I'm open for any solution and even preparing PR (but I will need some help with preparing the solution)

Dziczek 😉

Package

[ ] I don't know. Or some 3rd-party package
[ ] code>@nestjs/common</code
[ ] code>@nestjs/core</code
[X] code>@nestjs/microservices</code
[ ] code>@nestjs/platform-express</code
[ ] code>@nestjs/platform-fastify</code
[ ] code>@nestjs/platform-socket.io</code
[ ] code>@nestjs/platform-ws</code
[ ] code>@nestjs/testing</code
[ ] code>@nestjs/websockets</code
[ ] Other (see below)

Other package

No response

NestJS version

9

Packages versions

Node.js version

16

In which operating systems have you tested?

[X] macOS
[ ] Windows
[ ] Linux

Other

No response

jmcdo29 commented 2 years ago

Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

why reproductions are required

Dzixxx commented 2 years ago

Added, maybe it was needed 😅

Dzixxx commented 2 years ago

I think that the problem is with serializer and it's execution (from publisher) which should respect those headers (now it's omitting them) ;)

kamilmysliwiec commented 2 years ago

Would you like to create a PR for this issue?

Dzixxx commented 2 years ago

@kamilmysliwiec no problem, the question is -> should I add check if return value from message pattern method is NatsRecord instance and then respect it's headers in default serializer? This could do the job but it will be a breaking change 💩

nats-record.serializer.ts

    serialize(packet) {

        if (packet?.data) {
            let natsMessage = void 0; 
            if (packet.data instanceof NatsRecord) {
                natsMessage = packet.data;
            } else {
                // should we handle packet.headers? 
                natsMessage = new NatsRecordBuilder(packet?.data).build();
            }

            return {
                data: this.jsonCodec.encode({ ...packet, data: natsMessage.data })),
                headers: natsMessage.headers,
            };

        } else if (packet?.response) {
            if (packet.response instanceof NatsRecord) {
                return {
                    data: this.jsonCodec.encode({ ...packet, data: void 0 })),
                    headers: packet.response.headers,
                };
            }

           // should we handle packet.headers? 
           const natsMessage = new NatsRecordBuilder(packet?.data).build();
            return {
                data: this.jsonCodec.encode({ ...packet, data: natsMessage.data })),
                headers: natsMessage.headers,
            };
        }

        // throw? 
    }

client-nats.ts in createSubscriptionHandler method, line 93

// ...
            const { err, response, isDisposed } = message;

            // sick but could do the job 🤔 the problem is that we're not able to recognize if payload was not serialized with custom serializer 😢 
            response.headers = natsMsg.headers;

            if (isDisposed || err) {
                return callback({
                    err,
                    response,
                    isDisposed: true,
                });
            }
//...

If we're able to discuss the solution first and agree on smth then I can implement everything 😂

Dzixxx commented 2 years ago

Line changer in client-nats.ts is a problem for me cause if someone will have custom serializers and deserializers atm then response formula may differ from default which may lead to situation where we're not able to apply them in this way (by applying field to response object if is and object ofc)

However forcing developers from v10 to respect serializers and deserializers to implement interface (object with data and headers at least as a response) could be nice standarization. WDYT? 😄

Dzixxx commented 1 year ago

@jmcdo29 @kamilmysliwiec so what's the plan? 😅

kamilmysliwiec commented 1 year ago

However forcing developers from v10 to respect serializers and deserializers to implement interface (object with data and headers at least as a response) could be nice standarization. WDYT? 😄

SGTM

nestjs / nest