Bug 17260

Summary: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns
Product: Koha Reporter: Blou <philippe.blouin>
Component: Installation and upgrade (command-line installer)Assignee: Blou <philippe.blouin>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: eric.begin, francois.charbonnier, jonathan.druart, katrin.fischer, kyle, mehdi.hamidi, mtj, ztajoli
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17234
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17324
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16583
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17258    
Attachments: Bug 17260 - updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns
Bug 17260: Update existing installs

Description Blou 2016-09-06 16:05:37 UTC
See also Bug 17234

The loading of file admin/searchengine/elasticsearch/mappings.yaml specifies 'type' as empty, which fails with Mysql 5.7+ which is more stringent.

Also, forcing an empty value into a boolean also fails.

Both issues arise when updatedatabase.pl hit 3.23.00.050.
Comment 1 Blou 2016-09-06 16:12:51 UTC
Created attachment 55263 [details] [review]
Bug 17260 - updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns

The loading of file admin/searchengine/elasticsearch/mappings.yaml specifies 'type' as empty, which fails with Mysql 5.7+ which is more stringent.

Also, forcing an empty value into a boolean also fails.

Both issues arise when updatedatabase.pl hit 3.23.00.050.

NOTE: both issues could also be resolved by actually setting values in the load file.
      This doesn't make this solution incorrect, though.

To Test/reproduce:
   -1) Happens with Mysql 5.7.4.  Maybe earlier, but certainly at that point.
   0) Find a database on 3.22 or earlier, save it.
   1) Set your code base to master)
   2) run updatedatabase.pl
   3) See the errors on 3.23.00.050
   4) Apply the patch
   5) Reload the 3.22 db.
   6) succeed with updatedatabase.pl
Comment 2 Mark Tompsett 2016-09-20 01:27:45 UTC
Comment on attachment 55263 [details] [review]
Bug 17260 - updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns

Review of attachment 55263 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/updatedatabase.pl
@@ +12280,4 @@
>               `id` int(11) NOT NULL AUTO_INCREMENT, 
>               `name` varchar(255) NOT NULL COMMENT 'the name of the field as it will be stored in the search engine',
>               `label` varchar(255) NOT NULL COMMENT 'the human readable name of the field, for display', 
> +             `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine',

I'm not sure how to test for this.

@@ +12337,4 @@
>              my $search_field = Koha::SearchFields->find_or_create({ name => $field_name, label => $field_label, type => $field_type }, { key => 'name' });
>              for my $mapping ( @$mappings ) {
>                  my $marc_field = Koha::SearchMarcMaps->find_or_create({ index_name => $index_name, marc_type => $mapping->{marc_type}, marc_field => $mapping->{marc_field} });
> +                $search_field->add_to_search_marc_maps($marc_field, { facet => $mapping->{facet} || 0, suggestible => $mapping->{suggestible} || 0, sort => $mapping->{sort} } );

I can voucher for this change easily. I reproduced it, and this fixes it.
Comment 3 Jonathan Druart 2016-10-03 10:28:49 UTC
(In reply to M. Tompsett from comment #2)
> Comment on attachment 55263 [details] [review] [review]
> Bug 17260 - updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN
> columns
> 
> Review of attachment 55263 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/updatedatabase.pl
> @@ +12280,4 @@
> >               `id` int(11) NOT NULL AUTO_INCREMENT, 
> >               `name` varchar(255) NOT NULL COMMENT 'the name of the field as it will be stored in the search engine',
> >               `label` varchar(255) NOT NULL COMMENT 'the human readable name of the field, for display', 
> > +             `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine',
> 
> I'm not sure how to test for this.

I do not have MySQL5.7 but:

mysql> insert into search_field(name, label, type)values('name', 'label', '');
Query OK, 1 row affected, 1 warning (0.01 sec)

mysql> show warnings;
+---------+------+-------------------------------------------+
| Level   | Code | Message                                   |
+---------+------+-------------------------------------------+
| Warning | 1265 | Data truncated for column 'type' at row 1 |
+---------+------+-------------------------------------------+
1 row in set (0.00 sec)

And given https://dev.mysql.com/doc/refman/5.7/en/constraint-enum.html:
"For a column defined as ENUM('a','b','c'), values such as '', 'd', or 'ax' are invalid and are rejected."
I guess MySQL5.7 will raise an error instead of a warning.

ENUM should be avoided, but that would be for another bug report.
Comment 4 Mark Tompsett 2017-01-27 17:56:51 UTC
Created attachment 59621 [details] [review]
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns

The loading of file admin/searchengine/elasticsearch/mappings.yaml
specifies 'type' as empty, which fails with Mysql 5.7+ which is more
stringent.

Also, forcing an empty value into a boolean also fails.

Both issues arise when updatedatabase.pl hit 3.23.00.050.

NOTE: both issues could also be resolved by actually setting values in
      the load file. This doesn't make this solution incorrect, though.

To Test/reproduce:
-1) Happens with Mysql 5.7.4.  Maybe earlier, but certainly at
    that point. Use a Xenial kohadevbox to more easily test.
0) Find a database on 3.22 or earlier, save it.
    a) place
           sql_mode=IGNORE_SPACE,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
       into the [mysqld] section of /etc/mysql/mysql.conf.d/mysqld.cnf
    b) restart you mysql server
    c) drop the db and recreate it
    d) checkout the 3.22.x branch
    e) do a web install
    f) remove the added sql_mode
    g) restart the mysql server
1) Set your code base to master
2) run updatedatabase.pl
3) See the errors on 3.23.00.050
4) Apply the patch
5) Reload the 3.22 db.
   a) repeat steps 0(a)-0(g)
   b) don't forget the caching issues
6) succeed with updatedatabase.pl
7) drop the db and recreate it
8) run the web installer
9) notice no issues either.
10) run koha qa test tools

NOTE: This bug only solved the upgrade portion.
      I added the kohastructure.sql change as well.
      I confirmed that all the code changes were
      triggered with this test plan.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 5 Mehdi Hamidi 2017-02-21 17:40:02 UTC
Created attachment 60532 [details] [review]
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns

The loading of file admin/searchengine/elasticsearch/mappings.yaml
specifies 'type' as empty, which fails with Mysql 5.7+ which is more
stringent.

Also, forcing an empty value into a boolean also fails.

Both issues arise when updatedatabase.pl hit 3.23.00.050.

NOTE: both issues could also be resolved by actually setting values in
      the load file. This doesn't make this solution incorrect, though.

To Test/reproduce:
-1) Happens with Mysql 5.7.4.  Maybe earlier, but certainly at
    that point. Use a Xenial kohadevbox to more easily test.
0) Find a database on 3.22 or earlier, save it.
    a) place
           sql_mode=IGNORE_SPACE,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
       into the [mysqld] section of /etc/mysql/mysql.conf.d/mysqld.cnf
    b) restart you mysql server
    c) drop the db and recreate it
    d) checkout the 3.22.x branch
    e) do a web install
    f) remove the added sql_mode
    g) restart the mysql server
1) Set your code base to master
2) run updatedatabase.pl
3) See the errors on 3.23.00.050
4) Apply the patch
5) Reload the 3.22 db.
   a) repeat steps 0(a)-0(g)
   b) don't forget the caching issues
6) succeed with updatedatabase.pl
7) drop the db and recreate it
8) run the web installer
9) notice no issues either.
10) run koha qa test tools

NOTE: This bug only solved the upgrade portion.
      I added the kohastructure.sql change as well.
      I confirmed that all the code changes were
      triggered with this test plan.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Mehdi Hamidi <mehdi.hamidi@inlibro.com>
Comment 6 Jonathan Druart 2017-02-22 14:35:46 UTC
Who is the original author of this patch?
The patch says "From: Your Full Name <your_email>"
Comment 7 Mehdi Hamidi 2017-02-22 14:40:33 UTC
(In reply to Jonathan Druart from comment #6)
> Who is the original author of this patch?
> The patch says "From: Your Full Name <your_email>"

The original author is : Blou <philippe.blouin@inlibro.com>
Comment 8 Jonathan Druart 2017-02-22 14:53:57 UTC
So I guess you can resubmit the patch with the correct author line.
Comment 9 Mehdi Hamidi 2017-02-22 15:04:14 UTC
Created attachment 60561 [details] [review]
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns

The loading of file admin/searchengine/elasticsearch/mappings.yaml
specifies 'type' as empty, which fails with Mysql 5.7+ which is more
stringent.

Also, forcing an empty value into a boolean also fails.

Both issues arise when updatedatabase.pl hit 3.23.00.050.

NOTE: both issues could also be resolved by actually setting values in
      the load file. This doesn't make this solution incorrect, though.

To Test/reproduce:
-1) Happens with Mysql 5.7.4.  Maybe earlier, but certainly at
    that point. Use a Xenial kohadevbox to more easily test.
0) Find a database on 3.22 or earlier, save it.
    a) place
           sql_mode=IGNORE_SPACE,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
       into the [mysqld] section of /etc/mysql/mysql.conf.d/mysqld.cnf
    b) restart you mysql server
    c) drop the db and recreate it
    d) checkout the 3.22.x branch
    e) do a web install
    f) remove the added sql_mode
    g) restart the mysql server
1) Set your code base to master
2) run updatedatabase.pl
3) See the errors on 3.23.00.050
4) Apply the patch
5) Reload the 3.22 db.
   a) repeat steps 0(a)-0(g)
   b) don't forget the caching issues
6) succeed with updatedatabase.pl
7) drop the db and recreate it
8) run the web installer
9) notice no issues either.
10) run koha qa test tools

NOTE: This bug only solved the upgrade portion.
      I added the kohastructure.sql change as well.
      I confirmed that all the code changes were
      triggered with this test plan.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Mehdi Hamidi <mehdi.hamidi@inlibro.com>
Comment 10 Jonathan Druart 2017-02-24 08:09:05 UTC
Created attachment 60635 [details] [review]
Bug 17260: updatedatabase.pl fails on invalid entries in ENUM and BOOLEAN columns

The loading of file admin/searchengine/elasticsearch/mappings.yaml
specifies 'type' as empty, which fails with Mysql 5.7+ which is more
stringent.

Also, forcing an empty value into a boolean also fails.

Both issues arise when updatedatabase.pl hit 3.23.00.050.

NOTE: both issues could also be resolved by actually setting values in
      the load file. This doesn't make this solution incorrect, though.

To Test/reproduce:
-1) Happens with Mysql 5.7.4.  Maybe earlier, but certainly at
    that point. Use a Xenial kohadevbox to more easily test.
0) Find a database on 3.22 or earlier, save it.
    a) place
           sql_mode=IGNORE_SPACE,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION
       into the [mysqld] section of /etc/mysql/mysql.conf.d/mysqld.cnf
    b) restart you mysql server
    c) drop the db and recreate it
    d) checkout the 3.22.x branch
    e) do a web install
    f) remove the added sql_mode
    g) restart the mysql server
1) Set your code base to master
2) run updatedatabase.pl
3) See the errors on 3.23.00.050
4) Apply the patch
5) Reload the 3.22 db.
   a) repeat steps 0(a)-0(g)
   b) don't forget the caching issues
6) succeed with updatedatabase.pl
7) drop the db and recreate it
8) run the web installer
9) notice no issues either.
10) run koha qa test tools

NOTE: This bug only solved the upgrade portion.
      I added the kohastructure.sql change as well.
      I confirmed that all the code changes were
      triggered with this test plan.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Mehdi Hamidi <mehdi.hamidi@inlibro.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2017-02-24 08:09:15 UTC
Created attachment 60636 [details] [review]
Bug 17260: Update existing installs

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Kyle M Hall 2017-03-03 16:47:38 UTC
Pushed to master for 17.05, thanks Phillipe, Jonathan!
Comment 13 Katrin Fischer 2017-03-12 22:05:19 UTC
These patches have been pushed to 16.11.x and will be in 16.11.05.
Comment 14 Katrin Fischer 2017-03-12 22:13:09 UTC
These patches have been pushed to 16.11.x and will be in 16.11.05.
Comment 15 Mason James 2017-04-03 14:42:46 UTC
Pushed to 16.05.x, for 16.05.11 release