[Bug 42585] New: Add ability for Koha to analyze reports and warn for potentially dangerous reports
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Bug ID: 42585 Summary: Add ability for Koha to analyze reports and warn for potentially dangerous reports Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Reports Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: lisette@bywatersolutions.com It is quite possible to write reports in Koha that, when run, create huge temp tables that crash a database server or cause runreport.pl to use so much RAM that the kernel oom-kills Koha. It would be nice if we had a tool built in to Koha to help warn librarians about reports with the potential to bring down a Koha server. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |phil@chetcolibrary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #1 from David Cook <dcook@prosentient.com.au> --- I thought that there was a report open for this already but I can't find it with a quick search. Maybe I've just talked with people about this. Anyway, I think it's a good idea. And there's some options. The first one I was thinking of was doing EXPLAIN queries and seeing what the query planner thinks of the query. Another option (which would probably be best served by plugins) would be to have a LLM take a look at it. I've pointed AIs at Koha SQL queries in the past to see how well they would do, and it seemed like they did pretty good. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #2 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to David Cook from comment #1)
I thought that there was a report open for this already but I can't find it with a quick search. Maybe I've just talked with people about this. Anyway, I think it's a good idea.
And there's some options. The first one I was thinking of was doing EXPLAIN queries and seeing what the query planner thinks of the query.
Yes! I've code something just nearing completion and it includes EXPLAIN and some static analysis tools.
Another option (which would probably be best served by plugins) would be to have a LLM take a look at it. I've pointed AIs at Koha SQL queries in the past to see how well they would do, and it seemed like they did pretty good.
This is actually where I started before I veered off into explain and static analysis. I've found https://github.com/mlc-ai/web-llm which is intriguing. I have no idea if it's practical yet, but if it is, that will be a followup bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42643 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42643 [Bug 42643] [OMNIBUS] Assorted performance and stability work -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Angela Berrett <angela.berrett@familysearch.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |angela.berrett@familysearch | |.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43016 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43016 [Bug 43016] [OMNIBUS] Server resource protection -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|42643 | CC| |andrew@bywatersolutions.com Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42643 [Bug 42643] [OMNIBUS] Assorted performance and stability work -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- FYI, I've got something I need to post up, but I've been re-mashing it this way and that to make it as grokkable as possible. I'll get it up asap! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #4 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203792 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203792&action=edit Bug 42585: Add analyzer infrastructure Librarians write saved SQL reports that pull all of items, sort the whole borrowers table, or buffer millions of rows into the Plack worker. There's no warning. They click Run, the database or the app server falls over, and the report never finishes. This patch adds the runner, the analysis context, the check base class and the registry every check will hang off, along with the placeholder, EXPLAIN and server statistics helpers. No checks are registered yet, they come in the following patches. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/ t/db_dependent/Koha/Reports/Analyzer/ 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "SELECT 1" })' 4) Note the empty findings list, the runner works but no checks are registered yet! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #5 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203793 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203793&action=edit Bug 42585: Add safety analyzers Koha won't run a saved report that fails its is_sql_valid whitelist, but the librarian doesn't find out until they click Run and the report errors out. This patch adds three checks that surface the same problems while the SQL is being written: forbidden_statement, missing_select and forbidden_column. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Safety/ \ t/db_dependent/Koha/Reports/Analyzer.t 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "UPDATE borrowers SET surname=1" })' 4) Note the forbidden_statement finding with severity high! 5) Repeat with "SELECT password FROM borrowers", note forbidden_column! 6) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #6 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203794 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203794&action=edit Bug 42585: Add REST API for report analysis This patch adds POST /api/v1/reports/analyze, which runs the analyzer over the given SQL and returns the severity and findings as JSON. It takes the SQL and any placeholder parameters, and is guarded by the reports.create_reports permission. Test Plan: 1) Apply this patch 2) yarn api:bundle 3) Restart all the things! 4) prove t/db_dependent/api/v1/reports_analyze.t 5) Run: curl -s -X POST \ "http://koha:koha@kohadev-intra.localhost/api/v1/reports/analyze" \ -H "Content-Type: application/json" \ -d '{"sql":"UPDATE borrowers SET surname=1"}' | jq . 6) Note the forbidden_statement finding with severity high! 7) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list and a null severity! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #7 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203795 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203795&action=edit Bug 42585: Add Analyze button to the reports editor This patch adds an Analyze button to the SQL editor on guided_reports.pl. Clicking it opens a modal that prompts for any placeholder values, calls the REST endpoint and lists the findings, so a librarian can check a report before saving or running it. Placeholder dropdowns are filled from the matching lookup table for the common placeholder types (branches, categorycode, itemtypes) and from the authorised values for the rest, so the librarian picks a real value instead of typing a code. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Create a new report with the SQL "SELECT barcode FROM items" 4) Note the new Analyze button beside "Save report"! 5) Click it, then click "Run analysis", note the empty findings list! 6) Change the SQL to "UPDATE borrowers SET surname='X' WHERE 1=1" and analyze again, note the high severity forbidden_statement finding! 7) Change the SQL to "SELECT * FROM borrowers WHERE branchcode = <<Branch|branches>>" and analyze again, note the modal prompts for a branch with a dropdown of your real branches! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #8 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203796 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203796&action=edit Bug 42585: Add static correctness analyzers This patch adds five checks for SQL that's wrong no matter how much data is in the database: * cartesian_product, comma separated FROM with no WHERE * join_without_on, a JOIN missing its ON or USING * sleep_call, SLEEP() in a saved report * select_for_update, FOR UPDATE or LOCK IN SHARE MODE * having_without_group_by, HAVING with no GROUP BY None of them are scale dependent, so the runner never suppresses them on a small database. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report for each of these: * SELECT * FROM borrowers, branches * SELECT * FROM borrowers JOIN branches * SELECT SLEEP(5) * SELECT * FROM borrowers FOR UPDATE * SELECT COUNT(*) FROM borrowers HAVING COUNT(*) > 5 4) Note each one gets the matching finding! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #9 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203797 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203797&action=edit Bug 42585: Add static performance analyzers This patch adds ten checks for SQL that's only a problem once there's real data behind it: order_by_rand, many_joins_no_where, order_by_no_limit_no_where, group_by_no_where, not_in_subquery, group_concat_no_where, leading_wildcard_like, correlated_subquery, union_not_all and function_on_column_in_where. All ten are scale dependent, so the runner drops them when every table the EXPLAIN plan touches is small. Dev instances stay quiet, production instances get the warning. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report with the SQL "SELECT * FROM borrowers ORDER BY RAND() LIMIT 1" 4) On a dev database with few borrowers, note the finding is suppressed! 5) Load enough borrowers that the table isn't small any more and analyze again, note the order_by_rand finding now appears! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #10 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203798 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203798&action=edit Bug 42585: Add EXPLAIN-based analyzers This patch adds five checks that read the EXPLAIN plan instead of the SQL text: * large_temp_table_risk, a temp table bigger than the server's in memory cap, so it spills to disk * uses_join_buffer, the planner picked a Block Nested Loop because there's no index on the join column * dependent_subquery, the planner expects to run the subquery again for every outer row * full_table_scan, an access type of ALL on a table that isn't small * low_filter_efficiency, a full scan that keeps less than 10% of the rows it reads Scans of small lookup tables like branches and authorised_values are suppressed. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Explain/ 3) Analyze a report with the SQL "SELECT b.*, ( SELECT COUNT(*) FROM issues i WHERE i.borrowernumber = b.borrowernumber ) FROM borrowers b" 4) Note the dependent_subquery finding! 5) Analyze a report with the SQL "SELECT borrowernumber FROM borrowers WHERE borrowernumber = 1" 6) Note there are no EXPLAIN findings at all! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #11 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 203799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203799&action=edit Bug 42585: Add schema-aware and meta analyzers This patch adds the last two checks: * select_star_on_large_payload_table, a wildcard SELECT whose plan touches one of Koha's big payload tables such as biblio_metadata or action_logs, dragging every row's full payload through the database, Plack and the download * explain_failed, emitted when EXPLAIN itself errored, with the trimmed database error in the message Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/SchemaAware/ \ t/Koha/Reports/Analyzer/Check/Meta/ 3) On an instance with a real sized catalog, analyze a report with the SQL "SELECT * FROM biblio_metadata" 4) Note the select_star_on_large_payload_table finding! 5) Analyze a report with the SQL "SELECT no_such_column FROM borrowers" 6) Note the explain_failed finding with the database error in it! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #12 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- (In reply to Kyle M Hall (khall) from comment #10)
3) Analyze a report with the SQL "SELECT b.*, ( SELECT COUNT(*) FROM issues i WHERE i.borrowernumber = b.borrowernumber ) FROM borrowers b" 4) Note the dependent_subquery finding!
We did not see any findings on this query. Failing QA for this. On your tests that depend on size of collection to determine whether or not to give an error, we still see the error when limiting the number of results with LIMIT or WHERE. It would be good for the analyzer to reflect all limitations on what can be saved in a query -- one cannot save a report that returns a password field, but the analyzer doesn't warn on it (while the analyzer does warn on DELETE or UPDATE) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43352 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43352 [Bug 43352] Optionally analyze all reports at creation and update and block saving of reports with warnings -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |zoho.roboto@bywatersolution | |s.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203792|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203793|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203794|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203795|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203796|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203797|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203798|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203799|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #13 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207236 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207236&action=edit Bug 42585: Add analyzer infrastructure Librarians write saved SQL reports that pull all of items, sort the whole borrowers table, or buffer millions of rows into the Plack worker. There's no warning. They click Run, the database or the app server falls over, and the report never finishes. This patch adds the runner, the analysis context, the check base class and the registry every check will hang off, along with the placeholder, EXPLAIN and server statistics helpers. No checks are registered yet, they come in the following patches. A check can mark its findings scale dependent, meaning they only matter once there's enough data behind them. The runner asks EXPLAIN how many rows the query expects to examine and sets those findings aside when the answer is small. That estimate already accounts for the WHERE clause, and for a trailing LIMIT when the plan can stream rows straight out, so "SELECT * FROM items LIMIT 10" doesn't get warned about while "SELECT * FROM items ORDER BY RAND() LIMIT 1" still does, because the sort reads every row before the LIMIT applies. A finding set aside this way is still returned, marked suppressed and carrying the reason, so a librarian on a small database can tell the difference between the analyzer noticing nothing and the analyzer noticing nothing that matters here. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/ t/db_dependent/Koha/Reports/ 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "SELECT 1" })' 4) Note the empty findings list, the runner works but no checks are registered yet! 5) Repeat with "SELECT * FROM borrowers LIMIT 5", note estimated_rows is 5 rather than the number of rows in the table! 6) Repeat with "SELECT * FROM borrowers ORDER BY RAND() LIMIT 5", note estimated_rows is the whole table again because the sort has to read every row before the LIMIT applies! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #14 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207237 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207237&action=edit Bug 42585: Add safety analyzers Koha won't run a saved report that fails its is_sql_valid whitelist, but the librarian doesn't find out until they click Run and the report errors out. This patch adds three checks that report the same problems while the SQL is being written: forbidden_statement, missing_select and forbidden_column. forbidden_column covers both of the ways Koha refuses a report. The first is a forbidden column named in the SQL, which is_sql_valid catches from the text. The second is one that only shows up in the result set, which Koha catches from the names of the columns it got back, after the query has already run. That's why "SELECT * FROM borrowers" saves happily, runs, and only then says "Illegal column in results". The check expands the wildcards against the schema so the librarian hears about it while they're still writing the report. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/Check/Safety/ \ t/db_dependent/Koha/Reports/Analyzer.t 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "UPDATE borrowers SET surname=1" })' 4) Note the forbidden_statement finding with severity high! 5) Repeat with "SELECT password FROM borrowers", note forbidden_column! 6) Repeat with "SELECT * FROM borrowers", note forbidden_column names borrowers.password even though the SQL never mentions it! 7) Save that same report and run it, note Koha refuses it with "Illegal column in results", which is what the finding warned about! 8) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #15 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207238 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207238&action=edit Bug 42585: Add REST API for report analysis This patch adds POST /api/v1/reports/analyze, which runs the analyzer over the given SQL and returns the severity and findings as JSON. It takes the SQL and any placeholder parameters, and is guarded by the reports.create_reports permission. Alongside the findings the response carries the EXPLAIN plan and estimated_rows, the row estimate the runner sized its warnings against. A finding the runner set aside comes back with suppressed set and the reason, so the caller can show it without counting it towards the severity. Test Plan: 1) Apply this patch 2) yarn api:bundle 3) Restart all the things! 4) prove t/db_dependent/api/v1/reports_analyze.t 5) Run: curl -s -X POST \ "http://koha:koha@kohadev-intra.localhost/api/v1/reports/analyze" \ -H "Content-Type: application/json" \ -d '{"sql":"UPDATE borrowers SET surname=1"}' | jq . 6) Note the forbidden_statement finding with severity high! 7) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list and a null severity! 8) Repeat with "SELECT borrowernumber FROM borrowers WHERE borrowernumber = 1 ORDER BY RAND()", note the order_by_rand finding comes back with "suppressed": 1, the reason why, and a null severity! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #16 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207239 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207239&action=edit Bug 42585: Add Analyze button to the reports editor This patch adds an Analyze button to the SQL editor on guided_reports.pl. Clicking it opens a modal that prompts for any placeholder values, calls the REST endpoint and lists the findings, so a librarian can check a report before saving or running it. Findings the analyzer set aside because this database is too small for them to bite are listed under a "Not a problem on this database" toggle, with the reason, rather than being dropped. The modal never comes back empty when there was something to say. Placeholder dropdowns are filled from the matching lookup table for the common placeholder types (branches, categorycode, itemtypes) and from the authorised values for the rest, so the librarian picks a real value instead of typing a code. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Create a new report with the SQL "SELECT barcode FROM items" 4) Note the new Analyze button beside "Save report"! 5) Click it, then click "Run analysis", note the empty findings list! 6) Change the SQL to "UPDATE borrowers SET surname='X' WHERE 1=1" and analyze again, note the high severity forbidden_statement finding! 7) Change the SQL to "SELECT surname FROM borrowers ORDER BY RAND()" and analyze again, note the "Not a problem on this database" toggle, and that opening it shows the order_by_rand finding with the reason it was set aside! 8) Change the SQL to "SELECT surname FROM borrowers WHERE branchcode = <<Branch|branches>>" and analyze again, note the modal prompts for a branch with a dropdown of your real branches! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #17 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207240 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207240&action=edit Bug 42585: Add static correctness analyzers This patch adds five checks for SQL that's wrong no matter how much data is in the database: * cartesian_product, comma separated FROM with no WHERE * join_without_on, a JOIN missing its ON or USING * sleep_call, SLEEP() in a saved report * select_for_update, FOR UPDATE or LOCK IN SHARE MODE * having_without_group_by, HAVING with no GROUP BY None of them are scale dependent, so the runner never suppresses them on a small database. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report for each of these: * SELECT * FROM borrowers, branches * SELECT * FROM borrowers JOIN branches * SELECT SLEEP(5) * SELECT * FROM borrowers FOR UPDATE * SELECT COUNT(*) FROM borrowers HAVING COUNT(*) > 5 4) Note each one gets the matching finding! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #18 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207241 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207241&action=edit Bug 42585: Add static performance analyzers This patch adds ten checks for SQL that's only a problem once there's real data behind it: order_by_rand, many_joins_no_where, order_by_no_limit_no_where, group_by_no_where, not_in_subquery, group_concat_no_where, leading_wildcard_like, correlated_subquery, union_not_all and function_on_column_in_where. All ten are scale dependent, so the runner sets them aside when the plan only expects to examine a few rows. Dev instances stay quiet, production instances get the warning. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report with the SQL "SELECT surname FROM borrowers ORDER BY RAND() LIMIT 1" 4) On a dev database with few borrowers, note the order_by_rand finding is listed under "Not a problem on this database", with the row estimate that decided it! 5) Load enough borrowers that the table isn't small any more and analyze again, note the order_by_rand finding is now a real one! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #19 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207242 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207242&action=edit Bug 42585: Add EXPLAIN-based analyzers This patch adds five checks that read the EXPLAIN plan instead of the SQL text: * large_temp_table_risk, a temp table bigger than the server's in memory cap, so it spills to disk * uses_join_buffer, the planner picked a Block Nested Loop because there's no index on the join column * dependent_subquery, the planner expects to run the subquery again for every outer row * full_table_scan, an access type of ALL on a table that isn't small * low_filter_efficiency, a full scan that keeps less than 10% of the rows it reads Scans of small lookup tables like branches and authorised_values are skipped outright. The scan checks are scale dependent on top of that, so a query the plan can bound -- an indexed WHERE, or a LIMIT the plan can stream straight to -- doesn't get warned about either. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/Check/Explain/ 3) Analyze a report with the SQL "SELECT b.borrowernumber, ( SELECT COUNT(*) FROM issues i WHERE i.borrowernumber = b.borrowernumber ) FROM borrowers b" 4) Note the dependent_subquery finding, listed under "Not a problem on this database" with the row estimate that decided it! 5) Analyze a report with the SQL "SELECT * FROM items", note the full_table_scan finding once items is big enough to matter! 6) Add a LIMIT 10 to it and analyze again, note the finding moves to "Not a problem on this database" because the plan only reads the 10 rows asked for! 7) Analyze a report with the SQL "SELECT borrowernumber FROM borrowers WHERE borrowernumber = 1" 8) Note there are no EXPLAIN findings at all! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #20 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207243 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207243&action=edit Bug 42585: Add schema-aware and meta analyzers This patch adds the last two checks: * select_star_on_large_payload_table, a wildcard SELECT whose plan touches one of Koha's big payload tables such as biblio_metadata or action_logs, dragging every row's full payload through the database, Plack and the download * explain_failed, emitted when EXPLAIN itself errored, with the trimmed database error in the message Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/SchemaAware/ \ t/Koha/Reports/Analyzer/Check/Meta/ 3) Analyze a report with the SQL "SELECT * FROM biblio_metadata" 4) On a stock dev database, note the select_star_on_large_payload_table finding is listed under "Not a problem on this database", with the row estimate that decided it! 5) Load enough records that biblio_metadata holds more than 1000, then analyze again, note the finding is now a real one! 6) Analyze a report with the SQL "SELECT no_such_column FROM borrowers" 7) Note the explain_failed finding with the database error in it! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #21 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to Andrew Fuerste-Henry from comment #12) Thanks Andrew! All three should be fixed! The no findings on your query was two bugs. correlated_subquery pulled the subquery body out with a regex that stopped at the first closing paren, s the COUNT(*) hid it. It counts bracket depth now. And scale dependent findings were *dropped* on a small database instead of shown. They come back suppressed now, with the row estimate, under a "Not a problem on this database" toggle, and don't count toward the severity. Regular expressions be hard! The size limit was looking at how big the tables are, not how much of them the query reads. It uses the EXPLAIN row estimate now, capped by a trailing LIMIT when the plan can stream rows straight out. So "SELECT * FROM items LIMIT 10" is quiet, but "ORDER BY RAND() LIMIT 1" still warns, because the sort reads every row before the LIMIT applies. On the password column, is_sql_valid catches one named in the SQL text and I had that covered. A wildcard ( SELECT * FROM borrowers ) isn't caught until execute_query looks at the result column names. forbidden_column expands wildcards against the schema now, so your query reports borrowers.password, borrowers.secret and borrowers.overdrive_auth_token. Please give it another shot and let me know how it goes! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Amit Gupta <amitddng135@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |amitddng135@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207236|0 |1 is obsolete| | Attachment #207237|0 |1 is obsolete| | Attachment #207238|0 |1 is obsolete| | Attachment #207239|0 |1 is obsolete| | Attachment #207240|0 |1 is obsolete| | Attachment #207241|0 |1 is obsolete| | Attachment #207242|0 |1 is obsolete| | Attachment #207243|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #22 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207478 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207478&action=edit Bug 42585: Add analyzer infrastructure Librarians write saved SQL reports that pull all of items, sort the whole borrowers table, or buffer millions of rows into the Plack worker. There's no warning. They click Run, the database or the app server falls over, and the report never finishes. This patch adds the runner, the analysis context, the check base class and the registry every check will hang off, along with the placeholder, EXPLAIN and server statistics helpers. No checks are registered yet, they come in the following patches. A check can mark its findings scale dependent, meaning they only matter once there's enough data behind them. The runner asks EXPLAIN how many rows the query expects to examine and sets those findings aside when the answer is small. That estimate already accounts for the WHERE clause, and for a trailing LIMIT when the plan can stream rows straight out, so "SELECT * FROM items LIMIT 10" doesn't get warned about while "SELECT * FROM items ORDER BY RAND() LIMIT 1" still does, because the sort reads every row before the LIMIT applies. A finding set aside this way is still returned, marked suppressed and carrying the reason, so a librarian on a small database can tell the difference between the analyzer noticing nothing and the analyzer noticing nothing that matters here. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/ t/db_dependent/Koha/Reports/ 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "SELECT 1" })' 4) Note the empty findings list, the runner works but no checks are registered yet! 5) Repeat with "SELECT * FROM borrowers LIMIT 5", note estimated_rows is 5 rather than the number of rows in the table! 6) Repeat with "SELECT * FROM borrowers ORDER BY RAND() LIMIT 5", note estimated_rows is the whole table again because the sort has to read every row before the LIMIT applies! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #23 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207479 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207479&action=edit Bug 42585: Add safety analyzers Koha won't run a saved report that fails its is_sql_valid whitelist, but the librarian doesn't find out until they click Run and the report errors out. This patch adds three checks that report the same problems while the SQL is being written: forbidden_statement, missing_select and forbidden_column. forbidden_column covers both of the ways Koha refuses a report. The first is a forbidden column named in the SQL, which is_sql_valid catches from the text. The second is one that only shows up in the result set, which Koha catches from the names of the columns it got back, after the query has already run. That's why "SELECT * FROM borrowers" saves happily, runs, and only then says "Illegal column in results". The check expands the wildcards against the schema so the librarian hears about it while they're still writing the report. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/Check/Safety/ \ t/db_dependent/Koha/Reports/Analyzer.t 3) In a koha-shell, run: perl -MKoha::Reports::Analyzer=analyze -MData::Dumper -e \ 'print Dumper analyze({ sql => "UPDATE borrowers SET surname=1" })' 4) Note the forbidden_statement finding with severity high! 5) Repeat with "SELECT password FROM borrowers", note forbidden_column! 6) Repeat with "SELECT * FROM borrowers", note forbidden_column names borrowers.password even though the SQL never mentions it! 7) Save that same report and run it, note Koha refuses it with "Illegal column in results", which is what the finding warned about! 8) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #24 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207480 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207480&action=edit Bug 42585: Add REST API for report analysis This patch adds POST /api/v1/reports/analyze, which runs the analyzer over the given SQL and returns the severity and findings as JSON. It takes the SQL and any placeholder parameters, and is guarded by the reports.create_reports permission. Alongside the findings the response carries the EXPLAIN plan and estimated_rows, the row estimate the runner sized its warnings against. A finding the runner set aside comes back with suppressed set and the reason, so the caller can show it without counting it towards the severity. Test Plan: 1) Apply this patch 2) yarn api:bundle 3) Restart all the things! 4) prove t/db_dependent/api/v1/reports_analyze.t 5) Run: curl -s -X POST \ "http://koha:koha@kohadev-intra.localhost/api/v1/reports/analyze" \ -H "Content-Type: application/json" \ -d '{"sql":"UPDATE borrowers SET surname=1"}' | jq . 6) Note the forbidden_statement finding with severity high! 7) Repeat with "SELECT borrowernumber FROM borrowers LIMIT 1", note the empty findings list and a null severity! 8) Repeat with "SELECT borrowernumber FROM borrowers WHERE borrowernumber = 1 ORDER BY RAND()", note the order_by_rand finding comes back with "suppressed": 1, the reason why, and a null severity! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #25 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207481 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207481&action=edit Bug 42585: Add Analyze button to the reports editor This patch adds an Analyze button to the SQL editor on guided_reports.pl. Clicking it opens a modal that prompts for any placeholder values, calls the REST endpoint and lists the findings, so a librarian can check a report before saving or running it. Findings the analyzer set aside because this database is too small for them to bite are listed under a "Not a problem on this database" toggle, with the reason, rather than being dropped. The modal never comes back empty when there was something to say. Placeholder dropdowns are filled from the matching lookup table for the common placeholder types (branches, categorycode, itemtypes) and from the authorised values for the rest, so the librarian picks a real value instead of typing a code. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Create a new report with the SQL "SELECT barcode FROM items" 4) Note the new Analyze button beside "Save report"! 5) Click it, then click "Run analysis", note the empty findings list! 6) Change the SQL to "UPDATE borrowers SET surname='X' WHERE 1=1" and analyze again, note the high severity forbidden_statement finding! 7) Change the SQL to "SELECT surname FROM borrowers ORDER BY RAND()" and analyze again, note the "Not a problem on this database" toggle, and that opening it shows the order_by_rand finding with the reason it was set aside! 8) Change the SQL to "SELECT surname FROM borrowers WHERE branchcode = <<Branch|branches>>" and analyze again, note the modal prompts for a branch with a dropdown of your real branches! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #26 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207482 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207482&action=edit Bug 42585: Add static correctness analyzers This patch adds five checks for SQL that's wrong no matter how much data is in the database: * cartesian_product, comma separated FROM with no WHERE * join_without_on, a JOIN missing its ON or USING * sleep_call, SLEEP() in a saved report * select_for_update, FOR UPDATE or LOCK IN SHARE MODE * having_without_group_by, HAVING with no GROUP BY None of them are scale dependent, so the runner never suppresses them on a small database. Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report for each of these: * SELECT * FROM borrowers, branches * SELECT * FROM borrowers JOIN branches * SELECT SLEEP(5) * SELECT * FROM borrowers FOR UPDATE * SELECT COUNT(*) FROM borrowers HAVING COUNT(*) > 5 4) Note each one gets the matching finding! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #27 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207483 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207483&action=edit Bug 42585: Add static performance analyzers This patch adds ten checks for SQL that's only a problem once there's real data behind it: order_by_rand, many_joins_no_where, order_by_no_limit_no_where, group_by_no_where, not_in_subquery, group_concat_no_where, leading_wildcard_like, correlated_subquery, union_not_all and function_on_column_in_where. All ten are scale dependent, so the runner sets them aside when the plan only expects to examine a few rows. Dev instances stay quiet, production instances get the warning. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/Check/Static/ 3) Analyze a report with the SQL "SELECT surname FROM borrowers ORDER BY RAND() LIMIT 1" 4) On a dev database with few borrowers, note the order_by_rand finding is listed under "Not a problem on this database", with the row estimate that decided it! 5) Load enough borrowers that the table isn't small any more and analyze again, note the order_by_rand finding is now a real one! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #28 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207484 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207484&action=edit Bug 42585: Add EXPLAIN-based analyzers This patch adds five checks that read the EXPLAIN plan instead of the SQL text: * large_temp_table_risk, a temp table bigger than the server's in memory cap, so it spills to disk * uses_join_buffer, the planner picked a Block Nested Loop because there's no index on the join column * dependent_subquery, the planner expects to run the subquery again for every outer row * full_table_scan, an access type of ALL on a table that isn't small * low_filter_efficiency, a full scan that keeps less than 10% of the rows it reads Scans of small lookup tables like branches and authorised_values are skipped outright. The scan checks are scale dependent on top of that, so a query the plan can bound -- an indexed WHERE, or a LIMIT the plan can stream straight to -- doesn't get warned about either. Test Plan: 1) Apply this patch 2) prove -r t/Koha/Reports/Analyzer/Check/Explain/ 3) Analyze a report with the SQL "SELECT b.borrowernumber, ( SELECT COUNT(*) FROM issues i WHERE i.borrowernumber = b.borrowernumber ) FROM borrowers b" 4) Note the dependent_subquery finding, listed under "Not a problem on this database" with the row estimate that decided it! 5) Analyze a report with the SQL "SELECT * FROM items", note the full_table_scan finding once items is big enough to matter! 6) Add a LIMIT 10 to it and analyze again, note the finding moves to "Not a problem on this database" because the plan only reads the 10 rows asked for! 7) Analyze a report with the SQL "SELECT borrowernumber FROM borrowers WHERE borrowernumber = 1" 8) Note there are no EXPLAIN findings at all! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 --- Comment #29 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207485 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207485&action=edit Bug 42585: Add schema-aware and meta analyzers This patch adds the last two checks: * select_star_on_large_payload_table, a wildcard SELECT whose plan touches one of Koha's big payload tables such as biblio_metadata or action_logs, dragging every row's full payload through the database, Plack and the download * explain_failed, emitted when EXPLAIN itself errored, with the trimmed database error in the message Test Plan: 1) Apply this patch 2) prove t/Koha/Reports/Analyzer/Check/SchemaAware/ \ t/Koha/Reports/Analyzer/Check/Meta/ 3) Analyze a report with the SQL "SELECT * FROM biblio_metadata" 4) On a stock dev database, note the select_star_on_large_payload_table finding is listed under "Not a problem on this database", with the row estimate that decided it! 5) Load enough records that biblio_metadata holds more than 1000, then analyze again, note the finding is now a real one! 6) Analyze a report with the SQL "SELECT no_such_column FROM borrowers" 7) Note the explain_failed finding with the database error in it! Signed-off-by: Marisha Sullivan <msullivan@holdenfg.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Brendan Lawlor <blawlor@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |blawlor@clamsnet.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42585 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |jacob.omara@openfifth.co.uk |y.org | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org