Bug 24883

Summary: Add `misc/load_yaml.pl` utility script to allow manual loading of yaml data files
Product: Koha Reporter: Bernardo Gonzalez Kriegel <bgkriegel>
Component: Command-line UtilitiesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, jonathan.druart, m.de.rooy, martin.renvoize, robin
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13897
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
During the 20.05 cycle a number of improvements were made to the installation mechanisms to enhance the translation workflows for this area. As part of that work many existing translated .sql files were moved to a yaml based file.
Version(s) released in:
20.05.00
Bug Depends on: 24904    
Bug Blocks: 24912    
Attachments: Bug 24883: Command line utility to load yaml files
Bug 24883: Command line utility to load yaml files
Bug 24883: Centralize code to load YAML installer files to a sub
Bug 24883: Move to a flat array with all values to an array of arrayref
Bug 24883: Add a new yml2sql sub to C4::Installer
Bug 24883: Move new sub from load_yaml.pl
Bug 24883: Centralize code to load YAML installer files to a sub
Bug 24883: Move to a flat array with all values to an array of arrayref
Bug 24883: Move new sub from load_yaml.pl
Bug 24883: Add POD entry and remove unused variable
Bug 24883: Add POD entry and remove unused variable
Bug 24883: Command line utility to load yaml files
Bug 24883: Centralize code to load YAML installer files to a sub
Bug 24883: Move to a flat array with all values to an array of arrayref
Bug 24883: Move new sub from load_yaml.pl
Bug 24883: Add POD entry and remove unused variable
Bug 24883: Command line utility to load yaml files
Bug 24883: Centralize code to load YAML installer files to a sub
Bug 24883: Move to a flat array with all values to an array of arrayref
Bug 24883: Move new sub from load_yaml.pl
Bug 24883: Add POD entry and remove unused variable

Description Bernardo Gonzalez Kriegel 2020-03-16 21:33:05 UTC
Now that the installation files are in YAML, a tool to load them would be handy.
Comment 1 Bernardo Gonzalez Kriegel 2020-03-16 22:06:22 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2020-03-19 10:33:12 UTC
Moving out of NSO queue, I am trying something.
Comment 3 Jonathan Druart 2020-03-19 12:24:35 UTC
Created attachment 101009 [details] [review]
Bug 24883: Command line utility to load yaml files

This patch adds a command line utility to load
yaml files.

To test:
1) Apply the patch
2) Try loading a file, e.g. sample notices
 a) mysql -e "delete from letter"
 b) misc/load_yaml.pl -f installer/data/mysql/en/mandatory/sample_notices.yml
3) Try loading a file with a syntax error,
   edit previous file adding ':' at the end of a field without quotes
   then load again
Comment 4 Jonathan Druart 2020-03-19 12:24:38 UTC
Created attachment 101010 [details] [review]
Bug 24883: Centralize code to load YAML installer files to a sub
Comment 5 Jonathan Druart 2020-03-19 12:24:42 UTC
Created attachment 101011 [details] [review]
Bug 24883: Move to a flat array with all values to an array of arrayref

For the following SQL query:
INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina');

We move from:
[ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ]
to:
[ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ]

Which make more sense to split, build and construct the queries
Comment 6 Jonathan Druart 2020-03-19 12:24:45 UTC
Created attachment 101012 [details] [review]
Bug 24883: Add a new yml2sql sub to C4::Installer

It will allow us to generate sql code from the yml files
Comment 7 Jonathan Druart 2020-03-19 12:24:48 UTC
Created attachment 101013 [details] [review]
Bug 24883: Move new sub from load_yaml.pl

Use the sub from C4::Installer to avoid dup of code.
Also add an option to convert the yml file to SQL queries

Note:
We are going to modify the script and so will do more stuffs.
We may want to rename it, maybe installer_utilities.pl,
misc/installer/yaml_utility.pl, any suggestions?
Comment 8 Jonathan Druart 2020-03-19 12:25:49 UTC
Bernardo, what about those follow-ups? Do they make sense of I went to far?

More work needed:
 - Use Pod2Usage
 - Write tests for the new subroutines

But I'd like you to agree first before I continue.
Comment 9 Bernardo Gonzalez Kriegel 2020-03-19 13:17:47 UTC
(In reply to Jonathan Druart from comment #8)
> Bernardo, what about those follow-ups? Do they make sense of I went to far?
> 
> More work needed:
>  - Use Pod2Usage
>  - Write tests for the new subroutines
> 
> But I'd like you to agree first before I continue.

Hi Jonathan,
trying this and Bug 24904.
I'm ok in general, I was also testing some changes of myself.

One change I tried (not related with this bug) was disabling AutoCommit after the insertion loop and enabling it afterwards. With that I got almost the same time as with sql files. Have you explored that option?
Comment 10 Jonathan Druart 2020-03-19 13:33:56 UTC
I tried to execute all the queries in a transaction (using txn_do), but no gain observed.
Comment 11 Bernardo Gonzalez Kriegel 2020-03-19 13:55:48 UTC
(In reply to Jonathan Druart from comment #10)
> I tried to execute all the queries in a transaction (using txn_do), but no
> gain observed.

With this diff in my original script

diff --git a/misc/load_yaml.pl b/misc/load_yaml.pl
index e670f56660..8eed5ab933 100755
--- a/misc/load_yaml.pl
+++ b/misc/load_yaml.pl
@@ -63,7 +63,7 @@ eval {
 if ($@){
     die "Something went wrong loading file $file ($@)";
 }
-
+$dbh->{'AutoCommit'} = 0;
 for my $table ( @{ $yaml->{'tables'} } ) {
     my $table_name   = ( keys %$table )[0];                          # table name
     my @rows         = @{ $table->{$table_name}->{rows} };           #
@@ -86,3 +86,4 @@ for my $table ( @{ $yaml->{'tables'} } ) {
 for my $statement ( @{ $yaml->{'sql_statements'} } ) {               # extra SQL statements
     $dbh->do($statement);
 }
+$dbh->{'AutoCommit'} = 1;

I see changes, form ~9s to ~1.3 for marc21_framework_DEFAULT.yml for example.

Of course I'm with you on putting the code in one place and other changes, just want to share my results.
Comment 12 Jonathan Druart 2020-03-19 14:23:25 UTC
We got problems with AutoCommit=0 and transactions, and decided to use DBIC transactions only. I am surprised however do not have the same results between both.
Comment 13 Bernardo Gonzalez Kriegel 2020-03-19 15:58:47 UTC
(In reply to Jonathan Druart from comment #12)
> We got problems with AutoCommit=0 and transactions, and decided to use DBIC
> transactions only. I am surprised however do not have the same results
> between both.

Understood.

Works well, nice to load multiple file in one go, a few qa errors.

Now, the conversion part. 
The problem here is that all values are printed as strings, producing
INSERT INTO marc_tag_structure ( `authorised_value`,`frameworkcode`,`liblibrarian`,`libopac`,`mandatory`,`repeatable`,`tagfield` ) VALUES 
('', '', 'LEADER', 'LEADER', '1', '0', '000'),

instead of
('', '', 'LEADER', 'LEADER', 1, 0, '000'),

A file like this will fail to load with strict mode.

This could be fixed if we declare on each table which fields are numeric, and take that into account when doing the conversion.
What do you think?
Comment 14 Jonathan Druart 2020-03-20 11:20:44 UTC
I moved my patches to a separate bug report, see bug 24912.
Comment 15 Jonathan Druart 2020-03-20 15:25:58 UTC
Created attachment 101292 [details] [review]
Bug 24883: Centralize code to load YAML installer files to a sub
Comment 16 Jonathan Druart 2020-03-20 15:26:01 UTC
Created attachment 101293 [details] [review]
Bug 24883: Move to a flat array with all values to an array of arrayref

For the following SQL query:
INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina');

We move from:
[ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ]
to:
[ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ]

Which make more sense to split, build and construct the queries
Comment 17 Jonathan Druart 2020-03-20 15:26:05 UTC
Created attachment 101294 [details] [review]
Bug 24883: Move new sub from load_yaml.pl

Use the sub from C4::Installer to avoid dup of code.

Note:
We are going to modify the script and so will do more stuffs.
We may want to rename it, maybe installer_utilities.pl,
misc/installer/yaml_utility.pl, any suggestions?
Comment 18 Bernardo Gonzalez Kriegel 2020-03-21 12:11:11 UTC
Created attachment 101332 [details] [review]
Bug 24883: Add POD entry and remove unused variable

https://bugs.koha-community.org/show_bug.cgi?id=24833
Comment 19 Bernardo Gonzalez Kriegel 2020-03-21 12:12:09 UTC
Created attachment 101333 [details] [review]
Bug 24883: Add POD entry and remove unused variable
Comment 20 Martin Renvoize 2020-03-26 16:35:30 UTC
This is looking good so far.. and in testing seems to all work for me.

It looks like you are perhaps still working on it together.. or is it ready for a signoff?
Comment 21 Bernardo Gonzalez Kriegel 2020-03-26 17:14:26 UTC
(In reply to Martin Renvoize from comment #20)
> This is looking good so far.. and in testing seems to all work for me.
> 
> It looks like you are perhaps still working on it together.. or is it ready
> for a signoff?

I'm using it, works Ok :)
My last patch was only to remove some complains from qa tools, don't know if Jonathan agrees with the pod text, but can be amended in the future.

It's waiting a signoff
Comment 22 Martin Renvoize 2020-03-26 18:15:47 UTC
Created attachment 101901 [details] [review]
Bug 24883: Command line utility to load yaml files

This patch adds a command line utility to load
yaml files.

To test:
1) Apply the patch
2) Try loading a file, e.g. sample notices
 a) mysql -e "delete from letter"
 b) misc/load_yaml.pl -f installer/data/mysql/en/mandatory/sample_notices.yml
3) Try loading a file with a syntax error,
   edit previous file adding ':' at the end of a field without quotes
   then load again

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2020-03-26 18:15:50 UTC
Created attachment 101902 [details] [review]
Bug 24883: Centralize code to load YAML installer files to a sub

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2020-03-26 18:15:53 UTC
Created attachment 101903 [details] [review]
Bug 24883: Move to a flat array with all values to an array of arrayref

For the following SQL query:
INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina');

We move from:
[ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ]
to:
[ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ]

Which make more sense to split, build and construct the queries

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-03-26 18:15:56 UTC
Created attachment 101904 [details] [review]
Bug 24883: Move new sub from load_yaml.pl

Use the sub from C4::Installer to avoid dup of code.

Note:
We are going to modify the script and so will do more stuffs.
We may want to rename it, maybe installer_utilities.pl,
misc/installer/yaml_utility.pl, any suggestions?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2020-03-26 18:15:59 UTC
Created attachment 101905 [details] [review]
Bug 24883: Add POD entry and remove unused variable

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2020-03-26 18:16:24 UTC
Works well, passes QA scripts.. Signing off..
Comment 28 Marcel de Rooy 2020-03-27 07:11:31 UTC
QAing
Comment 29 Marcel de Rooy 2020-03-27 08:06:25 UTC
Created attachment 101950 [details] [review]
Bug 24883: Command line utility to load yaml files

This patch adds a command line utility to load
yaml files.

To test:
1) Apply the patch
2) Try loading a file, e.g. sample notices
 a) mysql -e "delete from letter"
 b) misc/load_yaml.pl -f installer/data/mysql/en/mandatory/sample_notices.yml
