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(instr-express): normalize paths with double slashes #1995

Merged

Conversation

JamieDanielson
Copy link
Member

@JamieDanielson JamieDanielson commented Mar 5, 2024

Which problem is this PR solving?

Short description of the changes

  • Add replace to remove duplicate slashes that may exist in defined routes
  • Add unit test confirming route does not contain extra slash
  • Add route to util serverWithMiddleware that contains double slashes, for use in test

Test results before the change:

+ expected - actual
-/double-slashes//:id
+/double-slashes/:id

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Merging #1995 (c90c654) into main (e24797e) will increase coverage by 5.08%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

❗ Current head c90c654 differs from pull request most recent head 754361c. Consider uploading reports for the commit 754361c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1995      +/-   ##
==========================================
+ Coverage   90.97%   96.06%   +5.08%     
==========================================
  Files         146       14     -132     
  Lines        7492      914    -6578     
  Branches     1502      199    -1303     
==========================================
- Hits         6816      878    -5938     
+ Misses        676       36     -640     

see 132 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good already, thanks for working on this 🙂
Just one question about the added test

@pichlermarc pichlermarc added bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect labels Mar 6, 2024
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for working on this 🙂

@pichlermarc pichlermarc merged commit 65a9553 into open-telemetry:main Mar 7, 2024
15 checks passed
@dyladan dyladan mentioned this pull request Mar 7, 2024
@JamieDanielson JamieDanielson deleted the jamie.fix-double-slash branch March 8, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:instrumentation-express priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Express instrumentation does not normalize paths (removing double slashes)
3 participants