Tighten config parser

[quinox]

While mucking around with the codebase I discovered the configuration parser is rather liberal. Example configs that were OK:

expire_sessions_after_seconds 180h
expire_sessions_after_seconds 180 hours

In both cases the value is 180 seconds: the extra text is fully ignored.

This had two causes:

• std::stoi and friends can leave a part of the string unparsed. To fix this I made a full_stoi that checks how much work was done to ensure the entire value was used for the conversion
• key_value_regex scans for lines with 3 values, of which the third one is optional. I'd guess this was done to support the QoS of publish / subscribe (which aren't documented btw), but caused every other setting to accept and ignore a third value. To make the smallest possible changeset I fixed this by adding a check at the bottom using a new var called number_of_expected_values. A nicer solution would be to add a new regex but that requires far more changes to the code.

Please double-check the correctness: it's good enough to compile and to pass nearly all testcases* but I'm not intimately familiar with the FlashMQ codebase.

_{* testRetainedAclReadCheck fails with plugins/libtest_plugin.so.0.0.1 is not there which seem unrelated to my changes}

[halfgaar]

Looks like a good change.

The arguments to full_stoi etc everywhere is now the hard-coded string name. When the if-statements enter, the key will contain that value. Can you rewrite to pass that instead of the redundant name?

The function 'testKeyValidity()' function should have been called equalsApprovedKey(). I think that would make the purpose more clear: that function is used to force the programmer to add config directives you scan for to the list of approved keys.

As for the plugins/libtest_plugin.so.0.0.1; you need to (q)make the FlashMqTestsMeta project. Did you?

[quinox]

Can you rewrite to pass that instead of the redundant name?

Ah, much cleaner this way, thanks.

As for the plugins/libtest_plugin.so.0.0.1; you need to (q)make the FlashMqTestsMeta project. Did you?

I used ./run-make-from-ci.sh && ./buildtests/FlashMQTests which I now see fails because of the wrong working directory. When I run the test executable from inside the builtests folder I get 122 tests pass, 0 failed.

[halfgaar]

Some comments. Some may be a bit nit-picky, but also easy to change.

I pushed one tiny commit that wasn't on Github yet. You can rebase.

brace style is next line. At this point the code is consistent enough to keep to that style. (unlike variable name convention, where underscore and camel case is mixed...).

Likewise for Doxygen style function documentation. Like:

/**
 * @brief Like std::stoul, but demands that the entire value is consumed.
 */

(the @brief isn't that important. QtCreator automatically makes it when I start the block, but there is no actual documentation generation)

value argument to the new parse functions should be a reference.

The number_of_expected_values is never used. The places where you assign 2 have a code path that end with a continue, because it's parsing a nested block.

In test case, can you use uint32_t instead of u_int32_t?

I will update the man page with the QoS of the bridge subscribe and publish.

In the test case: naked new in c++ is hardly necessary. In this case it's a memory leak (liken it to a malloc()) and it doesn't remove the temp file on scope exit, which is the intended use. What you want is probably ConfFileTemp humanConfig;. Funny enough, the code would now allow misuse when you would later do humanConfig = ConfFileTemp();, this doesn't give an error, but breaks intended functionality, because the ConfFileTemp doesn't delete its assignment operators / constructors. I pushed a commit to change that.

If you want to assign over your previous one, use a std::unique_ptr<ConfFileTemp> bla = std::make_unique<ConfFileTemp>(argsIfAny); In that case, you don't use the assignment operator of ConfFileTemp, but of the std::unique_ptr. However, I think what you really want, is to place the sub-sections of the test in their own scope blocks (curly braces).

[quinox]

Some comments. Some may be a bit nit-picky, but also easy to change.

I'm happy with the nit-picking: it's your project and I'm basically a neophyte when it comes to C/C++ anyway.

Fixed all points mentioned. I had to add a helper function to not repeat the expected-values check. I put it on the class since that felt appropriate.

However, I think what you really want, is to place the sub-sections of the test in their own scope blocks (curly braces).

Thanks, that was indeed what I had in mind in the first place.

[halfgaar]

Looks good, at a 'medium glance'. Before I look closer and merge, can I ask you to rebase on master and refactor a bit? I refactored the tests to not need Qt anymore. You can put your new tests in configtests.cpp. Just put this at the top and it should work:

#include "maintests.h"
#include "testhelpers.h"
#include "conffiletemp.h"

I try to put tests in more implementation files, because one big file is slow to compile and annoying to work with if there are multiple branches.

[quinox]

Rebased. Adapting the testcases to the new setup was easy :+1:

It's not blocking in the slightest, but while you're revamping the test setup: right now it initializes the FlashMQ server during the execution of my testcases, if I run ./flashmq-tests test_loading_second_value I see a lot of output. It feels like it might be possible not to do that when I'm merely testing the configuration parser.

[halfgaar]

Here I am again, with the nitty-gritty.

The function testCorrectNumberOfValues can be static, and it's matches argument can be const. The reason in C++ to be const-correct, is because it propagates. If there's a non-const function, you can't call it from another const function when the variable happens to be a class member. Or, you can't call a non-const function on a const object. And API wise, a const reference is clear to be a borrowed resource. When it's non-const, there is the potential for change.

About the staticness; granted, testKeyValidity() can also be static. Or, a free function; the distinction between those can quickly become vague in C++. I do agree with that full_stoi() and full_stoul() are free functions.

Then, the std::smatch element access is undefined when it's not ready(). It being a function, it should produce valid results on all possible inputs. The exception given can be std::runtime_error, because the programmer likely made a mistake, as opposed to the user in their config file.

Lastly, really nit-picky; there is an implicit conversion from size_t to int, with the amount of matches. Currently, it behaves well enough, but it becomes a problem when that is fixed at some point. Because then the format string %d will be given a long long. The format family of functions (C-style) can crash nastily on type miss-matches. Therefore, I've been moving away from that, towards c++ streams. The logger for instance, uses C++ streams. They are much safer. But, unfortunately, I don't have an easy exception generator with streams, so in this case, it would mean using an std::ostringstream manually. Despite the apparent clunkyness, I'd like to request that ;).

Edit: oh, and the tests. There could potentially be a different class for these kinds of tests, true.

[quinox]

The nitpicking is greatly appreciated.

Fixed all the point you mentioned + got rid of a few naked news that were left in the testcases.

[halfgaar]

Two more things :grimacing:

1) There is a stray trailing curly brace.

2) Exceptions need to be caught by reference, so catch (ConfigFileException&). If you don't, only that exact type (no base or sub classes) will be caught. In this case, that would have worked, but better be safe.

[quinox]

Nice opportunity to test the formatter:

quinox@ghost~/p/FlashMQ (feat-fix-config-parsing|✔)> git diff HEAD~1 | clang-format-diff-15 -p1
--- configfileparser.cpp        (before formatting)
+++ configfileparser.cpp        (after formatting)
@@ -60,7 +60,8 @@

 void ConfigFileParser::testCorrectNumberOfValues(const std::string &key, size_t expected_values, const std::smatch &matches)
 {
-    if (!matches.ready()) {
+    if (!matches.ready())
+    {
         throw std::runtime_error("It appears the programmer made a mistake: please provide a ready() smatch object");
     }
     size_t encountered_values = matches.size() - 2; // 0 = full line, 1 = key, 2 = first value, 3 = second value etc.

Both points have been fixed.