[APMS-20023] Lenient parsing of JDBC URLs - #6225
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 17d0e23 | Docs | View more details | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69615b9266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def parse_jdbc_uri(uri) | ||
| result = {host: nil, port: nil, database: nil} | ||
| return result unless uri.is_a?(String) && uri.valid_encoding? | ||
| return result unless uri.valid_encoding? |
There was a problem hiding this comment.
When parse_jdbc_uri(nil) is called, this line now raises NoMethodError, whereas the previous implementation explicitly accepted String? and returned empty metadata for nil. Because the method remains publicly callable, narrowing the RBS signature does not protect existing untyped Ruby callers; retain the is_a?(String) guard to avoid a breaking API change.
AGENTS.md reference: AGENTS.md:L106-L111
Useful? React with 👍 / 👎.
| database = path[1..-1] | ||
| return if database.empty? || database.include?("/") | ||
| # Stop at the first URI delimiter (except `/`). | ||
| database = path.split(RFC_3986_URI_DELIMITER_PATTERN, 2).first |
There was a problem hiding this comment.
When a driver permits these characters in its database path, splitting on the generic delimiter regexp silently reports only a prefix of the database name. For example, the valid H2 TCP form jdbc:h2:tcp://db-host:9092/C:/data/orders is tagged as database C rather than C:/data/orders; PostgreSQL-style names containing @, :, or = are similarly truncated even though the previous URI-based parser retained them. Path termination needs to follow the selected driver's syntax rather than applying this delimiter set to every JDBC subprotocol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I did not apply this suggestion as making it work with all vendors would be a lot of work and it currently works without it. cc @marcotc
| if uri.bytesize > MAX_JDBC_URI_BYTES | ||
| # Keep one extra byte, then let `chop` safely remove multi-byte unicode characters. | ||
| uri = uri.byteslice(0, MAX_JDBC_URI_BYTES + 1).chop |
There was a problem hiding this comment.
When an oversized JDBC URL has its userinfo terminator (@) beyond the 8 KiB boundary, this truncation turns the credential prefix into a syntactically complete authority. For example, a long user:<password-with-a-comma>@db-host/... URL is parsed with the truncated user:<password...> value as a multi-host :host, after which set_common_tags exports that credential fragment in network and peer-hostname tags. Reject an over-limit URL when truncation can change authority structure, or parse and remove userinfo before truncating opaque properties.
AGENTS.md reference: AGENTS.md:L99-L104
Useful? React with 👍 / 👎.
| # some drivers report no URL, whereas the opts URL is free and already present for | ||
| # direct connections (and non-JDBC adapters have no such metadata at all). | ||
| if parsed[:host].nil? && parsed[:port].nil? && parsed[:database].nil? | ||
| parsed = jdbc_metadata_from_connection(db) || parsed |
There was a problem hiding this comment.
I have a question, if the jdbc_metadata_from_connection call returns nil or false, or anything that lead to else condition, why do we need that assignment if we know that parsed keys as host, port, and database anyway nil values based on condition of the if-statement?
| # Parses URI-style JDBC connection strings, extracting host, port, and | ||
| # (best-effort) database name. Unsupported or ambiguous forms return empty | ||
| # metadata rather than potentially incorrect tags. | ||
| # JDBC URLs are not URIs (as per RFC 3986). We can't parse it with `URI.parse`. |
There was a problem hiding this comment.
I find it confusing if we call method parse uri and in the comment we deny that what we parsing is uri 😕
My suggestion is to call it a connection string (like any other adapter calls it) and work with that terminology. That will allows us to skip comments that what I see is not what I'm dealing with
| @@ -14,10 +12,26 @@ module Contrib | |||
| module Sequel | |||
| # General purpose functions for Sequel | |||
| module Utils | |||
There was a problem hiding this comment.
I have a feeling that dealing with connection string of such is misplaced into Utils, that allows the code to be abstraction-less and function-like, making it harder to get. Instead it should be encapsulated in a module that is dealing with JDBC connection strings and only that - SRP
3 participants
What does this PR do?
Stop using URI.parse for JDBC URL, as this was silently causing errors, and use lenient parsing instead
Motivation:
URI.parse crashes if there are special characters, e.g. in the query parameters or the password... Which is common in JDBC URLs
Change log entry
None. We do not officially support JRuby anymore
Additional Notes:
Most of the code was written by @marcotc , which is why I'm reviewing it although I've opened the PR. I'm opening the PR so it can be merged before the next release
How to test the change?