3) Try loading a file with a syntax error,
   edit previous file adding ':' at the end of a field without quotes
   then load again

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2020-03-27 08:06:30 UTC
Created attachment 101951 [details] [review]
Bug 24883: Centralize code to load YAML installer files to a sub

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2020-03-27 08:06:34 UTC
Created attachment 101952 [details] [review]
Bug 24883: Move to a flat array with all values to an array of arrayref

For the following SQL query:
INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina');

We move from:
[ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ]
to:
[ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ]

Which make more sense to split, build and construct the queries

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2020-03-27 08:06:38 UTC
Created attachment 101953 [details] [review]
Bug 24883: Move new sub from load_yaml.pl

Use the sub from C4::Installer to avoid dup of code.

Note:
We are going to modify the script and so will do more stuffs.
We may want to rename it, maybe installer_utilities.pl,
misc/installer/yaml_utility.pl, any suggestions?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2020-03-27 08:06:42 UTC
Created attachment 101954 [details] [review]
Bug 24883: Add POD entry and remove unused variable

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2020-03-27 08:07:08 UTC
Great! Looks good to me.
No test for process_yml_table, but no blocker for me.
We could add some more helpful messages in the future?
E.g. If I am forgetting a : after tables, file is silently ignored. Could probably think of more examples.
Comment 35 Martin Renvoize 2020-03-27 12:24:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 36 Joy Nelson 2020-04-03 22:50:48 UTC
enhancement not backported to 19.11.x