Remove debug logging for URL rewrite (#4754)

* Remove debug logging for URL rewrite * Use url rewrite params object for future compatibility Amend test to ensure logger param is present * Change rewriteUrl params to 'this' * Call rewriteUrl with this bound to the Fastify instance * Improve docs for rewriteUrl Co-authored-by: James Sumners <james@sumners.email> --------- Co-authored-by: James Sumners <james@sumners.email>
fastify · May 23, 2023 · 410957f · 410957f
1 parent ca75d4c
commit 410957f
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 14 deletions.
diff --git a/docs/Reference/Server.md b/docs/Reference/Server.md
@@ -829,8 +829,16 @@ URLs.
 > Rewriting a URL will modify the `url` property of the `req` object
 
 ```js
-function rewriteUrl (req) { // req is the Node.js HTTP request
-  return req.url === '/hi' ? '/hello' : req.url;
+// @param {object} req The raw Node.js HTTP request, not the `FastifyRequest` object.
+// @this Fastify The root Fastify instance (not an encapsulated instance).
+// @returns {string} The path that the request should be mapped to.
+function rewriteUrl (req) { 
+  if (req.url === '/hi') {
+    this.log.debug({ originalUrl: req.url, url: '/hello' }, 'rewrite url');
+    return '/hello'
+  } else {
+    return req.url;
+  }
 }
 ```
 

diff --git a/fastify.d.ts b/fastify.d.ts
@@ -148,7 +148,12 @@ declare namespace fastify {
       req: FastifyRequest<RequestGeneric, RawServer, RawRequestDefaultExpression<RawServer>, FastifySchema, TypeProvider>,
       res: FastifyReply<RawServer, RawRequestDefaultExpression<RawServer>, RawReplyDefaultExpression<RawServer>, RequestGeneric, FastifyContextConfig, SchemaCompiler, TypeProvider>
     ) => void,
-    rewriteUrl?: (req: RawRequestDefaultExpression<RawServer>) => string,
+    rewriteUrl?: (
+      // The RawRequestDefaultExpression, RawReplyDefaultExpression, and FastifyTypeProviderDefault parameters
+      // should be narrowed further but those generic parameters are not passed to this FastifyServerOptions type 
+      this: FastifyInstance<RawServer, RawRequestDefaultExpression<RawServer>, RawReplyDefaultExpression<RawServer>, Logger, FastifyTypeProviderDefault>,
+      req: RawRequestDefaultExpression<RawServer>
+    ) => string,
     schemaErrorFormatter?: SchemaErrorFormatter,
     /**
      * listener to error events emitted by client connections

diff --git a/fastify.js b/fastify.js
@@ -794,16 +794,12 @@ function fastify (options) {
       // only call isAsyncConstraint once
       if (isAsync === undefined) isAsync = router.isAsyncConstraint()
       if (rewriteUrl) {
-        const originalUrl = req.url
-        const url = rewriteUrl(req)
-        if (originalUrl !== url) {
-          logger.debug({ originalUrl, url }, 'rewrite url')
-          if (typeof url === 'string') {
-            req.url = url
-          } else {
-            const err = new FST_ERR_ROUTE_REWRITE_NOT_STR(req.url, typeof url)
-            req.destroy(err)
-          }
+        const url = rewriteUrl.call(fastify, req)
+        if (typeof url === 'string') {
+          req.url = url
+        } else {
+          const err = new FST_ERR_ROUTE_REWRITE_NOT_STR(req.url, typeof url)
+          req.destroy(err)
         }
       }
       router.routing(req, res, buildAsyncConstraintCallback(isAsync, req, res))

diff --git a/test/types/fastify.test-d.ts b/test/types/fastify.test-d.ts
@@ -198,7 +198,10 @@ expectAssignable<FastifyInstance>(fastify({
 }))
 expectAssignable<FastifyInstance>(fastify({ frameworkErrors: () => { } }))
 expectAssignable<FastifyInstance>(fastify({
-  rewriteUrl: (req) => req.url === '/hi' ? '/hello' : req.url!
+  rewriteUrl: function (req) {
+    this.log.debug('rewrite url')
+    return req.url === '/hi' ? '/hello' : req.url!
+  }
 }))
 expectAssignable<FastifyInstance>(fastify({
   schemaErrorFormatter: (errors, dataVar) => {

diff --git a/test/url-rewriting.test.js b/test/url-rewriting.test.js
@@ -10,6 +10,7 @@ test('Should rewrite url', t => {
   const fastify = Fastify({
     rewriteUrl (req) {
       t.equal(req.url, '/this-would-404-without-url-rewrite')
+      this.log.info('rewriting url')
       return '/'
     }
   })
@@ -43,6 +44,7 @@ test('Should not rewrite if the url is the same', t => {
   const fastify = Fastify({
     rewriteUrl (req) {
       t.equal(req.url, '/this-would-404-without-url-rewrite')
+      this.log.info('rewriting url')
       return req.url
     }
   })
@@ -74,6 +76,7 @@ test('Should throw an error', t => {
   const fastify = Fastify({
     rewriteUrl (req) {
       t.equal(req.url, '/this-would-404-without-url-rewrite')
+      this.log.info('rewriting url')
       return undefined
     }
   })