Bug 24695

(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?

(In reply to Bernardo Gonzalez Kriegel from comment #2)
> (In reply to paxed from comment #1)
> > Created attachment 99305 [details] [review] [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.

Great to see some cleanup going on around this area :)

Created attachment 99625 [details] [review]
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.

Why in Koha::Reports and not Koha::Report?

I think this code would make sense here:
  $report->store if $report->is_sql_valid;

It could even prevent the store in an overridden ->store method. And throw an exception if not valid.

(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.

Created attachment 99694 [details] [review]
Bug 24695: Move to Koha::Report->is_sql_valid

(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?

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 !

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 :)

Created attachment 117352 [details] [review]
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>

Created attachment 117353 [details] [review]
Bug 24695: Move to Koha::Report->is_sql_valid

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 119421 [details] [review]
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>

Created attachment 119422 [details] [review]
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>

Created attachment 119423 [details] [review]
Bug 24695: (QA follow-up) Fix number of tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 119424 [details] [review]
Bug 24695: (QA follow-up) Fix POD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Pushed to master for 21.05, thanks to everybody involved!

Enhancement not pushed to 20.11.x