Bug 20930 - Validate and cache parsed YAML/JSON type system preferences
Summary: Validate and cache parsed YAML/JSON type system preferences
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Ere Maijala
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-13 10:40 UTC by Lari Taskula
Modified: 2020-05-14 11:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20930: Dump and load system preferences of YAML/JSON type (8.65 KB, patch)
2018-06-25 14:58 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 20930: First example - Change OpacHiddenItems to type YAML (8.16 KB, patch)
2018-06-25 14:58 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 20930: Display human readable errors in preferences page (5.46 KB, patch)
2018-06-25 14:59 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 20930: Dump and load system preferences of YAML/JSON type (15.62 KB, patch)
2019-07-30 05:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 20930: First example - Change OpacHiddenItems to type YAML (8.31 KB, patch)
2019-07-30 05:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 20930: Display human readable errors in preferences page (5.57 KB, patch)
2019-07-30 05:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 20930: Display human readable errors in preferences page (5.98 KB, patch)
2019-10-18 14:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 20930: Make line and column number alert translatable (2.64 KB, patch)
2019-10-18 14:42 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 20930: Dump and load system preferences of YAML/JSON type (15.69 KB, patch)
2019-12-09 18:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20930: First example - Change OpacHiddenItems to type YAML (8.41 KB, patch)
2019-12-09 18:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20930: Display human readable errors in preferences page (6.06 KB, patch)
2019-12-09 18:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20930: Make line and column number alert translatable (2.70 KB, patch)
2019-12-09 18:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20930: (follow-up) Improve formatting of error message (4.51 KB, patch)
2019-12-09 18:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20930: Dump and load system preferences of YAML/JSON type (15.73 KB, patch)
2020-01-04 20:49 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20930: First example - Change OpacHiddenItems to type YAML (8.45 KB, patch)
2020-01-04 20:49 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20930: Display human readable errors in preferences page (6.10 KB, patch)
2020-01-04 20:49 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20930: Make line and column number alert translatable (2.74 KB, patch)
2020-01-04 20:49 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20930: (follow-up) Improve formatting of error message (4.55 KB, patch)
2020-01-04 20:49 UTC, Michal Denar
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2018-06-13 10:40:11 UTC
Currently, C4::Context->preference('YAMLpref') returns a string that needs to be parsed by any code using the preference.

My suggestion is to return a parsed version instead (a hash/array ref or a string) and using the syspref cache with the parsed value. Like this, we could:
- centralize parsing of YAML system preferences
- optimize use of system preference cache by caching the parsed version instead. Parse once and store in cache; no need to re-parse on each call!

We could centralize this functionality into Koha::Config::SysPref->value() (used by C4::Context->preference). Use database column systempreferences.type with value of "yaml" to trigger this functionality in "value()"-method.
Comment 1 Lari Taskula 2018-06-25 14:58:45 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2018-06-25 14:58:57 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2018-06-25 14:59:11 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2018-06-25 15:04:37 UTC
Changed Bug title 

from "Handle YAML type system preferences as parsed references" 
to "Validate and cache parsed YAML/JSON type system preferences" 

as this now also adds syntax validation for librarians, so that it is no longer possible to store invalid YAML system preferences.
Comment 5 Lari Taskula 2018-06-25 15:07:38 UTC
Also fyi, these patches do not automatically change the functionality for all YAML/JSON type system preferences.

The type (systempreferences.type) needs to be YAML before this applies for the preference.
Comment 6 Katrin Fischer 2018-07-11 05:51:29 UTC
Hi Lari, please set the assignee to yourself if you plan to work on a bug or are providing patches for it, thx!
Comment 7 Ere Maijala 2018-07-11 06:36:42 UTC
I'm afraid Lari might be unable to work on this, so I'm taking it for any further work.
Comment 8 Michal Denar 2019-05-03 18:04:25 UTC
Hi Ere,
any progress with this patch? Can you try to rebase it on the master?

Thank you.
Comment 9 Ere Maijala 2019-07-30 05:50:45 UTC
Created attachment 91861 [details] [review]
Bug 20930: Dump and load system preferences of YAML/JSON type

Currently, C4::Context->preference('YAMLpref') returns a string that needs to be
parsed by any code using the preference.

My suggestion is to return a parsed version instead (a hash/array ref or a string)
and using the syspref cache with the parsed value. Like this, we could:
- centralize parsing of YAML system preferences
- optimize use of system preference cache by caching the parsed version instead.
  Parse once and store in cache; no need to re-parse on each call!

We can centralize this functionality into Koha::Config::SysPref->value() (used by
C4::Context->preference). Use database column systempreferences.type with value of
"YAML" to trigger this functionality in "value()"-method.

To test:
1. Make sure memcached is available
2. Run tests with caching enabled, e.g.: MEMCACHED_SERVERS=localhost:11211 prove t/db_dependent/Context.t
3. Make sure all tests pass and none were skipped
Comment 10 Ere Maijala 2019-07-30 05:50:48 UTC
Created attachment 91862 [details] [review]
Bug 20930: First example - Change OpacHiddenItems to type YAML

To test:
1. prove t/db_dependent/Items.t
2. Check that desired items are still hidden in OPAC by using
   OpacHiddenItems as documented here:
   https://wiki.koha-community.org/wiki/OpacHiddenItems
Comment 11 Ere Maijala 2019-07-30 05:50:51 UTC Comment hidden (obsolete)
Comment 12 Ere Maijala 2019-07-30 07:26:52 UTC
I believe I've managed to make this apply and work...
Comment 13 Lari Taskula 2019-10-18 13:23:22 UTC
Would be nice to get this signed-off - it makes both sysadmins' and librarians' life easier as it is very simple to make mistakes in YAML syntax. Validations are usually pretty nice, especially in this case.
Comment 14 Lari Taskula 2019-10-18 14:16:22 UTC
Created attachment 94412 [details] [review]
Bug 20930: Display human readable errors in preferences page

Displays human readable error when YAML/JSON preference has invalid syntax.

If multiple preferences are modified at the same time, notifies librarian of the
preferences that were saved and the ones that had errors.

To test:
1. Apply patches
2. Write invalid YAML syntax to OpacHiddenItems system preference
3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error
   You should also receive a hint on line and column numbers of where
   the error occured.
4. Write valid YAML syntax to OpacHiddenItems system preference
5. Observe "Saved preference OpacHiddenItems" message
6. Test steps 2-5 for both single preference modification and together
   with other system preferences.
Comment 15 Lari Taskula 2019-10-18 14:42:06 UTC
Created attachment 94413 [details] [review]
Bug 20930: Make line and column number alert translatable

To test:
1. Apply patch
2. Store invalid YAML syntax into OpacHiddenItems preference
3. Observe error
4. You should also see line and column number of where the error occured
Comment 16 Ere Maijala 2019-11-15 13:59:08 UTC
Just one minor thing: I'd like to assume we'll never just tell the user that there's invalid syntax without telling the position, so could the two messages be combined into one, like "Error: Preference '%s' has invalid syntax at line %s, column %s."?
Comment 17 Owen Leonard 2019-12-09 18:00:37 UTC
Created attachment 96108 [details] [review]
Bug 20930: Dump and load system preferences of YAML/JSON type

Currently, C4::Context->preference('YAMLpref') returns a string that needs to be
parsed by any code using the preference.

My suggestion is to return a parsed version instead (a hash/array ref or a string)
and using the syspref cache with the parsed value. Like this, we could:
- centralize parsing of YAML system preferences
- optimize use of system preference cache by caching the parsed version instead.
  Parse once and store in cache; no need to re-parse on each call!

We can centralize this functionality into Koha::Config::SysPref->value() (used by
C4::Context->preference). Use database column systempreferences.type with value of
"YAML" to trigger this functionality in "value()"-method.

