From 3861b22bf6d5dd0dfcf7809a438bbfcfa7e0027e Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Thu, 1 Jun 2023 11:03:48 +0200 Subject: [PATCH 01/64] Add unit test for JSONRPC object validation Related to #1105. It seems that we are missing a proper test for the validation routine of a JSONRPC object. This commit introduces one. Some of the tests are disabled because they would either fail or crash completely. They will be enabled as the corresponding bugs are fixed. --- MANIFEST | 3 +- t/rpc_validation.t | 266 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 268 insertions(+), 1 deletion(-) create mode 100644 t/rpc_validation.t diff --git a/MANIFEST b/MANIFEST index 9c33049a1..9245fd6fe 100644 --- a/MANIFEST +++ b/MANIFEST @@ -69,7 +69,6 @@ share/zm-rpcapi.lsb share/zm-testagent.lsb share/zm_rpcapi-bsd share/zm_testagent-bsd -t/TestUtil.pm t/00-load.t t/batches.t t/config.t @@ -78,8 +77,10 @@ t/idn.data t/idn.t t/lifecycle.t t/parameters_validation.t +t/rpc_validation.t t/test01.data t/test01.t t/test_profile.json t/test_validate_syntax.t +t/TestUtil.pm t/validator.t diff --git a/t/rpc_validation.t b/t/rpc_validation.t new file mode 100644 index 000000000..2bb28fdda --- /dev/null +++ b/t/rpc_validation.t @@ -0,0 +1,266 @@ +use strict; +use warnings; +use 5.14.2; +use utf8; + +use Test::More tests => 30; +use Test::NoWarnings; + +use Cwd; +use File::Temp qw[tempdir]; +use Zonemaster::Backend::Config; +use Zonemaster::Backend::RPCAPI; +use JSON::Validator::Joi "joi"; +use JSON::PP; + +### +### Setup +### + +my $tempdir = tempdir( CLEANUP => 1 ); +my $cwd = cwd(); + +my $config = Zonemaster::Backend::Config->parse( <new( + { + dbtype => $config->DB_engine, + config => $config, + } +); + +### +### JSONRPC request object construction helper +### + +sub jsonrpc +{ + my ($method, $params, $force_undef) = @_; + my $object = { + jsonrpc => '2.0', + id => 'testing', + method => $method + }; + if (defined $params or $force_undef) { + $object->{params} = $params; + } + + return $object; +} + +### +### JSONRPC error response construction helpers +### + +sub jsonrpc_error +{ + my ($message, $code, $data, $id) = @_; + my $object = { + jsonrpc => '2.0', + id => $id, + error => { + message => $message, + code => $code + } + }; + $object->{error}{data} = $data if defined $data; + return $object; +} + +sub error_bad_jsonrpc +{ + my ($data) = @_; + + jsonrpc_error('The JSON sent is not a valid request object.', '-32600', $data, undef); +} + +sub error_missing_params +{ + jsonrpc_error("Missing 'params' object", '-32602', undef, 'testing'); +} + +sub error_bad_params +{ + my ($messages) = @_; + + my @data; + + while (@$messages) { + my $path = shift @$messages; + my $message = shift @$messages; + push @data, { path => $path, message => $message }; + } + + jsonrpc_error('Invalid method parameter(s).', '-32602', \@data, 'testing'); +} + +sub no_error +{ + return ''; +} + +### +### Test wrapper functions +### + +sub test_validation +{ + my ($input, $output, $message) = @_; + + my $res = $rpcapi->jsonrpc_validate($input); + is_deeply($res, $output, $message) or diag(encode_json($res)); +} + + +### +### The tests themselves +### + +test_validation undef, + error_bad_jsonrpc('/: Expected object - got null.'), + "Sending undef is an error"; + +TODO: { + todo_skip "These tests trigger crashes", 4; + + test_validation JSON::PP::false, + error_bad_jsonrpc('/: Expected object - got boolean.'), + "Sending a boolean is an error"; + + test_validation -1, + error_bad_jsonrpc('/: Expected object - got number.'), + "Sending a number is an error"; + + test_validation "hello", + error_bad_jsonrpc('/: Expected object - got string.'), + "Sending a string is an error"; + + test_validation [qw(a b c)], + error_bad_jsonrpc('/: Expected object - got array.'), + "Sending an array is an error"; +} + +test_validation {}, + error_bad_jsonrpc('/jsonrpc: Missing property. /method: Missing property.'), + "Sending an empty object is an error"; + +test_validation { jsonrpc => '2.0' }, + error_bad_jsonrpc('/method: Missing property.'), + "Sending an incomplete object is an error"; + +test_validation { jsonrpc => '2.0', method => 'system_versions' }, + error_bad_jsonrpc(''), + "Sending an object with no ID is an error"; + +TODO: { + local $TODO = "Code does not enforce type of ID yet"; + + test_validation { jsonrpc => '2.0', method => 'system_versions', id => JSON::PP::false }, + error_bad_jsonrpc(''), + "Sending an object whose ID is a boolean is an error"; + + test_validation { jsonrpc => '2.0', method => 'system_versions', id => [qw(a b c)] }, + error_bad_jsonrpc(''), + "Sending an object whose ID is an array is an error"; + + test_validation { jsonrpc => '2.0', method => 'system_versions', id => { a => 1 } }, + error_bad_jsonrpc(''), + "Sending an object whose ID is an object is an error"; +} + +TODO: { + local $TODO = 'See #1105'; + + test_validation jsonrpc("job_status"), + error_missing_params(), + "Calling job_status without parameters is an error"; +} + +test_validation jsonrpc("job_status", undef, 1), + error_bad_params(["/" => "Expected object - got null."]), + "Passing null as parameter to job_status is an error"; + +TODO: { + todo_skip "These tests trigger crashes", 4; + + test_validation jsonrpc("job_status", JSON::PP::false), + error_bad_params(["/" => "Expected object - got boolean."]), + "Passing boolean as parameter to job_status is an error"; + + test_validation jsonrpc("job_status", 1), + error_bad_params(["/" => "Expected object - got number."]), + "Passing number as parameter to job_status is an error"; + + test_validation jsonrpc("job_status", "hello"), + error_bad_params(["/" => "Expected object - got string."]), + "Passing string as parameter to job_status is an error"; + + test_validation jsonrpc("job_status", [qw(a b c)]), + error_bad_params(["/" => "Expected object - got array."]), + "Passing array as parameter to job_status is an error"; +} + +TODO: { + local $TODO = 'See #1105'; + + test_validation jsonrpc("job_status", {}), + error_bad_params(["/test_id" => "Missing property"]), + "Passing empty object as parameter to job_status is an error"; + + test_validation jsonrpc("job_status", { test_id => 'this_will_definitely_never_ever_exist' }), + error_bad_params(["/test_id" => 'String does not match (?^u:^[0-9a-f]{16}$).']), + "Calling job_status with a bad test_id is an error"; + + test_validation jsonrpc("job_status", { test_id => '0123456789abcdef', data => "something" }), + error_bad_params(["/" => "Properties not allowed: data."]), + "Calling job_status with unknown parameters is an error"; +} + +test_validation jsonrpc("job_status", { test_id => '0123456789abcdef' }), + no_error, + "Calling job_status with a good test_id succeeds"; + +test_validation jsonrpc("system_versions"), + no_error, + "Calling system_versions with no parameters is OK"; + +TODO: { + local $TODO = "Not validating parameters for system_versions yet"; + + test_validation jsonrpc("system_versions", undef, 1), + error_bad_params(["/" => "Expected object - got null."]), + "Passing null as parameter to system_versions is an error"; + + test_validation jsonrpc("system_versions", JSON::PP::false), + error_bad_params(["/" => "Expected object - got boolean."]), + "Passing number as parameter to system_versions is an error"; + + test_validation jsonrpc("system_versions", -1), + error_bad_params(["/" => "Expected object - got number."]), + "Passing number as parameter to system_versions is an error"; + + test_validation jsonrpc("system_versions", "hello"), + error_bad_params(["/" => "Expected object - got string."]), + "Passing string as parameter to system_versions is an error"; + + test_validation jsonrpc("system_versions", [qw(a b c)]), + error_bad_params(["/" => "Expected object - got array."]), + "Passing array as parameter to system_versions is an error"; + + test_validation jsonrpc("system_versions", { data => "something" }), + error_bad_params(["/" => "Properties not allowed: data."]), + "Calling system_versions with unrecognized parameter is an error"; +} + +test_validation jsonrpc("system_versions", {}), + no_error, + "Calling system_versions with empty object succeeds"; From 50bef271f3006591132f857b186085902fe3d8c3 Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Thu, 1 Jun 2023 11:27:39 +0200 Subject: [PATCH 02/64] Compile JSON schemas before examining them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Related to #1105. A subtle bug could cause RPCAPI methods, whose parameters were validated against JSON schemas generated with JSON::Validator::Joi, to skip validation of parameters against that schema entirely. The offending code took a $method_schema, which could either be a raw Perl hash containing a JSON schema or an opaque object made with JSON::Validator::Joi, and tried to check whether the schema specified any keys as required. It did so by checking whether the 'required' key exists in $method_schema. This works fine if $method_schema is a Perl hash. But when it’s a JSON::Validator::Joi object, we are touching an implementation detail of that class, which could sooner or later break! And indeed, it broke: between JSON::Validator version 5.02 and 5.03, an implementation detail was changed, causing issue #1105 on systems where JSON::Validator version 5.03 or later is installed. The change seems to have been introduced by commit 788fd42 [1] on JSON::Validator’s Github repo [2]. [1]: https://github.com/jhthorsen/json-validator/commit/788fd4255899339d974d28141a5243f4409067d8 [2]: https://github.com/jhthorsen/json-validator --- lib/Zonemaster/Backend/RPCAPI.pm | 4 ++++ t/rpc_validation.t | 32 ++++++++++++-------------------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/lib/Zonemaster/Backend/RPCAPI.pm b/lib/Zonemaster/Backend/RPCAPI.pm index 7be20ee77..837177810 100644 --- a/lib/Zonemaster/Backend/RPCAPI.pm +++ b/lib/Zonemaster/Backend/RPCAPI.pm @@ -729,6 +729,10 @@ sub jsonrpc_validate { } my $method_schema = $json_schemas{$jsonrpc_request->{method}}; + if (blessed $method_schema) { + $method_schema = $method_schema->compile; + } + # the JSON schema for the method has a 'required' key if ( exists $method_schema->{required} ) { if ( not exists $jsonrpc_request->{params} ) { diff --git a/t/rpc_validation.t b/t/rpc_validation.t index 2bb28fdda..a00e8287c 100644 --- a/t/rpc_validation.t +++ b/t/rpc_validation.t @@ -177,13 +177,9 @@ TODO: { "Sending an object whose ID is an object is an error"; } -TODO: { - local $TODO = 'See #1105'; - - test_validation jsonrpc("job_status"), - error_missing_params(), - "Calling job_status without parameters is an error"; -} +test_validation jsonrpc("job_status"), + error_missing_params(), + "Calling job_status without parameters is an error"; test_validation jsonrpc("job_status", undef, 1), error_bad_params(["/" => "Expected object - got null."]), @@ -209,21 +205,17 @@ TODO: { "Passing array as parameter to job_status is an error"; } -TODO: { - local $TODO = 'See #1105'; +test_validation jsonrpc("job_status", {}), + error_bad_params(["/test_id" => "Missing property"]), + "Passing empty object as parameter to job_status is an error"; - test_validation jsonrpc("job_status", {}), - error_bad_params(["/test_id" => "Missing property"]), - "Passing empty object as parameter to job_status is an error"; +test_validation jsonrpc("job_status", { test_id => 'this_will_definitely_never_ever_exist' }), + error_bad_params(["/test_id" => 'String does not match (?^u:^[0-9a-f]{16}$).']), + "Calling job_status with a bad test_id is an error"; - test_validation jsonrpc("job_status", { test_id => 'this_will_definitely_never_ever_exist' }), - error_bad_params(["/test_id" => 'String does not match (?^u:^[0-9a-f]{16}$).']), - "Calling job_status with a bad test_id is an error"; - - test_validation jsonrpc("job_status", { test_id => '0123456789abcdef', data => "something" }), - error_bad_params(["/" => "Properties not allowed: data."]), - "Calling job_status with unknown parameters is an error"; -} +test_validation jsonrpc("job_status", { test_id => '0123456789abcdef', data => "something" }), + error_bad_params(["/" => "Properties not allowed: data."]), + "Calling job_status with unknown parameters is an error"; test_validation jsonrpc("job_status", { test_id => '0123456789abcdef' }), no_error, From c94896ec2170db1b3a035fb50c116051de7a6e9e Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Thu, 1 Jun 2023 11:54:17 +0200 Subject: [PATCH 03/64] RPCAPI: check type before dereferencing hashref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling the RPCAPI endpoint with a number or a string instead of null or an object caused a “500 Internal Server Error” to be returned, instead of a propre RPCAPI error. This was due to the offending code failing to check whether we were passed a hashref before attempting to dereference it and checking the existence of an “id” key in it. --- lib/Zonemaster/Backend/RPCAPI.pm | 6 +++- t/rpc_validation.t | 56 ++++++++++++++------------------ 2 files changed, 29 insertions(+), 33 deletions(-) diff --git a/lib/Zonemaster/Backend/RPCAPI.pm b/lib/Zonemaster/Backend/RPCAPI.pm index 837177810..993b95202 100644 --- a/lib/Zonemaster/Backend/RPCAPI.pm +++ b/lib/Zonemaster/Backend/RPCAPI.pm @@ -677,6 +677,10 @@ sub _get_locale { my ( $self, $params ) = @_; my @error; + if ( ref $params ne 'HASH' ) { + return undef; + } + my $language = $params->{language}; if ( !defined $language ) { return undef; @@ -715,7 +719,7 @@ sub jsonrpc_validate { my ( $self, $jsonrpc_request) = @_; my @error_rpc = $rpc_request->validate($jsonrpc_request); - if (!exists $jsonrpc_request->{id} || @error_rpc) { + if ((ref($jsonrpc_request) eq 'HASH' && !exists $jsonrpc_request->{id}) || @error_rpc) { $self->_set_error_message_locale; return { jsonrpc => '2.0', diff --git a/t/rpc_validation.t b/t/rpc_validation.t index a00e8287c..9d3b4e0cf 100644 --- a/t/rpc_validation.t +++ b/t/rpc_validation.t @@ -129,25 +129,21 @@ test_validation undef, error_bad_jsonrpc('/: Expected object - got null.'), "Sending undef is an error"; -TODO: { - todo_skip "These tests trigger crashes", 4; - - test_validation JSON::PP::false, - error_bad_jsonrpc('/: Expected object - got boolean.'), - "Sending a boolean is an error"; +test_validation JSON::PP::false, + error_bad_jsonrpc('/: Expected object - got boolean.'), + "Sending a boolean is an error"; - test_validation -1, - error_bad_jsonrpc('/: Expected object - got number.'), - "Sending a number is an error"; +test_validation -1, + error_bad_jsonrpc('/: Expected object - got number.'), + "Sending a number is an error"; - test_validation "hello", - error_bad_jsonrpc('/: Expected object - got string.'), - "Sending a string is an error"; +test_validation "hello", + error_bad_jsonrpc('/: Expected object - got string.'), + "Sending a string is an error"; - test_validation [qw(a b c)], - error_bad_jsonrpc('/: Expected object - got array.'), - "Sending an array is an error"; -} +test_validation [qw(a b c)], + error_bad_jsonrpc('/: Expected object - got array.'), + "Sending an array is an error"; test_validation {}, error_bad_jsonrpc('/jsonrpc: Missing property. /method: Missing property.'), @@ -185,25 +181,21 @@ test_validation jsonrpc("job_status", undef, 1), error_bad_params(["/" => "Expected object - got null."]), "Passing null as parameter to job_status is an error"; -TODO: { - todo_skip "These tests trigger crashes", 4; - - test_validation jsonrpc("job_status", JSON::PP::false), - error_bad_params(["/" => "Expected object - got boolean."]), - "Passing boolean as parameter to job_status is an error"; +test_validation jsonrpc("job_status", JSON::PP::false), + error_bad_params(["/" => "Expected object - got boolean."]), + "Passing boolean as parameter to job_status is an error"; - test_validation jsonrpc("job_status", 1), - error_bad_params(["/" => "Expected object - got number."]), - "Passing number as parameter to job_status is an error"; +test_validation jsonrpc("job_status", 1), + error_bad_params(["/" => "Expected object - got number."]), + "Passing number as parameter to job_status is an error"; - test_validation jsonrpc("job_status", "hello"), - error_bad_params(["/" => "Expected object - got string."]), - "Passing string as parameter to job_status is an error"; +test_validation jsonrpc("job_status", "hello"), + error_bad_params(["/" => "Expected object - got string."]), + "Passing string as parameter to job_status is an error"; - test_validation jsonrpc("job_status", [qw(a b c)]), - error_bad_params(["/" => "Expected object - got array."]), - "Passing array as parameter to job_status is an error"; -} +test_validation jsonrpc("job_status", [qw(a b c)]), + error_bad_params(["/" => "Expected object - got array."]), + "Passing array as parameter to job_status is an error"; test_validation jsonrpc("job_status", {}), error_bad_params(["/test_id" => "Missing property"]), From 8e8ba5ce032cc05e732daa8d66014755ce185f28 Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Mon, 5 Jun 2023 11:17:41 +0200 Subject: [PATCH 04/64] RPCAPI: enforce permissible types on id According to the specification, IDs must be strings, numbers (although numbers with fractional parts are discouraged) or null (also discouraged, but permissible). We might as well enforce it. --- lib/Zonemaster/Backend/RPCAPI.pm | 3 ++- t/rpc_validation.t | 22 +++++++++------------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/Zonemaster/Backend/RPCAPI.pm b/lib/Zonemaster/Backend/RPCAPI.pm index 993b95202..a5f24190e 100644 --- a/lib/Zonemaster/Backend/RPCAPI.pm +++ b/lib/Zonemaster/Backend/RPCAPI.pm @@ -714,7 +714,8 @@ sub _set_error_message_locale { my $rpc_request = joi->object->props( jsonrpc => joi->string->required, - method => $zm_validator->jsonrpc_method()->required); + method => $zm_validator->jsonrpc_method()->required, + id => joi->type([qw(null number string)])); sub jsonrpc_validate { my ( $self, $jsonrpc_request) = @_; diff --git a/t/rpc_validation.t b/t/rpc_validation.t index 9d3b4e0cf..7ea80d609 100644 --- a/t/rpc_validation.t +++ b/t/rpc_validation.t @@ -157,21 +157,17 @@ test_validation { jsonrpc => '2.0', method => 'system_versions' }, error_bad_jsonrpc(''), "Sending an object with no ID is an error"; -TODO: { - local $TODO = "Code does not enforce type of ID yet"; - - test_validation { jsonrpc => '2.0', method => 'system_versions', id => JSON::PP::false }, - error_bad_jsonrpc(''), - "Sending an object whose ID is a boolean is an error"; +test_validation { jsonrpc => '2.0', method => 'system_versions', id => JSON::PP::false }, + error_bad_jsonrpc('/id: Expected null/number/string - got boolean.'), + "Sending an object whose ID is a boolean is an error"; - test_validation { jsonrpc => '2.0', method => 'system_versions', id => [qw(a b c)] }, - error_bad_jsonrpc(''), - "Sending an object whose ID is an array is an error"; +test_validation { jsonrpc => '2.0', method => 'system_versions', id => [qw(a b c)] }, + error_bad_jsonrpc('/id: Expected null/number/string - got array.'), + "Sending an object whose ID is an array is an error"; - test_validation { jsonrpc => '2.0', method => 'system_versions', id => { a => 1 } }, - error_bad_jsonrpc(''), - "Sending an object whose ID is an object is an error"; -} +test_validation { jsonrpc => '2.0', method => 'system_versions', id => { a => 1 } }, + error_bad_jsonrpc('/id: Expected null/number/string - got object.'), + "Sending an object whose ID is an object is an error"; test_validation jsonrpc("job_status"), error_missing_params(), From 4654318e022339e9ecdfa2fa86d34b2b51c76ce5 Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Mon, 5 Jun 2023 11:53:43 +0200 Subject: [PATCH 05/64] RPCAPI: fix logic error in JSON schema enforcement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was another situation where JSON schema enforcement could be skipped: when a method only has optional parameters and the “params” key in the JSONRPC object was set to something, the value of that key was not validated. The offending code skipped validation altogether if none of the method’s parameters is required. What was probably meant is that if none of the method’s parameters are required, then the absence of a “params” key in the JSONRPC object is acceptable. But a method requiring no parameters doesn’t mean that it has no parameters at all (e.g. when a method has default values for all its parameters), so validation should still be done on a “params” key if it’s there. The rewritten code implements a different, but more correct logic: if the “params” key exists, validate its value and return an error if it fails validation; if the “params” key does not exist and the method requires at least one parameter, then return an error; else return success. --- lib/Zonemaster/Backend/RPCAPI.pm | 27 +++++++++++---------- t/rpc_validation.t | 40 ++++++++++++++------------------ 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/lib/Zonemaster/Backend/RPCAPI.pm b/lib/Zonemaster/Backend/RPCAPI.pm index a5f24190e..65ff3ffee 100644 --- a/lib/Zonemaster/Backend/RPCAPI.pm +++ b/lib/Zonemaster/Backend/RPCAPI.pm @@ -738,18 +738,21 @@ sub jsonrpc_validate { $method_schema = $method_schema->compile; } - # the JSON schema for the method has a 'required' key - if ( exists $method_schema->{required} ) { - if ( not exists $jsonrpc_request->{params} ) { - return { - jsonrpc => '2.0', - id => $jsonrpc_request->{id}, - error => { - code => '-32602', - message => "Missing 'params' object", - } - }; - } + # The "params" key of the JSONRPC object is optional per the JSONRPC 2.0 + # specification, but if the method being called requires at least one + # parameter, omitting it is an error. + + if ( exists $method_schema->{required} and not exists $jsonrpc_request->{params} ) { + return { + jsonrpc => '2.0', + id => $jsonrpc_request->{id}, + error => { + code => '-32602', + message => "Missing 'params' object", + } + }; + } + elsif ( exists $jsonrpc_request->{params} ) { my @error_response = $self->validate_params($method_schema, $jsonrpc_request->{params}); if ( scalar @error_response ) { diff --git a/t/rpc_validation.t b/t/rpc_validation.t index 7ea80d609..cd90d61cd 100644 --- a/t/rpc_validation.t +++ b/t/rpc_validation.t @@ -213,33 +213,29 @@ test_validation jsonrpc("system_versions"), no_error, "Calling system_versions with no parameters is OK"; -TODO: { - local $TODO = "Not validating parameters for system_versions yet"; - - test_validation jsonrpc("system_versions", undef, 1), - error_bad_params(["/" => "Expected object - got null."]), - "Passing null as parameter to system_versions is an error"; +test_validation jsonrpc("system_versions", undef, 1), + error_bad_params(["/" => "Expected object - got null."]), + "Passing null as parameter to system_versions is an error"; - test_validation jsonrpc("system_versions", JSON::PP::false), - error_bad_params(["/" => "Expected object - got boolean."]), - "Passing number as parameter to system_versions is an error"; +test_validation jsonrpc("system_versions", JSON::PP::false), + error_bad_params(["/" => "Expected object - got boolean."]), + "Passing number as parameter to system_versions is an error"; - test_validation jsonrpc("system_versions", -1), - error_bad_params(["/" => "Expected object - got number."]), - "Passing number as parameter to system_versions is an error"; +test_validation jsonrpc("system_versions", -1), + error_bad_params(["/" => "Expected object - got number."]), + "Passing number as parameter to system_versions is an error"; - test_validation jsonrpc("system_versions", "hello"), - error_bad_params(["/" => "Expected object - got string."]), - "Passing string as parameter to system_versions is an error"; +test_validation jsonrpc("system_versions", "hello"), + error_bad_params(["/" => "Expected object - got string."]), + "Passing string as parameter to system_versions is an error"; - test_validation jsonrpc("system_versions", [qw(a b c)]), - error_bad_params(["/" => "Expected object - got array."]), - "Passing array as parameter to system_versions is an error"; +test_validation jsonrpc("system_versions", [qw(a b c)]), + error_bad_params(["/" => "Expected object - got array."]), + "Passing array as parameter to system_versions is an error"; - test_validation jsonrpc("system_versions", { data => "something" }), - error_bad_params(["/" => "Properties not allowed: data."]), - "Calling system_versions with unrecognized parameter is an error"; -} +test_validation jsonrpc("system_versions", { data => "something" }), + error_bad_params(["/" => "Properties not allowed: data."]), + "Calling system_versions with unrecognized parameter is an error"; test_validation jsonrpc("system_versions", {}), no_error, From 8da793d0d39d0fca8f2f5038068efc12027ea5d9 Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Mon, 5 Jun 2023 12:01:41 +0200 Subject: [PATCH 06/64] Whitespace changes --- lib/Zonemaster/Backend/RPCAPI.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Zonemaster/Backend/RPCAPI.pm b/lib/Zonemaster/Backend/RPCAPI.pm index 65ff3ffee..91f94a713 100644 --- a/lib/Zonemaster/Backend/RPCAPI.pm +++ b/lib/Zonemaster/Backend/RPCAPI.pm @@ -717,7 +717,7 @@ my $rpc_request = joi->object->props( method => $zm_validator->jsonrpc_method()->required, id => joi->type([qw(null number string)])); sub jsonrpc_validate { - my ( $self, $jsonrpc_request) = @_; + my ( $self, $jsonrpc_request ) = @_; my @error_rpc = $rpc_request->validate($jsonrpc_request); if ((ref($jsonrpc_request) eq 'HASH' && !exists $jsonrpc_request->{id}) || @error_rpc) { From dc279ed03659601133e9d0c33427400ea10bceb9 Mon Sep 17 00:00:00 2001 From: Mats Dufberg Date: Wed, 12 Jul 2023 23:03:51 +0200 Subject: [PATCH 07/64] Removes restriction on multiple active batches --- lib/Zonemaster/Backend/DB.pm | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 9bd560b30..42a727cf2 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -398,23 +398,6 @@ sub create_new_batch_job { my ( $self, $username ) = @_; my $dbh = $self->dbh; - my ( $batch_id, $created_at ) = $dbh->selectrow_array( " - SELECT - batch_id, - batch_jobs.created_at AS batch_created_at - FROM - test_results - JOIN batch_jobs - ON batch_id = batch_jobs.id - AND username = ? - WHERE - test_results.progress <> 100 - LIMIT 1 - ", undef, $username ); - - die Zonemaster::Backend::Error::Conflict->new( message => 'Batch job still running', data => { batch_id => $batch_id, created_at => $created_at } ) - if ( $batch_id ); - $dbh->do( q[ INSERT INTO batch_jobs (username, created_at) VALUES (?,?) ], undef, $username, From 4460229a6d8eb0a78d4d95ed2618c37d2e2e2801 Mon Sep 17 00:00:00 2001 From: Mats Dufberg Date: Wed, 12 Jul 2023 23:35:30 +0200 Subject: [PATCH 08/64] Adjusts unit test to new code --- t/batches.t | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/t/batches.t b/t/batches.t index 701e6ea25..e14cdaedb 100644 --- a/t/batches.t +++ b/t/batches.t @@ -256,19 +256,18 @@ subtest 'batch job still running' => sub { is( $batch_id, 1, 'correct batch job id returned' ); - dies_ok { - my $new_batch_id = $rpcapi->add_batch_job( + + subtest 'a batch is already running for the user, new batch creation should not fail' => sub { + my $batch_id = $rpcapi->add_batch_job( { %$user, domains => \@domains, test_params => $params } ); - } 'a batch is already running for the user, new batch creation should fail' ; - my $res = $@; - is( $res->{message}, 'Batch job still running', 'correct error message' ); - is( $res->{data}->{batch_id}, $batch_id, 'error returned current running batch id' ); - ok( $res->{data}->{created_at}, 'error data contains batch creation time' ); + + is( $batch_id, 2, 'same user can create another batch' ); + }; subtest 'use another user' => sub { my $another_user = { username => 'another', api_key => 'token' }; @@ -281,7 +280,7 @@ subtest 'batch job still running' => sub { } ); - is( $batch_id, 2, 'another_user can create another batch' ); + is( $batch_id, 3, 'another_user can create another batch' ); }; }; From b1a6d56311b732a94a4cdc86a02abbe757478e95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Tue, 18 Jul 2023 01:02:42 +0200 Subject: [PATCH 09/64] Diagnostics: Better feedback when test crashes halfway through --- t/lifecycle.t | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/t/lifecycle.t b/t/lifecycle.t index 6eaf2eb92..3d22b9fc8 100644 --- a/t/lifecycle.t +++ b/t/lifecycle.t @@ -77,19 +77,20 @@ subtest 'Everything but Test::NoWarnings' => sub { subtest 'Testid reuse' => sub { my $testid1 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); + is ref $testid1, '', 'create_new_test returns "testid" scalar'; + advance_time( 11 ); my $testid2 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); + is $testid2, $testid1, 'reuse is determined from start time (as opposed to creation time)'; $db->test_progress( $testid1, 1 ); # mark test as started advance_time( 10 ); my $testid3 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); + is $testid3, $testid1, 'old testid is reused before it expires'; + advance_time( 1 ); my $testid4 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); - - is ref $testid1, '', 'start_domain_test returns "testid" scalar'; - is $testid2, $testid1, 'reuse is determined from start time (as opposed to creation time)'; - is $testid3, $testid1, 'old testid is reused before it expires'; isnt $testid4, $testid1, 'a new testid is generated after the old one expires'; }; From 64838daf22675d13def203a5a81b146ec7e705a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Tue, 18 Jul 2023 17:08:07 +0200 Subject: [PATCH 10/64] New: test_state() The immediate use case is to unit testing. --- lib/Zonemaster/Backend/DB.pm | 53 +++++++++++++++++++++++++++++++++ t/lifecycle.t | 57 ++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 9bd560b30..feb799915 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -9,9 +9,11 @@ use 5.14.2; use DBI qw(:sql_types); use Digest::MD5 qw(md5_hex); use Encode; +use Exporter qw( import ); use JSON::PP; use Log::Any qw( $log ); use POSIX qw( strftime ); +use Readonly; use Try::Tiny; use Zonemaster::Backend::Errors; @@ -50,6 +52,21 @@ has 'dbhandle' => ( required => 1, ); +Readonly our $TEST_WAITING => 'waiting'; +Readonly our $TEST_RUNNING => 'running'; +Readonly our $TEST_COMPLETED => 'completed'; +Readonly our $TEST_CRASHED => 'crashed'; +Readonly our $TEST_LAPSED => 'lapsed'; + +our @EXPORT_OK = qw( + $TEST_WAITING + $TEST_RUNNING + $TEST_COMPLETED + $TEST_CRASHED + $TEST_LAPSED +); + + =head2 get_db_class Get the database adapter class for the given database type. @@ -263,6 +280,42 @@ sub test_progress { return $result; } +sub test_state { + my ( $self, $test_id ) = @_; + + my ( $progress, $results ) = $self->dbh->selectrow_array( + q[ + SELECT progress, results + FROM test_results + WHERE hash_id = ? + ], + undef, + $test_id, + ); + if ( !defined $progress ) { + die Zonemaster::Backend::Error::Internal->new( reason => 'job not found' ); + } + + if ( $progress == 0 ) { + return $TEST_WAITING; + } + elsif ( $progress < 100 ) { + return $TEST_RUNNING; + } + elsif ( $progress == 100 ) { + if ( $results =~ /"tag":"TEST_DIED"/ ) { + return $TEST_CRASHED; + } elsif ( $results =~ /"tag":"UNABLE_TO_FINISH_TEST"/ ) { + return $TEST_LAPSED; + } else { + return $TEST_COMPLETED; + } + } + else { + die Zonemaster::Backend::Error::Internal->new( reason => 'state could not be determined' ); + } +} + sub select_test_results { my ( $self, $test_id ) = @_; diff --git a/t/lifecycle.t b/t/lifecycle.t index 3d22b9fc8..903669761 100644 --- a/t/lifecycle.t +++ b/t/lifecycle.t @@ -15,6 +15,7 @@ BEGIN { *CORE::GLOBAL::time = sub { $TIME }; } +use Data::Dumper; use File::ShareDir qw[dist_file]; use File::Temp qw[tempdir]; @@ -29,6 +30,7 @@ use TestUtil; use Zonemaster::Engine; use Zonemaster::Backend::Config; +use Zonemaster::Backend::DB qw( $TEST_WAITING $TEST_RUNNING $TEST_COMPLETED ); sub advance_time { my ( $delta ) = @_; @@ -75,6 +77,61 @@ subtest 'Everything but Test::NoWarnings' => sub { lives_ok { # Make sure we get to print log messages in case of errors. my $db = TestUtil::init_db( $config ); + subtest 'State transitions' => sub { + my $testid1 = $db->create_new_test( "1.transition.test", {}, 10 ); + is ref $testid1, '', "create_new_test should return 'testid' scalar"; + my $current_state = $db->test_state( $testid1 ); + is $current_state, $TEST_WAITING, "New test starts out in 'waiting' state."; + + my @cases = ( + { + old_state => $TEST_WAITING, + transition => [ 'test_progress', 1 ], + returns => 1, + new_state => $TEST_RUNNING, + }, + { + old_state => $TEST_RUNNING, + transition => [ 'store_results', '{}' ], + returns => 1, + new_state => $TEST_COMPLETED, + }, + ); + + for my $case ( @cases ) { + if ( $case->{old_state} ne $current_state ) { + BAIL_OUT( "Assuming to be in '$case->{old_state}' but we're actually in '$current_state'!" ); + } + + my ( $transition, @args ) = @{ $case->{transition} }; + + if ( exists $case->{returns} ) { + my $rv_string = Data::Dumper->new( [ $case->{returns} ] )->Indent( 0 )->Terse( 1 )->Dump; + + my $result = $db->$transition( $testid1, @args ); + is $result, + $case->{returns}, + "In state '$case->{old_state}' transition '$transition' should return $rv_string,"; + + if ( $case->{new_state} ) { + $current_state = $db->test_state( $testid1 ); + is $current_state, + $case->{new_state}, + "and it should move the test to '$case->{new_state}' state."; + } + else { + $current_state = $db->test_state( $testid1 ); + is $current_state, + $case->{old_state}, + "and it should not affect the actual state."; + } + } + else { + BAIL_OUT( "Invalid case specification!" ); + } + } + }; + subtest 'Testid reuse' => sub { my $testid1 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); is ref $testid1, '', 'create_new_test returns "testid" scalar'; From d62e4de5e62bc925182363b699b005daeb4a8d53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Tue, 18 Jul 2023 23:20:43 +0200 Subject: [PATCH 11/64] Maintenance: Touch up existing functionality * Add unit test for test progress * Add unit test for claiming of waiting tests * Add documentation * Various tiny refactors --- lib/Zonemaster/Backend/DB.pm | 86 ++++++++++++++++++++++++++++++------ t/lifecycle.t | 12 +++++ t/queue.t | 86 ++++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 13 deletions(-) create mode 100644 t/queue.t diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index feb799915..8cb321f3d 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -226,25 +226,35 @@ sub recent_test_hash_id { return $recent_hash_id; } -=head2 test_progress($test_id, $progress) +=head2 test_progress( $test_id, $progress ) Takes a test_id and returns the current progress value for the associated test. If progress is set, update the progress value of the test. If the progress value is 1, set the C field to the current time in UTC. -The C<$progress> value is an integer in the range 1-99. +If defined, C<$progress> is clamped to 1-99 inclusive. + +Dies when: + +=over 2 + +=item + +an error occurs in the database interface + +=back =cut sub test_progress { my ( $self, $test_id, $progress ) = @_; - my $dbh = $self->dbh; if ( $progress ) { $progress = $progress < 1 ? 1 : $progress > 99 ? 99 : $progress; + if ( $progress == 1 ) { - $dbh->do( + $self->dbh->do( q[ UPDATE test_results SET progress = ?, @@ -259,7 +269,7 @@ sub test_progress { ); } else { - $dbh->do( + $self->dbh->do( q[ UPDATE test_results SET progress = ? @@ -275,7 +285,15 @@ sub test_progress { return $progress; } - my ( $result ) = $self->dbh->selectrow_array( "SELECT progress FROM test_results WHERE hash_id=?", undef, $test_id ); + my ( $result ) = $self->dbh->selectrow_array( + q[ + SELECT progress + FROM test_results + WHERE hash_id = ? + ], + undef, + $test_id, + ); return $result; } @@ -505,24 +523,66 @@ sub batch_exists_in_db { return $id; } +=head2 get_test_request( $queue_label ) + +Find a waiting test and claim it for processing. + +If $queue_label is defined it must be an integer. +If defined, only tests in the associated queue are considered. +Otherwise tests from all queues are considered. + +Only tests in the "waiting" state are considered. +When a test is claimed it is removed from the queue and it transitions to the +"running" state. + +Dies when an error occurs in the database interface. + +=cut + sub get_test_request { my ( $self, $queue_label ) = @_; - my $result_id; - my $dbh = $self->dbh; - + # Identify a candidate for allocation ... my ( $hash_id, $batch_id ); if ( defined $queue_label ) { - ( $hash_id, $batch_id ) = $dbh->selectrow_array( qq[ SELECT hash_id, batch_id FROM test_results WHERE progress=0 AND queue=? ORDER BY priority DESC, id ASC LIMIT 1 ], undef, $queue_label ); + ( $hash_id, $batch_id ) = $self->dbh->selectrow_array( + q[ + SELECT hash_id, + batch_id + FROM test_results + WHERE progress = 0 + AND queue = ? + ORDER BY priority DESC, + id ASC + LIMIT 1 + ], + undef, + $queue_label, + ); } else { - ( $hash_id, $batch_id ) = $dbh->selectrow_array( q[ SELECT hash_id, batch_id FROM test_results WHERE progress=0 ORDER BY priority DESC, id ASC LIMIT 1 ] ); + ( $hash_id, $batch_id ) = $self->dbh->selectrow_array( + q[ + SELECT hash_id, + batch_id + FROM test_results + WHERE progress = 0 + ORDER BY priority DESC, + id ASC + LIMIT 1 + ], + ); } - if ( $hash_id ) { + if ( defined $hash_id ) { + + # ... and race to be the first to claim it ... $self->test_progress( $hash_id, 1 ); + + return ( $hash_id, $batch_id ); } - return ( $hash_id, $batch_id ); + + return ( undef, undef ); } sub get_test_params { diff --git a/t/lifecycle.t b/t/lifecycle.t index 903669761..fa59fdfd6 100644 --- a/t/lifecycle.t +++ b/t/lifecycle.t @@ -132,6 +132,18 @@ subtest 'Everything but Test::NoWarnings' => sub { } }; + subtest 'Progress' => sub { + my $testid1 = $db->create_new_test( "1.progress.test", {}, 10 ); + is ref $testid1, '', "create_new_test should return 'testid' scalar"; + + is $db->test_progress( $testid1, 2 ), 2, "Setting a higher progress should be allowed,"; + is $db->test_progress( $testid1 ), 2, "and it should persist at the new value."; + is $db->test_progress( $testid1, 2 ), 2, "Setting the same progress again should succeed."; + + is $db->test_progress( $testid1, 100 ), 99, "Setting progress to 100 should succeed, but actual clamped value is returned,"; + is $db->test_progress( $testid1 ), 99, "and it should persist at the clamped value."; + }; + subtest 'Testid reuse' => sub { my $testid1 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); is ref $testid1, '', 'create_new_test returns "testid" scalar'; diff --git a/t/queue.t b/t/queue.t new file mode 100644 index 000000000..ff7354d27 --- /dev/null +++ b/t/queue.t @@ -0,0 +1,86 @@ +use strict; +use warnings; +use 5.14.2; + +use Test::More tests => 2; +use Test::NoWarnings; +use Log::Any::Test; + +use File::Basename qw( dirname ); +use File::Spec::Functions qw( rel2abs ); +use File::Temp qw( tempdir ); +use Log::Any qw( $log ); +use Test::Differences; +use Test::Exception; +use Zonemaster::Backend::Config; +use Zonemaster::Backend::DB qw( $TEST_RUNNING ); +use Zonemaster::Engine; + +my $t_path; +BEGIN { + $t_path = dirname( rel2abs( $0 ) ); +} +use lib $t_path; +use TestUtil; + +my $db_backend = TestUtil::db_backend(); +my $tempdir = tempdir( CLEANUP => 1 ); +my $config = Zonemaster::Backend::Config->parse( < sub { + lives_ok { # Make sure we get to print log messages in case of errors. + my $db = TestUtil::init_db( $config ); + + subtest 'Claiming waiting tests for processing' => sub { + eq_or_diff + [ $db->get_test_request( undef ) ], + [ undef, undef ], + "An empty list is returned when queue is empty"; + + my $testid1 = $db->create_new_test( "1.claim.test", {}, 10 ); + eq_or_diff + [ $db->get_test_request( undef ) ], + [ $testid1, undef ], + "A waiting test is returned if one is available"; + eq_or_diff + [ $db->get_test_request( undef ) ], + [ undef, undef ], + "Claimed test is removed from queue"; + is + $db->test_state( $testid1 ), + $TEST_RUNNING, + "Claimed test is in 'running' state"; + }; + }; +}; + +for my $msg ( @{ $log->msgs } ) { + my $text = sprintf( "%s: %s", $msg->{level}, $msg->{message} ); + if ( $msg->{level} =~ /trace|debug|info|notice/ ) { + note $text; + } + else { + diag $text; + } +} From 534da0c186c860a168bf8440528436b41017e7b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Wed, 19 Jul 2023 01:16:37 +0200 Subject: [PATCH 12/64] Refactor: Dedicated method for transition to "running" --- lib/Zonemaster/Backend/DB.pm | 70 ++++++++++++++++++++---------------- t/idn.t | 2 ++ t/lifecycle.t | 15 +++++--- t/test01.t | 2 ++ 4 files changed, 54 insertions(+), 35 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 8cb321f3d..bdbdf8be0 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -230,7 +230,6 @@ sub recent_test_hash_id { Takes a test_id and returns the current progress value for the associated test. If progress is set, update the progress value of the test. -If the progress value is 1, set the C field to the current time in UTC. If defined, C<$progress> is clamped to 1-99 inclusive. @@ -252,35 +251,17 @@ sub test_progress { if ( $progress ) { $progress = $progress < 1 ? 1 : $progress > 99 ? 99 : $progress; - - if ( $progress == 1 ) { - $self->dbh->do( - q[ - UPDATE test_results - SET progress = ?, - started_at = ? - WHERE hash_id = ? - AND progress <> 100 - ], - undef, - $progress, - $self->format_time( time() ), - $test_id, - ); - } - else { - $self->dbh->do( - q[ - UPDATE test_results - SET progress = ? - WHERE hash_id = ? - AND progress <> 100 - ], - undef, - $progress, - $test_id, - ); - } + $self->dbh->do( + q[ + UPDATE test_results + SET progress = ? + WHERE hash_id = ? + AND progress <> 100 + ], + undef, + $progress, + $test_id, + ); return $progress; } @@ -577,7 +558,7 @@ sub get_test_request { if ( defined $hash_id ) { # ... and race to be the first to claim it ... - $self->test_progress( $hash_id, 1 ); + $self->claim_test( $hash_id ); return ( $hash_id, $batch_id ); } @@ -585,6 +566,33 @@ sub get_test_request { return ( undef, undef ); } +=head2 claim_test( $test_id ) + +Claim a test for processing. + +Dies when an error occurs in the database interface. + +=cut + +sub claim_test { + my ( $self, $test_id ) = @_; + + $self->dbh->do( + q[ + UPDATE test_results + SET progress = 1, + started_at = ? + WHERE hash_id = ? + AND progress <> 100 + ], + undef, + $self->format_time( time() ), + $test_id, + ); + + return; +} + sub get_test_params { my ( $self, $test_id ) = @_; diff --git a/t/idn.t b/t/idn.t index eaf35359a..c7e9d4b89 100644 --- a/t/idn.t +++ b/t/idn.t @@ -75,6 +75,7 @@ subtest 'test IDN domain' => sub { }; # run the test +$db->claim_test( $test_id ); $agent->run( $test_id ); # blocking call subtest 'test get_test_results' => sub { @@ -95,6 +96,7 @@ subtest 'test IDN nameserver' => sub { }; # run the test + $db->claim_test( $test_id ); $agent->run( $test_id ); # blocking call subtest 'test_results' => sub { diff --git a/t/lifecycle.t b/t/lifecycle.t index fa59fdfd6..d124b876e 100644 --- a/t/lifecycle.t +++ b/t/lifecycle.t @@ -136,6 +136,13 @@ subtest 'Everything but Test::NoWarnings' => sub { my $testid1 = $db->create_new_test( "1.progress.test", {}, 10 ); is ref $testid1, '', "create_new_test should return 'testid' scalar"; + $db->claim_test( $testid1 ); + + # Logically progress is 0 entering the 'running' state, but because + # of implementation details we're clamping it to the range 1-99 + # inclusive. + is $db->test_progress( $testid1 ), 1, "Progress should be 1 entering the 'running' state."; + is $db->test_progress( $testid1, 2 ), 2, "Setting a higher progress should be allowed,"; is $db->test_progress( $testid1 ), 2, "and it should persist at the new value."; is $db->test_progress( $testid1, 2 ), 2, "Setting the same progress again should succeed."; @@ -152,7 +159,7 @@ subtest 'Everything but Test::NoWarnings' => sub { my $testid2 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); is $testid2, $testid1, 'reuse is determined from start time (as opposed to creation time)'; - $db->test_progress( $testid1, 1 ); # mark test as started + $db->claim_test( $testid1 ); advance_time( 10 ); my $testid3 = $db->create_new_test( "zone1.rpcapi.example", {}, 10 ); @@ -168,9 +175,9 @@ subtest 'Everything but Test::NoWarnings' => sub { my $testid3 = $db->create_new_test( "zone3.rpcapi.example", {}, 10 ); # testid2 started 11 seconds ago, testid3 started 10 seconds ago - $db->test_progress( $testid2, 1 ); + $db->claim_test( $testid2 ); advance_time( 1 ); - $db->test_progress( $testid3, 1 ); + $db->claim_test( $testid3 ); advance_time( 10 ); $db->process_unfinished_tests( undef, 10 ); @@ -184,7 +191,7 @@ subtest 'Everything but Test::NoWarnings' => sub { subtest 'Termination of crashed tests' => sub { my $testid4 = $db->create_new_test( "zone4.rpcapi.example", {}, 10 ); - $db->test_progress( $testid4, 1 ); # mark test as started + $db->claim_test( $testid4 ); $db->process_dead_test( $testid4 ); diff --git a/t/test01.t b/t/test01.t index cf3692cf5..488948dc2 100644 --- a/t/test01.t +++ b/t/test01.t @@ -210,6 +210,7 @@ subtest 'API calls' => sub { # start a second test with IPv6 disabled $params->{ipv6} = 0; $hash_id = $rpcapi->start_domain_test( $params ); +$rpcapi->{db}->claim_test( $hash_id ); diag "running the agent on test $hash_id"; $agent->run($hash_id); @@ -338,6 +339,7 @@ subtest 'check historic tests' => sub { foreach my $param ($params_un1, $params_un2, $params_dn1) { my $testid = $rpcapi->start_domain_test( $param ); ok( $testid, "API start_domain_test ID OK" ); + $rpcapi->{db}->claim_test( $testid ); diag "running the agent on test $testid"; $agent->run( $testid ); is( $rpcapi->test_progress( { test_id => $testid } ), 100 , 'API test_progress -> Test finished' ); From 512c8293e25a7edf0c849435ba3300ecb6072769 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Wed, 19 Jul 2023 00:56:32 +0200 Subject: [PATCH 13/64] Fix: Make some functions stricter * test_progress() * store_results() --- lib/Zonemaster/Backend/DB.pm | 49 ++++++++++++++++++++++++++++++------ t/lifecycle.t | 37 ++++++++++++++++++++++++--- 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index bdbdf8be0..07cf6faa6 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -228,8 +228,10 @@ sub recent_test_hash_id { =head2 test_progress( $test_id, $progress ) -Takes a test_id and returns the current progress value for the associated test. -If progress is set, update the progress value of the test. +Get/set the progress value of the test associated with C<$test_id>. + +The given C<$progress> must be either C (when getting) or an number in +the range 0-100 inclusive (when setting). If defined, C<$progress> is clamped to 1-99 inclusive. @@ -239,6 +241,18 @@ Dies when: =item +attempting to access a test that does not exist + +=item + +attempting to update a test that is in a state other than "running" + +=item + +attempting to set a progress value that is lower than the current one + +=item + an error occurs in the database interface =back @@ -249,19 +263,30 @@ sub test_progress { my ( $self, $test_id, $progress ) = @_; if ( $progress ) { - $progress = $progress < 1 ? 1 : - $progress > 99 ? 99 : $progress; - $self->dbh->do( + if ( $progress < 0 || 100 < $progress ) { + die Zonemaster::Backend::Error::Internal->new( reason => "progress out of range" ); + } elsif ( $progress < 1 ) { + $progress = 1; + } elsif ( 99 < $progress ) { + $progress = 99; + } + + my $rows_affected = $self->dbh->do( q[ UPDATE test_results SET progress = ? WHERE hash_id = ? - AND progress <> 100 + AND 1 <= progress + AND progress <= ? ], undef, $progress, $test_id, + $progress, ); + if ( $rows_affected == 0 ) { + die Zonemaster::Backend::Error::Internal->new( reason => 'job not found or illegal update' ); + } return $progress; } @@ -275,6 +300,9 @@ sub test_progress { undef, $test_id, ); + if ( !defined $result ) { + die Zonemaster::Backend::Error::Internal->new( reason => 'job not found' ); + } return $result; } @@ -347,13 +375,14 @@ sub select_test_results { sub store_results { my ( $self, $test_id, $new_results ) = @_; - $self->dbh->do( + my $rows_affected = $self->dbh->do( q[ UPDATE test_results SET progress = 100, ended_at = ?, results = ? WHERE hash_id = ? + AND 0 < progress AND progress < 100 ], undef, @@ -361,6 +390,12 @@ sub store_results { $new_results, $test_id, ); + + if ( $rows_affected == 0 ) { + die Zonemaster::Backend::Error::Internal->new( reason => "job not found or illegal transition" ); + } + + return; } sub test_results { diff --git a/t/lifecycle.t b/t/lifecycle.t index d124b876e..ee1ab1362 100644 --- a/t/lifecycle.t +++ b/t/lifecycle.t @@ -86,16 +86,26 @@ subtest 'Everything but Test::NoWarnings' => sub { my @cases = ( { old_state => $TEST_WAITING, - transition => [ 'test_progress', 1 ], - returns => 1, + transition => [ 'store_results', '{}' ], + throws => qr/illegal transition/, + }, + { + old_state => $TEST_WAITING, + transition => ['claim_test'], + returns => undef, new_state => $TEST_RUNNING, }, { old_state => $TEST_RUNNING, transition => [ 'store_results', '{}' ], - returns => 1, + returns => undef, new_state => $TEST_COMPLETED, }, + { + old_state => $TEST_COMPLETED, + transition => [ 'store_results', '{}' ], + throws => qr/illegal transition/, + }, ); for my $case ( @cases ) { @@ -126,6 +136,17 @@ subtest 'Everything but Test::NoWarnings' => sub { "and it should not affect the actual state."; } } + elsif ( exists $case->{throws} ) { + throws_ok { + $db->$transition( $testid1, @args ) + } + $case->{throws}, "In state '$case->{old_state}' transition '$transition' should throw an exception,"; + + $current_state = $db->test_state( $testid1 ); + is $current_state, + $case->{old_state}, + "and it should not affect the actual state."; + } else { BAIL_OUT( "Invalid case specification!" ); } @@ -136,6 +157,8 @@ subtest 'Everything but Test::NoWarnings' => sub { my $testid1 = $db->create_new_test( "1.progress.test", {}, 10 ); is ref $testid1, '', "create_new_test should return 'testid' scalar"; + throws_ok { $db->test_progress( $testid1, 1 ) } qr/illegal update/, "Setting progress should throw an exception in 'waiting' state."; + $db->claim_test( $testid1 ); # Logically progress is 0 entering the 'running' state, but because @@ -143,12 +166,20 @@ subtest 'Everything but Test::NoWarnings' => sub { # inclusive. is $db->test_progress( $testid1 ), 1, "Progress should be 1 entering the 'running' state."; + is $db->test_progress( $testid1, 0 ), 1, "Setting progress to 0 should succeed, but actual clamped value is returned,"; + is $db->test_progress( $testid1 ), 1, "and it should persist at the clamped value."; + is $db->test_progress( $testid1, 0 ), 1, "Setting the same progress again should succeed."; + is $db->test_progress( $testid1, 2 ), 2, "Setting a higher progress should be allowed,"; is $db->test_progress( $testid1 ), 2, "and it should persist at the new value."; is $db->test_progress( $testid1, 2 ), 2, "Setting the same progress again should succeed."; is $db->test_progress( $testid1, 100 ), 99, "Setting progress to 100 should succeed, but actual clamped value is returned,"; is $db->test_progress( $testid1 ), 99, "and it should persist at the clamped value."; + + $db->store_results( $testid1, '{}' ); + + throws_ok { $db->test_progress( $testid1, 100 ) } qr/illegal update/, "Setting progress should throw an exception in 'completed' state."; }; subtest 'Testid reuse' => sub { From 40f2cdba365c920710d7a5ad0465944c8f05c09d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Wed, 19 Jul 2023 15:40:03 +0200 Subject: [PATCH 14/64] Fix: Make test_progress(0) mean set progress to 0 --- lib/Zonemaster/Backend/DB.pm | 2 +- t/lifecycle.t | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 07cf6faa6..18c8f35b1 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -262,7 +262,7 @@ an error occurs in the database interface sub test_progress { my ( $self, $test_id, $progress ) = @_; - if ( $progress ) { + if ( defined $progress ) { if ( $progress < 0 || 100 < $progress ) { die Zonemaster::Backend::Error::Internal->new( reason => "progress out of range" ); } elsif ( $progress < 1 ) { diff --git a/t/lifecycle.t b/t/lifecycle.t index ee1ab1362..ff0440e79 100644 --- a/t/lifecycle.t +++ b/t/lifecycle.t @@ -174,6 +174,9 @@ subtest 'Everything but Test::NoWarnings' => sub { is $db->test_progress( $testid1 ), 2, "and it should persist at the new value."; is $db->test_progress( $testid1, 2 ), 2, "Setting the same progress again should succeed."; + throws_ok { $db->test_progress( $testid1, 0 ) } qr/illegal update/, "Setting a lower progress should throw an exception,"; + is $db->test_progress( $testid1 ), 2, "and it should persist at the old value."; + is $db->test_progress( $testid1, 100 ), 99, "Setting progress to 100 should succeed, but actual clamped value is returned,"; is $db->test_progress( $testid1 ), 99, "and it should persist at the clamped value."; From 0b27c4c5e5aca18c722ef7ffd910fa52f5b61779 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Wed, 19 Jul 2023 01:16:37 +0200 Subject: [PATCH 15/64] Fix: Make get_test_request() synchronize claims --- lib/Zonemaster/Backend/DB.pm | 21 +++++++++++++++------ t/idn.t | 6 ++++-- t/lifecycle.t | 12 +++++++++++- t/test01.t | 6 ++++-- 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 18c8f35b1..d798f38a7 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -551,6 +551,9 @@ Only tests in the "waiting" state are considered. When a test is claimed it is removed from the queue and it transitions to the "running" state. +It is safe for multiple callers running in parallel to allocate tests from the +same queues. + Dies when an error occurs in the database interface. =cut @@ -593,9 +596,9 @@ sub get_test_request { if ( defined $hash_id ) { # ... and race to be the first to claim it ... - $self->claim_test( $hash_id ); - - return ( $hash_id, $batch_id ); + if ( $self->claim_test( $hash_id ) ) { + return ( $hash_id, $batch_id ); + } } return ( undef, undef ); @@ -605,6 +608,12 @@ sub get_test_request { Claim a test for processing. +Transitions a test from the "waiting" state to the "running" state. + +Returns true on successful transition. +Returns false if the given test does not exist or if it is not in the "waiting" +state. + Dies when an error occurs in the database interface. =cut @@ -612,20 +621,20 @@ Dies when an error occurs in the database interface. sub claim_test { my ( $self, $test_id ) = @_; - $self->dbh->do( + my $rows_affected = $self->dbh->do( q[ UPDATE test_results SET progress = 1, started_at = ? WHERE hash_id = ? - AND progress <> 100 + AND progress = 0 ], undef, $self->format_time( time() ), $test_id, ); - return; + return $rows_affected == 1; } sub get_test_params { diff --git a/t/idn.t b/t/idn.t index c7e9d4b89..9f6dbc1fc 100644 --- a/t/idn.t +++ b/t/idn.t @@ -75,7 +75,8 @@ subtest 'test IDN domain' => sub { }; # run the test -$db->claim_test( $test_id ); +$db->claim_test( $test_id ) + or BAIL_OUT( "test needs to be claimed before calling run()" ); $agent->run( $test_id ); # blocking call subtest 'test get_test_results' => sub { @@ -96,7 +97,8 @@ subtest 'test IDN nameserver' => sub { }; # run the test - $db->claim_test( $test_id ); + $db->claim_test( $test_id ) + or BAIL_OUT( "test needs to be claimed before calling run()" ); $agent->run( $test_id ); # blocking call subtest 'test_results' => sub { diff --git a/t/lifecycle.t b/t/lifecycle.t index ff0440e79..6655ac2d2 100644 --- a/t/lifecycle.t +++ b/t/lifecycle.t @@ -92,15 +92,25 @@ subtest 'Everything but Test::NoWarnings' => sub { { old_state => $TEST_WAITING, transition => ['claim_test'], - returns => undef, + returns => 1, # true new_state => $TEST_RUNNING, }, + { + old_state => $TEST_RUNNING, + transition => ['claim_test'], + returns => '', # false + }, { old_state => $TEST_RUNNING, transition => [ 'store_results', '{}' ], returns => undef, new_state => $TEST_COMPLETED, }, + { + old_state => $TEST_COMPLETED, + transition => ['claim_test'], + returns => '', #false + }, { old_state => $TEST_COMPLETED, transition => [ 'store_results', '{}' ], diff --git a/t/test01.t b/t/test01.t index 488948dc2..7a5118f56 100644 --- a/t/test01.t +++ b/t/test01.t @@ -210,7 +210,8 @@ subtest 'API calls' => sub { # start a second test with IPv6 disabled $params->{ipv6} = 0; $hash_id = $rpcapi->start_domain_test( $params ); -$rpcapi->{db}->claim_test( $hash_id ); +$rpcapi->{db}->claim_test( $hash_id ) + or BAIL_OUT( "test needs to be claimed before calling run()" ); diag "running the agent on test $hash_id"; $agent->run($hash_id); @@ -339,7 +340,8 @@ subtest 'check historic tests' => sub { foreach my $param ($params_un1, $params_un2, $params_dn1) { my $testid = $rpcapi->start_domain_test( $param ); ok( $testid, "API start_domain_test ID OK" ); - $rpcapi->{db}->claim_test( $testid ); + $rpcapi->{db}->claim_test( $testid ) + or BAIL_OUT( "test needs to be claimed before calling run()" ); diag "running the agent on test $testid"; $agent->run( $testid ); is( $rpcapi->test_progress( { test_id => $testid } ), 100 , 'API test_progress -> Test finished' ); From 472b03043a953829cc6c8a674710ce4d77dfcda0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Wed, 19 Jul 2023 15:49:51 +0200 Subject: [PATCH 16/64] Fix: Make get_test_request() try again Removes one of its failure modes. --- lib/Zonemaster/Backend/DB.pm | 82 ++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 37 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index d798f38a7..58a9ff1a9 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -547,6 +547,9 @@ If $queue_label is defined it must be an integer. If defined, only tests in the associated queue are considered. Otherwise tests from all queues are considered. +Returns the test id and the batch id of the claimed test. +If there are no waiting tests to claim, C is returned for both ids. + Only tests in the "waiting" state are considered. When a test is claimed it is removed from the queue and it transitions to the "running" state. @@ -561,47 +564,52 @@ Dies when an error occurs in the database interface. sub get_test_request { my ( $self, $queue_label ) = @_; - # Identify a candidate for allocation ... - my ( $hash_id, $batch_id ); - if ( defined $queue_label ) { - ( $hash_id, $batch_id ) = $self->dbh->selectrow_array( - q[ - SELECT hash_id, - batch_id - FROM test_results - WHERE progress = 0 - AND queue = ? - ORDER BY priority DESC, - id ASC - LIMIT 1 - ], - undef, - $queue_label, - ); - } - else { - ( $hash_id, $batch_id ) = $self->dbh->selectrow_array( - q[ - SELECT hash_id, - batch_id - FROM test_results - WHERE progress = 0 - ORDER BY priority DESC, - id ASC - LIMIT 1 - ], - ); - } + while ( 1 ) { + + # Identify a candidate for allocation ... + my ( $hash_id, $batch_id ); + if ( defined $queue_label ) { + ( $hash_id, $batch_id ) = $self->dbh->selectrow_array( + q[ + SELECT hash_id, + batch_id + FROM test_results + WHERE progress = 0 + AND queue = ? + ORDER BY priority DESC, + id ASC + LIMIT 1 + ], + undef, + $queue_label, + ); + } + else { + ( $hash_id, $batch_id ) = $self->dbh->selectrow_array( + q[ + SELECT hash_id, + batch_id + FROM test_results + WHERE progress = 0 + ORDER BY priority DESC, + id ASC + LIMIT 1 + ], + ); + } - if ( defined $hash_id ) { + if ( defined $hash_id ) { - # ... and race to be the first to claim it ... - if ( $self->claim_test( $hash_id ) ) { - return ( $hash_id, $batch_id ); + # ... and race to be the first to claim it ... + if ( $self->claim_test( $hash_id ) ) { + return ( $hash_id, $batch_id ); + } + } + else { + # ... or stop trying if there are no candidates. + return ( undef, undef ); } } - - return ( undef, undef ); } =head2 claim_test( $test_id ) From 03fa8e9e89ea72b2734f9fedd5974e60d9567186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Thu, 20 Jul 2023 10:31:30 +0200 Subject: [PATCH 17/64] Documentation for constants --- lib/Zonemaster/Backend/DB.pm | 45 ++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 58a9ff1a9..409d1d33e 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -52,11 +52,48 @@ has 'dbhandle' => ( required => 1, ); -Readonly our $TEST_WAITING => 'waiting'; -Readonly our $TEST_RUNNING => 'running'; +=head2 $TEST_WAITING + +The test is waiting to be processed. + +=cut + +Readonly our $TEST_WAITING => 'waiting'; + +=head2 $TEST_RUNNING + +The test is currently being processed. + +=cut + +Readonly our $TEST_RUNNING => 'running'; + +=head2 $TEST_COMPLETED + +The test was already processed. +The Zonemaster Engine test terminated normally. + +=cut + Readonly our $TEST_COMPLETED => 'completed'; -Readonly our $TEST_CRASHED => 'crashed'; -Readonly our $TEST_LAPSED => 'lapsed'; + +=head2 $TEST_CRASHED + +The test was already processed. +A critical error occurred while processing. + +=cut + +Readonly our $TEST_CRASHED => 'crashed'; + +=head2 $TEST_LAPSED + +The test was already processed. +The processing was cancelled because it took too long. + +=cut + +Readonly our $TEST_LAPSED => 'lapsed'; our @EXPORT_OK = qw( $TEST_WAITING From e6c93789d5a0ed8e1c20bb1cea03f88d4332e0c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Thu, 20 Jul 2023 11:06:49 +0200 Subject: [PATCH 18/64] Remove ugly hack --- lib/Zonemaster/Backend/DB.pm | 41 +++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 409d1d33e..0aac4fd8b 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -58,7 +58,7 @@ The test is waiting to be processed. =cut -Readonly our $TEST_WAITING => 'waiting'; +Readonly our $TEST_WAITING => 'WAITING'; =head2 $TEST_RUNNING @@ -66,21 +66,37 @@ The test is currently being processed. =cut -Readonly our $TEST_RUNNING => 'running'; +Readonly our $TEST_RUNNING => 'RUNNING'; =head2 $TEST_COMPLETED The test was already processed. + +This state encompasses all of the following: + +=over 2 + +=item + The Zonemaster Engine test terminated normally. +=item + +A critical error occurred while processing. + +=item + +The processing was cancelled because it took too long. + +=back + =cut -Readonly our $TEST_COMPLETED => 'completed'; +Readonly our $TEST_COMPLETED => 'COMPLETED'; =head2 $TEST_CRASHED The test was already processed. -A critical error occurred while processing. =cut @@ -89,7 +105,6 @@ Readonly our $TEST_CRASHED => 'crashed'; =head2 $TEST_LAPSED The test was already processed. -The processing was cancelled because it took too long. =cut @@ -99,8 +114,6 @@ our @EXPORT_OK = qw( $TEST_WAITING $TEST_RUNNING $TEST_COMPLETED - $TEST_CRASHED - $TEST_LAPSED ); @@ -347,9 +360,9 @@ sub test_progress { sub test_state { my ( $self, $test_id ) = @_; - my ( $progress, $results ) = $self->dbh->selectrow_array( + my ( $progress ) = $self->dbh->selectrow_array( q[ - SELECT progress, results + SELECT progress FROM test_results WHERE hash_id = ? ], @@ -363,17 +376,11 @@ sub test_state { if ( $progress == 0 ) { return $TEST_WAITING; } - elsif ( $progress < 100 ) { + elsif ( 0 < $progress && $progress < 100 ) { return $TEST_RUNNING; } elsif ( $progress == 100 ) { - if ( $results =~ /"tag":"TEST_DIED"/ ) { - return $TEST_CRASHED; - } elsif ( $results =~ /"tag":"UNABLE_TO_FINISH_TEST"/ ) { - return $TEST_LAPSED; - } else { - return $TEST_COMPLETED; - } + return $TEST_COMPLETED; } else { die Zonemaster::Backend::Error::Internal->new( reason => 'state could not be determined' ); From c5cc56ad2a0713e60d5d6a27eb32a380144cd8ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Fri, 21 Jul 2023 06:40:27 +0200 Subject: [PATCH 19/64] Clean up --- lib/Zonemaster/Backend/DB.pm | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 0aac4fd8b..b8dc021d5 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -94,22 +94,6 @@ The processing was cancelled because it took too long. Readonly our $TEST_COMPLETED => 'COMPLETED'; -=head2 $TEST_CRASHED - -The test was already processed. - -=cut - -Readonly our $TEST_CRASHED => 'crashed'; - -=head2 $TEST_LAPSED - -The test was already processed. - -=cut - -Readonly our $TEST_LAPSED => 'lapsed'; - our @EXPORT_OK = qw( $TEST_WAITING $TEST_RUNNING From 70549d4cea711305db8ff6215c07effb199248dd Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Mon, 27 Feb 2023 10:43:21 +0100 Subject: [PATCH 20/64] Fix unit test, remove deleted Basic04 testcase --- t/test_profile.json | 1 - 1 file changed, 1 deletion(-) diff --git a/t/test_profile.json b/t/test_profile.json index 7e6da9e28..8b2d4e56a 100644 --- a/t/test_profile.json +++ b/t/test_profile.json @@ -19,7 +19,6 @@ "basic01", "basic02", "basic03", - "basic04", "connectivity01", "connectivity02", "connectivity03", From 6b697dddbe2bfa94bc1e4efac2f1f9d3aba134ac Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Tue, 26 Apr 2022 15:05:52 +0200 Subject: [PATCH 21/64] Create new "result_entries" table --- lib/Zonemaster/Backend/DB/MySQL.pm | 30 +++++++++++++++++++++++++ lib/Zonemaster/Backend/DB/PostgreSQL.pm | 24 ++++++++++++++++++++ lib/Zonemaster/Backend/DB/SQLite.pm | 24 ++++++++++++++++++++ 3 files changed, 78 insertions(+) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index dd43da97c..2fd44d1b6 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -115,6 +115,36 @@ sub create_schema { ); } + #################################################################### + # RESULT ENTRIES + #################################################################### + $dbh->do( + 'CREATE TABLE IF NOT EXISTS result_entries ( + id BIGINT AUTO_INCREMENT PRIMARY KEY, + hash_id VARCHAR(16) NOT NULL, + level VARCHAR(15) NOT NULL, + module VARCHAR(255) NOT NULL, + testcase VARCHAR(255) NOT NULL, + tag VARCHAR(255) NOT NULL, + timestamp REAL NOT NULL, + args BLOB NOT NULL + ) ENGINE=InnoDB + ' + ) or die Zonemaster::Backend::Error::Internal->new( reason => "MySQL error, could not create 'result_entries' table", data => $dbh->errstr() ); + + $indexes = $dbh->selectall_hashref( 'SHOW INDEXES FROM result_entries', 'Key_name' ); + if ( not exists($indexes->{result_entries__hash_id}) ) { + $dbh->do( + 'CREATE INDEX result_entries__hash_id ON result_entries (hash_id)' + ); + } + + if ( not exists($indexes->{result_entries__level}) ) { + $dbh->do( + 'CREATE INDEX result_entries__level ON result_entries (level)' + ); + } + #################################################################### # BATCH JOBS diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index 292780bb8..f250ef695 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -101,6 +101,30 @@ sub create_schema { 'CREATE INDEX IF NOT EXISTS test_results__progress_priority_id ON test_results (progress, priority DESC, id) WHERE (progress = 0)' ); + #################################################################### + # RESULT ENTRIES + #################################################################### + $dbh->do( + 'CREATE TABLE IF NOT EXISTS result_entries ( + id BIGSERIAL PRIMARY KEY, + hash_id VARCHAR(16) NOT NULL, + level VARCHAR(15) NOT NULL, + module VARCHAR(255) NOT NULL, + testcase VARCHAR(255) NOT NULL, + tag VARCHAR(255) NOT NULL, + timestamp REAL NOT NULL, + args JSON NOT NULL + ) + ' + ) or die Zonemaster::Backend::Error::Internal->new( reason => "PostgreSQL error, could not create 'result_entries' table", data => $dbh->errstr() ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__hash_id ON result_entries (hash_id)' + ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__level ON result_entries (level)' + ); #################################################################### # BATCH JOBS diff --git a/lib/Zonemaster/Backend/DB/SQLite.pm b/lib/Zonemaster/Backend/DB/SQLite.pm index e718df9f9..7c0167557 100644 --- a/lib/Zonemaster/Backend/DB/SQLite.pm +++ b/lib/Zonemaster/Backend/DB/SQLite.pm @@ -96,6 +96,30 @@ sub create_schema { 'CREATE INDEX IF NOT EXISTS test_results__domain_undelegated ON test_results (domain, undelegated)' ); + #################################################################### + # RESULT ENTRIES + #################################################################### + $dbh->do( + 'CREATE TABLE IF NOT EXISTS result_entries ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + hash_id VARCHAR(16) NOT NULL, + level VARCHAR(15) NOT NULL, + module VARCHAR(255) NOT NULL, + testcase VARCHAR(255) NOT NULL, + tag VARCHAR(255) NOT NULL, + timestamp REAL NOT NULL, + args BLOB NOT NULL + ) + ' + ) or die Zonemaster::Backend::Error::Internal->new( reason => "SQLite error, could not create 'result_entries' table", data => $dbh->errstr() ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__hash_id ON result_entries (hash_id)' + ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__level ON result_entries (level)' + ); #################################################################### # BATCH JOBS From 576c23bb28cbe5da05d7f16b5bf211c6e26dd80e Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Tue, 26 Apr 2022 15:08:10 +0200 Subject: [PATCH 22/64] Use new "result_entries" table --- lib/Zonemaster/Backend/DB.pm | 156 +++++++++++++++++------- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 1 + lib/Zonemaster/Backend/TestAgent.pm | 20 ++- 3 files changed, 129 insertions(+), 48 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index a84debbba..cffe136ac 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -371,6 +371,34 @@ sub test_state { } } +sub set_test_completed { + my ( $self, $test_id ) = @_; + + my $current_state = $self->test_state( $test_id ); + + if ( $current_state ne $TEST_RUNNING ) { + die Zonemaster::Backend::Error::Internal->new( reason => 'illegal transition to COMPLETED' ); + } + + my $rows_affected = $self->dbh->do( + q[ + UPDATE test_results + SET progress = 100, + ended_at = ? + WHERE hash_id = ? + AND 0 < progress + AND progress < 100 + ], + undef, + $self->format_time( time() ), + $test_id, + ); + + if ( $rows_affected == 0 ) { + die Zonemaster::Backend::Error::Internal->new( reason => "job not found or illegal transition" ); + } +} + sub select_test_results { my ( $self, $test_id ) = @_; @@ -379,8 +407,7 @@ sub select_test_results { SELECT hash_id, created_at, - params, - results + params FROM test_results WHERE hash_id = ? ], @@ -431,14 +458,33 @@ sub test_results { my $result = $self->select_test_results( $test_id ); + my @result_entries = $self->dbh->selectall_array( + q[ + SELECT + level, + module, + testcase, + tag, + timestamp, + args + FROM result_entries + WHERE hash_id = ? + ], + { Slice => {} }, + $test_id + ); + eval { $result->{params} = decode_json( $result->{params} ); - if (defined $result->{results}) { - $result->{results} = decode_json( $result->{results} ); - } else { - $result->{results} = []; - } + @result_entries = map { + { + %$_, + args => decode_json( $_->{args} ), + } + } @result_entries; + + $result->{results} = \@result_entries; }; die Zonemaster::Backend::Error::JsonError->new( reason => "$@", data => { test_id => $test_id } ) @@ -462,11 +508,13 @@ sub get_test_history { my @results; my $query = q[ SELECT + (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = 'CRITICAL') AS nb_critical, + (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = 'ERROR') AS nb_error, + (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = 'WARNING') AS nb_warning, id, hash_id, created_at, - undelegated, - results + undelegated FROM test_results WHERE progress = 100 AND domain = ? AND ( ? IS NULL OR undelegated = ? ) ORDER BY id DESC @@ -484,16 +532,16 @@ sub get_test_history { $sth->execute(); while ( my $h = $sth->fetchrow_hashref ) { - $h->{results} = decode_json($h->{results}) if $h->{results}; - my $critical = ( grep { $_->{level} eq 'CRITICAL' } @{ $h->{results} } ); - my $error = ( grep { $_->{level} eq 'ERROR' } @{ $h->{results} } ); - my $warning = ( grep { $_->{level} eq 'WARNING' } @{ $h->{results} } ); - - # More important overwrites - my $overall = 'ok'; - $overall = 'warning' if $warning; - $overall = 'error' if $error; - $overall = 'critical' if $critical; + my $overall_result = 'ok'; + if ( $h->{nb_critical} ) { + $overall_result = 'critical'; + } + elsif ( $h->{nb_error} ) { + $overall_result = 'error'; + } + elsif ( $h->{nb_warning} ) { + $overall_result = 'warning'; + } push( @results, @@ -501,7 +549,7 @@ sub get_test_history { id => $h->{hash_id}, created_at => $self->to_iso8601( $h->{created_at} ), undelegated => $h->{undelegated}, - overall_result => $overall, + overall_result => $overall_result, } ); } @@ -726,14 +774,15 @@ sub process_unfinished_tests { ); my $msg = { - "level" => "CRITICAL", - "module" => "BACKEND_TEST_AGENT", - "tag" => "UNABLE_TO_FINISH_TEST", - "args" => { max_execution_time => $test_run_timeout }, - "timestamp" => $test_run_timeout + level => "CRITICAL", + module => "BACKEND_TEST_AGENT", + testcase => "", + tag => "UNABLE_TO_FINISH_TEST", + args => { max_execution_time => $test_run_timeout }, + timestamp => $test_run_timeout }; while ( my $h = $sth1->fetchrow_hashref ) { - $self->force_end_test($h->{hash_id}, $h->{results}, $msg); + $self->force_end_test($h->{hash_id}, $msg); } } @@ -775,44 +824,38 @@ sub select_unfinished_tests { } } -=head2 force_end_test($hash_id, $results, $msg) +=head2 force_end_test($hash_id, $msg) -Append the $msg log entry to the $results arrayref and store the results into -the database. +Store the $msg log entry into the database and mark test with $hash_id as +finished (progress to 100) in database. =cut sub force_end_test { - my ( $self, $hash_id, $results, $msg ) = @_; - my $result; - if ( defined $results && $results =~ /^\[/ ) { - $result = decode_json( $results ); - } - else { - $result = []; - } - push @$result, $msg; + my ( $self, $hash_id, $msg ) = @_; - $self->store_results( $hash_id, encode_json($result) ); + $self->add_result_entry( $hash_id, $msg ); + $self->set_test_completed( $hash_id ); } =head2 process_dead_test($hash_id) -Append a new log entry C to the test with $hash_id. -Then store the results in database. +Store a new log entry C in database for the test +with $hash_id. =cut sub process_dead_test { my ( $self, $hash_id ) = @_; - my ( $results ) = $self->dbh->selectrow_array("SELECT results FROM test_results WHERE hash_id = ?", undef, $hash_id); my $msg = { - "level" => "CRITICAL", - "module" => "BACKEND_TEST_AGENT", - "tag" => "TEST_DIED", - "timestamp" => $self->get_relative_start_time($hash_id) + level => "CRITICAL", + module => "BACKEND_TEST_AGENT", + testcase => "", + tag => "TEST_DIED", + args => {}, + timestamp => $self->get_relative_start_time($hash_id) }; - $self->force_end_test($hash_id, $results, $msg); + $self->force_end_test($hash_id, $msg); } # Converts the domain to lowercase and if the domain is not the root ('.') @@ -945,6 +988,25 @@ sub to_iso8601 { return $time; } +sub add_result_entry { + my ( $self, $hash_id, $entry ) = @_; + + my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; + + my $nb_inserted = $self->dbh->do( + "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES (?, ?, ?, ?, ?, ?, ?)", + undef, + $hash_id, + $entry->{level}, + $entry->{module}, + $entry->{testcase}, + $entry->{tag}, + $entry->{timestamp}, + $json->encode( $entry->{args} ), + ); + return $nb_inserted; +} + no Moose::Role; 1; diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index f250ef695..e61e8bd99 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -258,6 +258,7 @@ sub get_relative_start_time { return $self->dbh->selectrow_array( q[ + SELECT EXTRACT(EPOCH FROM ? - started_at) FROM test_results WHERE hash_id=? diff --git a/lib/Zonemaster/Backend/TestAgent.pm b/lib/Zonemaster/Backend/TestAgent.pm index 514818f5e..180f6a22e 100644 --- a/lib/Zonemaster/Backend/TestAgent.pm +++ b/lib/Zonemaster/Backend/TestAgent.pm @@ -10,6 +10,7 @@ use JSON::PP; use Scalar::Util qw( blessed ); use File::Slurp; use Locale::TextDomain qw[Zonemaster-Backend]; +use Log::Any qw( $log ); use Zonemaster::LDNS; @@ -18,6 +19,7 @@ use Zonemaster::Engine::Translator; use Zonemaster::Backend::Config; use Zonemaster::Engine::Profile; use Zonemaster::Engine::Util; +use Zonemaster::Engine::Logger::Entry; sub new { my ( $class, $params ) = @_; @@ -62,6 +64,7 @@ sub run { die "Must give the name of a domain to test.\n"; } $domain = $self->to_idn( $domain ); + my %numeric = Zonemaster::Engine::Logger::Entry->levels(); if ( $params->{nameservers} && @{ $params->{nameservers} } > 0 ) { $self->add_fake_delegation( $domain, $params->{nameservers} ); @@ -110,6 +113,21 @@ sub run { Zonemaster::Engine->logger->callback( sub { my ( $entry ) = @_; + + # TODO: Make minimum level configurable + if ( $entry->numeric_level >= $numeric{INFO} ) { + $log->debug("Adding result entry in database: " . $entry->string); + + $self->{_db}->add_result_entry( $test_id, { + timestamp => $entry->timestamp, + module => $entry->module, + testcase => $entry->testcase, + tag => $entry->tag, + level => $entry->numeric_level, + args => $entry->args // {}, + }); + } + if ( $entry->{tag} and $entry->{tag} eq 'TEST_CASE_END' ) { $nbr_testcases_finished++; # limit to max 99%, 100% is reached when data is stored in database @@ -132,7 +150,7 @@ sub run { } } - $self->{_db}->store_results( $test_id, Zonemaster::Engine->logger->json( 'INFO' ) ); + $self->{_db}->set_test_completed( $test_id ); return; } ## end sub run From 36328f7f0e556f9c3d9acd04eea711bd42039ee6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Mon, 20 Sep 2021 12:00:40 +0200 Subject: [PATCH 23/64] Insert result entries in one query --- lib/Zonemaster/Backend/DB.pm | 25 +++++++++++++++++++++++++ lib/Zonemaster/Backend/TestAgent.pm | 19 +++++-------------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index cffe136ac..eb0420e43 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -1007,6 +1007,31 @@ sub add_result_entry { return $nb_inserted; } +sub add_result_entries { + my ( $self, $hash_id, $entries ) = @_; + my @records; + + my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; + + foreach my $m ( @$entries ) { + my $r = [ + $hash_id, + $m->level, + $m->module, + $m->testcase, + $m->tag, + $m->timestamp, + $json->encode( $m->args // {} ), + ]; + + push @records, $r; + } + my $query_values = join ", ", ("(?, ?, ?, ?, ?, ?, ?)") x @records; + my $query = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES $query_values"; + my $sth = $self->dbh->prepare($query); + $sth = $sth->execute(map { @$_ } @records); +} + no Moose::Role; 1; diff --git a/lib/Zonemaster/Backend/TestAgent.pm b/lib/Zonemaster/Backend/TestAgent.pm index 180f6a22e..4ebfa5b71 100644 --- a/lib/Zonemaster/Backend/TestAgent.pm +++ b/lib/Zonemaster/Backend/TestAgent.pm @@ -114,20 +114,6 @@ sub run { sub { my ( $entry ) = @_; - # TODO: Make minimum level configurable - if ( $entry->numeric_level >= $numeric{INFO} ) { - $log->debug("Adding result entry in database: " . $entry->string); - - $self->{_db}->add_result_entry( $test_id, { - timestamp => $entry->timestamp, - module => $entry->module, - testcase => $entry->testcase, - tag => $entry->tag, - level => $entry->numeric_level, - args => $entry->args // {}, - }); - } - if ( $entry->{tag} and $entry->{tag} eq 'TEST_CASE_END' ) { $nbr_testcases_finished++; # limit to max 99%, 100% is reached when data is stored in database @@ -150,6 +136,11 @@ sub run { } } + # TODO: Make minimum level configurable + my @entries = grep { $_->numeric_level >= $numeric{INFO} } @{ Zonemaster::Engine->logger->entries }; + + $self->{_db}->add_result_entries( $test_id, \@entries); + $self->{_db}->set_test_completed( $test_id ); return; From f8256ff7dd54068e51079a75c505a2faa6fc94f8 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Mon, 2 May 2022 15:09:26 +0200 Subject: [PATCH 24/64] Drop table "result_entries" --- lib/Zonemaster/Backend/DB/MySQL.pm | 1 + lib/Zonemaster/Backend/DB/PostgreSQL.pm | 1 + lib/Zonemaster/Backend/DB/SQLite.pm | 1 + 3 files changed, 3 insertions(+) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index 2fd44d1b6..dd0533918 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -186,6 +186,7 @@ sub drop_tables { my ( $self ) = @_; $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); + $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index e61e8bd99..06cd7df2b 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -173,6 +173,7 @@ sub drop_tables { try { $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); + $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); } diff --git a/lib/Zonemaster/Backend/DB/SQLite.pm b/lib/Zonemaster/Backend/DB/SQLite.pm index 7c0167557..506f7a4fe 100644 --- a/lib/Zonemaster/Backend/DB/SQLite.pm +++ b/lib/Zonemaster/Backend/DB/SQLite.pm @@ -161,6 +161,7 @@ sub drop_tables { my ( $self ) = @_; $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); + $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); From cc5de7712d56ebb322f38b9f713d31df229fd695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Mon, 20 Sep 2021 13:45:15 +0200 Subject: [PATCH 25/64] test more optimizations --- lib/Zonemaster/Backend/TestAgent.pm | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/Zonemaster/Backend/TestAgent.pm b/lib/Zonemaster/Backend/TestAgent.pm index 4ebfa5b71..3b1b0c328 100644 --- a/lib/Zonemaster/Backend/TestAgent.pm +++ b/lib/Zonemaster/Backend/TestAgent.pm @@ -11,15 +11,17 @@ use Scalar::Util qw( blessed ); use File::Slurp; use Locale::TextDomain qw[Zonemaster-Backend]; use Log::Any qw( $log ); +use Time::HiRes qw[time sleep gettimeofday tv_interval]; use Zonemaster::LDNS; use Zonemaster::Engine; use Zonemaster::Engine::Translator; -use Zonemaster::Backend::Config; use Zonemaster::Engine::Profile; use Zonemaster::Engine::Util; use Zonemaster::Engine::Logger::Entry; +use Zonemaster::Backend::Config; +use Zonemaster::Backend::Metrics; sub new { my ( $class, $params ) = @_; @@ -136,11 +138,22 @@ sub run { } } + #Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_duration", $callback_duration * 1000); + + my $start_time_2 = [ gettimeofday ]; + # TODO: Make minimum level configurable my @entries = grep { $_->numeric_level >= $numeric{INFO} } @{ Zonemaster::Engine->logger->entries }; + Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_add_result_entry_grep_duration", tv_interval($start_time_2) * 1000); + $self->{_db}->add_result_entries( $test_id, \@entries); + my $callback_add_result_entry_duration = tv_interval($start_time_2); + Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_add_result_entry_duration", $callback_add_result_entry_duration * 1000); + + #$log->debug("Callback timing for $test_id: $callback_duration / $callback_add_result_entry_duration "); + $self->{_db}->set_test_completed( $test_id ); return; From 65fc4f1a64d0ff309f3c6886e6e0a91508e495f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Thu, 23 Sep 2021 17:30:27 +0200 Subject: [PATCH 26/64] Use enum instead of strings (PostgreSQL and MySQL) --- lib/Zonemaster/Backend/DB/MySQL.pm | 6 +++--- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index dd0533918..f0ad6737d 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -119,17 +119,17 @@ sub create_schema { # RESULT ENTRIES #################################################################### $dbh->do( - 'CREATE TABLE IF NOT EXISTS result_entries ( + "CREATE TABLE IF NOT EXISTS result_entries ( id BIGINT AUTO_INCREMENT PRIMARY KEY, hash_id VARCHAR(16) NOT NULL, - level VARCHAR(15) NOT NULL, + level ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL') NOT NULL, module VARCHAR(255) NOT NULL, testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, timestamp REAL NOT NULL, args BLOB NOT NULL ) ENGINE=InnoDB - ' + " ) or die Zonemaster::Backend::Error::Internal->new( reason => "MySQL error, could not create 'result_entries' table", data => $dbh->errstr() ); $indexes = $dbh->selectall_hashref( 'SHOW INDEXES FROM result_entries', 'Key_name' ); diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index 06cd7df2b..1e5265aaa 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -104,11 +104,15 @@ sub create_schema { #################################################################### # RESULT ENTRIES #################################################################### + $dbh->do( + "CREATE TYPE log_level AS ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL')" + ); + $dbh->do( 'CREATE TABLE IF NOT EXISTS result_entries ( id BIGSERIAL PRIMARY KEY, hash_id VARCHAR(16) NOT NULL, - level VARCHAR(15) NOT NULL, + level log_level NOT NULL, module VARCHAR(255) NOT NULL, testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, @@ -176,6 +180,7 @@ sub drop_tables { $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); + $self->dbh->do( "DROP TYPE IF EXISTS log_level" ); } finally { $self->dbh->do( "SET client_min_messages = ?", undef, $old_client_min_messages ); From b4f0f20c41dfb9dc5fd4f5518bd64bacdd4491ff Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Tue, 14 Mar 2023 20:40:10 +0100 Subject: [PATCH 27/64] Provide migration script --- MANIFEST | 1 + .../patch_db_zonemaster_backend_ver_11.0.3.pl | 219 ++++++++++++++++++ 2 files changed, 220 insertions(+) create mode 100644 share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl diff --git a/MANIFEST b/MANIFEST index 3979567e1..cd3641124 100644 --- a/MANIFEST +++ b/MANIFEST @@ -52,6 +52,7 @@ share/locale/nb/LC_MESSAGES/Zonemaster-Backend.mo share/locale/sv/LC_MESSAGES/Zonemaster-Backend.mo share/Makefile share/patch/patch_db_zonemaster_backend_ver_9.0.0.pl +share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl share/patch/patch_mysql_db_zonemaster_backend_ver_1.0.3.pl share/patch/patch_mysql_db_zonemaster_backend_ver_5.0.0.pl share/patch/patch_mysql_db_zonemaster_backend_ver_5.0.2.pl diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl new file mode 100644 index 000000000..51c75fdcc --- /dev/null +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -0,0 +1,219 @@ +use strict; +use warnings; + +use JSON::PP; +use Try::Tiny; + +use Zonemaster::Backend::Config; + +my $config = Zonemaster::Backend::Config->load_config(); + +my %patch = ( + mysql => \&patch_db_mysql, + postgresql => \&patch_db_postgresql, + sqlite => \&patch_db_sqlite, +); + +my $db_engine = $config->DB_engine; + +if ( $db_engine =~ /^(MySQL|PostgreSQL|SQLite)$/ ) { + $patch{ lc $db_engine }(); +} +else { + die "Unknown database engine configured: $db_engine\n"; +} + +sub _update_data { + my ( $dbh ) = @_; + + my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; + + my $sth1 = $dbh->prepare( 'SELECT hash_id, results FROM test_results', undef ); + $sth1->execute; + while ( my $row = $sth1->fetchrow_arrayref ) { + my ( $hash_id, $results ) = @$row; + + next unless $results; + + my @records; + my $entries = $json->decode( $results ); + + foreach my $m ( @$entries ) { + my $r = [ + $hash_id, + $m->{level}, + $m->{module}, + $m->{testcase} // '', + $m->{tag}, + $m->{timestamp}, + $json->encode( $m->{args} // {} ), + ]; + + push @records, $r; + } + + my $query_values = join ", ", ("(?, ?, ?, ?, ?, ?, ?)") x @records; + my $query = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES $query_values"; + my $sth = $dbh->prepare( $query ); + $sth = $sth->execute( map { @$_ } @records ); + + $dbh->do( "UPDATE test_results SET results = NULL WHERE hash_id = ?", undef, $hash_id ); + } +} + +sub patch_db_mysql { + use Zonemaster::Backend::DB::MySQL; + + my $db = Zonemaster::Backend::DB::MySQL->from_config( $config ); + my $dbh = $db->dbh; + + $dbh->{AutoCommit} = 0; + + try { + $dbh->do( + "CREATE TABLE IF NOT EXISTS result_entries ( + id integer AUTO_INCREMENT PRIMARY KEY, + hash_id VARCHAR(16) not null, + level ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL') not null, + module VARCHAR(255) not null, + testcase VARCHAR(255) not null, + tag VARCHAR(255) not null, + timestamp REAL not null, + args BLOB not null + ) ENGINE=InnoDB + " + ); + + my $indexes = $dbh->selectall_hashref( 'SHOW INDEXES FROM result_entries', 'Key_name' ); + if ( not exists($indexes->{result_entries__hash_id}) ) { + $dbh->do( + 'CREATE INDEX result_entries__hash_id ON result_entries (hash_id)' + ); + } + if ( not exists($indexes->{result_entries__level}) ) { + $dbh->do( + 'CREATE INDEX result_entries__level ON result_entries (level)' + ); + } + + _update_data( $dbh ); + + $dbh->commit(); + } catch { + print( "Could not upgrade database: " . $_ ); + + eval { $dbh->rollback() }; + }; +} + +sub patch_db_postgresql { + use Zonemaster::Backend::DB::PostgreSQL; + + my $db = Zonemaster::Backend::DB::PostgreSQL->from_config( $config ); + my $dbh = $db->dbh; + + $dbh->{AutoCommit} = 0; + + try { + + $dbh->do( + "CREATE TYPE log_level AS ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL')" + ); + + $dbh->do( + 'CREATE TABLE IF NOT EXISTS result_entries ( + id serial primary key, + hash_id VARCHAR(16) not null, + level log_level not null, + module varchar(255) not null, + testcase varchar(255) not null, + tag varchar(255) not null, + timestamp real not null, + args json not null + ) + ' + ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__hash_id ON result_entries (hash_id)' + ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__level ON result_entries (level)' + ); + + $dbh->do(q[ + INSERT INTO result_entries ( + hash_id, args, module, level, tag, timestamp, testcase + ) + ( + select + hash_id, + (CASE WHEN res->'args' IS NULL THEN '{}' ELSE res->'args' END) AS args, + res->>'module' AS module, + (res->>'level')::log_level AS level, + res->>'tag' AS tag, + (res->>'timestamp')::real AS timestamp, + (CASE WHEN res->>'testcase' IS NULL THEN '' ELSE res->>'testcase' END) AS testcase + FROM + ( + SELECT + json_array_elements(results) AS res, + hash_id + FROM test_results + ) AS s1 + ) + ]); + + $dbh->do( + 'UPDATE test_results SET results = NULL WHERE results IS NOT NULL' + ); + + $dbh->commit(); + } catch { + print( "Could not upgrade database: " . $_ ); + + eval { $dbh->rollback() }; + }; +} + +sub patch_db_sqlite { + use Zonemaster::Backend::DB::SQLite; + + my $db = Zonemaster::Backend::DB::SQLite->from_config( $config ); + my $dbh = $db->dbh; + + $dbh->{AutoCommit} = 0; + + try { + $dbh->do( + 'CREATE TABLE IF NOT EXISTS result_entries ( + id integer PRIMARY KEY AUTOINCREMENT, + hash_id VARCHAR(16) not null, + level varchar(15) not null, + module varchar(255) not null, + testcase varchar(255) not null, + tag varchar(255) not null, + timestamp real not null, + args blob not null + ) + ' + ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__hash_id ON result_entries (hash_id)' + ); + + $dbh->do( + 'CREATE INDEX IF NOT EXISTS result_entries__level ON result_entries (level)' + ); + + _update_data( $dbh ); + + $dbh->commit(); + } catch { + print( "Error while upgrading database: " . $_ ); + + eval { $dbh->rollback() }; + }; +} From 397f8aafbb272b74cd39acdec8528cda4d46cf99 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Thu, 23 Mar 2023 10:04:49 +0100 Subject: [PATCH 28/64] Update data in chunks to avoid memory exhaustion For MySQL and SQLite --- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 69 +++++++++++-------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 51c75fdcc..71e4433a7 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -15,6 +15,7 @@ ); my $db_engine = $config->DB_engine; +print "engine: $db_engine\n"; if ( $db_engine =~ /^(MySQL|PostgreSQL|SQLite)$/ ) { $patch{ lc $db_engine }(); @@ -28,36 +29,48 @@ sub _update_data { my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; - my $sth1 = $dbh->prepare( 'SELECT hash_id, results FROM test_results', undef ); - $sth1->execute; - while ( my $row = $sth1->fetchrow_arrayref ) { - my ( $hash_id, $results ) = @$row; - - next unless $results; - - my @records; - my $entries = $json->decode( $results ); - - foreach my $m ( @$entries ) { - my $r = [ - $hash_id, - $m->{level}, - $m->{module}, - $m->{testcase} // '', - $m->{tag}, - $m->{timestamp}, - $json->encode( $m->{args} // {} ), - ]; - - push @records, $r; + my ( $row_total ) = $dbh->selectrow_array( 'SELECT count(*) FROM test_results' ); + print "count: $row_total\n"; + + # depending on the resources available to select all data in database + # update $row_count to your needs + my $row_count = 50000; + my $row_done = 0; + while ( $row_done < $row_total ) { + print "row_done/row_total: $row_done / $row_total\n"; + my $sth1 = $dbh->prepare( 'SELECT hash_id, results FROM test_results ORDER BY id ASC LIMIT ?,?' ); + $sth1->execute( $row_done, $row_count ); + while ( my $row = $sth1->fetchrow_arrayref ) { + my ( $hash_id, $results ) = @$row; + + next unless $results; + + my @records; + my $entries = $json->decode( $results ); + + foreach my $m ( @$entries ) { + my $r = [ + $hash_id, + $m->{level}, + $m->{module}, + $m->{testcase} // '', + $m->{tag}, + $m->{timestamp}, + $json->encode( $m->{args} // {} ), + ]; + + push @records, $r; + } + + my $query_values = join ", ", ("(?, ?, ?, ?, ?, ?, ?)") x @records; + my $query = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES $query_values"; + my $sth = $dbh->prepare( $query ); + $sth = $sth->execute( map { @$_ } @records ); + + $dbh->do( "UPDATE test_results SET results = NULL WHERE hash_id = ?", undef, $hash_id ); } - my $query_values = join ", ", ("(?, ?, ?, ?, ?, ?, ?)") x @records; - my $query = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES $query_values"; - my $sth = $dbh->prepare( $query ); - $sth = $sth->execute( map { @$_ } @records ); - - $dbh->do( "UPDATE test_results SET results = NULL WHERE hash_id = ?", undef, $hash_id ); + $row_done += $row_count; } } From 7f44e4a03819bd386beb91dc3fa29e842e7bf74b Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 12 Jul 2023 11:37:03 +0200 Subject: [PATCH 29/64] Refactor * make "add_result_entries" variadic (suggested by @marc-vanderwal) * "force_end_test" should take a "Zonemaster::Engine::Logger::Entry" reference instead of a simple hashref --- lib/Zonemaster/Backend/DB.pm | 79 ++++++++++++----------------- lib/Zonemaster/Backend/TestAgent.pm | 2 +- 2 files changed, 34 insertions(+), 47 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index eb0420e43..bbcf555e6 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -17,6 +17,7 @@ use Readonly; use Try::Tiny; use Zonemaster::Backend::Errors; +use Zonemaster::Engine::Logger::Entry; requires qw( add_batch_job @@ -773,14 +774,16 @@ sub process_unfinished_tests { $test_run_timeout, ); - my $msg = { - level => "CRITICAL", - module => "BACKEND_TEST_AGENT", - testcase => "", - tag => "UNABLE_TO_FINISH_TEST", - args => { max_execution_time => $test_run_timeout }, - timestamp => $test_run_timeout - }; + my $msg = Zonemaster::Engine::Logger::Entry->new( + { + level => "CRITICAL", + module => "BACKEND_TEST_AGENT", + testcase => "", + tag => "UNABLE_TO_FINISH_TEST", + args => { max_execution_time => $test_run_timeout }, + timestamp => $test_run_timeout + } + ); while ( my $h = $sth1->fetchrow_hashref ) { $self->force_end_test($h->{hash_id}, $msg); } @@ -826,15 +829,15 @@ sub select_unfinished_tests { =head2 force_end_test($hash_id, $msg) -Store the $msg log entry into the database and mark test with $hash_id as -finished (progress to 100) in database. +Store the L $msg log entry into the database +and mark test with $hash_id as COMPLETED. =cut sub force_end_test { my ( $self, $hash_id, $msg ) = @_; - $self->add_result_entry( $hash_id, $msg ); + $self->add_result_entries( $hash_id, $msg ); $self->set_test_completed( $hash_id ); } @@ -847,14 +850,17 @@ with $hash_id. sub process_dead_test { my ( $self, $hash_id ) = @_; - my $msg = { - level => "CRITICAL", - module => "BACKEND_TEST_AGENT", - testcase => "", - tag => "TEST_DIED", - args => {}, - timestamp => $self->get_relative_start_time($hash_id) - }; + my $msg = Zonemaster::Engine::Logger::Entry->new( + { + level => "CRITICAL", + module => "BACKEND_TEST_AGENT", + testcase => "", + tag => "TEST_DIED", + args => {}, + timestamp => $self->get_relative_start_time($hash_id) + } + ); + #$msg = bless( $msg, "Zonemaster::Engine::Logger::Entry" ); $self->force_end_test($hash_id, $msg); } @@ -988,40 +994,21 @@ sub to_iso8601 { return $time; } -sub add_result_entry { - my ( $self, $hash_id, $entry ) = @_; - - my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; - - my $nb_inserted = $self->dbh->do( - "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES (?, ?, ?, ?, ?, ?, ?)", - undef, - $hash_id, - $entry->{level}, - $entry->{module}, - $entry->{testcase}, - $entry->{tag}, - $entry->{timestamp}, - $json->encode( $entry->{args} ), - ); - return $nb_inserted; -} - sub add_result_entries { - my ( $self, $hash_id, $entries ) = @_; + my ( $self, $hash_id, @entries ) = @_; my @records; my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; - foreach my $m ( @$entries ) { + foreach my $e ( @entries ) { my $r = [ $hash_id, - $m->level, - $m->module, - $m->testcase, - $m->tag, - $m->timestamp, - $json->encode( $m->args // {} ), + $e->level, + $e->module, + $e->testcase, + $e->tag, + $e->timestamp, + $json->encode( $e->args // {} ), ]; push @records, $r; diff --git a/lib/Zonemaster/Backend/TestAgent.pm b/lib/Zonemaster/Backend/TestAgent.pm index 3b1b0c328..7aed970dd 100644 --- a/lib/Zonemaster/Backend/TestAgent.pm +++ b/lib/Zonemaster/Backend/TestAgent.pm @@ -147,7 +147,7 @@ sub run { Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_add_result_entry_grep_duration", tv_interval($start_time_2) * 1000); - $self->{_db}->add_result_entries( $test_id, \@entries); + $self->{_db}->add_result_entries( $test_id, @entries); my $callback_add_result_entry_duration = tv_interval($start_time_2); Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_add_result_entry_duration", $callback_add_result_entry_duration * 1000); From f5691bcd591ebb429255ddb85b70f61faeea927c Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 26 Jul 2023 12:00:22 +0200 Subject: [PATCH 30/64] Apply suggestions from review --- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 1 - share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index 1e5265aaa..736c85537 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -264,7 +264,6 @@ sub get_relative_start_time { return $self->dbh->selectrow_array( q[ - SELECT EXTRACT(EPOCH FROM ? - started_at) FROM test_results WHERE hash_id=? diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 71e4433a7..a474580ce 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -115,7 +115,7 @@ sub patch_db_mysql { } catch { print( "Could not upgrade database: " . $_ ); - eval { $dbh->rollback() }; + $dbh->rollback(); }; } @@ -186,7 +186,7 @@ sub patch_db_postgresql { } catch { print( "Could not upgrade database: " . $_ ); - eval { $dbh->rollback() }; + $dbh->rollback(); }; } @@ -227,6 +227,6 @@ sub patch_db_sqlite { } catch { print( "Error while upgrading database: " . $_ ); - eval { $dbh->rollback() }; + $dbh->rollback(); }; } From a6c38b033a1b3cde081c76e608741bafae3eaaec Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 27 Sep 2023 09:28:21 +0200 Subject: [PATCH 31/64] New table "log_level" --- lib/Zonemaster/Backend/DB/MySQL.pm | 33 ++++++++++++++++++++++++- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 30 +++++++++++++++++++--- lib/Zonemaster/Backend/DB/SQLite.pm | 31 +++++++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index f0ad6737d..064622547 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -115,6 +115,36 @@ sub create_schema { ); } + #################################################################### + # LOG LEVEL + #################################################################### + $dbh->do( + "CREATE TABLE IF NOT EXISTS log_level ( + level VARCHAR(15), + value INT, + + UNIQUE (level) + ) ENGINE=InnoDB + " + ) or die Zonemaster::Backend::Error::Internal->new( reason => "MySQL error, could not create 'log_level' table", data => $dbh->errstr() ); + + my ( $c ) = $dbh->selectrow_array( "SELECT count(*) FROM log_level" ); + if ( $c == 0 ) { + $dbh->do( + "INSERT INTO log_level (level, value) + VALUES + ('DEBUG3' , -2), + ('DEBUG2' , -1), + ('DEBUG' , 0), + ('INFO' , 1), + ('NOTICE' , 2), + ('WARNING' , 3), + ('ERROR' , 4), + ('CRITICAL' , 5) + " + ); + } + #################################################################### # RESULT ENTRIES #################################################################### @@ -122,7 +152,7 @@ sub create_schema { "CREATE TABLE IF NOT EXISTS result_entries ( id BIGINT AUTO_INCREMENT PRIMARY KEY, hash_id VARCHAR(16) NOT NULL, - level ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL') NOT NULL, + level VARCHAR(15) NOT NULL, module VARCHAR(255) NOT NULL, testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, @@ -187,6 +217,7 @@ sub drop_tables { $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); + $self->dbh->do( "DROP TABLE IF EXISTS log_level" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index 736c85537..bb6ea6484 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -105,14 +105,36 @@ sub create_schema { # RESULT ENTRIES #################################################################### $dbh->do( - "CREATE TYPE log_level AS ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL')" - ); + "CREATE TABLE IF NOT EXISTS log_level ( + level VARCHAR(15), + value INT, + + UNIQUE (level) + ) + " + ) or die Zonemaster::Backend::Error::Internal->new( reason => "PostgreSQL error, could not create 'log_level' table", data => $dbh->errstr() ); + my ( $c ) = $dbh->selectrow_array( "SELECT count(*) FROM log_level" ); + if ( $c == 0 ) { + $dbh->do( + "INSERT INTO log_level (level, value) + VALUES + ('DEBUG3' , -2), + ('DEBUG2' , -1), + ('DEBUG' , 0), + ('INFO' , 1), + ('NOTICE' , 2), + ('WARNING' , 3), + ('ERROR' , 4), + ('CRITICAL' , 5) + " + ); + } $dbh->do( 'CREATE TABLE IF NOT EXISTS result_entries ( id BIGSERIAL PRIMARY KEY, hash_id VARCHAR(16) NOT NULL, - level log_level NOT NULL, + level VARCHAR(15) NOT NULL, module VARCHAR(255) NOT NULL, testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, @@ -178,9 +200,9 @@ sub drop_tables { try { $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); + $self->dbh->do( "DROP TABLE IF EXISTS log_level" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); - $self->dbh->do( "DROP TYPE IF EXISTS log_level" ); } finally { $self->dbh->do( "SET client_min_messages = ?", undef, $old_client_min_messages ); diff --git a/lib/Zonemaster/Backend/DB/SQLite.pm b/lib/Zonemaster/Backend/DB/SQLite.pm index 506f7a4fe..7c3fa04d6 100644 --- a/lib/Zonemaster/Backend/DB/SQLite.pm +++ b/lib/Zonemaster/Backend/DB/SQLite.pm @@ -96,6 +96,36 @@ sub create_schema { 'CREATE INDEX IF NOT EXISTS test_results__domain_undelegated ON test_results (domain, undelegated)' ); + #################################################################### + # LOG LEVEL + #################################################################### + $dbh->do( + "CREATE TABLE IF NOT EXISTS log_level ( + level VARCHAR(15), + value INTEGER, + + UNIQUE (level) + ) + " + ) or die Zonemaster::Backend::Error::Internal->new( reason => "SQLite error, could not create 'log_level' table", data => $dbh->errstr() ); + + my ( $c ) = $dbh->selectrow_array( "SELECT count(*) FROM log_level" ); + if ( $c == 0 ) { + $dbh->do( + "INSERT INTO log_level (level, value) + VALUES + ('DEBUG3' , -2), + ('DEBUG2' , -1), + ('DEBUG' , 0), + ('INFO' , 1), + ('NOTICE' , 2), + ('WARNING' , 3), + ('ERROR' , 4), + ('CRITICAL' , 5) + " + ); + } + #################################################################### # RESULT ENTRIES #################################################################### @@ -162,6 +192,7 @@ sub drop_tables { $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); + $self->dbh->do( "DROP TABLE IF EXISTS log_level" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); From 0a3e32b0ee83bc71f54da1132e4780857ae04421 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 27 Sep 2023 11:20:13 +0200 Subject: [PATCH 32/64] Add FOREIGN KEY constraint on result_entries.level --- lib/Zonemaster/Backend/DB/MySQL.pm | 6 ++++-- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 11 ++++++++--- lib/Zonemaster/Backend/DB/SQLite.pm | 7 ++++++- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index 064622547..81b3578e7 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -157,7 +157,9 @@ sub create_schema { testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, timestamp REAL NOT NULL, - args BLOB NOT NULL + args BLOB NOT NULL, + + CONSTRAINT fk_level FOREIGN KEY (level) REFERENCES log_level(level) ) ENGINE=InnoDB " ) or die Zonemaster::Backend::Error::Internal->new( reason => "MySQL error, could not create 'result_entries' table", data => $dbh->errstr() ); @@ -216,7 +218,7 @@ sub drop_tables { my ( $self ) = @_; $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); - $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); + $self->dbh->do( "DROP TABLE IF EXISTS result_entries CASCADE" ); $self->dbh->do( "DROP TABLE IF EXISTS log_level" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index bb6ea6484..7f424097f 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -102,7 +102,7 @@ sub create_schema { ); #################################################################### - # RESULT ENTRIES + # LOG LEVEL #################################################################### $dbh->do( "CREATE TABLE IF NOT EXISTS log_level ( @@ -130,6 +130,9 @@ sub create_schema { ); } + #################################################################### + # RESULT ENTRIES + #################################################################### $dbh->do( 'CREATE TABLE IF NOT EXISTS result_entries ( id BIGSERIAL PRIMARY KEY, @@ -139,7 +142,9 @@ sub create_schema { testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, timestamp REAL NOT NULL, - args JSON NOT NULL + args JSON NOT NULL, + + CONSTRAINT fk_level FOREIGN KEY(level) REFERENCES log_level(level) ) ' ) or die Zonemaster::Backend::Error::Internal->new( reason => "PostgreSQL error, could not create 'result_entries' table", data => $dbh->errstr() ); @@ -199,7 +204,7 @@ sub drop_tables { try { $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); - $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); + $self->dbh->do( "DROP TABLE IF EXISTS result_entries CASCADE" ); $self->dbh->do( "DROP TABLE IF EXISTS log_level" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); diff --git a/lib/Zonemaster/Backend/DB/SQLite.pm b/lib/Zonemaster/Backend/DB/SQLite.pm index 7c3fa04d6..eee207cbc 100644 --- a/lib/Zonemaster/Backend/DB/SQLite.pm +++ b/lib/Zonemaster/Backend/DB/SQLite.pm @@ -55,6 +55,9 @@ sub create_schema { my $dbh = $self->dbh; + # enable FOREIGN KEY support + $dbh->do( 'PRAGMA foreign_keys = ON;' ); + #################################################################### # TEST RESULTS #################################################################### @@ -138,7 +141,9 @@ sub create_schema { testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, timestamp REAL NOT NULL, - args BLOB NOT NULL + args BLOB NOT NULL, + + FOREIGN KEY(level) REFERENCES log_level(level) ) ' ) or die Zonemaster::Backend::Error::Internal->new( reason => "SQLite error, could not create 'result_entries' table", data => $dbh->errstr() ); From 0da996599615a545141f8a4d3a42720029424114 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 27 Sep 2023 11:20:53 +0200 Subject: [PATCH 33/64] Unit test for database creation, drop, constraints --- t/db_ddl.t | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 t/db_ddl.t diff --git a/t/db_ddl.t b/t/db_ddl.t new file mode 100644 index 000000000..d82ef5151 --- /dev/null +++ b/t/db_ddl.t @@ -0,0 +1,133 @@ +use strict; +use warnings; + +use Test::More tests => 2; +use Test::Exception; +use Test::NoWarnings qw(warnings clear_warnings); + +use File::ShareDir qw[dist_file]; +use File::Temp qw[tempdir]; + +my $t_path; +BEGIN { + use File::Spec::Functions qw( rel2abs ); + use File::Basename qw( dirname ); + $t_path = dirname( rel2abs( $0 ) ); +} +use lib $t_path; +use TestUtil; + +use Zonemaster::Engine; +use Zonemaster::Backend::Config; + +my $db_backend = TestUtil::db_backend(); + +my $tempdir = tempdir( CLEANUP => 1 ); +my $config = Zonemaster::Backend::Config->parse( <get_db_class( $db_backend ); +my $db = $dbclass->from_config( $config ); + + +subtest 'Everything but Test::NoWarnings' => sub { + + subtest 'drop and create' => sub { + subtest 'first drop (cleanup) ... ' => sub { + $db->drop_tables(); + dies_ok { + $db->dbh->do( 'SELECT 1 FROM test_results' ) + } + 'table "test_results" sould not exist'; + }; + subtest '... then drop after create ...' => sub { + $db->create_schema(); + my ( $res ) = $db->dbh->selectrow_array( 'SELECT count(*) FROM test_results' ); + is $res, 0, 'a. after create, table "test_results" should exist and be empty'; + + $db->drop_tables(); + dies_ok { + $db->dbh->do( 'SELECT 1 FROM test_results' ) + } + 'b. after drop, table "test_results" sould be removed'; + }; + }; + + subtest 'constraints' => sub { + $db->create_schema(); + + subtest 'constraint unique' => sub { + my $time = $db->format_time( time() ); + my @constraints = ( + { + table => 'test_results', + key => 'hash_id', + sql => "INSERT INTO test_results (hash_id,domain,created_at,params) + VALUES ('0123456789abcdef', 'domain.test', '$time', '{}')" + }, + { + table => 'log_level', + key => 'level', + sql => "INSERT INTO log_level (level, value) VALUES ('OTHER', 10)" + }, + { + table => 'users', + key => 'username', + sql => "INSERT INTO users (username) VALUES ('user1')" + }, + ); + + for my $c (@constraints) { + $db->dbh->do( $c->{sql} ); + throws_ok { + $db->dbh->do( $c->{sql} ); + } + qr/(unique constraint|duplicate entry)/i, "$c->{table}($c->{key}) key should be unique"; + } + }; + + subtest 'constraint on foreign key' => sub { + subtest 'result_entries - level should exist in log_level(level)' => sub { + my $level = "INFO"; + my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) + VALUES ('0123456789abcdef', '$level', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; + my $inserted_rows = $db->dbh->do( $sql ); + is $inserted_rows, 1, 'can insert an entry with an existing level'; + + throws_ok { + my $level = "DOESNTEXIST"; + my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) + VALUES ('0123456789abcdef', '$level', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; + $db->dbh->do( $sql ); + } + qr/foreign key/i, 'cannot insert an entry with an non-existing level'; + } + }; + }; +}; + +# FIXME: hack to avoid getting warnings from Test::NoWarnings +my @warn = warnings(); +if ( @warn == 6 ) { + clear_warnings(); +} From 73d23be02502c01a5ed028e39ff0f9150dff8bf7 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Mon, 25 Sep 2023 17:26:44 +0200 Subject: [PATCH 34/64] PostgreSQL: Store args in JSONb --- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index 7f424097f..ee586de1b 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -142,7 +142,7 @@ sub create_schema { testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, timestamp REAL NOT NULL, - args JSON NOT NULL, + args JSONb NOT NULL, CONSTRAINT fk_level FOREIGN KEY(level) REFERENCES log_level(level) ) From 8a58683dff0b8346afad0244f044ba2a7053a001 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 27 Sep 2023 12:31:58 +0200 Subject: [PATCH 35/64] MariaDB: drop FOREIGN KEY before droping a table From the documentations on DROP TABLE [1] If a foreign key references this table, the table cannot be dropped. In this case, it is necessary to drop the foreign key first. [1]: https://mariadb.com/kb/en/drop-table/#description --- lib/Zonemaster/Backend/DB/MySQL.pm | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index 81b3578e7..e9a50b16a 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -217,8 +217,11 @@ Drop all the tables if they exist. sub drop_tables { my ( $self ) = @_; + # remove any FOREIGN KEY before droping the table + $self->dbh->do( "ALTER TABLE IF EXISTS result_entries DROP FOREIGN KEY IF EXISTS fk_level" ); + $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); - $self->dbh->do( "DROP TABLE IF EXISTS result_entries CASCADE" ); + $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); $self->dbh->do( "DROP TABLE IF EXISTS log_level" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); $self->dbh->do( "DROP TABLE IF EXISTS batch_jobs" ); From 65e95cac85c7eab390f14c2d1d5ca8d73d1416c4 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 27 Sep 2023 12:35:02 +0200 Subject: [PATCH 36/64] Add FOREIGN KEY constraint on result_entries.hash_id --- lib/Zonemaster/Backend/DB/MySQL.pm | 3 ++- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 4 ++-- lib/Zonemaster/Backend/DB/SQLite.pm | 2 +- t/db_ddl.t | 20 ++++++++++++++++++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index e9a50b16a..de5c619f3 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -150,7 +150,6 @@ sub create_schema { #################################################################### $dbh->do( "CREATE TABLE IF NOT EXISTS result_entries ( - id BIGINT AUTO_INCREMENT PRIMARY KEY, hash_id VARCHAR(16) NOT NULL, level VARCHAR(15) NOT NULL, module VARCHAR(255) NOT NULL, @@ -159,6 +158,7 @@ sub create_schema { timestamp REAL NOT NULL, args BLOB NOT NULL, + CONSTRAINT fk_hash_id FOREIGN KEY (hash_id) REFERENCES test_results(hash_id), CONSTRAINT fk_level FOREIGN KEY (level) REFERENCES log_level(level) ) ENGINE=InnoDB " @@ -218,6 +218,7 @@ sub drop_tables { my ( $self ) = @_; # remove any FOREIGN KEY before droping the table + $self->dbh->do( "ALTER TABLE IF EXISTS result_entries DROP FOREIGN KEY IF EXISTS fk_hash_id" ); $self->dbh->do( "ALTER TABLE IF EXISTS result_entries DROP FOREIGN KEY IF EXISTS fk_level" ); $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index ee586de1b..7b96030d9 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -135,7 +135,6 @@ sub create_schema { #################################################################### $dbh->do( 'CREATE TABLE IF NOT EXISTS result_entries ( - id BIGSERIAL PRIMARY KEY, hash_id VARCHAR(16) NOT NULL, level VARCHAR(15) NOT NULL, module VARCHAR(255) NOT NULL, @@ -144,6 +143,7 @@ sub create_schema { timestamp REAL NOT NULL, args JSONb NOT NULL, + CONSTRAINT fk_hash_id FOREIGN KEY (hash_id) REFERENCES test_results(hash_id), CONSTRAINT fk_level FOREIGN KEY(level) REFERENCES log_level(level) ) ' @@ -203,7 +203,7 @@ sub drop_tables { $self->dbh->do( "SET client_min_messages = warning" ); try { - $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); + $self->dbh->do( "DROP TABLE IF EXISTS test_results CASCADE" ); $self->dbh->do( "DROP TABLE IF EXISTS result_entries CASCADE" ); $self->dbh->do( "DROP TABLE IF EXISTS log_level" ); $self->dbh->do( "DROP TABLE IF EXISTS users" ); diff --git a/lib/Zonemaster/Backend/DB/SQLite.pm b/lib/Zonemaster/Backend/DB/SQLite.pm index eee207cbc..3802f1f52 100644 --- a/lib/Zonemaster/Backend/DB/SQLite.pm +++ b/lib/Zonemaster/Backend/DB/SQLite.pm @@ -134,7 +134,6 @@ sub create_schema { #################################################################### $dbh->do( 'CREATE TABLE IF NOT EXISTS result_entries ( - id INTEGER PRIMARY KEY AUTOINCREMENT, hash_id VARCHAR(16) NOT NULL, level VARCHAR(15) NOT NULL, module VARCHAR(255) NOT NULL, @@ -143,6 +142,7 @@ sub create_schema { timestamp REAL NOT NULL, args BLOB NOT NULL, + FOREIGN KEY(hash_id) REFERENCES test_results(hash_id), FOREIGN KEY(level) REFERENCES log_level(level) ) ' diff --git a/t/db_ddl.t b/t/db_ddl.t index d82ef5151..962fe637d 100644 --- a/t/db_ddl.t +++ b/t/db_ddl.t @@ -107,6 +107,22 @@ subtest 'Everything but Test::NoWarnings' => sub { }; subtest 'constraint on foreign key' => sub { + subtest 'result_entries - hash_id should exist in test_results(hash_id)' => sub { + my $hash_id_ok = "0123456789abcdef"; + my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) + VALUES ('$hash_id_ok', 'INFO', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; + my $inserted_rows = $db->dbh->do( $sql ); + is $inserted_rows, 1, 'can insert an entry with an existing hash_id'; + + throws_ok { + my $hash_id_ko = "aaaaaaaaaaaaaaaa"; + my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) + VALUES ('$hash_id_ko', 'INFO', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; + $db->dbh->do( $sql ); + } + qr/foreign key/i, 'cannot insert an entry with an non-existing hash_id'; + }; + subtest 'result_entries - level should exist in log_level(level)' => sub { my $level = "INFO"; my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) @@ -121,13 +137,13 @@ subtest 'Everything but Test::NoWarnings' => sub { $db->dbh->do( $sql ); } qr/foreign key/i, 'cannot insert an entry with an non-existing level'; - } + }; }; }; }; # FIXME: hack to avoid getting warnings from Test::NoWarnings my @warn = warnings(); -if ( @warn == 6 ) { +if ( @warn == 7 ) { clear_warnings(); } From 878883f4b94cfa84a88abad88231f840ad16665f Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 27 Sep 2023 12:42:49 +0200 Subject: [PATCH 37/64] Update migration script --- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 77 +------------------ 1 file changed, 4 insertions(+), 73 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index a474580ce..05fc23636 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -83,31 +83,7 @@ sub patch_db_mysql { $dbh->{AutoCommit} = 0; try { - $dbh->do( - "CREATE TABLE IF NOT EXISTS result_entries ( - id integer AUTO_INCREMENT PRIMARY KEY, - hash_id VARCHAR(16) not null, - level ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL') not null, - module VARCHAR(255) not null, - testcase VARCHAR(255) not null, - tag VARCHAR(255) not null, - timestamp REAL not null, - args BLOB not null - ) ENGINE=InnoDB - " - ); - - my $indexes = $dbh->selectall_hashref( 'SHOW INDEXES FROM result_entries', 'Key_name' ); - if ( not exists($indexes->{result_entries__hash_id}) ) { - $dbh->do( - 'CREATE INDEX result_entries__hash_id ON result_entries (hash_id)' - ); - } - if ( not exists($indexes->{result_entries__level}) ) { - $dbh->do( - 'CREATE INDEX result_entries__level ON result_entries (level)' - ); - } + $db->create_schema(); _update_data( $dbh ); @@ -128,32 +104,7 @@ sub patch_db_postgresql { $dbh->{AutoCommit} = 0; try { - - $dbh->do( - "CREATE TYPE log_level AS ENUM ('DEBUG3', 'DEBUG2', 'DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL')" - ); - - $dbh->do( - 'CREATE TABLE IF NOT EXISTS result_entries ( - id serial primary key, - hash_id VARCHAR(16) not null, - level log_level not null, - module varchar(255) not null, - testcase varchar(255) not null, - tag varchar(255) not null, - timestamp real not null, - args json not null - ) - ' - ); - - $dbh->do( - 'CREATE INDEX IF NOT EXISTS result_entries__hash_id ON result_entries (hash_id)' - ); - - $dbh->do( - 'CREATE INDEX IF NOT EXISTS result_entries__level ON result_entries (level)' - ); + $db->create_schema(); $dbh->do(q[ INSERT INTO result_entries ( @@ -164,7 +115,7 @@ sub patch_db_postgresql { hash_id, (CASE WHEN res->'args' IS NULL THEN '{}' ELSE res->'args' END) AS args, res->>'module' AS module, - (res->>'level')::log_level AS level, + (res->>'level') AS level, res->>'tag' AS tag, (res->>'timestamp')::real AS timestamp, (CASE WHEN res->>'testcase' IS NULL THEN '' ELSE res->>'testcase' END) AS testcase @@ -199,27 +150,7 @@ sub patch_db_sqlite { $dbh->{AutoCommit} = 0; try { - $dbh->do( - 'CREATE TABLE IF NOT EXISTS result_entries ( - id integer PRIMARY KEY AUTOINCREMENT, - hash_id VARCHAR(16) not null, - level varchar(15) not null, - module varchar(255) not null, - testcase varchar(255) not null, - tag varchar(255) not null, - timestamp real not null, - args blob not null - ) - ' - ); - - $dbh->do( - 'CREATE INDEX IF NOT EXISTS result_entries__hash_id ON result_entries (hash_id)' - ); - - $dbh->do( - 'CREATE INDEX IF NOT EXISTS result_entries__level ON result_entries (level)' - ); + $db->create_schema(); _update_data( $dbh ); From 38187ee1d5b42fa51c3cc98004d9ea87f97f6fbe Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Thu, 28 Sep 2023 14:25:32 +0200 Subject: [PATCH 38/64] Support MariaDB <10.4 and MySQL These database engines do not support the IF EXISTS syntax on ALTER TABLE and DROP FOREIGN KEY. --- lib/Zonemaster/Backend/DB/MySQL.pm | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index de5c619f3..853a8e8a9 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -218,8 +218,21 @@ sub drop_tables { my ( $self ) = @_; # remove any FOREIGN KEY before droping the table - $self->dbh->do( "ALTER TABLE IF EXISTS result_entries DROP FOREIGN KEY IF EXISTS fk_hash_id" ); - $self->dbh->do( "ALTER TABLE IF EXISTS result_entries DROP FOREIGN KEY IF EXISTS fk_level" ); + # MariaDB <10.4 and MySQL do not support the IF EXISTS syntax + # on ALTER TABLE and DROP FOREIGN KEY + # MariaDB 10.3 is used on Ubuntu 20.04 LTS (eol 2023-04) + # MySQL is used on FreeBSD + my $tables = $self->dbh->selectall_hashref( 'SHOW TABLE STATUS', 'Name' ); + if ( exists $tables->{result_entries} ) { + my @fk = $self->dbh->selectall_array( 'SELECT constraint_name FROM information_schema.referential_constraints' ); + @fk = map { ref eq 'ARRAY' ? @$_ : $_ } @fk; + if ( grep( /^fk_hash_id$/, @fk ) ) { + $self->dbh->do( "ALTER TABLE result_entries DROP FOREIGN KEY fk_hash_id" ); + } + if ( grep( /^fk_level$/, @fk ) ) { + $self->dbh->do( "ALTER TABLE result_entries DROP FOREIGN KEY fk_level" ); + } + } $self->dbh->do( "DROP TABLE IF EXISTS test_results" ); $self->dbh->do( "DROP TABLE IF EXISTS result_entries" ); From d2a4d1c971a71bf0bf4f3f2f2ac72d615b9f53fb Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Wed, 27 Sep 2023 18:57:23 +0200 Subject: [PATCH 39/64] Store the level as integer instead of string --- lib/Zonemaster/Backend/DB.pm | 40 +++++++++++++++---------- lib/Zonemaster/Backend/DB/MySQL.pm | 26 ++++++++-------- lib/Zonemaster/Backend/DB/PostgreSQL.pm | 26 ++++++++-------- lib/Zonemaster/Backend/DB/SQLite.pm | 26 ++++++++-------- t/db_ddl.t | 9 +++--- 5 files changed, 68 insertions(+), 59 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index bbcf555e6..706a6f04d 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -462,13 +462,15 @@ sub test_results { my @result_entries = $self->dbh->selectall_array( q[ SELECT - level, - module, - testcase, - tag, - timestamp, - args - FROM result_entries + l.level, + r.module, + r.testcase, + r.tag, + r.timestamp, + r.args + FROM result_entries r + INNER JOIN log_level l + ON r.level = l.value WHERE hash_id = ? ], { Slice => {} }, @@ -509,9 +511,9 @@ sub get_test_history { my @results; my $query = q[ SELECT - (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = 'CRITICAL') AS nb_critical, - (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = 'ERROR') AS nb_error, - (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = 'WARNING') AS nb_warning, + (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = ?) AS nb_critical, + (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = ?) AS nb_error, + (SELECT count(*) FROM result_entries WHERE result_entries.hash_id = test_results.hash_id AND level = ?) AS nb_warning, id, hash_id, created_at, @@ -524,11 +526,15 @@ sub get_test_history { my $sth = $dbh->prepare( $query ); - $sth->bind_param( 1, _normalize_domain( $p->{frontend_params}{domain} ) ); - $sth->bind_param( 2, $undelegated, SQL_INTEGER ); - $sth->bind_param( 3, $undelegated, SQL_INTEGER ); - $sth->bind_param( 4, $p->{limit} ); - $sth->bind_param( 5, $p->{offset} ); + my %levels = Zonemaster::Engine::Logger::Entry->levels(); + $sth->bind_param( 1, $levels{CRITICAL} ); + $sth->bind_param( 2, $levels{ERROR} ); + $sth->bind_param( 3, $levels{WARNING} ); + $sth->bind_param( 4, _normalize_domain( $p->{frontend_params}{domain} ) ); + $sth->bind_param( 5, $undelegated, SQL_INTEGER ); + $sth->bind_param( 6, $undelegated, SQL_INTEGER ); + $sth->bind_param( 7, $p->{limit} ); + $sth->bind_param( 8, $p->{offset} ); $sth->execute(); @@ -1000,10 +1006,12 @@ sub add_result_entries { my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; + my %levels = Zonemaster::Engine::Logger::Entry->levels(); + foreach my $e ( @entries ) { my $r = [ $hash_id, - $e->level, + $levels{ $e->level }, $e->module, $e->testcase, $e->tag, diff --git a/lib/Zonemaster/Backend/DB/MySQL.pm b/lib/Zonemaster/Backend/DB/MySQL.pm index 853a8e8a9..86ec1d7df 100644 --- a/lib/Zonemaster/Backend/DB/MySQL.pm +++ b/lib/Zonemaster/Backend/DB/MySQL.pm @@ -120,10 +120,10 @@ sub create_schema { #################################################################### $dbh->do( "CREATE TABLE IF NOT EXISTS log_level ( - level VARCHAR(15), value INT, + level VARCHAR(15), - UNIQUE (level) + UNIQUE (value) ) ENGINE=InnoDB " ) or die Zonemaster::Backend::Error::Internal->new( reason => "MySQL error, could not create 'log_level' table", data => $dbh->errstr() ); @@ -131,16 +131,16 @@ sub create_schema { my ( $c ) = $dbh->selectrow_array( "SELECT count(*) FROM log_level" ); if ( $c == 0 ) { $dbh->do( - "INSERT INTO log_level (level, value) + "INSERT INTO log_level (value, level) VALUES - ('DEBUG3' , -2), - ('DEBUG2' , -1), - ('DEBUG' , 0), - ('INFO' , 1), - ('NOTICE' , 2), - ('WARNING' , 3), - ('ERROR' , 4), - ('CRITICAL' , 5) + (-2, 'DEBUG3'), + (-1, 'DEBUG2'), + ( 0, 'DEBUG'), + ( 1, 'INFO'), + ( 2, 'NOTICE'), + ( 3, 'WARNING'), + ( 4, 'ERROR'), + ( 5, 'CRITICAL') " ); } @@ -151,7 +151,7 @@ sub create_schema { $dbh->do( "CREATE TABLE IF NOT EXISTS result_entries ( hash_id VARCHAR(16) NOT NULL, - level VARCHAR(15) NOT NULL, + level INT NOT NULL, module VARCHAR(255) NOT NULL, testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, @@ -159,7 +159,7 @@ sub create_schema { args BLOB NOT NULL, CONSTRAINT fk_hash_id FOREIGN KEY (hash_id) REFERENCES test_results(hash_id), - CONSTRAINT fk_level FOREIGN KEY (level) REFERENCES log_level(level) + CONSTRAINT fk_level FOREIGN KEY (level) REFERENCES log_level(value) ) ENGINE=InnoDB " ) or die Zonemaster::Backend::Error::Internal->new( reason => "MySQL error, could not create 'result_entries' table", data => $dbh->errstr() ); diff --git a/lib/Zonemaster/Backend/DB/PostgreSQL.pm b/lib/Zonemaster/Backend/DB/PostgreSQL.pm index 7b96030d9..617d12abc 100644 --- a/lib/Zonemaster/Backend/DB/PostgreSQL.pm +++ b/lib/Zonemaster/Backend/DB/PostgreSQL.pm @@ -106,26 +106,26 @@ sub create_schema { #################################################################### $dbh->do( "CREATE TABLE IF NOT EXISTS log_level ( - level VARCHAR(15), value INT, + level VARCHAR(15), - UNIQUE (level) + UNIQUE (value) ) " ) or die Zonemaster::Backend::Error::Internal->new( reason => "PostgreSQL error, could not create 'log_level' table", data => $dbh->errstr() ); my ( $c ) = $dbh->selectrow_array( "SELECT count(*) FROM log_level" ); if ( $c == 0 ) { $dbh->do( - "INSERT INTO log_level (level, value) + "INSERT INTO log_level (value, level) VALUES - ('DEBUG3' , -2), - ('DEBUG2' , -1), - ('DEBUG' , 0), - ('INFO' , 1), - ('NOTICE' , 2), - ('WARNING' , 3), - ('ERROR' , 4), - ('CRITICAL' , 5) + (-2, 'DEBUG3'), + (-1, 'DEBUG2'), + ( 0, 'DEBUG'), + ( 1, 'INFO'), + ( 2, 'NOTICE'), + ( 3, 'WARNING'), + ( 4, 'ERROR'), + ( 5, 'CRITICAL') " ); } @@ -136,7 +136,7 @@ sub create_schema { $dbh->do( 'CREATE TABLE IF NOT EXISTS result_entries ( hash_id VARCHAR(16) NOT NULL, - level VARCHAR(15) NOT NULL, + level INT NOT NULL, module VARCHAR(255) NOT NULL, testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, @@ -144,7 +144,7 @@ sub create_schema { args JSONb NOT NULL, CONSTRAINT fk_hash_id FOREIGN KEY (hash_id) REFERENCES test_results(hash_id), - CONSTRAINT fk_level FOREIGN KEY(level) REFERENCES log_level(level) + CONSTRAINT fk_level FOREIGN KEY(level) REFERENCES log_level(value) ) ' ) or die Zonemaster::Backend::Error::Internal->new( reason => "PostgreSQL error, could not create 'result_entries' table", data => $dbh->errstr() ); diff --git a/lib/Zonemaster/Backend/DB/SQLite.pm b/lib/Zonemaster/Backend/DB/SQLite.pm index 3802f1f52..e04108f89 100644 --- a/lib/Zonemaster/Backend/DB/SQLite.pm +++ b/lib/Zonemaster/Backend/DB/SQLite.pm @@ -104,10 +104,10 @@ sub create_schema { #################################################################### $dbh->do( "CREATE TABLE IF NOT EXISTS log_level ( - level VARCHAR(15), value INTEGER, + level VARCHAR(15), - UNIQUE (level) + UNIQUE (value) ) " ) or die Zonemaster::Backend::Error::Internal->new( reason => "SQLite error, could not create 'log_level' table", data => $dbh->errstr() ); @@ -115,16 +115,16 @@ sub create_schema { my ( $c ) = $dbh->selectrow_array( "SELECT count(*) FROM log_level" ); if ( $c == 0 ) { $dbh->do( - "INSERT INTO log_level (level, value) + "INSERT INTO log_level (value, level) VALUES - ('DEBUG3' , -2), - ('DEBUG2' , -1), - ('DEBUG' , 0), - ('INFO' , 1), - ('NOTICE' , 2), - ('WARNING' , 3), - ('ERROR' , 4), - ('CRITICAL' , 5) + (-2, 'DEBUG3'), + (-1, 'DEBUG2'), + ( 0, 'DEBUG'), + ( 1, 'INFO'), + ( 2, 'NOTICE'), + ( 3, 'WARNING'), + ( 4, 'ERROR'), + ( 5, 'CRITICAL') " ); } @@ -135,7 +135,7 @@ sub create_schema { $dbh->do( 'CREATE TABLE IF NOT EXISTS result_entries ( hash_id VARCHAR(16) NOT NULL, - level VARCHAR(15) NOT NULL, + level INT NOT NULL, module VARCHAR(255) NOT NULL, testcase VARCHAR(255) NOT NULL, tag VARCHAR(255) NOT NULL, @@ -143,7 +143,7 @@ sub create_schema { args BLOB NOT NULL, FOREIGN KEY(hash_id) REFERENCES test_results(hash_id), - FOREIGN KEY(level) REFERENCES log_level(level) + FOREIGN KEY(level) REFERENCES log_level(value) ) ' ) or die Zonemaster::Backend::Error::Internal->new( reason => "SQLite error, could not create 'result_entries' table", data => $dbh->errstr() ); diff --git a/t/db_ddl.t b/t/db_ddl.t index 962fe637d..2cef57227 100644 --- a/t/db_ddl.t +++ b/t/db_ddl.t @@ -109,29 +109,30 @@ subtest 'Everything but Test::NoWarnings' => sub { subtest 'constraint on foreign key' => sub { subtest 'result_entries - hash_id should exist in test_results(hash_id)' => sub { my $hash_id_ok = "0123456789abcdef"; + # INFO is 1 my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) - VALUES ('$hash_id_ok', 'INFO', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; + VALUES ('$hash_id_ok', 1, 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; my $inserted_rows = $db->dbh->do( $sql ); is $inserted_rows, 1, 'can insert an entry with an existing hash_id'; throws_ok { my $hash_id_ko = "aaaaaaaaaaaaaaaa"; my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) - VALUES ('$hash_id_ko', 'INFO', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; + VALUES ('$hash_id_ko', 1, 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; $db->dbh->do( $sql ); } qr/foreign key/i, 'cannot insert an entry with an non-existing hash_id'; }; subtest 'result_entries - level should exist in log_level(level)' => sub { - my $level = "INFO"; + my $level = 1; # INFO my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES ('0123456789abcdef', '$level', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; my $inserted_rows = $db->dbh->do( $sql ); is $inserted_rows, 1, 'can insert an entry with an existing level'; throws_ok { - my $level = "DOESNTEXIST"; + my $level = 42; # does not exist my $sql = "INSERT INTO result_entries (hash_id, level, module, testcase, tag, timestamp, args) VALUES ('0123456789abcdef', '$level', 'MODULE', 'TESTCASE', 'TAG', 42, '{}')"; $db->dbh->do( $sql ); From 2022a7b068d37fea10e3107dbd8774f00520ccbd Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Mon, 6 Nov 2023 14:17:21 +0100 Subject: [PATCH 40/64] Update after review * remove unnecessary comments * rename a variable * rename a metric --- lib/Zonemaster/Backend/DB.pm | 1 - lib/Zonemaster/Backend/TestAgent.pm | 11 +++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 706a6f04d..c88cce8a7 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -866,7 +866,6 @@ sub process_dead_test { timestamp => $self->get_relative_start_time($hash_id) } ); - #$msg = bless( $msg, "Zonemaster::Engine::Logger::Entry" ); $self->force_end_test($hash_id, $msg); } diff --git a/lib/Zonemaster/Backend/TestAgent.pm b/lib/Zonemaster/Backend/TestAgent.pm index 7aed970dd..a62e9ed14 100644 --- a/lib/Zonemaster/Backend/TestAgent.pm +++ b/lib/Zonemaster/Backend/TestAgent.pm @@ -10,7 +10,6 @@ use JSON::PP; use Scalar::Util qw( blessed ); use File::Slurp; use Locale::TextDomain qw[Zonemaster-Backend]; -use Log::Any qw( $log ); use Time::HiRes qw[time sleep gettimeofday tv_interval]; use Zonemaster::LDNS; @@ -138,22 +137,18 @@ sub run { } } - #Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_duration", $callback_duration * 1000); - - my $start_time_2 = [ gettimeofday ]; + my $insert_result_start_time = [ gettimeofday ]; # TODO: Make minimum level configurable my @entries = grep { $_->numeric_level >= $numeric{INFO} } @{ Zonemaster::Engine->logger->entries }; - Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_add_result_entry_grep_duration", tv_interval($start_time_2) * 1000); + Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_add_result_entry_filter_duration", tv_interval($insert_result_start_time) * 1000); $self->{_db}->add_result_entries( $test_id, @entries); - my $callback_add_result_entry_duration = tv_interval($start_time_2); + my $callback_add_result_entry_duration = tv_interval($insert_result_start_time); Zonemaster::Backend::Metrics::timing("zonemaster.testagent.log_callback_add_result_entry_duration", $callback_add_result_entry_duration * 1000); - #$log->debug("Callback timing for $test_id: $callback_duration / $callback_add_result_entry_duration "); - $self->{_db}->set_test_completed( $test_id ); return; From 33e32e4927fd9263097b10c222252928befbd9cc Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Thu, 16 Nov 2023 17:41:14 +0100 Subject: [PATCH 41/64] Fix upgrade script * output the total number of updated rows at the end (SQLite and MySQL) * store level value * only update job with results (SQLite and MySQL) --- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 05fc23636..d36419525 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -29,16 +29,20 @@ sub _update_data { my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; - my ( $row_total ) = $dbh->selectrow_array( 'SELECT count(*) FROM test_results' ); + # update only jobs with results + my ( $row_total ) = $dbh->selectrow_array( 'SELECT count(*) FROM test_results WHERE results IS NOT NULL' ); print "count: $row_total\n"; + my %levels = Zonemaster::Engine::Logger::Entry->levels(); + # depending on the resources available to select all data in database # update $row_count to your needs my $row_count = 50000; my $row_done = 0; while ( $row_done < $row_total ) { print "row_done/row_total: $row_done / $row_total\n"; - my $sth1 = $dbh->prepare( 'SELECT hash_id, results FROM test_results ORDER BY id ASC LIMIT ?,?' ); + my $row_updated = 0; + my $sth1 = $dbh->prepare( 'SELECT hash_id, results FROM test_results WHERE results IS NOT NULL ORDER BY id ASC LIMIT ?,?' ); $sth1->execute( $row_done, $row_count ); while ( my $row = $sth1->fetchrow_arrayref ) { my ( $hash_id, $results ) = @$row; @@ -51,7 +55,7 @@ sub _update_data { foreach my $m ( @$entries ) { my $r = [ $hash_id, - $m->{level}, + $levels{ $m->{level} }, $m->{module}, $m->{testcase} // '', $m->{tag}, @@ -67,11 +71,13 @@ sub _update_data { my $sth = $dbh->prepare( $query ); $sth = $sth->execute( map { @$_ } @records ); - $dbh->do( "UPDATE test_results SET results = NULL WHERE hash_id = ?", undef, $hash_id ); + $row_updated += $dbh->do( "UPDATE test_results SET results = NULL WHERE hash_id = ?", undef, $hash_id ); } - $row_done += $row_count; + # increase by min(row_updated, row_count) + $row_done += ( $row_updated < $row_count ) ? $row_updated : $row_count; } + print "row_done/row_total: $row_done / $row_total\n"; } sub patch_db_mysql { @@ -115,7 +121,7 @@ sub patch_db_postgresql { hash_id, (CASE WHEN res->'args' IS NULL THEN '{}' ELSE res->'args' END) AS args, res->>'module' AS module, - (res->>'level') AS level, + (SELECT value FROM log_level WHERE level = (res->>'level')) AS level, res->>'tag' AS tag, (res->>'timestamp')::real AS timestamp, (CASE WHEN res->>'testcase' IS NULL THEN '' ELSE res->>'testcase' END) AS testcase From b380c85a15462e38e5f6e32539b878f78b4dca19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Tue, 24 Oct 2023 15:10:02 +0200 Subject: [PATCH 42/64] use name normalization method --- lib/Zonemaster/Backend/DB.pm | 7 ++++--- lib/Zonemaster/Backend/RPCAPI.pm | 10 ++++------ lib/Zonemaster/Backend/Validator.pm | 31 +++++------------------------ 3 files changed, 13 insertions(+), 35 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index c88cce8a7..8ef7c819e 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -16,6 +16,7 @@ use POSIX qw( strftime ); use Readonly; use Try::Tiny; +use Zonemaster::Engine::Normalization; use Zonemaster::Backend::Errors; use Zonemaster::Engine::Logger::Entry; @@ -874,10 +875,10 @@ sub process_dead_test { sub _normalize_domain { my ( $domain ) = @_; - $domain = lc( $domain ); - $domain =~ s/\.$// unless $domain eq '.'; + # Discard errors, at this point errors have already checked + my ( $_errors, $normalized_domain ) = normalize_name( $domain ); - return $domain; + return $normalized_domain; } sub _project_params { diff --git a/lib/Zonemaster/Backend/RPCAPI.pm b/lib/Zonemaster/Backend/RPCAPI.pm index 87290a12b..48d98830e 100644 --- a/lib/Zonemaster/Backend/RPCAPI.pm +++ b/lib/Zonemaster/Backend/RPCAPI.pm @@ -22,6 +22,7 @@ use Encode; # Zonemaster Modules use Zonemaster::Engine; +use Zonemaster::Engine::Normalization; use Zonemaster::Engine::Profile; use Zonemaster::Engine::Recursor; use Zonemaster::Backend; @@ -223,16 +224,17 @@ sub get_data_from_parent_zone { my $result = eval { my %result; my $domain = $params->{domain}; + my ( $_errors, $normalized_domain ) = normalize_name( $domain ); my @ns_list; my @ns_names; - my $zone = Zonemaster::Engine->zone( $domain ); + my $zone = Zonemaster::Engine->zone( $normalized_domain ); push @ns_list, { ns => $_->name->string, ip => $_->address->short} for @{$zone->glue}; my @ds_list; - $zone = Zonemaster::Engine->zone($domain); + $zone = Zonemaster::Engine->zone($normalized_domain); my $ds_p = $zone->parent->query_one( $zone->name, 'DS', { dnssec => 1, cd => 1, recurse => 1 } ); if ($ds_p) { my @ds = $ds_p->get_records( 'DS', 'answer' ); @@ -291,10 +293,6 @@ sub start_domain_test { my $result = 0; eval { - $params->{domain} =~ s/^\.// unless ( !$params->{domain} || $params->{domain} eq '.' ); - - die "No domain in parameters\n" unless ( defined $params->{domain} && length($params->{domain}) ); - $params->{profile} //= "default"; $params->{priority} //= 10; $params->{queue} //= 0; diff --git a/lib/Zonemaster/Backend/Validator.pm b/lib/Zonemaster/Backend/Validator.pm index 9c85d8a3f..d23bc377f 100644 --- a/lib/Zonemaster/Backend/Validator.pm +++ b/lib/Zonemaster/Backend/Validator.pm @@ -13,6 +13,7 @@ use Readonly; use Locale::TextDomain qw[Zonemaster-Backend]; use Net::IP::XS; use Zonemaster::Engine::Logger::Entry; +use Zonemaster::Engine::Normalization; use Zonemaster::LDNS; our @EXPORT_OK = qw( @@ -281,35 +282,13 @@ sub check_domain { return N__ 'Domain name required'; } - if ( $domain =~ m/[^[:ascii:]]+/ ) { - if ( Zonemaster::LDNS::has_idn() ) { - eval { $domain = Zonemaster::LDNS::to_idn( $domain ); }; - if ( $@ ) { - return N__ 'The domain name is IDNA invalid'; - } - } - else { - return N__ 'The domain name contains non-ascii characters and IDNA support is not installed'; - } - } - - if ( $domain !~ m{^[a-z0-9_./-]+$}i ) { - return N__ 'The domain name character(s) are not supported'; - } - - if ( $domain =~ m/\.\./i ) { - return N__ 'The domain name contains consecutive dots'; - } + my ( $errors, $_domain ) = normalize_name( $domain ); - my %levels = Zonemaster::Engine::Logger::Entry::levels(); - my @res; - @res = Zonemaster::Engine::Test::Basic->basic00( $domain ); - @res = grep { $_->numeric_level >= $levels{ERROR} } @res; - if ( @res != 0 ) { - return N__ 'The domain name or label is too long'; + if ( @{$errors} != 0 ) { + return @{$errors}[0]->message; } - return undef; + return undef } =head2 check_language_tag($value, %locales) From 8b28668f0cadbf82c8074e9f8ad0b78ae8c13672 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Mon, 6 Nov 2023 10:22:35 +0100 Subject: [PATCH 43/64] fix typo in comment --- lib/Zonemaster/Backend/DB.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 8ef7c819e..3aa5e9fa1 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -875,7 +875,7 @@ sub process_dead_test { sub _normalize_domain { my ( $domain ) = @_; - # Discard errors, at this point errors have already checked + # Discard errors, at this point errors have already been checked my ( $_errors, $normalized_domain ) = normalize_name( $domain ); return $normalized_domain; From a8ab03d7554eec270593989208c84a56ae1263e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Mon, 6 Nov 2023 10:27:59 +0100 Subject: [PATCH 44/64] use suggestion --- lib/Zonemaster/Backend/Validator.pm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Zonemaster/Backend/Validator.pm b/lib/Zonemaster/Backend/Validator.pm index d23bc377f..9a8dca524 100644 --- a/lib/Zonemaster/Backend/Validator.pm +++ b/lib/Zonemaster/Backend/Validator.pm @@ -284,8 +284,8 @@ sub check_domain { my ( $errors, $_domain ) = normalize_name( $domain ); - if ( @{$errors} != 0 ) { - return @{$errors}[0]->message; + if ( @{$errors} ) { + return $errors->[0]->message; } return undef From 192a77d9f704f7aa862d22d58ca3cf905049bec2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Mon, 6 Nov 2023 10:28:33 +0100 Subject: [PATCH 45/64] fix unit test --- t/db.t | 4 ++-- t/parameters_validation.t | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/t/db.t b/t/db.t index 6d85faed8..b9ea2cd36 100644 --- a/t/db.t +++ b/t/db.t @@ -170,8 +170,8 @@ subtest 'encoding and fingerprint' => sub { }; subtest 'IDN domain' => sub { - my $expected_encoded_params = encode_utf8( '{"domain":"café.example","ds_info":[],"ipv4":true,"ipv6":true,"nameservers":[],"profile":"default"}' ); - my $expected_fingerprint = '8c64f7feaa3f13b77e769720991f2a79'; + my $expected_encoded_params = encode_utf8( '{"domain":"xn--caf-dma.example","ds_info":[],"ipv4":true,"ipv6":true,"nameservers":[],"profile":"default"}' ); + my $expected_fingerprint = '8cb027ff2c175f48aed2623abad0cdd2'; my %params = ( domain => "café.example" ); $params{ipv4} = JSON::PP->true; diff --git a/t/parameters_validation.t b/t/parameters_validation.t index 550165029..f0a5331eb 100644 --- a/t/parameters_validation.t +++ b/t/parameters_validation.t @@ -218,7 +218,7 @@ subtest 'Test custom formats' => sub { }, output => [{ path => '/my_domain', - message => 'The domain name character(s) are not supported' + message => 'Domain name has an ASCII label ("not a domain") with a character not permitted.' }] }, { From 18f7b8b2e1608e397b299e337f4c8f4f767b7a3f Mon Sep 17 00:00:00 2001 From: Mats Dufberg Date: Thu, 23 Nov 2023 13:50:02 +0100 Subject: [PATCH 46/64] Adds missing files in MANIFEST --- MANIFEST | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MANIFEST b/MANIFEST index cd3641124..915e08795 100644 --- a/MANIFEST +++ b/MANIFEST @@ -76,10 +76,12 @@ t/00-load.t t/batches.t t/config.t t/db.t +t/db_ddl.t t/idn.data t/idn.t t/lifecycle.t t/parameters_validation.t +t/queue.t t/rpc_validation.t t/test01.data t/test01.t From 1a3fbe051681128cbe3dac7b6c050a0383e8baa8 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Mon, 4 Dec 2023 14:57:11 +0100 Subject: [PATCH 47/64] CI: Add missing dependency Zonemaster-Engine relies on Mail::SPF and there is a bug preventing its installation from cpanm. See Zonemaster-Engine commit `3ac72b0c` and/or pull request for more information on this. --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index d12a3227e..478e9a9bc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -45,6 +45,7 @@ addons: - liblist-moreutils-perl - liblocale-msgfmt-perl - libmail-rfc822-address-perl + - libmail-spf-perl - libmodule-find-perl - libnet-ip-xs-perl - libpod-coverage-perl From 09f203fa688f59e25302b973dfff43d8d913d761 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Mon, 4 Dec 2023 15:01:01 +0100 Subject: [PATCH 48/64] CI: update Perl versions --- .travis.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index 478e9a9bc..f71a13899 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,7 +2,7 @@ dist: focal language: perl -perl: "5.32" +perl: "5.36" stages: - test @@ -19,11 +19,11 @@ jobs: - env: TARGET=PostgreSQL ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_postgresql_backend_config.ini services: postgresql # Cover supported Perl versions with SQLite - - perl: "5.30.2" + - perl: "5.36" env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini - - perl: "5.26" + - perl: "5.30" env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini - - perl: "5.16" + - perl: "5.26" dist: bionic env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini From e2898c1ccdff575d08ce3981dadce3866cc102c1 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Mon, 4 Dec 2023 16:06:19 +0100 Subject: [PATCH 49/64] CI: update Ubuntu versions --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index f71a13899..549305628 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,4 +1,4 @@ -dist: focal +dist: jammy language: perl @@ -24,7 +24,7 @@ jobs: - perl: "5.30" env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini - perl: "5.26" - dist: bionic + dist: focal env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini addons: From 6c85dbddb2f93be59ace48f8a6a3807e26a08e0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Tue, 5 Dec 2023 15:24:40 +0100 Subject: [PATCH 50/64] update database migration script --- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 66 +++++++++++++++++-- 1 file changed, 59 insertions(+), 7 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index d36419525..120808ead 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -15,23 +15,25 @@ ); my $db_engine = $config->DB_engine; -print "engine: $db_engine\n"; +print "Configured database engine: $db_engine\n"; if ( $db_engine =~ /^(MySQL|PostgreSQL|SQLite)$/ ) { + print( "Starting database migration\n" ); $patch{ lc $db_engine }(); + print( "\nMigration done\n" ); } else { die "Unknown database engine configured: $db_engine\n"; } -sub _update_data { +sub _update_data_result_entries { my ( $dbh ) = @_; my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; # update only jobs with results my ( $row_total ) = $dbh->selectrow_array( 'SELECT count(*) FROM test_results WHERE results IS NOT NULL' ); - print "count: $row_total\n"; + print "Will update $row_total rows\n"; my %levels = Zonemaster::Engine::Logger::Entry->levels(); @@ -40,7 +42,7 @@ sub _update_data { my $row_count = 50000; my $row_done = 0; while ( $row_done < $row_total ) { - print "row_done/row_total: $row_done / $row_total\n"; + print "Progress update: $row_done / $row_total\n"; my $row_updated = 0; my $sth1 = $dbh->prepare( 'SELECT hash_id, results FROM test_results WHERE results IS NOT NULL ORDER BY id ASC LIMIT ?,?' ); $sth1->execute( $row_done, $row_count ); @@ -77,7 +79,43 @@ sub _update_data { # increase by min(row_updated, row_count) $row_done += ( $row_updated < $row_count ) ? $row_updated : $row_count; } - print "row_done/row_total: $row_done / $row_total\n"; + print "Progress update: $row_done / $row_total\n"; +} + +sub _update_data_nomalize_domains { + my ( $db ) = @_; + + my ( $row_total ) = $db->dbh->selectrow_array( 'SELECT count(*) FROM test_results' ); + print "Will update $row_total rows\n"; + + + my $sth1 = $db->dbh->prepare( 'SELECT hash_id, params FROM test_results' ); + $sth1->execute; + + my $row_done = 0; + my $progress = 0; + + while ( my $row = $sth1->fetchrow_hashref ) { + my $hash_id = $row->{hash_id}; + my $raw_params = decode_json($row->{params}); + my $domain = $raw_params->{domain}; + + # This has never been cleaned + delete $raw_params->{user_ip}; + + my $params = $db->encode_params( $raw_params ); + my $fingerprint = $db->generate_fingerprint( $raw_params ); + + $domain = Zonemaster::Backend::DB::_normalize_domain( $domain ); + + $db->dbh->do('UPDATE test_results SET domain = ?, params = ?, fingerprint = ? where hash_id = ?', undef, $domain, $params, $fingerprint, $hash_id); + $row_done += 1; + my $new_progress = int(($row_done / $row_total) * 100); + if ( $new_progress != $progress ) { + $progress = $new_progress; + print("$progress%\n"); + } + } } sub patch_db_mysql { @@ -91,7 +129,11 @@ sub patch_db_mysql { try { $db->create_schema(); - _update_data( $dbh ); + print( "\n-> (1/2) Populating new result_entries table\n" ); + _update_data_result_entries( $dbh ); + + print( "\n-> (2/2) Normalizing domain names\n" ); + _update_data_nomalize_domains( $db ); $dbh->commit(); } catch { @@ -112,6 +154,8 @@ sub patch_db_postgresql { try { $db->create_schema(); + print( "\n-> (1/2) Populating new result_entries table\n" ); + $dbh->do(q[ INSERT INTO result_entries ( hash_id, args, module, level, tag, timestamp, testcase @@ -139,6 +183,10 @@ sub patch_db_postgresql { 'UPDATE test_results SET results = NULL WHERE results IS NOT NULL' ); + + print( "\n-> (2/2) Normalizing domain names\n" ); + _update_data_nomalize_domains( $db ); + $dbh->commit(); } catch { print( "Could not upgrade database: " . $_ ); @@ -158,7 +206,11 @@ sub patch_db_sqlite { try { $db->create_schema(); - _update_data( $dbh ); + print( "\n-> (1/2) Populating new result_entries table\n" ); + _update_data_result_entries( $dbh ); + + print( "\n-> (2/2) Normalizing domain names\n" ); + _update_data_nomalize_domains( $db ); $dbh->commit(); } catch { From a6737968509dece33f714d9e68e5124aa38f1095 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Tue, 5 Dec 2023 15:39:32 +0100 Subject: [PATCH 51/64] die if errors occur in normaliz_domain method --- lib/Zonemaster/Backend/DB.pm | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index 3aa5e9fa1..e66acd65c 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -875,8 +875,11 @@ sub process_dead_test { sub _normalize_domain { my ( $domain ) = @_; - # Discard errors, at this point errors have already been checked - my ( $_errors, $normalized_domain ) = normalize_name( $domain ); + my ( $errors, $normalized_domain ) = normalize_name( $domain ); + + if ( scalar( @{$errors} ) ) { + die Zonemaster::Backend::Error::Internal->new( reason => "Normalizing domain returned errors.", data => $errors ); + } return $normalized_domain; } From 4365edacbb67cdb256df57929333103a4bc4f9e4 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Thu, 7 Dec 2023 15:03:36 +0100 Subject: [PATCH 52/64] Remove test with previous LTS Ubuntu --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 549305628..16ce52dd2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,7 +24,6 @@ jobs: - perl: "5.30" env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini - perl: "5.26" - dist: focal env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini addons: From d5fe30a195fec357c3649d63d075a967506ac5cd Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Thu, 7 Dec 2023 15:07:09 +0100 Subject: [PATCH 53/64] Remove duplicate test --- .travis.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 16ce52dd2..a04a17d71 100644 --- a/.travis.yml +++ b/.travis.yml @@ -19,8 +19,6 @@ jobs: - env: TARGET=PostgreSQL ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_postgresql_backend_config.ini services: postgresql # Cover supported Perl versions with SQLite - - perl: "5.36" - env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini - perl: "5.30" env: TARGET=SQLite ZONEMASTER_RECORD=0 ZONEMASTER_BACKEND_CONFIG_FILE=./share/travis_sqlite_backend_config.ini - perl: "5.26" From 7c461560d69b6782b6969146bbfc5d79a09eb3d0 Mon Sep 17 00:00:00 2001 From: Alexandre Pion Date: Thu, 7 Dec 2023 15:59:02 +0100 Subject: [PATCH 54/64] CI: Ubuntu 22.04 comes with PostgreSQL 14 --- .travis.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index a04a17d71..39edaf9ff 100644 --- a/.travis.yml +++ b/.travis.yml @@ -114,9 +114,9 @@ before_install: - mkdir -p ./lib/auto/share/dist/ - ln -s ../../../../share ./lib/auto/share/dist/Zonemaster-Backend - # Change default PostgreSQL 12 port to 5432 - - if [[ "$TARGET" == "PostgreSQL" ]]; then sudo sed -i 's/port = 5433/port = 5432/' /etc/postgresql/12/main/postgresql.conf; fi - - if [[ "$TARGET" == "PostgreSQL" ]]; then sudo pg_ctlcluster 12 main restart; fi + # Change default PostgreSQL 14 port to 5432 + - if [[ "$TARGET" == "PostgreSQL" ]]; then sudo sed -i 's/port = 5433/port = 5432/' /etc/postgresql/14/main/postgresql.conf; fi + - if [[ "$TARGET" == "PostgreSQL" ]]; then sudo pg_ctlcluster 14 main restart; fi before_script: - if [[ "$TARGET" == "PostgreSQL" ]]; then psql -U travis -c "CREATE USER travis_zonemaster WITH PASSWORD 'travis_zonemaster';"; fi From 396bd31c42c6905363d6681bdbffc72a137caa11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Fri, 8 Dec 2023 14:12:09 +0100 Subject: [PATCH 55/64] ignore error in migration script --- lib/Zonemaster/Backend/DB.pm | 2 +- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 23 +++++++++++-------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/lib/Zonemaster/Backend/DB.pm b/lib/Zonemaster/Backend/DB.pm index e66acd65c..1a3e2d5fd 100644 --- a/lib/Zonemaster/Backend/DB.pm +++ b/lib/Zonemaster/Backend/DB.pm @@ -878,7 +878,7 @@ sub _normalize_domain { my ( $errors, $normalized_domain ) = normalize_name( $domain ); if ( scalar( @{$errors} ) ) { - die Zonemaster::Backend::Error::Internal->new( reason => "Normalizing domain returned errors.", data => $errors ); + die Zonemaster::Backend::Error::Internal->new( reason => "Normalizing domain returned errors.", data => [ map { $_->string } @{$errors} ] ); } return $normalized_domain; diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 120808ead..0e001f440 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -96,19 +96,24 @@ sub _update_data_nomalize_domains { my $progress = 0; while ( my $row = $sth1->fetchrow_hashref ) { - my $hash_id = $row->{hash_id}; - my $raw_params = decode_json($row->{params}); - my $domain = $raw_params->{domain}; + eval { + my $hash_id = $row->{hash_id}; + my $raw_params = decode_json($row->{params}); + my $domain = $raw_params->{domain}; - # This has never been cleaned - delete $raw_params->{user_ip}; + # This has never been cleaned + delete $raw_params->{user_ip}; - my $params = $db->encode_params( $raw_params ); - my $fingerprint = $db->generate_fingerprint( $raw_params ); + my $params = $db->encode_params( $raw_params ); + my $fingerprint = $db->generate_fingerprint( $raw_params ); - $domain = Zonemaster::Backend::DB::_normalize_domain( $domain ); + $domain = Zonemaster::Backend::DB::_normalize_domain( $domain ); - $db->dbh->do('UPDATE test_results SET domain = ?, params = ?, fingerprint = ? where hash_id = ?', undef, $domain, $params, $fingerprint, $hash_id); + $db->dbh->do('UPDATE test_results SET domain = ?, params = ?, fingerprint = ? where hash_id = ?', undef, $domain, $params, $fingerprint, $hash_id); + }; + if ($@) { + warn "Caught error while updating record, ignoring: $@\n"; + } $row_done += 1; my $new_progress = int(($row_done / $row_total) * 100); if ( $new_progress != $progress ) { From 235970356a297009892bd0b729371c6216fe0e1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABl=20Berthaud-M=C3=BCller?= Date: Mon, 11 Dec 2023 11:46:13 +0100 Subject: [PATCH 56/64] add hash id in warning message --- share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 0e001f440..6ccf28c0c 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -96,8 +96,8 @@ sub _update_data_nomalize_domains { my $progress = 0; while ( my $row = $sth1->fetchrow_hashref ) { + my $hash_id = $row->{hash_id}; eval { - my $hash_id = $row->{hash_id}; my $raw_params = decode_json($row->{params}); my $domain = $raw_params->{domain}; @@ -112,7 +112,7 @@ sub _update_data_nomalize_domains { $db->dbh->do('UPDATE test_results SET domain = ?, params = ?, fingerprint = ? where hash_id = ?', undef, $domain, $params, $fingerprint, $hash_id); }; if ($@) { - warn "Caught error while updating record, ignoring: $@\n"; + warn "Caught error while updating record with hash id $hash_id, ignoring: $@\n"; } $row_done += 1; my $new_progress = int(($row_done / $row_total) * 100); From 878823692218ae3f07057db2abaf8a07a4bd9fc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Sat, 13 Jan 2024 11:03:26 +0100 Subject: [PATCH 57/64] Fix message strings in test reports after migration The get_test_results RPCAPI method reads the result_entries.module and result_entries.testcase columns case sensitively. It expects most of their values to be capitalized (rather than all-caps). The migration script should write these values the way zm-rpcapi expects them. --- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 6ccf28c0c..61465ee3f 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -5,9 +5,15 @@ use Try::Tiny; use Zonemaster::Backend::Config; +use Zonemaster::Engine; my $config = Zonemaster::Backend::Config->load_config(); +my %module_mapping; +for my $module ( Zonemaster::Engine->modules ) { + $module_mapping{lc $module} = $module; +} + my %patch = ( mysql => \&patch_db_mysql, postgresql => \&patch_db_postgresql, @@ -55,11 +61,17 @@ sub _update_data_result_entries { my $entries = $json->decode( $results ); foreach my $m ( @$entries ) { + my $module = $module_mapping{ lc $m->{module} } // ucfirst lc $m->{module}; + my $testcase = + ( $m->{testcase} eq 'UNSPECIFIED' ) + ? 'Unspecified' + : $m->{testcase} =~ s/[a-z_]*/$module/ir; + my $r = [ $hash_id, $levels{ $m->{level} }, - $m->{module}, - $m->{testcase} // '', + $module, + $testcase, $m->{tag}, $m->{timestamp}, $json->encode( $m->{args} // {} ), From 107798fad5f337ea61d66d8ff859529d4a17bbea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Sun, 14 Jan 2024 02:25:30 +0100 Subject: [PATCH 58/64] Fix migration of SQLite databases We need to limit the number of variables we use in prepared statements for older versions of SQLite with a hard maximum of 999 variables. --- share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 61465ee3f..69a8e9962 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -32,8 +32,10 @@ die "Unknown database engine configured: $db_engine\n"; } +# depending on the resources available to select all data in database +# update $row_count to your needs sub _update_data_result_entries { - my ( $dbh ) = @_; + my ( $dbh, $row_count ) = @_; my $json = JSON::PP->new->allow_blessed->convert_blessed->canonical; @@ -43,9 +45,6 @@ sub _update_data_result_entries { my %levels = Zonemaster::Engine::Logger::Entry->levels(); - # depending on the resources available to select all data in database - # update $row_count to your needs - my $row_count = 50000; my $row_done = 0; while ( $row_done < $row_total ) { print "Progress update: $row_done / $row_total\n"; @@ -147,7 +146,7 @@ sub patch_db_mysql { $db->create_schema(); print( "\n-> (1/2) Populating new result_entries table\n" ); - _update_data_result_entries( $dbh ); + _update_data_result_entries( $dbh, 50000 ); print( "\n-> (2/2) Normalizing domain names\n" ); _update_data_nomalize_domains( $db ); @@ -224,7 +223,7 @@ sub patch_db_sqlite { $db->create_schema(); print( "\n-> (1/2) Populating new result_entries table\n" ); - _update_data_result_entries( $dbh ); + _update_data_result_entries( $dbh, 142 ); print( "\n-> (2/2) Normalizing domain names\n" ); _update_data_nomalize_domains( $db ); From 54238f968bce515696311b56ece5b6708f68ee7c Mon Sep 17 00:00:00 2001 From: Mats Dufberg Date: Mon, 15 Jan 2024 23:41:21 +0100 Subject: [PATCH 59/64] Makes sure that /usr/local/bin is in PATH --- share/zm_testagent-bsd | 3 +++ 1 file changed, 3 insertions(+) diff --git a/share/zm_testagent-bsd b/share/zm_testagent-bsd index 31aa94d3b..a6454efac 100755 --- a/share/zm_testagent-bsd +++ b/share/zm_testagent-bsd @@ -18,6 +18,9 @@ load_rc_config $name export ZONEMASTER_BACKEND_CONFIG_FILE="/usr/local/etc/zonemaster/backend_config.ini" #ZM_BACKEND_TESTAGENT_LOGLEVEL='info' # Set this variable to override the default log level +# Make Perl available for service() when executed via env() in script +export PATH="$PATH:/usr/local/bin" + command="/usr/local/bin/zonemaster_backend_testagent" command_args="--user=${zm_testagent_user} --group=${zm_testagent_group} --pidfile=${zm_testagent_pidfile}" if [ -n "$ZM_BACKEND_TESTAGENT_LOGLEVEL" ] ; then From 19bbf86fa27e693bba68d4d5a86e575334b43033 Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Wed, 17 Jan 2024 14:10:20 +0100 Subject: [PATCH 60/64] Improve style in PostgreSQL migration script Apply code style changes that should not affect the behavior of the database migration when performed on PostgreSQL. --- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index 69a8e9962..b25886ab3 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -177,21 +177,17 @@ sub patch_db_postgresql { hash_id, args, module, level, tag, timestamp, testcase ) ( - select - hash_id, - (CASE WHEN res->'args' IS NULL THEN '{}' ELSE res->'args' END) AS args, + SELECT + test_results.hash_id, + COALESCE(res->'args', '{}') AS args, res->>'module' AS module, - (SELECT value FROM log_level WHERE level = (res->>'level')) AS level, + log_level.value AS level, res->>'tag' AS tag, (res->>'timestamp')::real AS timestamp, - (CASE WHEN res->>'testcase' IS NULL THEN '' ELSE res->>'testcase' END) AS testcase - FROM - ( - SELECT - json_array_elements(results) AS res, - hash_id - FROM test_results - ) AS s1 + COALESCE(res->>'testcase', '') AS testcase + FROM test_results, + json_array_elements(results) as res + LEFT JOIN log_level ON (res->>'level' = log_level.level) ) ]); From cb82654757929517e408ae692e991e3620846794 Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Wed, 17 Jan 2024 14:25:42 +0100 Subject: [PATCH 61/64] PostgreSQL migration: adjust cases of columns Use initcap() to turn module names (e.g. 'NAMESERVER') and test case names (e.g. 'NAMESERVER01') into strings with initial capitalization (e.g. 'Nameserver' and 'Nameserver01' respectively), with the exception of DNSSEC. --- share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index b25886ab3..bf56d605d 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -180,11 +180,18 @@ sub patch_db_postgresql { SELECT test_results.hash_id, COALESCE(res->'args', '{}') AS args, - res->>'module' AS module, + CASE res->>'module' + WHEN 'DNSSEC' THEN res->>'module' + ELSE initcap(res->>'module') + END AS module, log_level.value AS level, res->>'tag' AS tag, (res->>'timestamp')::real AS timestamp, - COALESCE(res->>'testcase', '') AS testcase + CASE + WHEN res->>'testcase' IS NULL THEN '' + WHEN res->>'testcase' LIKE 'DNSSEC%' THEN res->>'testcase' + ELSE initcap(res->>'testcase') + END AS testcase FROM test_results, json_array_elements(results) as res LEFT JOIN log_level ON (res->>'level' = log_level.level) From db154beeb99ba441995b33fd2af7683dd9762a48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20P=C3=A4iv=C3=A4rinta?= Date: Thu, 18 Jan 2024 18:32:58 +0100 Subject: [PATCH 62/64] Make get_test_results return ns arguments again The database schema was updated to store capitalized module names instead of uppercase module names. Here we make get_test_results understand the new format. --- lib/Zonemaster/Backend/RPCAPI.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Zonemaster/Backend/RPCAPI.pm b/lib/Zonemaster/Backend/RPCAPI.pm index 48d98830e..ebf6943e3 100644 --- a/lib/Zonemaster/Backend/RPCAPI.pm +++ b/lib/Zonemaster/Backend/RPCAPI.pm @@ -404,7 +404,7 @@ sub get_test_results { my $test_info = $self->{db}->test_results( $params->{id} ); foreach my $test_res ( @{ $test_info->{results} } ) { my $res; - if ( $test_res->{module} eq 'NAMESERVER' ) { + if ( $test_res->{module} eq 'Nameserver' ) { $res->{ns} = ( $test_res->{args}->{ns} ) ? ( $test_res->{args}->{ns} ) : ( 'All' ); } elsif ($test_res->{module} eq 'SYSTEM' From cf089377f1b07385b9c92017bc131271a8692f0f Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Thu, 18 Jan 2024 17:48:08 +0100 Subject: [PATCH 63/64] Migrate arguments to Delegation01 in database Since the last release, a few message IDs were changed in Delegation01 (see zonemaster/zonemaster-engine#1290). Instead of having separate {nsname_list} and {ns_ip_list} arguments, these messages now use a merged {ns_list}. After the database migration, looking up results for older tests could cause translated messages to be returned where the {ns_list} placeholder was not substituted, because nothing was done to migrate the nsname_list and ns_ip_list arguments to ns_list. This commit ensures that the messages in question are properly migrated as well, for PostgreSQL, MySQL/MariaDB and SQLite. Please note, however, that the name server names and IP addresses are not guaranteed to match (see zonemaster/zonemaster#1203). There is unfortunately no way to recover the correct name server name to IP mapping, so the migration code just assumes that the two lists can be zipped together. --- .../patch_db_zonemaster_backend_ver_11.0.3.pl | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl index bf56d605d..2a97bd3c7 100644 --- a/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl +++ b/share/patch/patch_db_zonemaster_backend_ver_11.0.3.pl @@ -1,6 +1,7 @@ use strict; use warnings; +use List::Util qw(zip); use JSON::PP; use Try::Tiny; @@ -66,6 +67,13 @@ sub _update_data_result_entries { ? 'Unspecified' : $m->{testcase} =~ s/[a-z_]*/$module/ir; + if ($testcase eq 'Delegation01' and $m->{tag} =~ /^(NOT_)?ENOUGH_IPV[46]_NS_(CHILD|DEL)$/) { + my @ips = split( /;/, delete $m->{args}{ns_ip_list} ); + my @names = split( /;/, delete $m->{args}{nsname_list} ); + my @ns_list = map { join( '/', @$_ ) } zip(\@names, \@ips); + $m->{args}{ns_list} = join( ';', @ns_list ); + } + my $r = [ $hash_id, $levels{ $m->{level} }, @@ -170,7 +178,7 @@ sub patch_db_postgresql { try { $db->create_schema(); - print( "\n-> (1/2) Populating new result_entries table\n" ); + print( "\n-> (1/3) Populating new result_entries table\n" ); $dbh->do(q[ INSERT INTO result_entries ( @@ -203,9 +211,25 @@ sub patch_db_postgresql { ); - print( "\n-> (2/2) Normalizing domain names\n" ); + print( "\n-> (2/3) Normalizing domain names\n" ); _update_data_nomalize_domains( $db ); + print( "\n-> (3/3) Migrating arguments to messages for Delegation01" ); + $dbh->do(q[ + UPDATE result_entries + SET args = ( + SELECT args + - ARRAY['ns_ip_list', 'nsname_list'] + || jsonb_build_object('ns_list', string_agg(name || '/' || ip, ';')) + FROM unnest( + string_to_array(args->>'ns_ip_list', ';'), + string_to_array(args->>'nsname_list', ';')) + AS unnest(ip, name)) + WHERE testcase = 'Delegation01' + AND tag ~ '^(NOT_)?ENOUGH_IPV[46]_NS_(CHILD|DEL)$' + AND (NOT args ? 'ns_list'); + ]); + $dbh->commit(); } catch { print( "Could not upgrade database: " . $_ ); From bc882683e53f8fc19ff248d8d8fc4dcb16d93bb9 Mon Sep 17 00:00:00 2001 From: Mats Dufberg Date: Sun, 17 Mar 2024 23:44:00 +0100 Subject: [PATCH 64/64] Sets new version, updates changes and updates dependencies --- Changes | 16 ++++++++++++++++ Makefile.PL | 4 ++-- lib/Zonemaster/Backend.pm | 2 +- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/Changes b/Changes index f9b3200c3..26acf673f 100644 --- a/Changes +++ b/Changes @@ -1,6 +1,22 @@ Release history for Zonemaster component Zonemaster-Backend +v11.1.0 2024-03-18 (public release version) + + [Release information] + - New database schema requires migration of existing database. + + [Features] + - Migrates to new test results database table (#1092, #1145, #1147) + - Adds possibility to run several Test Agents on the same or multiple + servers to the same queue to increase capacity (#1121) + - Adds input name normalization (#1132) + + [Fixes] + - Fixes FreeBSD testagent start script (#1146) + - Fixes a change in JSON::Validator code (#1109) + + v11.0.2 2023-09-08 (public fix version) [Fixes] diff --git a/Makefile.PL b/Makefile.PL index 7681ba1d5..6d5a16714 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -35,8 +35,8 @@ requires 'Router::Simple::Declare' => 0, 'Starman' => 0, 'Try::Tiny' => 0.12, - 'Zonemaster::LDNS' => 3.002000, # v3.2.0 - 'Zonemaster::Engine' => 4.007003, # v4.7.3 + 'Zonemaster::LDNS' => 4.000000, # v4.0.0 + 'Zonemaster::Engine' => 5.000000, # v5.0.0 ; test_requires 'DBD::SQLite' => 1.4702; diff --git a/lib/Zonemaster/Backend.pm b/lib/Zonemaster/Backend.pm index c306da022..69e52b313 100644 --- a/lib/Zonemaster/Backend.pm +++ b/lib/Zonemaster/Backend.pm @@ -1,6 +1,6 @@ package Zonemaster::Backend; -our $VERSION = '11.0.2'; +our $VERSION = '11.1.0'; use strict; use warnings;