Use uniform accessors #752

mattias-p · 2021-04-27T09:50:17Z

Context

This PR is a follow-up to #749.

Scope

Included:

Cleaning up after Uniform config property getter methods #749.
Documentation is added for the new getters from Uniform config property getter methods #749.
Documentation for the database connection related properties is updated for consistency.
Relevant pieces of cleaned up documentation is integrated into the backend config format specification.
SQL-queries adjacent to uses of the new property getters are refactored to use the ?-syntax instead of string concatenation. To facilitate a few other refactorings are also included.

Excluded:

Functional changes.

Changes

Security:

In a couple of places another layer of security is added by using SQL-injection resistant ? syntax. This also makes the code easier to review with regard to security.

Documentation:

The backend config format specification gets a tiny improvement by adopting a wording from the getter POD.
The POD for the uniform getters is made very light for easier maintenance and higher reliability by not duplicating information provided in the format specification. The idea is to make the format specification a single source of truth. Implementation details are still kept in the getter POD, e.g. the form in which the property values are returned.

Clean-up:

The free-form getters that received new replacements in Uniform config property getter methods #749 are removed. All calls to them are replaced with calls to the new getters.

How to test this PR

This PR contains no functional changes. It should be enough that the automatic tests pass.

ghost

This looks great ! Thanks
I have few remarks and questions to make it even greater ;)

ghost · 2021-04-27T12:02:30Z

docs/Configuration.md

+How old (in seconds) a previous test of the same zone name and parameters can be
+when it is reused instead of starting a new test.


Not sure if can conveys the right meaning. From my understanding if the time limit is reached, then a new test is run, otherwise the old test's results are displayed. And the usage of can here make it sounds like there is a possibility to run the test even if the time limit is not reached.

I understand that there might be something unclear with the current sentence.
If you feel we need to rewrite this sentence, I tried to formulate it another way (but I'm still not satisfied)
-> Time (in seconds) to wait before running a new test instead of reusing the results of a previous test of the same zone name and parameter.

When I tried to look for a real life analogy to what we're trying to express and the closes I came was "shelf life". I'll come up with a new wording based on that.

ghost · 2021-04-27T12:14:21Z

lib/Zonemaster/Backend/Config.pm


-'max_zonemaster_execution_time' from [ZONEMASTER] section in ini file. See
-L<https://github.com/zonemaster/zonemaster-backend/blob/master/docs/Configuration.md#max_zonemaster_execution_time>.
+=head2 DB_engine


Can you move all these POD (DB_engine ... ZONEMASTER_age_reuse_previous_test) above line 266 with the other ones ? I think it will make more sense to have them all together (however I should say that I don't know much on POD, so if there is a reason to keep them here, I can live with that :) )

POD doesn't care about order. There are coverage tools for verifying that every named subroutine has a corresponding POD heading but otherwise you may do as you please.

For this PR I've mainly tried to do something at least half-way reasonable regarding the placement of the POD while also keeping down the size of the diff. But I'll take another look.

This was just to have the POD close to the code (if you prefer reading the code file directly). Thanks for the update

I completely agree. The only reason I didn't make it so in the first place was a practical one. I had a large set of changes that I was trying to rebase into a series of PRs, so having smaller diffs made a big difference at the time. But I'm glad I got to set it straight.

ghost · 2021-04-27T12:16:02Z

lib/Zonemaster/Backend/Config.pm


-'number_of_processes_for_frontend_testing' from [ZONEMASTER] section in ini file. See
-L<https://github.com/zonemaster/zonemaster-backend/blob/master/docs/Configuration.md#number_of_processes_for_frontend_testing>.
+Returns an integer (number of seconds).


Why specifying what kind of value is returned here and not for all the other one ?

I'll remove it for consistency.

ghost · 2021-04-27T12:18:38Z

lib/Zonemaster/Backend/DB/MySQL.pm

            AND progress > 0
            AND progress < 100
-            AND queue = ".$self->config->lock_on_queue();
+            AND queue = ?" );
+        $sth->execute(    #


Is the trailing empty comment mandatory ?
(this question applies to all following execute() calls)

I put it there to coax perltidy into doing something reasonable. If I leave it out perltidy just pulls all the arguments up on the same line making it harder to read :)

I see. Thanks for the explanation. And I do agree that it is better to have them on several lines.

What a trick! I will remember that.

lib/Zonemaster/Backend/DB/SQLite.pm

mattias-p · 2021-04-27T15:38:17Z

This looks great ! Thanks
I have few remarks and questions to make it even greater ;)

Of course! Great is great but greater is greater :)

