feat: added originalUrl in request object #4704 by aarontravass · Pull Request #4758 · fastify/fastify

aarontravass · 2023-05-16T23:19:38Z

Added originalUrl as requested in #4704 in the req object itself. Added it in the fastify request type as well.

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

metcoder95 · 2023-05-17T08:23:16Z

Tests are failing 🤔

mcollina

why are you setting originalUrl twice?

mcollina · 2023-05-17T08:26:55Z

fastify.js

    let isAsync
    return function preRouting (req, res) {
      // only call isAsyncConstraint once
+      /* istanbul ignore next */


why ignoring this?

mcollina · 2023-05-17T08:27:27Z

fastify.js

+      /* istanbul ignore next */
+      if (req.originalUrl == null) {
+        req.originalUrl = req.url
+      }


You should place this block inside the if (rewriteUrl) one

I don't think that would be a good idea because it would be better if we always give access to originalUrl even when not redirected. Inherently, req.originalUrl is not set because at each request we do req.originalUrl = req.originalUrl || req.url which takes the old one, if not null, else the req.url.

mcollina · 2023-05-17T08:27:49Z

lib/route.js

    const request = new context.Request(id, params, req, query, childLogger, context)
    const reply = new context.Reply(res, request, childLogger)
-
+    /* istanbul ignore next */


should not be ignored

types/request.d.ts

mcollina

why are you setting originalUrl twice?

aarontravass · 2023-05-17T15:35:11Z

why are you setting originalUrl twice?

That is because at Line 469 a new Request is being created and it does not copy all the keys from req and hence if I don't set it again, it will not automatically be available in the next request.

aarontravass · 2023-05-17T15:37:56Z

Tests are failing 🤔

Why does the 404 test always give a SIGTERM error and fail? Happens occasionally on windows as well

mcollina · 2023-05-18T12:43:07Z

fastify.js

      // only call isAsyncConstraint once
+      req.originalUrl = req.originalUrl || req.url


Can you move this line within the if (rewriteUrl) block and then move the || in https://github.com/fastify/fastify/pull/4758/files#diff-cf6996cafd1df0bc168a5c5f3a7470f11fc3aea9f908091d5dd6be2e7b30a2c3R471.

This will otherwise generate a new hidden class for the req object unnecessarily.

…st case for request object

…astify into aaron/originalurl

…aron/originalurl

…astify into aaron/originalurl

mcollina · 2023-05-19T16:13:25Z

lib/request.js

    }
  },
+  originalUrl: {
+    value: '/',


why is it a slash by default?

If I set it to this.url, then it fails the request test

originalUrl: { value: this.url, writable: true },

FAIL test/internals/request.test.js ✖ should be equal test/internals/request.test.js 61 | t.equal(request.method, 'GET') 62 | t.equal(request.url, '/') > 63 | t.equal(request.originalUrl, '/') | ----^ 64 | t.equal(request.socket, req.socket) 65 | t.equal(request.protocol, 'http') 66 | t.equal(request.routerPath, context.config.url) test: test/internals/request.test.js Regular request found: null wanted: / compare: === stack: | Test.<anonymous> (test/internals/request.test.js:63:5) Object.<anonymous> (test/internals/request.test.js:15:1) Module.replacementCompile (node_modules/append-transform/index.js:60:13) Object.<anonymous> (node_modules/append-transform/index.js:64:4)

I can't have a getter because then getters cannot be set dynamically.

I can think of 2 solutions

avoid setting a default but then we need to remove the test t.equal(request.originalUrl, '/')

Or, set it to a default string of value: '/'

Which one should I use?

@mcollina ping

I think it should default to the this.raw.url by default. See my comment below about the rewriteUrl 👍

fastify.js

metcoder95 · 2023-06-01T08:00:46Z

lib/request.js

    }
  },
+  originalUrl: {
+    value: '/',


I think it should default to the this.raw.url by default. See my comment below about the rewriteUrl 👍

…getter and setter

lib/request.js

lib/route.js

fastify.js

metcoder95

It seems the branch has conflicts with main

lib/request.js

fastify.js

mcollina

lgtm

github-actions · 2024-06-11T00:22:45Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

feat: added originalUrl in request object fastify#4704

9f2b83f

mcollina requested changes May 17, 2023

View reviewed changes

fix: added type checking and simplified req.originalUrl assignment

1d8ad18

aarontravass requested a review from mcollina May 17, 2023 15:42

Merge branch 'main' into aaron/originalurl

8f44eb0

mcollina reviewed May 18, 2023

View reviewed changes

aarontravass and others added 5 commits May 18, 2023 12:09

fix: moved req.originalurl assignment in rewriteurl if and added a te…

bfacd6b

…st case for request object

Merge branch 'aaron/originalurl' of https://github.com/aarontravass/f…

d76c891

…astify into aaron/originalurl

Merge branch 'main' into aaron/originalurl

04dddc2

Merge branch 'main' of https://github.com/aarontravass/fastify into a…

d658cd5

…aron/originalurl

Merge branch 'aaron/originalurl' of https://github.com/aarontravass/f…

540e496

…astify into aaron/originalurl

aarontravass requested a review from mcollina May 18, 2023 16:12

mcollina reviewed May 19, 2023

View reviewed changes

aarontravass requested a review from mcollina May 22, 2023 23:56

metcoder95 reviewed Jun 1, 2023

View reviewed changes

fix: originalUrl now gets its default value from this.raw.url. added …

1313a2b

…getter and setter

aarontravass requested a review from metcoder95 June 2, 2023 03:00

metcoder95 reviewed Jun 3, 2023

View reviewed changes

lib/request.js Show resolved Hide resolved

lib/route.js Outdated Show resolved Hide resolved

fastify.js Outdated Show resolved Hide resolved

fix: used a symbol to store the originalUrl

5cc34b5

aarontravass requested a review from metcoder95 June 6, 2023 22:04

metcoder95 reviewed Jun 7, 2023

View reviewed changes

lib/request.js Show resolved Hide resolved

metcoder95 reviewed Jun 7, 2023

View reviewed changes

lib/request.js Show resolved Hide resolved

fastify.js Outdated Show resolved Hide resolved

aarontravass added 2 commits June 7, 2023 17:03

chore: merged main of upstream into aaron/originalurl

a19b93c

fix: corrected symbol usage as requested

65b86a2

metcoder95 approved these changes Jun 9, 2023

View reviewed changes

mcollina approved these changes Jun 10, 2023

View reviewed changes

mcollina merged commit f4a8f36 into fastify:main Jun 10, 2023

This was referenced Jan 29, 2024

fix(deps): update dependency fastify to v4.28.1 - autoclosed bryopsida/syslog-api#76

Closed

chore(deps): update dev dependency fastify to v4.26.2 eik-lib/postcss-plugin#331

Merged

fix(deps): update dependency fastify to ^4.26.2 gqty-dev/gqty#1498

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2024

		// only call isAsyncConstraint once
		req.originalUrl = req.originalUrl \|\| req.url

Uh oh!

Conversation

aarontravass commented May 16, 2023

Uh oh!

metcoder95 commented May 17, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

aarontravass commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aarontravass commented May 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarontravass May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aarontravass commented May 17, 2023 •

edited

Loading

aarontravass May 19, 2023 •

edited

Loading