Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(insights): strip sql comments without trailing newline #4493

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

mjq
Copy link
Member

@mjq mjq commented Feb 11, 2025

Found this case when investigating projects emitting high cardinality metrics. SQL comments that did not end in a newline aren't being stripped correctly. Updated the comment-matching regex to additionally match comments up to the end of the haystack.

Note that there's an interesting interaction with the regexes that meant that the test

    scrub_sql_test!(
        comment_without_ending_line_break,
        "SELECT * FROM comments LIMIT 1 -- comment", // note the explicit limit
        "SELECT * FROM comments LIMIT %s"
    );

already passed without these changes, while the version with the named limit param (LIMIT %(param_1)s) did not.

Found this case when investigating projects emitting high cardinality metrics.
SQL comments that did not end in a newline aren't being stripped correctly.
Updated the comment-matching regex to additionally match comments up to the end
of the haystack.
@mjq mjq requested a review from a team as a code owner February 11, 2025 15:49
@mjq mjq enabled auto-merge (squash) February 11, 2025 15:54
@mjq mjq merged commit 860374d into master Feb 11, 2025
25 checks passed
@mjq mjq deleted the mjq-fix-sql-comment-normalization branch February 11, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants