fix(dbapi): avoid IndexError in get_operation_name for comment/whites… - #4934
fix(dbapi): avoid IndexError in get_operation_name for comment/whites…#4934Atishyy27 wants to merge 2 commits into
Conversation
…pace-only statements A statement that is truthy but has no tokens left after leading-comment or whitespace stripping (e.g. a comment-only or whitespace-only query) made `.split()[0]` raise IndexError, which propagates out of the instrumented execute call. Guard the result the same way the `_Template` branch already does and return an empty operation name instead. Adds a regression test. Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
ae76d02 to
d95c7b3
Compare
henry3260
left a comment
henry3260
left a comment
There was a problem hiding this comment.
Thanks for the fix! Just some nits.
I think we should add a changelog fragment under .changelog/ instead of editing CHANGELOG.md directly.
| cursor.execute(query) | ||
| spans_list = self.memory_exporter.get_finished_spans() | ||
| self.assertEqual(len(spans_list), 2) | ||
| for span in spans_list: |
There was a problem hiding this comment.
Could we wrap the loop body in self.subTest(query=query)? Then the failure output names which query broke, and one failing input doesn't stop the other from running.
There was a problem hiding this comment.
done in bddd827
wrapped the loop body in self.subTest(query=query), so a failing input names its query and one bad case doesn't stop the others. thanks!
| if query and isinstance(query, str): | ||
| # Strip leading comments so we get the operation name. | ||
| return self._leading_comment_remover.sub("", query).split()[0] | ||
| tokens = self._leading_comment_remover.sub("", query).split() |
There was a problem hiding this comment.
Same shape exists in psycopg2's get_operation_name override — it has no guard at all, so even conn.execute("") from #2643 still raises.
There was a problem hiding this comment.
good catch. yeah, psycopg2's get_operation_name override has the same unguarded .split()[0], so conn.execute("") from #2643 still raises there. i'll fix psycopg2 (and check psycopg + sqlalchemy for the same shape) in a separate PR to keep this one a single logical change, rather than bundling it here.
… empty-token test Signed-off-by: Atishyy27 <142108881+Atishyy27@users.noreply.github.com>
|
also moved the changelog to a .changelog/4934.fixed fragment per your note. |
2 participants
Description
get_operation_namein the DB-API instrumentation strips a leading SQL comment andtakes the first token as the operation name. If a statement is truthy but has no tokens
left after stripping (a comment-only or whitespace-only statement),
.split()[0]raisesIndexError, which propagates out of the instrumentedexecute()call and breaks it.This guards the result the same way the adjacent
_Templatebranch already does(
tokens[0] if tokens else "") and returns an empty operation name instead. It extends theempty-string handling added in #2643 to the comment-only / whitespace-only case.
No open issue for this one — found while reading the operation-name parsing. Related: #2643.
Type of change
How Has This Been Tested?
Added
test_operation_name_no_tokens_after_comment_stripintest_dbapi_integration.py,which executes a comment-only (
/* comment only */) and a whitespace-only (" ")statement and asserts instrumentation does not raise. Verified locally that it passes and
that the existing
test_span_succeededstill passes.Does This PR Require a Core Repo Change?
Checklist: