Refactoring in preparation for validation #759
Conversation
|
I moved things around a little and tweaked a comment in the code to make it clearer what's covered by the code generation.
I have second thoughts about the way I implemented the code generation. I have an idea of how I can make it much better and I'll make an update during the day. |
|
I updated the code generation of the accessor methods. The table-like section for getter methods is kept, and the setter methods are defined in a way such that they mention both the name of the setter method and the name of the field. This way they'll show up when searching for those strings in the source code. The setters are also generated individually instead of using data structures and for-loops, so it should be easier to grok what's going on. Please review again. |
| @@ -149,101 +144,103 @@ sub parse { | |||
| if ( defined( my $value = $get_and_clear->( 'DB', 'database_host' ) ) ) { | |||
| push @warnings, "Use of deprecated config property DB.database_host. Use MYSQL.host or POSTGRESQL.host instead."; | |||
|
|
|||
| $obj->{_MYSQL_host} = $value | |||
| $obj->_set_MYSQL_host( $value ) | |||
| if $obj->DB_engine eq 'MySQL'; | |||
There was a problem hiding this comment.
What will happen if Mysql-host is already set by the new method, or reverse?
There was a problem hiding this comment.
The later value overrides the former. Just like normal assignment.
The only difference is the indirection. This indirection doesn't do anything for us on its own. I'm adding it so that the validation rules for a property value can be defined for the property once and for all, instead of requiring that validation is performed at every assignment.
There was a problem hiding this comment.
I suggest, in a later PR, that it is not accepted to have both.
There was a problem hiding this comment.
You mean to only allow single assignment? It's surely possible but I'm not sure there's a way to do it that's worth its price in complexity. There's no shortage of little constraints in parse().
| _create_setter( '_set_ZONEMASTER_number_of_processes_for_frontend_testing', '_ZONEMASTER_number_of_processes_for_frontend_testing' ); | ||
| _create_setter( '_set_ZONEMASTER_number_of_processes_for_batch_testing', '_ZONEMASTER_number_of_processes_for_batch_testing' ); | ||
| _create_setter( '_set_ZONEMASTER_age_reuse_previous_test', '_ZONEMASTER_age_reuse_previous_test' ); | ||
| } |
There was a problem hiding this comment.
The new solution is definitely easier to read.
|
|
||
| Readonly my $IPV4_RE => qr/^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\$/; | ||
| Readonly my $IPV6_RE => qr/^([0-9a-f]{1,4}:[0-9a-f:]{1,}(:[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})?)\$|([0-9a-f]{1,4}::[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\$/i; | ||
|
|
There was a problem hiding this comment.
Maybe a comment that these REs will accept more IPv4 and IPv6 addresses, respectively, than the legal ones and must be combined with some other method to check?
There was a problem hiding this comment.
Those are old regexes. But sure, I'll add a comment in a followup PR.
Context
This PR is split out from #757. (It is also an incremental step towards fixing #685.)
It focuses on refactoring and doesn't add any new validations. #757 is relieved from refactoring and instead focuses on adding new validation rules.
Changes
Zonemaster::Backend::Validator
Validator is refactored and extended so the same set of regexes are reused for validation in both Config and RPCAPI.
The interface of the existing primitives for JSON validation isn't really conductive to INI validation, so an alternative interface is introduced. It's mainly described in POD, but an example is also included in the form of a validator for the type of DB.engine. The DB.engine validator accepts the same set of strings as the old implementation.
Zonemaster::Backend::Config
The only behavioral change in this PR is that there's a new error message you get when config parsing fails because of DB.engine. The new message is better aligned with the new validations coming in #757.
This PR changes how property fields are assigned. Instead of assigning a property field directly, an indirection is added in the form of setter methods. For this PR the setters simply assign the given value without any validation. But in #757 they're updated to perform validation. The setter methods are made private to make sure validation rules that work on multiple properties aren't violated by external callers.
A table-like section of setter definitions is introduced. The setters are generated at compile time. For the functionality in this PR the compile time generation is completely overkill, but it's made in preparation for the next PR where the setters are extended with validation logic. This way we can avoid lots of repetitive code.
The DB.engine property is left out from code generation because unlike the other properties it employs normalization.
Unit tests
A new unit test is added for the new validation primitive.
How to test this PR
The refactoring in the Config module should be covered by t/config.t.
The refactoring in the Validator module affects the implementation of the RPCAPI validation. That surface area is rather large. AFAICT the most reasonable thing we can do here is to be extra thorough when reviewing the "Refactor Validator" and "Refactor regexes to use /i" commits.