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

[micro] Add API to read Reply subject from micro.Request #1579

Closed
mcosta74 opened this issue Mar 4, 2024 · 6 comments · Fixed by #1589
Closed

[micro] Add API to read Reply subject from micro.Request #1579

mcosta74 opened this issue Mar 4, 2024 · 6 comments · Fixed by #1589
Labels
proposal Enhancement idea or proposal

Comments

@mcosta74
Copy link
Contributor

mcosta74 commented Mar 4, 2024

Proposed change

To not introduce a breaking change we'd need to add a new interface (RequestV2 ??) that extends the Request and add a new method

type RequestV2 interface {
    Request

    Reply() string
}

Use case

In some cases you can trigger a microservice but you don't care about the results so maybe you just send a publish instead of a request.

In this case would be nice to know if a request has a reply subject just to avoid to Marshal a response and try to send back it

func MyHandler(r micro.RequestV2) {
    response := handleRequest()

    if r.Reply() == "" {
        return
    }

   err := r.RespondJSON(response)
   if err != nil {
      // do something
   }
} 

Contribution

I'm interested to help with implementation in case this will be accepted

@mcosta74 mcosta74 added the proposal Enhancement idea or proposal label Mar 4, 2024
@piotrpio
Copy link
Collaborator

Hey @mcosta74. It's a good idea to add the ability to check reply subject, but I'm not sure about introducing a new interface... That would force us to also add v2 of Handler interface, along with Handle method and HandlerFunc.

Another idea would be to add a RespondOpt which would allow skipping sending the response if reply is empty, but that's not ideal as well since you'd not know whether the response was sent or not.

@mcosta74
Copy link
Contributor Author

Hi @piotrpio , my need is to know if I need to prepare/Marshall the response or I can skip it because there isn't a Reply.

So your 2nd option doesn't fit what I'm looking for.

@mcosta74
Copy link
Contributor Author

That would force us to also add v2 of Handler interface, along with Handle method and HandlerFunc.

As I said, I'll be happy to implement the change of the proposal is accepted

@mcosta74
Copy link
Contributor Author

@piotrpio Can I just add a Reply() method to the Request interface? Is adding a new method the the interface considered a breaking change ?

mcosta74 added a commit to mcosta74/nats.go that referenced this issue Mar 21, 2024
mcosta74 added a commit to mcosta74/nats.go that referenced this issue Mar 21, 2024
Fixes nats-io#1579

Signed-off-by: Massimo Costa <costa.massimo@gmail.com>
@themartorana
Copy link

@piotrpio I commented on the PR as well, but for our purpose, sometimes we hand messages around multiple services or handlers before responding to a client, and need to be able to pass the reply subject to the down-line code. Easy to do with nats.Msg, but not micro.

@piotrpio
Copy link
Collaborator

piotrpio commented Apr 5, 2024

@mcosta74, @themartorana Sorry it took so long and thank you for the patience. We discussed it internally and agreed that we will not consider adding public methods to interfaces a breaking change for the client. We have done it in the past with JetStreamContext so we can do the same for micro. I've created a PR adding a note about that to readme to make it official.

I'll review your PR in a moment, on first glance it looks simple and straightforward enough 😉

piotrpio pushed a commit that referenced this issue Apr 9, 2024
Fixes #1579

Signed-off-by: Massimo Costa <costa.massimo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Enhancement idea or proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants