Bug 22812

Summary: Cannot add new subscription with strict SQL modes turned on
Product: Koha Reporter: Liz Rea <wizzyrea>
Component: SerialsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: colin.campbell, jonathan.druart, m.de.rooy, martin.renvoize, mtj, nick, wizzyrea
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24010
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23064
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06
Bug Depends on:    
Bug Blocks: 17258    
Attachments: Bug 22812: Fix new subscription with strict sql modes
Bug 22812: Add tests for checkrouting
Bug 22812: Fix GROUP BY in checkrouting
Bug 22812: Fix new subscription with strict sql modes
Bug 22812: Add tests for checkrouting
Bug 22812: Fix GROUP BY in checkrouting
Bug 22812: Fix new subscription with strict sql modes
Bug 22812: Add tests for checkrouting
Bug 22812: Fix GROUP BY in checkrouting
Bug 22812: Use Koha::Subscription in NewSubscription
Bug 22812: Fix new subscription with strict sql modes
Bug 22812: Add tests for checkrouting
Bug 22812: Fix GROUP BY in checkrouting
Bug 22812: Use Koha::Subscription in NewSubscription
Bug 22812: Fix new subscription with strict sql modes
Bug 22812: Add tests for checkrouting
Bug 22812: Fix GROUP BY in checkrouting
Bug 22812: Use Koha::Subscription in NewSubscription

Description Liz Rea 2019-04-30 15:17:43 UTC
I can't seem to add a new subscription without filling in every. single. box.

As an example, I created a subscription with no vendor, a biblio, no special anything, and I got an error

Can't call method "set_additional_fields" on an undefined value at /kohadevbox/koha/serials/subscription-add.pl line 401

there are also sql errors when you don't fill in various boxes:

Incorrect integer value: '' for column 'aqbooksellerid' at row 1 if you don't give a vendor

Incorrect integer value: '' for column 'innerloop1' at row 1 if you don't use a prediction pattern that fills all boxes x y z

My version of sql is 5.5.62 and this was in a docker test setup.
Comment 1 Liz Rea 2019-04-30 16:28:55 UTC
This is probably to do with strict mode, as KohaDevBox doesn't seem to manifest this behaviour. 

Off of blocker because most people have already turned off strict mode if they are using mysql, but it's still pretty major because we can't add subscriptions in strict mode and that seems wrong.
Comment 2 Jonathan Druart 2019-05-06 18:36:33 UTC
Created attachment 89391 [details] [review]
Bug 22812: Fix new subscription with strict sql modes

With sql_strict_modes turned on, if you try to create a new subscription
you will get:
Incorrect integer value: '' for column '$INT_COLUMN' at row 1
Comment 3 Jonathan Druart 2019-05-06 18:39:39 UTC
Note that now there is the following error in the log:

DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause [for Statement "SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist


but does not explode the interface.
Comment 4 Jonathan Druart 2019-05-06 18:52:27 UTC
Created attachment 89392 [details] [review]
Bug 22812: Add tests for checkrouting
Comment 5 Jonathan Druart 2019-05-06 18:52:30 UTC
Created attachment 89393 [details] [review]
Bug 22812: Fix GROUP BY in checkrouting

On the way, let fix this error
DBD::mysql::st execute failed: Mixing of GROUP columns
(MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is
no GROUP BY clause [for Statement "SELECT count(routingid) routingids
FROM subscription LEFT JOIN subscriptionroutinglist

Test plan:
Create a routing list, search for subscriptions
In the action list you should see "Edit routing list (1)" or "New
routing list"
Comment 6 Liz Rea 2019-05-07 19:15:17 UTC
Created attachment 89454 [details] [review]
Bug 22812: Fix new subscription with strict sql modes

With sql_strict_modes turned on, if you try to create a new subscription
you will get:
Incorrect integer value: '' for column '$INT_COLUMN' at row 1

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 7 Liz Rea 2019-05-07 19:15:25 UTC
Created attachment 89455 [details] [review]
Bug 22812: Add tests for checkrouting

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 8 Liz Rea 2019-05-07 19:15:28 UTC
Created attachment 89456 [details] [review]
Bug 22812: Fix GROUP BY in checkrouting

On the way, let fix this error
DBD::mysql::st execute failed: Mixing of GROUP columns
(MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is
no GROUP BY clause [for Statement "SELECT count(routingid) routingids
FROM subscription LEFT JOIN subscriptionroutinglist

Test plan:
Create a routing list, search for subscriptions
In the action list you should see "Edit routing list (1)" or "New
routing list"

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 9 Marcel de Rooy 2019-05-10 09:45:48 UTC
+    $_ ||= undef # Set to undef for integer values, not empty string
+      for (
+        $aqbooksellerid, $lastvalue1, $innerloop1, $lastvalue2,
+        $innerloop2,     $lastvalue3, $innerloop3,
+      );

This also sets 0 back to undef.
The comment is very confusing about what it does. Fortunately, integer values like 1 are not set to undef.
Comment 10 Marcel de Rooy 2019-05-10 09:52:15 UTC
SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist ON subscription.subscriptionid = subscriptionroutinglist.subscriptionid order by ranking asc;

does not raise a warning with me on mariadb with sql mode:
STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION

Do I overlook a setting?
Comment 11 Marcel de Rooy 2019-05-10 09:53:21 UTC
Created attachment 89535 [details] [review]
Bug 22812: Fix new subscription with strict sql modes

With sql_strict_modes turned on, if you try to create a new subscription
you will get:
Incorrect integer value: '' for column '$INT_COLUMN' at row 1

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2019-05-10 09:53:27 UTC
Created attachment 89536 [details] [review]
Bug 22812: Add tests for checkrouting

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2019-05-10 09:53:31 UTC
Created attachment 89537 [details] [review]
Bug 22812: Fix GROUP BY in checkrouting

On the way, let fix this error
DBD::mysql::st execute failed: Mixing of GROUP columns
(MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is
no GROUP BY clause [for Statement "SELECT count(routingid) routingids
FROM subscription LEFT JOIN subscriptionroutinglist

Test plan:
Create a routing list, search for subscriptions
In the action list you should see "Edit routing list (1)" or "New
routing list"

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2019-05-10 09:54:26 UTC
QA Comment:
Bug apparently solved. Some minor questions, no blockers.
Comment 15 Jonathan Druart 2019-05-10 12:36:12 UTC
(In reply to Marcel de Rooy from comment #9)
> +    $_ ||= undef # Set to undef for integer values, not empty string
> +      for (
> +        $aqbooksellerid, $lastvalue1, $innerloop1, $lastvalue2,
> +        $innerloop2,     $lastvalue3, $innerloop3,
> +      );
> 
> This also sets 0 back to undef.

You are right, and I think it should be followed-up before pushed. I initially write it for aqbooksellerid, and so wanted to catch 0. I reused the same logic for the other errors (I was not expecting to have them initially).
Comment 16 Jonathan Druart 2019-05-10 15:54:24 UTC
(In reply to Marcel de Rooy from comment #10)
> SELECT count(routingid) routingids FROM subscription LEFT JOIN
> subscriptionroutinglist ON subscription.subscriptionid =
> subscriptionroutinglist.subscriptionid order by ranking asc;
> 
> does not raise a warning with me on mariadb with sql mode:
> STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,
> NO_ENGINE_SUBSTITUTION
> 
> Do I overlook a setting?

MariaDB [koha_kohadev]> SET sql_mode = 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION';
Query OK, 0 rows affected (0.01 sec)

MariaDB [koha_kohadev]> SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist ON subscription.subscriptionid = subscriptionroutinglist.subscriptionid WHERE subscription.subscriptionid = 1 ORDER BY ranking ASC;
ERROR 1140 (42000): Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause
Comment 17 Jonathan Druart 2019-05-10 16:13:12 UTC
Created attachment 89561 [details] [review]
Bug 22812: Use Koha::Subscription in NewSubscription

Starting to write tests I realize that we are going to add too much
specific logic which is already handled in Koha::Object->store.
The easiest and safe way is to use it :)
Comment 18 Jonathan Druart 2019-05-10 16:15:01 UTC
Maybe I went a bit too far with this last patch, but it's definitely the way to go. Sending back to NSO as I rewrote the original approach.
Comment 19 Michal Denar 2019-05-10 20:22:17 UTC
Created attachment 89570 [details] [review]
Bug 22812: Fix new subscription with strict sql modes

With sql_strict_modes turned on, if you try to create a new subscription
you will get:
Incorrect integer value: '' for column '$INT_COLUMN' at row 1

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 20 Michal Denar 2019-05-10 20:22:24 UTC
Created attachment 89571 [details] [review]
Bug 22812: Add tests for checkrouting

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 21 Michal Denar 2019-05-10 20:22:29 UTC
Created attachment 89572 [details] [review]
Bug 22812: Fix GROUP BY in checkrouting

On the way, let fix this error
DBD::mysql::st execute failed: Mixing of GROUP columns
(MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is
no GROUP BY clause [for Statement "SELECT count(routingid) routingids
FROM subscription LEFT JOIN subscriptionroutinglist

Test plan:
Create a routing list, search for subscriptions
In the action list you should see "Edit routing list (1)" or "New
routing list"

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 22 Michal Denar 2019-05-10 20:22:33 UTC
Created attachment 89573 [details] [review]
Bug 22812: Use Koha::Subscription in NewSubscription

Starting to write tests I realize that we are going to add too much
specific logic which is already handled in Koha::Object->store.
The easiest and safe way is to use it :)

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 23 Katrin Fischer 2019-05-11 10:13:09 UTC
Created attachment 89590 [details] [review]
Bug 22812: Fix new subscription with strict sql modes

With sql_strict_modes turned on, if you try to create a new subscription
you will get:
Incorrect integer value: '' for column '$INT_COLUMN' at row 1

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2019-05-11 10:13:15 UTC
Created attachment 89591 [details] [review]
Bug 22812: Add tests for checkrouting

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2019-05-11 10:13:19 UTC
Created attachment 89592 [details] [review]
Bug 22812: Fix GROUP BY in checkrouting

On the way, let fix this error
DBD::mysql::st execute failed: Mixing of GROUP columns
(MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is
no GROUP BY clause [for Statement "SELECT count(routingid) routingids
FROM subscription LEFT JOIN subscriptionroutinglist

Test plan:
Create a routing list, search for subscriptions
In the action list you should see "Edit routing list (1)" or "New
routing list"

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2019-05-11 10:13:24 UTC
Created attachment 89593 [details] [review]
Bug 22812: Use Koha::Subscription in NewSubscription

Starting to write tests I realize that we are going to add too much
specific logic which is already handled in Koha::Object->store.
The easiest and safe way is to use it :)

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Marcel de Rooy 2019-05-13 08:03:53 UTC
(In reply to Jonathan Druart from comment #16)
> MariaDB [koha_kohadev]> SET sql_mode =
> 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,
> ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION';
> Query OK, 0 rows affected (0.01 sec)
> 
> MariaDB [koha_kohadev]> SELECT count(routingid) routingids FROM subscription
> LEFT JOIN subscriptionroutinglist ON subscription.subscriptionid =
> subscriptionroutinglist.subscriptionid WHERE subscription.subscriptionid = 1
> ORDER BY ranking ASC;
> ERROR 1140 (42000): Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with
> no GROUP columns is illegal if there is no GROUP BY clause

OK Confirmed
Comment 28 Nick Clemens 2019-05-15 12:15:12 UTC
Awesome work all!

Pushed to master for 19.05
Comment 29 Martin Renvoize 2019-05-24 13:55:25 UTC
Pushed to 18.11.x for 18.11.06