ghost

Thanks for all the updates. Sadly a small mistake slipped in ^^'

ghost · 2021-04-27T17:12:10Z

docs/Configuration.md

-Default value: `600`.
+
+The shelf life of a test in seconds after its creation.
+If a new test is requested for the same zone name and parameters within the
+shelf life of a previous test result, that test result is reused.
+Otherwise a new test request is enqueued.


I think you removed the default value line by mistake.
(I like the rewording btw)

Good spotting! I added it back.
I also also made a small edit to make age_reuse_previous_test to make its formatting more like the other simple properties.

(I like the rewording btw)

I'm glad to hear that :)

mattias-p · 2021-04-28T10:11:51Z

@pnax Thanks for reviewing.

@matsduf Could you review this too?

matsduf · 2021-04-29T14:48:42Z

lib/Zonemaster/Backend/Config.pm

+
+=head2 DB_engine
+
+Get the value of L<DB.engine|https://github.com/zonemaster/zonemaster-backend/blob/master/docs/Configuration.md#polling_interval>.


It should not be "polling_interval" here.

Good catch! I fixed it.

matsduf · 2021-04-29T14:53:46Z

lib/Zonemaster/Backend/DB/MySQL.pm

            AND progress > 0
            AND progress < 100
-            AND queue = ".$self->config->lock_on_queue();
+            AND queue = ?" );
+        $sth->execute(    #


What a trick! I will remember that.

matsduf · 2021-04-29T14:55:38Z

Just one small change.

ghost

While going through the code, I realize that the way some deprecated values are handled is not right. I am referring to number_of_processes_for_frontend_testing and number_of_processes_for_batch_testing. The code on line 183 and 192 of Config.pm will never be used because we set a default value to 20. So the value is always defined at this stage and will never be overridden by the config value.

If we care about stability for deprecated values, I think this needs to be reworked.

And to see the problem, you can update t/config.t with number_of_professes_for_frontend_testing instead of number_of_processes_for_frontend_testing and see the test fail.

mattias-p · 2021-04-29T16:53:37Z

Thanks for your reviews!

I must say I didn't see the default value problem coming. Getting the order right in this kind of code is surprisingly tricky. Unfortunately I can't fix it the way I want to without conflicting with #753, and I'd rather merge that one first.

After #753 is merged I'll rebase this one and fix the problems you guys found. I'll also add the unit tests I for number_of_processes_for_{frontend,batch}_testing that I forgot to add earlier.

ghost · 2021-04-29T17:48:10Z

Yes I agree that this looks tricky. I discovered this while trying to look how to properly deprecate the number_of_processes_for_* keys. And I said to myself, well let's see if @mattias-p can come up with a solution ;-p

I.e. ZONEMASTER_max_zonemaster_execution_time, ZONEMASTER_lock_on_queue and ZONEMASTER_maximal_number_of_retries.

…testing

mattias-p · 2021-04-30T10:58:18Z

Yeah, having test coverage of all the cases is key, though. Once you get the tests to pass you also want to make it look easy. It's so much more satisfying when the tricky stuff shows up in more glamorous places. Configuration file validation just doesn't get enough hype these days.

But I rebased the commits and fixed the problems. I ended up assigning the private fields multiple times going from least to most prioritized source instead of going the other way around and using guards. I think it turned out pretty good. I just hope I didn't forget more tests.

matsduf

This is a large and complex PR. It looks fine, but will have to test it thoroughly.

ghost

Well done ! You came up with a very nice solution :)

mattias-p force-pushed the use-uniform-accessors branch from 664d9e2 to 3b43e50 Compare April 27, 2021 09:58

