Log bound parameter values for prepared statements#9
Conversation
Hook mysqlnd stmt execute() to capture bound param values at execution time. Previously only the query template with ? placeholders was logged at prepare() time. C extension (PHP 7.0+): - Hook execute(): read stmt->data->param_bind zvals, build JSON array - Modify prepare(): store query template in HashTable, defer logging - Hook dtor(): clean up stored template when stmt is destroyed - Add profiler_log_query_with_params() for params-aware logging - JSONL output: new "params" field e.g. ["123","active",null] - Raw log output: "params: [...]" line after query PHP 5.x retains existing behavior (template-only at prepare time). JetBrains plugin: - QueryEntry: add params field, boundQuery computed property - QueryDetailPanel: show "Bound Parameters" section with ?N = value - SQL area shows interpolated query when params are available https://claude.ai/code/session_01RetwzD4CVW4uBT9hS2aSPw
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPHP7+でプリペアドステートメントのクエリテンプレートをリクエスト単位のハッシュテーブルに保存し、実行時にバインド値をJSON化してステータス付きでログ出力する機能を追加。JetBrainsプラグインはバインドパラメータ表示と結合済みクエリ表示、関連ユニットテストを導入。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant mysqlnd as MYSQLND_stmt
participant globals as ModuleGlobals\n(stmt_queries)
participant profiler as Profiler\n(profiler_log)
participant sink as LogSink
Client->>mysqlnd: prepare(query_template)
alt PHP>=7
mysqlnd->>globals: store(template)
else PHP<7
mysqlnd->>profiler: profiler_log_query(query_template)
end
Client->>mysqlnd: bind params / execute()
alt template exists (PHP>=7)
mysqlnd->>mysqlnd: profiler_build_params_json(params)
mysqlnd->>profiler: profiler_log_query_with_params(template, params_json, status)
profiler->>sink: write RAW/JSONL including "params" and "s"
else
mysqlnd->>profiler: profiler_log_query(query_text)
profiler->>sink: write log
end
mysqlnd->>globals: dtor(stmt) %% remove template from stmt_queries on destroy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ext/mariadb_profiler/profiler_mysqlnd_plugin.c`:
- Around line 168-275: profiler_build_params_json currently accesses internal
fields stmt->data->param_bind and stmt->data->param_count directly which is
unstable across PHP/mysqlnd versions; wrap that usage in an explicit
compile-time guard using MYSQLND_VERSION_ID (allow direct access only for
known-safe versions) and provide a safe fallback for other versions (e.g., use
the public mysqlnd stmt API helpers if available or return NULL/skip JSON
building), so add an `#if` MYSQLND_VERSION_ID >= <min_safe_version> check around
the direct accesses and otherwise avoid touching stmt->data, referencing the
symbols profiler_build_params_json, stmt->data->param_bind,
stmt->data->param_count and the macro MYSQLND_VERSION_ID to locate and fix the
code.
In
`@jetbrains-plugin/src/main/kotlin/com/mariadbprofiler/plugin/model/QueryEntry.kt`:
- Around line 22-32: The boundQuery getter currently replaces every "?" blindly
(using replaceFirst) and does not escape quotes in param values; update
QueryEntry.boundQuery to (1) escape single quotes in each param value by
doubling them (so O'Brien -> O''Brien) and wrap non-null params in single
quotes, and (2) perform placeholder substitution by scanning the query string
character-by-character while tracking whether the scanner is inside a
single-quoted string literal (handle doubled single-quote escapes inside
literals) and only replace question-mark characters that occur outside string
literals with successive param values from params; reference the boundQuery
property, the params list and the original query string when locating where to
change logic.
🧹 Nitpick comments (2)
ext/mariadb_profiler/profiler_mysqlnd_plugin.c (1)
217-227: 冗長なプリプロセッサガードこの
#if PHP_VERSION_ID >= 70000ブロックは、既に行163の#if PHP_VERSION_ID >= 70000ガード内にあるため冗長です。♻️ 修正案
-#if PHP_VERSION_ID >= 70000 case IS_TRUE: memcpy(buf + pos, "\"1\"", 3); pos += 3; break; case IS_FALSE: memcpy(buf + pos, "\"\"", 2); pos += 2; break; -#endifext/mariadb_profiler/profiler_log.c (1)
244-247: JSONL と raw ログ間のパラメータ出力条件の微妙な差異JSONL では
params_jsonが非 NULL であれば無条件に出力されますが（行245）、raw ログでは非空配列チェック（[0] == '[' && [1] != ']'）が行われています（行142）。現時点ではprofiler_build_params_jsonが空配列を返さないため実害はありませんが、将来の変更で空配列[]が渡された場合に出力の整合性が崩れる可能性があります。JSONL 側でも同じガードを適用するか、少なくともコメントで意図を明記することを検討してください。
♻️ 修正案
/* params_json is already a valid JSON array string e.g. ["123","active",null] */ - if (params_json) { + if (params_json && params_json[0] == '[' && params_json[1] != ']') { fprintf(fp, ",\"params\":%s", params_json); }
C extension: - Guard stmt->data->param_bind/param_count access with PROFILER_MYSQLND_PARAM_ACCESS_SAFE compile-time check (MYSQLND_VERSION_ID 70000..80499). Unknown future mysqlnd versions gracefully degrade to NULL (no params) instead of risking ABI mismatch crashes. - Remove redundant inner #if PHP_VERSION_ID >= 70000 around IS_TRUE/IS_FALSE cases (already inside outer PHP 7.0+ block). JetBrains plugin (QueryEntry.boundQuery): - Scan query character-by-character tracking single-quoted SQL string literal state; only substitute ? outside literals. - Handle '' (doubled quote escape) inside literals correctly. - Escape single quotes in param values by doubling them (O'Brien -> 'O''Brien'). https://claude.ai/code/session_01RetwzD4CVW4uBT9hS2aSPw
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for logging bound parameter values for prepared statements in both the C extension (PHP 7.0+) and the JetBrains plugin UI. Previously, only the query template with ? placeholders was logged at prepare() time. Now, the extension hooks execute() to capture actual bound parameter values, and the plugin displays both the interpolated query and individual parameter values.
Changes:
- C extension hooks
mysqlndstatementexecute()to capture bound parameters at execution time, storing query templates in a HashTable and logging them with JSON-serialized parameter arrays - Logging functions extended to accept optional
params_jsonparameter, adding a newparamsfield to JSONL output and a params line to raw logs - JetBrains plugin
QueryEntrymodel addsparamsfield andboundQuerycomputed property that interpolates parameters into the SQL template, with UI showing a "Bound Parameters" panel
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| ext/mariadb_profiler/profiler_mysqlnd_plugin.c | Hooks prepare(), execute(), and dtor() to store/retrieve query templates and serialize bound parameters to JSON |
| ext/mariadb_profiler/profiler_log.c | Adds profiler_log_query_with_params() function and updates JSONL/raw log writers to output params field |
| ext/mariadb_profiler/php_mariadb_profiler.h | Declares new profiler_log_query_with_params() function and adds stmt_queries HashTable to module globals |
| ext/mariadb_profiler/php_mariadb_profiler_compat.h | Adds PROFILER_BOOL_T compatibility macro for mysqlnd stmt dtor signature differences |
| ext/mariadb_profiler/mariadb_profiler.c | Initializes and destroys stmt_queries HashTable in request init/shutdown for PHP 7.0+ |
| jetbrains-plugin/src/main/kotlin/com/mariadbprofiler/plugin/model/QueryEntry.kt | Adds params field and boundQuery property with SQL parameter interpolation logic |
| jetbrains-plugin/src/main/kotlin/com/mariadbprofiler/plugin/ui/panel/QueryDetailPanel.kt | Adds "Bound Parameters" panel and displays interpolated query when parameters are available |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val paramLines = entry.params.mapIndexed { i, v -> | ||
| " ?${i + 1} = ${v ?: "NULL"}" | ||
| } |
There was a problem hiding this comment.
Parameter values in the UI are displayed with simple string concatenation using "?N = value". For NULL values, this shows "?1 = NULL" which is correct. However, for non-null string values, no quotes are shown, so "?1 = hello" is displayed instead of "?1 = 'hello'". This could be confusing for users who might not realize that the value is a string. Consider wrapping non-NULL parameter values in quotes or using a different format to make the data type clearer, similar to how the boundQuery property wraps values in quotes.
![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: 42861741e8
ℹ️ 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".
Co-authored-by: 39ff <7544687+39ff@users.noreply.github.com>
Handle backslash escapes in boundQuery SQL parser
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d7b46b68c
ℹ️ 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".
- Fix snprintf truncation in profiler_build_params_json: check return value and grow+retry when output was truncated, handle negative return values (encoding errors) - Enhance boundQuery SQL parser to handle double-quoted string literals, backtick-quoted identifiers, line comments (--), and block comments (/* */) so placeholders inside these contexts are not replaced - Escape backslashes in parameter values before interpolation to produce accurate SQL text for debugging - Document MYSQLND_VERSION_ID upper bound as a defensive estimate requiring verification when PHP 8.5 is released - Add comprehensive unit tests for boundQuery covering: no params, single/multiple placeholders, NULL params, param count mismatches, quoted strings, escaped quotes, backslash escaping, double-quoted strings, backtick identifiers, line/block comments, unclosed comments https://claude.ai/code/session_01RetwzD4CVW4uBT9hS2aSPw
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ext/mariadb_profiler/profiler_mysqlnd_plugin.c`:
- Around line 327-343: The current code calls profiler_log_query_with_params
(using profiler_build_params_json and PROFILER_G(stmt_queries) entry for stmt)
before calling orig_stmt_methods->execute, which logs queries even when execute
fails; move the logging block to after calling orig_stmt_methods->execute,
capture and check the execute return value (the value returned by
orig_stmt_methods->execute(stmt)) and only call profiler_log_query_with_params
when execute indicates success, ensure profiler_build_params_json(stmt) is
invoked after a successful execute (or include the execution status in the
logged data if you prefer) and still efree params_json when allocated; also
verify that any bound parameter state (e.g., param_bind associated with stmt)
remains valid after execute before accessing it.
In
`@jetbrains-plugin/src/main/kotlin/com/mariadbprofiler/plugin/model/QueryEntry.kt`:
- Around line 50-60: Update the `--` line-comment detection in QueryEntry (the
block handling ch == '-' with query, i, sb) to follow MySQL/MariaDB rules: only
treat `--` as a line comment when the character after the two hyphens is absent,
a whitespace, or a control character. Concretely, change the condition that
currently treats any `--` as a comment to additionally check (i+2 >=
query.length) || query[i+2].isWhitespace() || query[i+2].isISOControl() before
entering the comment-handling branch so expressions like `SELECT 1--?` are not
misclassified as comments.
- Line 15: QueryEntry の JSON キー名が部分的に短縮(`query`→`q`, `timestamp`→`ts`,
`jobKey`→`k`)されている一方で、params/tag/trace はアノテーションがなく全名で出力されているため、統一性を持たせるために
params、tag、trace フィールドに対しても `@SerialName` を追加するか（例:
短縮キーを決める）、現在の設計意図を示すクラス/ファイルレベルのコメントを追加して将来の保守性を確保してください（対象シンボル: class
QueryEntry, properties params, tag, trace, 使用中の `@SerialName` アノテーション）。
🧹 Nitpick comments (3)
jetbrains-plugin/src/test/kotlin/com/mariadbprofiler/plugin/model/QueryEntryTest.kt (1)
252-341: テストケースの重複があります以下のテストは既に上部で同等のケースが存在しています：
重複テスト (下部) 既存テスト (上部) boundQuery replaces placeholders with params(L253)boundQuery replaces single placeholder(L82) +boundQuery replaces multiple placeholders(L88)boundQuery escapes single quotes in params(L262)boundQuery escapes single quotes in param values(L142)boundQuery does not replace placeholders inside single-quoted strings(L271)boundQuery does not replace placeholder inside single-quoted string(L124)boundQuery handles doubled single-quotes inside strings(L280)boundQuery handles escaped single quotes in SQL literal(L133)重複を整理するとテストの見通しが良くなります。
ext/mariadb_profiler/profiler_mysqlnd_plugin.c (2)
197-210: カンマと閉じ括弧の書き込みに明示的な境界チェックがないLine 203 のカンマ書き込みは、Line 207 の容量チェック（
pos + 128 > buf_size）の前に行われます。実際には先行するバッファ拡張のマージン（最低 64〜256 バイト）によりオーバーフローは発生しませんが、ロジックが暗黙のマージンに依存しており脆弱です。同様に Line 309-310 の
]と\0もチェックなしです。容量チェックをカンマ書き込みの前に移動し、ループ後に閉じ括弧用のチェックを追加すると堅牢になります。
♻️ 提案: 明示的な境界チェックの追加
for (i = 0; i < data->param_count; i++) { zval *zv = &data->param_bind[i].zv; + /* Ensure space for comma + value (grow if needed) */ + if (pos + 128 > buf_size) { + buf_size *= 2; + buf = (char *)erealloc(buf, buf_size); + } + if (i > 0) { buf[pos++] = ','; } - /* Ensure space for this param (grow if needed) */ - if (pos + 128 > buf_size) { - buf_size *= 2; - buf = (char *)erealloc(buf, buf_size); - } - /* Dereference if reference (bind_param uses references) */ ZVAL_DEREF(zv);} + /* Ensure space for closing bracket and null terminator */ + if (pos + 2 > buf_size) { + buf_size = pos + 2; + buf = (char *)erealloc(buf, buf_size); + } buf[pos++] = ']'; buf[pos] = '\0';
221-255: IS_LONG と IS_DOUBLE の snprintf リトライロジックが重複両ケースのトランケーション検出・バッファ拡張・リトライのパターンが同一です。ヘルパーマクロまたはインライン関数への抽出で重複を減らせますが、現状でも正しく動作しています。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80d47ca1f2
ℹ️ 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".
- Move logging after execute in all mysqlnd hooks (query, send_query,
stmt_execute) so the result status is available at log time
- Add "s" field ("ok"/"err") to JSONL output and [ok]/[err] to raw log
- Update all log function signatures to accept status parameter
- Add status field with @SerialName("s") to QueryEntry.kt
- Add @SerialName annotations to params/tag/trace for consistency
- Fix -- line comment detection to require trailing whitespace per
MySQL/MariaDB spec (prevents misparse of expressions like 1--?)
- Add # hash comment support in boundQuery placeholder substitution
- Add tests for status parsing, hash comments, and -- edge cases
https://claude.ai/code/session_01RetwzD4CVW4uBT9hS2aSPw
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aea79077e
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@jetbrains-plugin/src/main/kotlin/com/mariadbprofiler/plugin/model/QueryEntry.kt`:
- Around line 56-57: The `--` line-comment check in QueryEntry (the conditional
inside the query parsing logic) currently only treats a space, tab or newline
after `--` as a comment start and misses `\r`; update that conditional in
QueryEntry (the if that checks "ch == '-' && i + 1 < query.length && query[i +
1] == '-' && i + 2 < query.length && (query[i + 2] == ' ' || query[i + 2] ==
'\t' || query[i + 2] == '\n')") to also accept '\r' (or replace the explicit set
with a whitespace check that includes '\r') so lines ending with `--\r\n` are
correctly recognized as comments.
🧹 Nitpick comments (4)
ext/mariadb_profiler/profiler_log.c (2)
256-258:statusが JSON エスケープされていません
statusはfprintfでそのまま JSON 文字列値に埋め込まれています。現在の呼び出し元では"ok"/"err"のみなので問題は発生しませんが、将来的に任意の文字列が渡された場合、"や\を含むとJSONが壊れます。防御的に
profiler_log_escape_json_stringを通すか、少なくとも呼び出し契約をコメントで明示することを検討してください。
109-146: raw ログのparams_jsonチェックと JSONL 側のチェックに差異があります（意図的であれば問題なし）Line 144 では
params_json[0] == '[' && params_json[1] != ']'で空配列[]を除外していますが、Line 247 ではif (params_json)のみで空配列も出力されます。JSONL では機械パース用に空配列でもフィールドを維持する意図であれば問題ありませんが、意図をコメントで明示するとより分かりやすくなります。ext/mariadb_profiler/profiler_mysqlnd_plugin.c (2)
207-213: バッファ管理：カンマ書き込みが grow チェックの前に行われていますLine 213 の
buf[pos++] = ','は Line 217 の grow チェックより前に実行されます。現在のコードでは各値の書き込み後に最低 2 バイトの余裕が残る（IS_STRING は+4の余裕を確保、その他のケースは 128 バイトの保証から消費分を差し引いた残り）ため、実際にはオーバーフローしませんが、このインバリアントは暗黙的です。将来のケース追加でバグを誘発しないよう、カンマ書き込み前にも最小限の容量チェックを入れるか、grow チェックをカンマの前に移動することを検討してください。
♻️ 修正案
if (i > 0) { + if (pos + 1 >= buf_size) { + buf_size *= 2; + buf = (char *)erealloc(buf, buf_size); + } buf[pos++] = ','; }Also applies to: 217-220
231-246: PHP 7+ での Windows LLP64 対応を検討してください
(long)Z_LVAL_P(zv)+%ldは PHP 7+ の Windows LLP64 環境では問題があります。PHP 7.0+ ではzend_longが 64-bit、longが 32-bit のため値が切り詰められます。ただし、ZEND_LONG_FMT マクロは PHP 7.0+ でのみ利用可能です。このコードが PHP 5.3-5.6 をサポートしているため、バージョン条件付きの修正が必要です：
♻️ 修正案
+#if PHP_VERSION_ID >= 70000 + int written = snprintf(buf + pos, buf_size - pos, "\"" ZEND_LONG_FMT "\"", + Z_LVAL_P(zv)); +#else int written = snprintf(buf + pos, buf_size - pos, "\"%ld\"", (long)Z_LVAL_P(zv)); +#endif（リトライ側の snprintf 呼び出しも同様に変更が必要です）
…nt detection - Log failed prepare() with status "err" on PHP 7+ (previously lost since no execute() follows a failed prepare) - Rewrite profiler_build_params_json to dispatch on the declared bind type (MYSQL_TYPE_LONG/DOUBLE/VAR_STRING/LONG_BLOB) instead of the zval's runtime type, matching the coercion mysqlnd applies before sending to the server - Extract string-writing helper to reduce duplication in param builder - Add \r to the -- line-comment whitespace check in QueryEntry.kt so that --\r\n line endings are recognised as comments - Add test for CRLF line-comment edge case https://claude.ai/code/session_01RetwzD4CVW4uBT9hS2aSPw
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3f67ac365
ℹ️ 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".
…pare logging - Fix CRLF comment test: --\r\n comment ends at \n, so text on the next line is outside the comment and its ? placeholder gets replaced - Fix profiler_build_params_json_write_string to propagate updated buf_size back to caller via pointer, preventing stale capacity after erealloc that could cause out-of-bounds writes on subsequent params - Log failed prepare() on PHP 5.x branch (previously only logged on PASS, making syntax/permission errors invisible) https://claude.ai/code/session_01RetwzD4CVW4uBT9hS2aSPw
Hook mysqlnd stmt execute() to capture bound param values at execution time. Previously only the query template with ? placeholders was logged at prepare() time.
C extension (PHP 7.0+):
PHP 5.x retains existing behavior (template-only at prepare time).
JetBrains plugin:
https://claude.ai/code/session_01RetwzD4CVW4uBT9hS2aSPw
Summary by CodeRabbit