fix(tracing-internal): Fix express middleware path param parsing #8668
Conversation
|
Hey @LubomirIgonda1, thanks for the PR! Could you add an integration test to https://github.com/getsentry/sentry-javascript/tree/develop/packages/node-integration-tests/suites/express to validate that this works? You can also run |
|
@AbhiPrasad Hi I added test for middle layer route path. Let me know if it is OK. thx |
|
Seems like there is some test failures Could you take a look? |
.../node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/server.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * @returns string in layer.route.path structure 'router/:pathParam' or undefined | ||
| */ | ||
| const extractOriginalRoute = ({ path, regexp, keys }: Layer): string | undefined => { |
There was a problem hiding this comment.
I'm not entirely sure I understand what's going on here. Would you mind explaining when we actually run into the situation that we need to extract the route from the regex? Is it just for the case you mentioned in the PR description or could this happen otherwise, too?
I'm assuming the integration test you added already covers this function but if there's another situation where we run into this function being called (that you know of), would you mind adding another integration test for it?
There was a problem hiding this comment.
From what I know the case is only when the middleware contains path param as I already mentioned in merge request. Should I add test for the function ?
There was a problem hiding this comment.
Should I add test for the function ?
Not strictly required from my end but if you have the time, I'd appreciate a test for the function.
…sed multiple middlewares that match request url
There was a problem hiding this comment.
Thanks for your patience sticking with this for so long @LubomirIgonda1!
|
Hi I am used RegExp /d flag for getting match.indices but this feature is available from node 16. That is the reason why tests for node <16 fails. Can you let me know how shold I fix this case ? thank you |
Sorry for the late feedback, kind of got lost in the meanwhile - we'd def. need a solution that works in all our supported node versions (8+ as of now) 😬 can you rewrite the regex to be Node 8+ compatible? Is there a way to do this without the |
Hi I used this package https://www.npmjs.com/package/regexp-match-indices as nodejs polyfill for regex d flag let me know if it is OK |
There was a problem hiding this comment.
let me know if it is OK
If possible, we'd like to avoid adding dependencies to our SDKs. Is there a way we can solve this without adding a dependency?
@Lms24 I don't think so. In this situation I see only two options. First is accept official polyfill as a new dependency or apply this fix only for nodejs 16+ with native regex d flag implementation and previous versions of node just let with current implementation. |
There was a problem hiding this comment.
@LubomirIgonda1 thanks for sticking with us!
or apply this fix only for nodejs 16+ with native regex d flag implementation and previous versions of node just let with current implementation
Talked with the team and we prefer this approach. We're currently trying to get rid of all 3rd party dependencies in the Node SDK (#9199) by vendoring them in. However, this polyfill would be quite big and I think it's totally fine for us to apply this fix only to Node 16+.
Let's try/catch the native /d regex flag and bail if it is not supported. This will require a Node version check in the tests but that's fine.
packages/node-integration-tests/suites/express/multiple-routers/complex-router/test.ts
Outdated
Show resolved
Hide resolved
...es/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/test.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * @returns string in layer.route.path structure 'router/:pathParam' or undefined | ||
| */ | ||
| const extractOriginalRoute = ({ path, regexp, keys }: Layer): string | undefined => { |
There was a problem hiding this comment.
Should I add test for the function ?
Not strictly required from my end but if you have the time, I'd appreciate a test for the function.
…unit test for extractOriginalRoute fn
|
@Lms24 Hi i updated core logic to handle node 16 regex d flag feature in code and tests as well. Let me know if I missed something. thx |
|
thanks for contributing @LubomirIgonda1! |
This PR fix usecase when path params are define in express middleware (.use) not in express endpoint route (.get, post, .etc).
Also posible releated for #3155
Solution does not fix case when layer.path is array or regexp.
Example:
Create endpoint with path param define in middleware layer route as
app.use( '/api', router.use( '/group/:pathParam', router.get('/endpoint', (req, res) => { return res.json() }) ) )then send request on URL:
request URL: /api/group/123/endpoint
Old behavior
sentry build req._reconstructedRoute as:
/api/group/123/endpoint
New behavior
sentry build req._reconstructedRoute as:
/api/group/:pathParam/endpoint
Param _reconstructedRoute is used also for transaction name what is used as group attribute in sentry performance tracing section