Skip to content

Bug 2054397: Test Perl code for vendor-specific SQL - #171

Open
justdave wants to merge 8 commits into
mainfrom
bug-2054397-harmony
Open

Bug 2054397: Test Perl code for vendor-specific SQL#171
justdave wants to merge 8 commits into
mainfrom
bug-2054397-harmony

Conversation

@justdave

Copy link
Copy Markdown
Member

Details

Tests perl code for vendor-specific SQL where we have db-agnostic helper methods available

Additional info

Test Plan

  1. bash docker/run-tests-in-docker.sh release prove -lv t/013db_portability.t
  2. Look at test results.

The initial run produced the following output:

# Failed test 'no raw vendor-specific SQL where a Bugzilla DB helper exists'
# at t/013db_portability.t line 80.
# got: '7'
# expected: '0'
# Bugzilla/Report/SecurityRisk.pm:291: use $dbh->sql_string_concat(...) instead of CONCAT() ( CONCAT(removed) AS removed,
# )
# Bugzilla/Report/SecurityRisk.pm:292: use $dbh->sql_string_concat(...) instead of CONCAT() ( CONCAT(added) AS added
# )
# extensions/BMO/lib/Reports/Triage.pm:317: use $dbh->sql_string_concat(...) instead of CONCAT() ( WHERE bugs_1.bug_id = bugs.bug_id AND CONCAT(flagtypes_1.name, flags_1.status) = 'needinfo?')))
# )
# extensions/BMO/lib/Reports/UserActivity.pm:185: use $dbh->sql_date_format(...) instead of DATE_FORMAT() ( DATE_FORMAT(longdescs.bug_when, '%Y-%m-%d %H:%i:%s') AS ts,
# )
# extensions/BugModal/lib/ActivityStream.pm:361: use $dbh->sql_date_format(...) instead of UNIX_TIMESTAMP() ( UNIX_TIMESTAMP(bug_when), " . $dbh->sql_date_format('bug_when') . ",
# )
# extensions/ProdCompSearch/lib/WebService.pm:210: use $dbh->sql_string_concat(...) instead of CONCAT() ( "CONCAT(products.name, components.name) LIKE "
# )
# extensions/UserProfile/lib/Util.pm:250: use $dbh->sql_string_concat(...) instead of CONCAT() ( SELECT CONCAT('RESOLVED/', added) AS status, COUNT(*) AS count
# )
# Looks like you failed 1 test of 1.
t/013db_portability.t .. not ok 1 - no raw vendor-specific SQL where a Bugzilla DB helper exists
1..1
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests Test Summary Report
-------------------
t/013db_portability.t (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
Failed test: 1
Non-zero exit status: 1
Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.29 cusr 0.11 csys = 0.42 CPU)
Result: FAIL

And this PR should fail tests for the same reason. Second commit will fix the failures.

@justdave
justdave requested a review from a teamJuly 13, 2026 01:03
Comment threadBugzilla/Report/SecurityRisk.pm Outdated
Comment on lines +292 to +295
}
. $dbh->sql_string_concat('removed') . qq{ AS removed,
}
. $dbh->sql_string_concat('added') . qq{ AS added

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure what this code is intending by using concat() on a single entity, but this fixes the compatibility issue.

justdaveand others added 5 commits July 12, 2026 22:15
SecurityRisk.pm's revert of the accidental spacing change missed the
two sql_string_concat continuation lines, and the same 2-vs-4-space
slip recurred unnoticed in nagios_blocker_checker.pl. No behavior
change, verified against .perltidyrc.
…y test
SQL function names are case-insensitive, so a lowercase concat(...) or
unix_timestamp(...) would silently bypass the check. No current
violations found repo-wide; this is a defense-in-depth follow-up.

@mrenvoizemrenvoize left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this looks good.. I added a couple of trivial followups to fix a newly failing tidy test and also make the check case insensitive as SQL itself is often case insensative.

I'm happy to approve this, but it did wonder if we might want to centralise more of the File::Find logic, around /scripts, into the shared helper class rather than do some or our own stuff here.. but I think that should be it's own bug if we opt to do so.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@justdave@mrenvoize