Skip to content

Conversation

@teeohhem
Copy link
Contributor

Ensures that trailing slashes are stripped before saving to DB

Ref: HDX-1612

Ensures that trailing slashes are stripped before saving to DB

Ref: HDX-1612
@changeset-bot
Copy link

changeset-bot bot commented Apr 14, 2025

🦋 Changeset detected

Latest commit: 3dadbbd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

// Make sure we don't save a trailing slash in the host
const normalizedData = {
...data,
host: stripTrailingSlash(data.host),
Copy link
Member

Choose a reason for hiding this comment

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

Probably out of scope. I wonder if we should handle this on the API side to always remove the trailing slash before any update/insert operations since I suspect the clickhouse client would throw with the malformed host

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely should also be handling this in the API, but it was a slippery slope exclusively making that solution. I'll add in the API side as well.

Comment on lines 726 to 728
if (!url) {
return '';
}
Copy link
Member

@wrn14897 wrn14897 Apr 15, 2025

Choose a reason for hiding this comment

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

maybe we shouldn't mutate the return value if its nullish. I'd imagine the caller should throw if host string is invalid?

Copy link
Contributor

Choose a reason for hiding this comment

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

agree. this should throw instead of a valid type that will just fail later on.

@vercel
Copy link

vercel bot commented Apr 29, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hyperdx-v2-oss-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2025 5:36pm

dhable
dhable previously approved these changes Apr 29, 2025
@teeohhem teeohhem merged commit 7de8916 into v2 Apr 29, 2025
6 checks passed
@teeohhem teeohhem deleted the tom/connection-trailing-slash branch April 29, 2025 17:38
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.

4 participants