You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "kpumuk (via GitHub)" <gi...@apache.org> on 2026/07/20 22:20:09 UTC
[PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
kpumuk opened a new pull request, #3649:
URL: https://github.com/apache/thrift/pull/3649
Replace the malformed diagnostic label returned by `Thrift::HTTPClientTransport#to_s` with an endpoint description that includes the scheme, host, port, and path.
The label distinguishes HTTP from HTTPS while leaving URL userinfo and query parameters out of logs and exception output.
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? [THRIFT-6109](https://issues.apache.org/jira/browse/THRIFT-6109)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3649:
URL: https://github.com/apache/thrift/pull/3649#discussion_r3617980853
##########
lib/rb/lib/thrift/transport/http_client_transport.rb:
##########
@@ -62,7 +62,7 @@ def flush
end
def to_s
- "@{self.url}"
+ "#{@url.scheme}(#{@url.host}:#{@url.port}#{@url.path})"
end
Review Comment:
Good catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3649:
URL: https://github.com/apache/thrift/pull/3649#discussion_r3617947905
##########
lib/rb/lib/thrift/transport/http_client_transport.rb:
##########
@@ -62,7 +62,7 @@ def flush
end
def to_s
- "@{self.url}"
+ "#{@url.scheme}(#{@url.host}:#{@url.port}#{@url.path})"
end
Review Comment:
The non-2xx response error message in `flush` still interpolates `@url`, which includes userinfo and query parameters (e.g., credentials/tokens). This undermines the goal of keeping endpoint labels safe for logs/exceptions; use the transport’s sanitized `to_s` (or equivalent) in the exception message instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3649:
URL: https://github.com/apache/thrift/pull/3649#discussion_r3618040340
##########
lib/rb/lib/thrift/transport/http_client_transport.rb:
##########
@@ -62,7 +62,9 @@ def flush
end
def to_s
- "@{self.url}"
+ path = @url.path.to_s
+ path = '/' if path.empty?
+ "#{@url.scheme}(#{@url.host}:#{@url.port}#{path})"
Review Comment:
`to_s` formats the endpoint as `host:port`, but for IPv6 literals `@url.host` contains `:` characters, making the label ambiguous (e.g., `http(2001:db8::1:80/...)`). Bracket the host when it looks like an IPv6 address so host/port remain clearly distinguishable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3649:
URL: https://github.com/apache/thrift/pull/3649#discussion_r3618081960
##########
lib/rb/lib/thrift/transport/http_client_transport.rb:
##########
@@ -62,7 +62,9 @@ def flush
end
def to_s
- "@{self.url}"
+ path = @url.path.to_s
+ path = '/' if path.empty?
+ "#{@url.scheme}(#{@url.host}:#{@url.port}#{path})"
Review Comment:
Added a test, we actually format the host correctly for ipv6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3649:
URL: https://github.com/apache/thrift/pull/3649#discussion_r3618056601
##########
lib/rb/lib/thrift/transport/http_client_transport.rb:
##########
@@ -62,7 +62,9 @@ def flush
end
def to_s
- "@{self.url}"
+ path = @url.path.to_s
+ path = '/' if path.empty?
+ "#{@url.scheme}(#{@url.host}:#{@url.port}#{path})"
Review Comment:
OMG, how do you come up with this edge cases, and why didn't you give them all at once to me? 😂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3649:
URL: https://github.com/apache/thrift/pull/3649#discussion_r3617980695
##########
lib/rb/lib/thrift/transport/http_client_transport.rb:
##########
@@ -62,7 +62,7 @@ def flush
end
def to_s
- "@{self.url}"
+ "#{@url.scheme}(#{@url.host}:#{@url.port}#{@url.path})"
end
Review Comment:
`to_s` uses `@url.path` directly, but Ruby's `URI` returns an empty string for root URLs (e.g., `http://example.com`), which would produce an endpoint label with no path at all. Since this label is intended to include a path, normalize an empty/nil path to `/` to keep output consistent and unambiguous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk merged PR #3649:
URL: https://github.com/apache/thrift/pull/3649
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6109: Format Ruby HTTP transport endpoint labels [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3649:
URL: https://github.com/apache/thrift/pull/3649#issuecomment-5028631209
### Code review
No issues found. Checked for bugs, AGENTS.md compliance, and security implications.
🤖 Generated with [Claude Code](https://claude.ai/code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org