-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lms24
force-pushed
the
lms-fix-express-arrays
branch
from
July 29, 2022 11:17
b155cf1
to
8dd304c
Compare
AbhiPrasad
approved these changes
Jul 29, 2022
size-limit report 📦
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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