mattias-p marked this pull request as ready for review April 27, 2021 10:04

mattias-p requested review from matsduf and a user April 27, 2021 10:04

ghost reviewed Apr 27, 2021

View reviewed changes

ghost approved these changes Apr 28, 2021

View reviewed changes

ghost mentioned this pull request Apr 29, 2021

Uniform config property getter methods #749

Merged

ghost self-requested a review April 29, 2021 14:51

matsduf requested changes Apr 29, 2021

View reviewed changes

ghost suggested changes Apr 29, 2021

View reviewed changes

mattias-p added 14 commits April 30, 2021 11:43

Fix some whitespace

70f5819

Make list more readable

d0bfcb1

Call execute near SQL string

1bbe8ea

Use ?-syntax for some SQL queries

d7c94f3

Use DB_polling_interval

2fb2e43

Use DB_engine instead of BackendDBType

e5f6d81

Use ZONEMASTER_age_reuse_previous_test

466773c

Use ZONEMASTER_number_of_processes_for_{fronted,batch}_testing

fae7b75

Use new config properties related to garbage collection

26cbdab

I.e. ZONEMASTER_max_zonemaster_execution_time, ZONEMASTER_lock_on_queue and ZONEMASTER_maximal_number_of_retries.

Make POD for database-related properties more like other properties

af3594a

Update wording in documentation

2795b69

Formatting

567b758

Improved consistency in documentation

30433e6

Better ordering of POD

83b5f9b

mattias-p added 4 commits April 30, 2021 11:43

Add back default value

52d5105

Fix link

6bc2015

Check unrecognized properties last

674b6c3

Fix fallback for ZONEMASTER.number_of_processes_for_{frontend,batch}_…

7f9c6fe

…testing

mattias-p force-pushed the use-uniform-accessors branch from 0e487bc to 7f9c6fe Compare April 30, 2021 10:39

matsduf approved these changes Apr 30, 2021

View reviewed changes

ghost approved these changes May 3, 2021

View reviewed changes

mattias-p merged commit 163cc68 into zonemaster:develop May 3, 2021

mattias-p deleted the use-uniform-accessors branch May 12, 2021 16:28

matsduf added this to the v2021.1 milestone Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use uniform accessors #752

Use uniform accessors #752

mattias-p commented Apr 27, 2021

ghost left a comment

ghost Apr 27, 2021

mattias-p Apr 27, 2021

ghost Apr 27, 2021

mattias-p Apr 27, 2021

ghost Apr 27, 2021

mattias-p Apr 28, 2021

ghost Apr 27, 2021

mattias-p Apr 27, 2021

ghost Apr 27, 2021

mattias-p Apr 27, 2021

ghost Apr 27, 2021

matsduf Apr 29, 2021

mattias-p commented Apr 27, 2021

ghost left a comment

ghost Apr 27, 2021

mattias-p Apr 28, 2021

mattias-p commented Apr 28, 2021

matsduf Apr 29, 2021

mattias-p Apr 30, 2021

matsduf Apr 29, 2021

matsduf commented Apr 29, 2021

ghost left a comment •

edited by ghost

mattias-p commented Apr 29, 2021

ghost commented Apr 29, 2021

mattias-p commented Apr 30, 2021 •

edited

matsduf left a comment

ghost left a comment

		How old (in seconds) a previous test of the same zone name and parameters can be
		when it is reused instead of starting a new test.


		=head2 DB_engine

		Get the value of L<DB.engine\|https://github.com/zonemaster/zonemaster-backend/blob/master/docs/Configuration.md#polling_interval>.

Use uniform accessors #752

Use uniform accessors #752

Conversation

mattias-p commented Apr 27, 2021

Context

Scope

Changes

How to test this PR

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattias-p commented Apr 27, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattias-p commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Apr 29, 2021

ghost left a comment • edited by ghost

Choose a reason for hiding this comment

mattias-p commented Apr 29, 2021

ghost commented Apr 29, 2021

mattias-p commented Apr 30, 2021 • edited

matsduf left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment •

edited by ghost

mattias-p commented Apr 30, 2021 •

edited