[Bug 24695] New: Improve SQL report validation
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Bug ID: 24695 Summary: Improve SQL report validation Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Reports Assignee: koha-bugs@lists.koha-community.org Reporter: pasi.kallinen@koha-suomi.fi QA Contact: testopia@bugs.koha-community.org The saved SQL report code validates the SQL in multiple places: when saving, when updating, and when executing the query. Move the validation code into Koha::Reports, and write tests for it. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 paxed <pasi.kallinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |pasi.kallinen@koha-suomi.fi |ity.org | Status|NEW |Needs Signoff --- Comment #1 from paxed <pasi.kallinen@koha-suomi.fi> --- Created attachment 99305 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99305&action=edit Bug 24695: Improve SQL report validation The saved SQL report code validates the SQL in multiple places: when saving, when updating, and when executing the query. Move the validation code into Koha::Reports, and write tests for it. Test plan: 1) Apply patch 2) Create a new valid SQL report, save it (success) 3) Create a new illegal SQL report, try to save (fails) 4) Update already saved SQL report by adding one of the forbidden words, eg. delete or drop (saving will fail) 5) Edit a save_sql in the database, changing it to eg. "drop borrowers", and try to execute it (fails) 6) Prove t/db_dependent/Koha/Reports.t -- 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=24695 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bgkriegel@gmail.com --- Comment #2 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- (In reply to paxed from comment #1)
Created attachment 99305 [details] [review] Bug 24695: Improve SQL report validation
Works well, no errors, test pass, but...
5) Edit a save_sql in the database, changing it to eg. "drop borrowers", and try to execute it (fails)
ended with an empty borrowers table :( (I wrote 'delete from borrowers') Works this for you? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #3 from paxed <pasi.kallinen@koha-suomi.fi> --- (In reply to Bernardo Gonzalez Kriegel from comment #2)
(In reply to paxed from comment #1)
Created attachment 99305 [details] [review] [review] Bug 24695: Improve SQL report validation
Works well, no errors, test pass, but...
5) Edit a save_sql in the database, changing it to eg. "drop borrowers", and try to execute it (fails)
ended with an empty borrowers table :( (I wrote 'delete from borrowers')
Works this for you?
update saved_sql set savedsql='DROP borrowers'; then try to run the report, it'll cause an error. Ideally, this should be changed to produce an actual koha page with an explanation instead, but that is outside the scope of this bug, and behaves the same as before this patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Great to see some cleanup going on around this area :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Bernardo Gonzalez Kriegel <bgkriegel@gmail.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=24695 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99305|0 |1 is obsolete| | --- Comment #5 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Created attachment 99625 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99625&action=edit Bug 24695: Improve SQL report validation The saved SQL report code validates the SQL in multiple places: when saving, when updating, and when executing the query. Move the validation code into Koha::Reports, and write tests for it. Test plan: 1) Apply patch 2) Create a new valid SQL report, save it (success) 3) Create a new illegal SQL report, try to save (fails) 4) Update already saved SQL report by adding one of the forbidden words, eg. delete or drop (saving will fail) 5) Edit a save_sql in the database, changing it to eg. "drop borrowers", and try to execute it (fails) 6) Prove t/db_dependent/Koha/Reports.t Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no qa errors. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Why in Koha::Reports and not Koha::Report? I think this code would make sense here: $report->store if $report->is_sql_valid; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- It could even prevent the store in an overridden ->store method. And throw an exception if not valid. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 paxed <pasi.kallinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|pasi.kallinen@koha-suomi.fi |koha-bugs@lists.koha-commun | |ity.org Status|Signed Off |In Discussion --- Comment #8 from paxed <pasi.kallinen@koha-suomi.fi> --- (In reply to Jonathan Druart from comment #7)
It could even prevent the store in an overridden ->store method. And throw an exception if not valid.
I don't know how to do it that way off the top of my head, so I'd have to spend several days on this, and I don't have that kind of time to spend. -- 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=24695 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 99694 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99694&action=edit Bug 24695: Move to Koha::Report->is_sql_valid -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to paxed from comment #8)
(In reply to Jonathan Druart from comment #7)
It could even prevent the store in an overridden ->store method. And throw an exception if not valid.
I don't know how to do it that way off the top of my head, so I'd have to spend several days on this, and I don't have that kind of time to spend.
Dropping it for now as we do not use Koha::Report->store yet to insert them. What about this patch? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |pasi.kallinen@koha-suomi.fi |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |21215 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21215 [Bug 21215] Saved SQL reports code improvements [Omnibus] -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Séverine Queune <severine.queune@bulac.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |severine.queune@bulac.fr --- Comment #11 from Séverine Queune <severine.queune@bulac.fr> --- On step 3, the SQL content is emptied after the warning alert. Is it the expected behaviour ? I use a lot this tool, and I think it will be quite frustating after a mistake to lose all the SQL I just (sometimes painfully...) write. All the other steps are ok for me ! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Hi Séverine, If I click the "Return to previous page" I get the form prefilled with what I entered. So what you described seems to have been fixed now :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch 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=24695 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99625|0 |1 is obsolete| | Attachment #99694|0 |1 is obsolete| | --- Comment #13 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 117352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117352&action=edit Bug 24695: Improve SQL report validation The saved SQL report code validates the SQL in multiple places: when saving, when updating, and when executing the query. Move the validation code into Koha::Reports, and write tests for it. Test plan: 1) Apply patch 2) Create a new valid SQL report, save it (success) 3) Create a new illegal SQL report, try to save (fails) 4) Update already saved SQL report by adding one of the forbidden words, eg. delete or drop (saving will fail) 5) Edit a save_sql in the database, changing it to eg. "drop borrowers", and try to execute it (fails) 6) Prove t/db_dependent/Koha/Reports.t Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no qa errors. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #14 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 117353 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117353&action=edit Bug 24695: Move to Koha::Report->is_sql_valid Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117352|0 |1 is obsolete| | --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119421 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119421&action=edit Bug 24695: Improve SQL report validation The saved SQL report code validates the SQL in multiple places: when saving, when updating, and when executing the query. Move the validation code into Koha::Reports, and write tests for it. Test plan: 1) Apply patch 2) Create a new valid SQL report, save it (success) 3) Create a new illegal SQL report, try to save (fails) 4) Update already saved SQL report by adding one of the forbidden words, eg. delete or drop (saving will fail) 5) Edit a save_sql in the database, changing it to eg. "drop borrowers", and try to execute it (fails) 6) Prove t/db_dependent/Koha/Reports.t Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no qa errors. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117353|0 |1 is obsolete| | --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119422&action=edit Bug 24695: Move to Koha::Report->is_sql_valid Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119423 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119423&action=edit Bug 24695: (QA follow-up) Fix number of tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #18 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119424 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119424&action=edit Bug 24695: (QA follow-up) Fix POD Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24695 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #20 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 20.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org