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

contrib/dimfeld/httptreemux.v5: default resource namer incorrectly replaces path variables #2881

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MNThomson
Copy link

What does this PR do?

This PR fixes a case where the path variable replacement fails under certain conditions. The current implementation "end of path replacement" isn't actually bound to the end of the path and can do a replacement anywhere in the string. E.x. a parameters value is he and the url is /hey/he it will be replaced as /:?y/he (which obviously isn't intended behaviour).

// replace parameter at end of the path, i.e. "../:param"
oldP = "/" + v
newP = "/:" + k
route = strings.Replace(route, oldP, newP, 1)

Motivation

Fixes #2812

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@MNThomson MNThomson requested review from a team as code owners September 19, 2024 20:46
@MNThomson MNThomson force-pushed the maxt/dimfield-httptreemux-duplicate-param-replacement branch from c5d08e6 to 40c7aca Compare September 23, 2024 18:04
@MNThomson
Copy link
Author

Just checking in, is there anything you folks need from my end? Looking forward to your initial review!

cc @devillecodes

@darccio darccio added the apm:ecosystem contrib/* related feature requests or bugs label Oct 9, 2024
@darccio
Copy link
Member

darccio commented Oct 9, 2024

@MNThomson Thanks for the contribution, and sorry for the late reply. I approved the CI run to verify that everything is good to go. From my first look, it looks fine to me.

@MNThomson
Copy link
Author

Thanks @darccio, looks like I'm not authorized to merge this :(. Anything else needed on my end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] contrib/dimfeld/httptreemux.v5: default resource namer incorrectly replaces path variables
2 participants