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

fix(node): Adjust Express URL parameterization for array routes #5495

Merged
merged 9 commits into from Aug 1, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jul 29, 2022

This PR fixes an error thrown from our Node SDK that when our Express router parameterization logic (introduced in #5450) would try to parameterize a route consisting of an array of paths (which could be strings or RegExes).

Previously, before #5450, the transaction name for a route such as

app.get(['/test/array1', /\/test\/array[2-9]/], (_req, res) => {});

would be GET /test/array1,/\/test\/array[2-9]/. This is because internally, express does not care about which item in the array matched successfully. Express simply combines all items into one regex and matches the incoming path against this regex. It then stores the whole, concatenated array as the matched route.

While it might be worth revisiting our transaction name strategy for array routes at a later time, this is out of scope for this PR and the bugfix. Therefore, transaction names (for now) stay the same but, the patch fixes the error that was thrown as well as adding URL parameterization in time for DSC population (well, mostly; see #5450 for more details on that).

Since a crucial part of our parameterization approach is to determine if the currently resolved layer is the "last" part of the route (in which case we update the transaction name and source), this patch also makes a few modifications to determine this correctly for arrays. In order to do so, we check for the number of URL segments in the original URL vs. that of the parameterized URL. In the case of arrays, we'll likely have more segments than in the raw URL path. Therefore, we balance this out by determining the number of extra segments we got from the array.

Additionally, added tests for array routes.

fixes #5489

@Lms24 Lms24 self-assigned this Jul 29, 2022
@Lms24 Lms24 requested a review from AbhiPrasad July 29, 2022 11:32
Lms24 added a commit that referenced this pull request Jul 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.36 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 59.92 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.93 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 52.79 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.69 KB (0%)
@sentry/browser - Webpack (minified) 64.13 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.71 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.11 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.76 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.01 KB (+0.01% 🔺)

@Lms24 Lms24 enabled auto-merge (squash) August 1, 2022 10:58
Lms24 added a commit that referenced this pull request Aug 1, 2022
@Lms24 Lms24 merged commit d5298b5 into master Aug 1, 2022
@Lms24 Lms24 deleted the lms-fix-express-arrays branch August 1, 2022 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: partialRoute.split is not a function when using array as path argument
2 participants