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

Cannot get fastify context in request handler. #708

Closed
acrazing opened this issue Jul 2, 2023 · 12 comments
Closed

Cannot get fastify context in request handler. #708

acrazing opened this issue Jul 2, 2023 · 12 comments
Labels
enhancement New feature or request

Comments

@acrazing
Copy link

acrazing commented Jul 2, 2023

In original fastify handler, we can access the fastify request context handy.

fastify.get('/', options, function (request, reply) {
  request.log.info('Some info about the current request')
  reply.send({ hello: 'world' })
})

Bug it is impossible in connect-es.

router.service(Health, {
  check(request, context) {
    // no any data passed here from fastify
    console.log(context)
  }
}
@acrazing acrazing added the bug Something isn't working label Jul 2, 2023
@smaye81
Copy link
Member

smaye81 commented Jul 6, 2023

Hi @acrazing can you elaborate on what details of the Fastify request you're looking for? Is it the logging instance or are there other bits of info you need?

@acrazing
Copy link
Author

acrazing commented Jul 7, 2023

logger is the most urgently needed.

@acrazing
Copy link
Author

acrazing commented Jul 7, 2023

But other information is also very important, such as ip address?

I think a mechanism is needed to allow users to inject context, similar to grpc's interceptor.

@acrazing
Copy link
Author

acrazing commented Jul 7, 2023

Here is my current patch file:

diff --git a/node_modules/@bufbuild/connect-fastify/dist/cjs/fastify-connect-plugin.js b/node_modules/@bufbuild/connect-fastify/dist/cjs/fastify-connect-plugin.js
index 505235f..1983a89 100644
--- a/node_modules/@bufbuild/connect-fastify/dist/cjs/fastify-connect-plugin.js
+++ b/node_modules/@bufbuild/connect-fastify/dist/cjs/fastify-connect-plugin.js
@@ -43,7 +43,7 @@ function fastifyConnectPlugin(instance, opts, done) {
     for (const uHandler of uHandlers) {
         instance.all(uHandler.requestPath, {}, async function handleFastifyRequest(req, reply) {
             try {
-                const uRes = await uHandler((0, connect_node_1.universalRequestFromNodeRequest)(req.raw, req.body));
+                const uRes = await uHandler(Object.assign((0, connect_node_1.universalRequestFromNodeRequest)(req.raw, req.body), { extraContext: { req, reply, log: req.log } }));
                 // Fastify maintains response headers on the reply object and only moves them to
                 // the raw response during reply.send, but we are not using reply.send with this plugin.
                 // So we need to manually copy them to the raw response before handing off to vanilla Node.
diff --git a/node_modules/@bufbuild/connect-fastify/dist/types/fastify-connect-plugin.d.ts b/node_modules/@bufbuild/connect-fastify/dist/types/fastify-connect-plugin.d.ts
index 675ac8a..fdb473a 100644
--- a/node_modules/@bufbuild/connect-fastify/dist/types/fastify-connect-plugin.d.ts
+++ b/node_modules/@bufbuild/connect-fastify/dist/types/fastify-connect-plugin.d.ts
@@ -1,5 +1,7 @@
 import type { ConnectRouter, ConnectRouterOptions } from "@bufbuild/connect";
 import type { FastifyInstance } from "fastify/types/instance";
+import { FastifyReply, FastifyRequest } from 'fastify';
+import { Logger } from 'pino';
 interface FastifyConnectPluginOptions extends ConnectRouterOptions {
     /**
      * Route definitions. We recommend the following pattern:
@@ -23,3 +25,11 @@ interface FastifyConnectPluginOptions extends ConnectRouterOptions {
  */
 export declare function fastifyConnectPlugin(instance: FastifyInstance, opts: FastifyConnectPluginOptions, done: (err?: Error) => void): void;
 export {};
+
+declare module '@bufbuild/connect' {
+  export interface HandlerContext {
+    req: FastifyRequest;
+    reply: FastifyReply;
+    log: Logger;
+  }
+}
diff --git a/node_modules/@bufbuild/connect/dist/cjs/protocol-grpc/handler-factory.js b/node_modules/@bufbuild/connect/dist/cjs/protocol-grpc/handler-factory.js
index e606c02..26d8f81 100644
--- a/node_modules/@bufbuild/connect/dist/cjs/protocol-grpc/handler-factory.js
+++ b/node_modules/@bufbuild/connect/dist/cjs/protocol-grpc/handler-factory.js
@@ -67,7 +67,7 @@ function createHandler(opt, spec) {
                 [headers_js_1.headerContentType]: type.binary ? content_type_js_1.contentTypeProto : content_type_js_1.contentTypeJson,
             }, responseTrailer: {
                 [headers_js_1.headerGrpcStatus]: trailer_status_js_1.grpcStatusOk,
-            } }));
+            } }, req.extraContext));
         const compression = (0, compression_js_1.compressionNegotiate)(opt.acceptCompression, req.header.get(headers_js_1.headerEncoding), req.header.get(headers_js_1.headerAcceptEncoding), headers_js_1.headerAcceptEncoding);
         if (compression.response) {
             context.responseHeader.set(headers_js_1.headerEncoding, compression.response.name);

@StarpTech
Copy link

StarpTech commented Jul 8, 2023

Hi @acrazing can you elaborate on what details of the Fastify request you're looking for? Is it the logging instance or are there other bits of info you need?

Fastify has a strong plugin system. If we can't access the request instance, we can't utilize it.

@smaye81
Copy link
Member

smaye81 commented Jul 10, 2023

Understood. We are currently preparing for an upcoming v1.0 release and once that is complete, we are going to focus on enhancements such as this, so stay tuned.

@timostamm timostamm added enhancement New feature or request and removed bug Something isn't working labels Jul 12, 2023
@StarpTech
Copy link

Hi @smaye81 any ETA for this? Would you accept a PR?

@smaye81
Copy link
Member

smaye81 commented Jul 26, 2023

No definitive ETA, but we are nearing our v1.0 release. We have an idea of how to accomplish this that will help with Express and Fastify, so I would say hold off on the PR for now. Hopefully we get to this very soon.

@StarpTech
Copy link

Alright, thank you!

@joemckenney
Copy link

Realize +1's aren't always helpful, but I just wanted to add that there is basically no way to work around this issue without patching the source. We'd happily own and maintain our own fastify connect plugin implementation if that solved this, but unfortunately (as the patch above makes clear) that isn't enough.

Also happy to contribute a PR if the internal timeline gets pushed out. This blocks a proper implementation of request logging (and therefore proper request tracing in prod).

@StarpTech
Copy link

StarpTech commented Aug 1, 2023

@joemckenney is it opensource? We would love to contribute. Unfortunately, we can't wait until it is fixed here which seems will take a while. The alternative is we roll out our own too. Besides access to fastify inside buf handlers, it would be also very beneficial if the integration would honor fastify request/response lifecycle instead of hijacking the response. That would provide compatible error / response hooks handling as well.

@srikrsna-buf
Copy link
Member

#835

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

No branches or pull requests

6 participants