Bug 20592 - updateitem.pl causes database errors when empty non-public item notes updated
Summary: updateitem.pl causes database errors when empty non-public item notes updated
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Target Milestone: ---
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-17 02:46 UTC by David Cook
Modified: 2018-11-28 09:11 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20592 - updateitem.pl causes database errors when empty non-public item notes updated (1.47 KB, patch)
2018-04-17 02:52 UTC, David Cook
Details | Diff | Splinter Review
Bug 20592: Checks for itemnumber sooner (1.21 KB, patch)
2018-04-18 00:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20592: Scope creep - fix other noise (1.27 KB, patch)
2018-04-18 00:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20592: Add tests (1.01 KB, patch)
2018-04-24 18:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20592: Checks for itemnumber sooner (1.22 KB, patch)
2018-04-24 18:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20592: Scope creep - fix other noise (1.28 KB, patch)
2018-04-24 18:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20592: Add tests (1.16 KB, patch)
2018-09-06 00:53 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20592: Checks for itemnumber sooner (1.22 KB, patch)
2018-09-06 00:53 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20592: Checks for itemnumber sooner (1.34 KB, patch)
2018-10-17 14:56 UTC, Cori Lynn Arnold
Details | Diff | Splinter Review
Bug 20592: Add tests (1.24 KB, patch)
2018-10-17 15:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20592: Return early in ModItem if nothing to update (1.43 KB, patch)
2018-10-17 15:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2018-04-17 02:46:14 UTC
If you go to /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1#item1, and click "Update" next to an empty 
"Non-public note" field, it will generate a database error.

Typically, this is hidden in your Apache logs, but if you have DEBUG turned on, the database error will be fatal. 

Example logs:

[Tue Apr 17 12:43:48.607368 2018] [cgi:error] [pid 1992] [client ...:22158] AH01215: [Tue Apr 17 12:43:48 2018] updateitem.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'WHERE itemnumber='1'' at line 1 [for Statement "UPDATE items SET  WHERE itemnumber=?" with ParamValues: 0="1"] at ../git/C4/Items.pm line 2038.: ../git/catalogue/updateitem.pl, referer: http://koha/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1
[Tue Apr 17 12:43:48.607852 2018] [cgi:error] [pid 1992] [client ...:22158] AH01215: [Tue Apr 17 12:43:48 2018] updateitem.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'WHERE itemnumber='1'' at line 1 [for Statement "UPDATE items SET  WHERE itemnumber=?" with ParamValues: 0="1"] at ../git/C4/Items.pm line 2038.: ../git/catalogue/updateitem.pl, referer: http://koha/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1
Comment 1 David Cook 2018-04-17 02:52:29 UTC
Created attachment 74286 [details] [review]
Bug 20592 - updateitem.pl causes database errors when empty non-public item notes updated

Previously, clicking "Update" next to non-public item note when the text
box was empty would generate a database error.

This patches adds a condition where ModItem() is only called
if there are actual changes to be made to the record.

_TEST PLAN_
1) Create a bibliographic record
2) Create an item for that bib record
3) Go to /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1
4) Click "Update" next to "Non-public note"
5) Note a "DBD::mysql::st execute failed" error in your logs

6) Apply patch

7) Click "Update" next to "Non-public note"
8) Note that there is no longer an error
9) Add text to the box and click "Update" to ensure update still works
Comment 2 M. Tompsett 2018-04-17 14:35:55 UTC
I believe this is the wrong way to do it. The problem is ModItem requires an itemnumber, and the itemnumber check is too late in the function.
Comment 3 M. Tompsett 2018-04-18 00:25:50 UTC
Created attachment 74399 [details] [review]
Bug 20592: Checks for itemnumber sooner

Two lines of code and move them earlier.
Add a check to make sure there are changes.
Comment 4 M. Tompsett 2018-04-18 00:25:53 UTC
Created attachment 74400 [details] [review]
Bug 20592: Scope creep - fix other noise
Comment 5 M. Tompsett 2018-04-18 00:35:04 UTC
Test plan is the same as comment #1.
Testing on a kohadevbox after a reset all.
blanking the log before clicking update:
 echo | sudo tee /var/log/koha/kohadev/plack-error.log
Used the same url given:
 http://localhost:8081/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=1&itemnumber=1

Before my two patches, noise and update triggers more.
After my two patches, no noise.

David's patch works, but if there are other places which need the same check they would have to be modified too. The ModItem function should be validating its parameters. Also, I don't believe you need to check for biblionumber in your condition, David.

My first patch adds that validation, so other functions receive the same fix. My second just cleans up some noise, since the first warning wasn't the only noise triggered.
Comment 6 David Cook 2018-04-18 02:22:46 UTC
(In reply to M. Tompsett from comment #2)
> I believe this is the wrong way to do it. The problem is ModItem requires an
> itemnumber, and the itemnumber check is too late in the function.

No, that's not the problem. The problem is an an empty $item hashref being passed into C4::Items::_koha_modify_item(), which creates an invalid SQL query.

But I agree that a better solution would be to change ModItem rather than updateitem.pl. I considered that, but I figured I would make the lightest touch possible and test the one place where I knew there was a problem. Do you know whether or not it's appropriate to return early from ModItem? What impact does that have across Koha?
Comment 7 Jonathan Druart 2018-04-24 18:55:06 UTC
Created attachment 74824 [details] [review]
Bug 20592: Add tests
Comment 8 Jonathan Druart 2018-04-24 18:55:14 UTC
Created attachment 74825 [details] [review]
Bug 20592: Checks for itemnumber sooner

Two lines of code and move them earlier.
Add a check to make sure there are changes.
Comment 9 Jonathan Druart 2018-04-24 18:55:17 UTC
Created attachment 74826 [details] [review]
Bug 20592: Scope creep - fix other noise
Comment 10 Jonathan Druart 2018-04-24 18:57:24 UTC
I agree with Mark, better to fix ModItem instead of hijacking the controller :)
Comment 11 Jonathan Druart 2018-04-24 18:58:22 UTC
(In reply to David Cook from comment #6)
> But I agree that a better solution would be to change ModItem rather than
> updateitem.pl. I considered that, but I figured I would make the lightest
> touch possible and test the one place where I knew there was a problem. Do
> you know whether or not it's appropriate to return early from ModItem? What
> impact does that have across Koha?

I would not expect side-effects as ModItem does not return a useful value.
Comment 12 David Cook 2018-04-26 00:16:16 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to David Cook from comment #6)
> > But I agree that a better solution would be to change ModItem rather than
> > updateitem.pl. I considered that, but I figured I would make the lightest
> > touch possible and test the one place where I knew there was a problem. Do
> > you know whether or not it's appropriate to return early from ModItem? What
> > impact does that have across Koha?
> 
> I would not expect side-effects as ModItem does not return a useful value.

I meant more in terms of other parts of Koha catching a fatal error with an eval{} or something like that, and acting on the error. 

But I'm not fussed either way :).
Comment 13 Katrin Fischer 2018-07-08 15:09:23 UTC
Can you please rebase?
Comment 14 M. Tompsett 2018-09-06 00:43:33 UTC
Comment on attachment 74826 [details] [review]
Bug 20592: Scope creep - fix other noise

This noise was fixed elsewhere (Bug 20620)
Comment 15 M. Tompsett 2018-09-06 00:53:19 UTC
Created attachment 78453 [details] [review]
Bug 20592: Add tests

NOTE: It isn't really a test, but it does trigger
      the return line which was added to ModItem.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 M. Tompsett 2018-09-06 00:53:24 UTC
Created attachment 78454 [details] [review]
Bug 20592: Checks for itemnumber sooner

Two lines of code and move them earlier.
Add a check to make sure there are changes.
Comment 17 M. Tompsett 2018-09-06 00:54:49 UTC
Second patch needs sign off, because as far as I can tell, I wrote it. :) First patch was provided by Jonathan, and I have signed it, off by running prove in combination with both patches.
Comment 18 Cori Lynn Arnold 2018-10-17 14:56:12 UTC
Created attachment 80728 [details] [review]
Bug 20592: Checks for itemnumber sooner

Two lines of code and move them earlier.
Add a check to make sure there are changes.

Followed the test plan, both the _BEFORE_ and after do as described.

Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>
Comment 19 Cori Lynn Arnold 2018-10-17 14:56:50 UTC
signed off
Comment 20 Jonathan Druart 2018-10-17 15:45:56 UTC
Created attachment 80731 [details] [review]
Bug 20592: Add tests

NOTE: It isn't really a test, but it does trigger
      the return line which was added to ModItem.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2018-10-17 15:46:01 UTC
Created attachment 80732 [details] [review]
Bug 20592: Return early in ModItem if nothing to update

Two lines of code and move them earlier.
Add a check to make sure there are changes.

Followed the test plan, both the _BEFORE_ and after do as described.

Signed-off-by: Cori Lynn Arnold <carnold@dgiinc.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Nick Clemens 2018-10-19 17:31:41 UTC
Awesome work all!

Pushed to master for 18.11
Comment 23 Martin Renvoize 2018-11-05 15:41:55 UTC
Pushed to 18.05.x for 18.05.06
Comment 24 Fridolin SOMERS 2018-11-28 09:11:03 UTC
Pushed to 17.11.x for 17.11.13