Optimize PDOIntegration::parseDsn()#3430
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3430 +/- ##
==========================================
- Coverage 61.85% 61.74% -0.12%
==========================================
Files 141 141
Lines 12481 12481
Branches 1630 1630
==========================================
- Hits 7720 7706 -14
- Misses 4041 4054 +13
- Partials 720 721 +1 see 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2025-09-29 19:24:10 Comparing candidate commit 67c6467 in PR branch Found 3 performance improvements and 0 performance regressions! Performance is the same for 191 metrics, 0 unstable metrics. scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:TraceSerializationBench/benchSerializeTrace
|
| { | ||
| $engine = substr($dsn, 0, strpos($dsn, ':')); | ||
| $tags = ['db.engine' => $engine]; | ||
| \preg_match('(\A(?<engine>[^:]++)(?::(?:(?:charset=(?<charset>[^;]++)|(?:database|dbname)=(?<db>[^;]++)|(?:server|unix_socket|host(?:name)?)=(?<host>[^;]++)|port=(?<port>[^;]++)|driver=(?<driver>[^;]++)|[^;]*+)(?:;|\Z))++)?)i', $dsn, $m); |
There was a problem hiding this comment.
This looks very limited:
- does not support DSNs without colon (ini lookup for pdo.dsn.{value}
- does not support uri:
- the driver specific part does not support colon escapes with ;; (on the generic path for mysql, oci, etc.) and does not properly parse sqlite or psql (sqlite:/path sqlite::memory: , psql:host=localhost port=5432 dbname=foo)
There was a problem hiding this comment.
Indeed, (?:;|\Z) should be ;*+\Z instead.
Everything else was not supported in the existing code though. I'll have a look.
| } else { | ||
| // There's technically also uri: but we don't support it and it's anyway deprecated | ||
| $tags = [ | ||
| Tag::DB_SYSTEM => 'other_sql', | ||
| Tag::DB_TYPE => 'other_sql', | ||
| ]; |
There was a problem hiding this comment.
In the fallback where the DSN can't be parsed (and there's no ini_get("pdo.dsn.$dsn") hit), we currently return only db.system/db.type.
Previously, we also set db.engine from the prefix (substring before the first :). Shouldn't we keep db.engine to avoid a tagging regression in metrics & dashboards that key off db.engine?
There was a problem hiding this comment.
The only case where the DSN cannot be parsed is if there's no colon in the dsn. So actually the case when you use a plain name, but the ini doesn't exist.
| foreach ($valStrings as $valString) { | ||
| if (!strpos($valString, '=')) { | ||
| continue; | ||
| if (\preg_match('(\A(?<engine>[^:]++)(?::(?:(?:(?:server|unix_socket|host(?:name)?)=(?<host>(?:[^;]*+(?:;;)?)++)|port=(?<port>(?&host))|charset=(?<charset>(?&host))|(?:database|dbname)=(?<db>(?&host))|driver=(?<driver>(?&host))|(?&host))(?:;|\Z))++)?)i', $dsn, $m)) { |
There was a problem hiding this comment.
I believe the regex tail (?:;|\Z) is a bit too string, as in that it only handles one trailing ;. If the DSN (for whatever reason) ends with multiples semicolons, like:
mysql:host=127.0.0.1;port=3306;dbname=foo;;
then I believe the match would either fail (or backtrack a bunch). Perhaps the easiest fix would be to just allow "zero or more" trailing semicolons?
- ... (?:;|\Z)
+ ... ;*+\z
PS: a nit of mine would be to put the a top-level constant (e.g., DSN_REGEX), and make it multiline... Just for readability.
There was a problem hiding this comment.
Also trailing semicolons are supported thanks to (?&host), but it's not obvious. Made multiline and added a comment.
62d08c8 to
b0f6933
Compare
b0f6933 to
83fd76c
Compare
4 participants
Avoid manual string parsing in a loop, just use a single regex.
And remove some redundant empty branch.