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

I'm not able to set headers for NATS response #10273

Open
3 of 15 tasks
Dzixxx opened this issue Sep 12, 2022 · 8 comments
Open
3 of 15 tasks

I'm not able to set headers for NATS response #10273

Dzixxx opened this issue Sep 12, 2022 · 8 comments

Comments

@Dzixxx
Copy link
Contributor

Dzixxx commented Sep 12, 2022

Is there an existing issue for this?

  • 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
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

9

Packages versions

Node.js version

16

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@Dzixxx Dzixxx added the needs triage This issue has not been looked into label Sep 12, 2022
@jmcdo29
Copy link
Member

jmcdo29 commented Sep 12, 2022

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

why reproductions are required

@Dzixxx
Copy link
Contributor Author

Dzixxx commented Sep 12, 2022

Added, maybe it was needed 😅

@Dzixxx
Copy link
Contributor Author

Dzixxx commented Sep 12, 2022

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
Copy link
Member

Would you like to create a PR for this issue?

@Dzixxx
Copy link
Contributor Author

Dzixxx commented Sep 13, 2022

@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
Copy link
Contributor Author

Dzixxx commented Sep 13, 2022

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
Copy link
Contributor Author

Dzixxx commented Sep 28, 2022

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

@kamilmysliwiec
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants