[FLOSS] Enable validation on read [Java example whitelist plugin]

[tucek]

The Java example plugin whitelist should also validate values on get instead on set only. Validation errors should result in warnings.

See also: #3889 (comment)

[markus2330]

I am okay with checking on kdbGet (and providing warnings), see also #1511, but I do not think adding an option for that in every plugin makes sense. Instead I would prefer to have one plugin "failonwarnings" (globally mounted) which allows to globally switch the behavior to fail also during kdbGet.

[tucek]

I am okay with checking on kdbGet (and providing warnings), see also #1511, but I do not think adding an option for that in every plugin makes sense. Instead I would prefer to have one plugin "failonwarnings" (globally mounted) which allows to globally switch the behavior to fail also during kdbGet.

ok, i've updated the description accordingly

[tucek]

Issue blocked by #4053

[markus2330]

@tucek for the range plugin this already gets implemented, there should be no blocker to also implement it for the whitelist plugin.

[marvin-the-parrot]

[FLOSS H1]

• The Java example plugin whitelist should also validate values on get instead on set only.

The implementation of validate values in set should also be applied to get. The way this was achieved for set should help as a guideline to use a similar approach for set.

• Validation errors should result in warnings.

This can be achieved in much the same way as it was achieved for the range plugin. The way it was achieved should be visible in this commit.

I was thinking of removing the lang/java tag and assigning the lang/c tag since this validation seems to be done in c code. If this is not the case I will leave the lang/java tag as it is.

[markus2330]

I was thinking of removing the lang/java tag and assigning the lang/c tag since this validation seems to be done in c code. If this is not the case I will leave the lang/java tag as it is.

The whitelist plugin is Java ./src/bindings/jna/plugins/whitelist and only this code needs to be touched. Otherwise your analysis looks good to me!

[bhampl]

We (@bhampl and @eiskasten ) would like to work on this issue, but we haven't fully understood how the code works. The main questions we have is where should the validation be done in the get method and what exactly is the relevant validation code in the set method, which we would move into its one method to do the validation only once.

[kodebach]

You correctly identified the get and set methods this issue is about. In theory they should have the exact same behaviour, except that get should only produce warnings where set produces errors. However, get is (currently) also used to access the contract of a plugin. This is why there is an additional if at the beginning. After this if (currently just return) is where the actual implementation of get should be.

For a bit of background on this issue: Producing errors in set ensures that a kdb set terminal command can result in an invalid configuration. However, the underlying configuration file could be modified manually too. This is why we also need validation in get. But we cannot produce errors in get, because kdb set needs to successfully execute get before it can call set. Therefore an error in get would prevent fixing the error via kdb set.

I hope this clarifies the issue a bit. If not feel free to ask again.

[eiskasten]

Ok, thank you. So, I think that the solution is supposed to outsource the error checks and use that in both, the get and the set method?

[kodebach]

Correct, the code duplication should be minimized for the sake of maintainability. How you achieve this is up to you. Extracting the checks is one possibility, another possibility would be a check function that takes something like an OnErrorListener as an argument.

[Bujuhu]

OK, just to sum it up once again. The issue is we want to validate values on get and validation errors should result in warnings, and in set and validation errors should result in failures in https://github.com/ElektraInitiative/libelektra/blob/master/src/bindings/jna/plugins/whitelist/src/main/java/org/libelektra/plugin/WhitelistPlugin.java

Currently, set is validating and returning warnings and errors and get is not validating at all.

I propose the following solution:

The quickest (albeit, not the most readable) solution would be to take advantage of the similar signature of setError and addWarning and just use the function itself as a parameter like this:

    public static int validate(Key key, Key parentKey, BiFunction<ErrorCode, String, Key> setErrorOrWarning, int problemStatus) {
        //should use the validation implementation of set without the part where the value is added; i.e. pretty much the whole set function without line 58
        //setting a problem that should always be a warning
        parentKey.addWarning(/*ErrorCode*/, /*Message*/);
        //setting a problem that should either be a warning or an error depndening on the context
        setErrorOrWarning.apply(/*ErrorCode*/, /*Message*/);
        //return problemStatus if an error has occured, otherwise return STATUS_SUCCESS
    }

We can then just call the validation function in set like this:

   validate(key, parentKey, parentKey::setError, STATUS_ERROR)
   //add key on success or return status_error on faiure

And for get like this:

   validate(key, parentKey, parentKey::addWarning, STATUS_SUCCESS)
  // return value

[kodebach]

The quickest (albeit, not the most readable) solution

I think the solution is fine, but we might want to have a different name instead of setValidationProblem to make it a bit more readable. How about setErrorOrWarning?