To test:
1. Make sure memcached is available
2. Run tests with caching enabled, e.g.: MEMCACHED_SERVERS=localhost:11211 prove t/db_dependent/Context.t
3. Make sure all tests pass and none were skipped

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Owen Leonard 2019-12-09 18:00:40 UTC
Created attachment 96109 [details] [review]
Bug 20930: First example - Change OpacHiddenItems to type YAML

To test:
1. prove t/db_dependent/Items.t
2. Check that desired items are still hidden in OPAC by using
   OpacHiddenItems as documented here:
   https://wiki.koha-community.org/wiki/OpacHiddenItems

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Owen Leonard 2019-12-09 18:00:43 UTC
Created attachment 96110 [details] [review]
Bug 20930: Display human readable errors in preferences page

Displays human readable error when YAML/JSON preference has invalid syntax.

If multiple preferences are modified at the same time, notifies librarian of the
preferences that were saved and the ones that had errors.

To test:
1. Apply patches
2. Write invalid YAML syntax to OpacHiddenItems system preference
3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error
   You should also receive a hint on line and column numbers of where
   the error occured.
4. Write valid YAML syntax to OpacHiddenItems system preference
5. Observe "Saved preference OpacHiddenItems" message
6. Test steps 2-5 for both single preference modification and together
   with other system preferences.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 20 Owen Leonard 2019-12-09 18:00:46 UTC
Created attachment 96111 [details] [review]
Bug 20930: Make line and column number alert translatable

To test:
1. Apply patch
2. Store invalid YAML syntax into OpacHiddenItems preference
3. Observe error
4. You should also see line and column number of where the error occured

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 21 Owen Leonard 2019-12-09 18:00:49 UTC
Created attachment 96112 [details] [review]
Bug 20930: (follow-up) Improve formatting of error message

This patch makes a few changes to system preferences JavaScript so that
the error message for invalid YAML is output on one line. CSS has been
modified to improve the readability of multiple error messages.

The order of CSS files loaded by preferences.tt has been changed to
ensure that CSS in preferences.css overrides plugin-related CSS.

Unrelated: An incorrect ";" has been replaced with ":" in
preferences.tt.

To test, apply the patch and repeat the previous test plan. Confirm that
the error messages appear as expected.
Comment 22 Michal Denar 2020-01-03 22:10:57 UTC
Ho Owen,
can You rebase this patch on master?

Applying: Bug 20930: Dump and load system preferences of YAML/JSON type
Using index info to reconstruct a base tree...
M	C4/Context.pm
M	Koha/Config/SysPref.pm
M	Koha/Exceptions/Config.pm
M	t/db_dependent/Context.t
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Bug 20930: First example - Change OpacHiddenItems to type YAML
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
M	C4/Items.pm
M	installer/data/mysql/sysprefs.sql
M	opac/opac-search.pl
M	t/db_dependent/Items.t
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Bug 20930: Display human readable errors in preferences page
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt
M	koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js
M	svc/config/systempreferences
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt
error: Failed to merge in the changes.
Comment 23 Michal Denar 2020-01-04 17:27:07 UTC
Im sorry, my fault. Patched twice.
Comment 24 Michal Denar 2020-01-04 20:49:22 UTC
Created attachment 96817 [details] [review]
Bug 20930: Dump and load system preferences of YAML/JSON type

Currently, C4::Context->preference('YAMLpref') returns a string that needs to be
parsed by any code using the preference.

My suggestion is to return a parsed version instead (a hash/array ref or a string)
and using the syspref cache with the parsed value. Like this, we could:
- centralize parsing of YAML system preferences
- optimize use of system preference cache by caching the parsed version instead.
  Parse once and store in cache; no need to re-parse on each call!

We can centralize this functionality into Koha::Config::SysPref->value() (used by
C4::Context->preference). Use database column systempreferences.type with value of
"YAML" to trigger this functionality in "value()"-method.

To test:
1. Make sure memcached is available
2. Run tests with caching enabled, e.g.: MEMCACHED_SERVERS=localhost:11211 prove t/db_dependent/Context.t
3. Make sure all tests pass and none were skipped

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

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 25 Michal Denar 2020-01-04 20:49:26 UTC
Created attachment 96818 [details] [review]
Bug 20930: First example - Change OpacHiddenItems to type YAML

To test:
1. prove t/db_dependent/Items.t
2. Check that desired items are still hidden in OPAC by using
   OpacHiddenItems as documented here:
   https://wiki.koha-community.org/wiki/OpacHiddenItems

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

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 26 Michal Denar 2020-01-04 20:49:29 UTC
Created attachment 96819 [details] [review]
Bug 20930: Display human readable errors in preferences page

Displays human readable error when YAML/JSON preference has invalid syntax.

If multiple preferences are modified at the same time, notifies librarian of the
preferences that were saved and the ones that had errors.

To test:
1. Apply patches
2. Write invalid YAML syntax to OpacHiddenItems system preference
3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error
   You should also receive a hint on line and column numbers of where
   the error occured.
4. Write valid YAML syntax to OpacHiddenItems system preference
5. Observe "Saved preference OpacHiddenItems" message
6. Test steps 2-5 for both single preference modification and together
   with other system preferences.

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

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 27 Michal Denar 2020-01-04 20:49:33 UTC
Created attachment 96820 [details] [review]
Bug 20930: Make line and column number alert translatable

To test:
1. Apply patch
2. Store invalid YAML syntax into OpacHiddenItems preference
3. Observe error
4. You should also see line and column number of where the error occured

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

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 28 Michal Denar 2020-01-04 20:49:36 UTC
Created attachment 96821 [details] [review]
Bug 20930: (follow-up) Improve formatting of error message

This patch makes a few changes to system preferences JavaScript so that
the error message for invalid YAML is output on one line. CSS has been
modified to improve the readability of multiple error messages.

The order of CSS files loaded by preferences.tt has been changed to
ensure that CSS in preferences.css overrides plugin-related CSS.

Unrelated: An incorrect ";" has been replaced with ":" in
preferences.tt.

To test, apply the patch and repeat the previous test plan. Confirm that
the error messages appear as expected.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 29 Jonathan Druart 2020-01-10 15:15:40 UTC
QA comments:
1. You remove the validation from GetHiddenItemnumbers and so assume that the data in the pref is correct, but maybe wrong data exist in the DB prior to this patch.
(Also, use YAML in C4::Items that is no longer needed)

2. I do not think we should not use YAML to convert the syspref's value to perl struct.
For instance, "field: [value1, value2]" does not work, ie. the ref($syspref_val) return an empty string (means it's a scalar):
To recreate, edit OpacHiddenItems with "field: [value1, value2]"
Apply:
diff --git a/C4/Items.pm b/C4/Items.pm
index 519ce41991..283def5e4c 100644
--- a/C4/Items.pm
+++ b/C4/Items.pm
@@ -1202,6 +1202,8 @@ sub GetHiddenItemnumbers {
     my @resultitems;

     my $hidingrules = C4::Context->preference('OpacHiddenItems');
+    warn $hidingrules;
+    warn ref($hidingrules);
     return () if !$hidingrules or ref($hidingrules) ne 'HASH';
     my $dbh = C4::Context->dbh;

Finally run:
use Koha::Items;                                                                              
use C4::Items;                                                                                
my $items = Koha::Items->search->unblessed;                                                   
C4::Items::GetHiddenItemnumbers({items => $items}); 

It displays:
field: [value1, value2] at /kohadevbox/koha/C4/Items.pm line 1205.
Warning: something's wrong at /kohadevbox/koha/C4/Items.pm line 1206.

3. "Error: your data might not have been saved", it is what I get when I generate an invalid yaml, I do not get info about the line, or what the error is.
Comment 30 Jonathan Druart 2020-01-10 15:18:22 UTC
4. Update DB entry needed to prevent regression if syspref contains only \s

-    my $opachiddenitems = $opac
-      && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ );
-
+    my $opachiddenitems = $opac && ( C4::Context->preference('OpacHiddenItems') );