Ensure config is present and not null

[Zabuzard]

Overview

Our code incorrectly allowed missing config properties. Such as just leaving out an entry, for example gistApiKey. Or even explicitly setting it to null:

"gistApiKey": null,

both lead to Jackson constructing our Config with null for this field.

This is a problem, because our code implicitly assumes @Nonnull on all these fields and getter methods, in the whole code base. And we obviously also never accounted for this situation in our code, nor do we want to.

Fix

This approaches the problem by correctly documenting and enforcing that all properties of configs are required. The code now throws for both those situations:

This is achieved by two mechanisms:

• this.foo = Objects.requireNonNull(foo) for explicit nulls like "foo" = null,
• @JsonProperty(value = "foo", required = true) for leaving it out alltogether

Unfortunately, this makes the already ugly config files even uglier. Doesnt seem like there is a better or more automatic way of applying this everywhere in the config.

[Tais993]

In my opinion we should modify some of our code to support disabling some functionalities, I don't know what you think about this?

This doesn't have to be super complicated, just somekind of a validation method, and if it fails none of the classes/commands are registered to begin with.

[Zabuzard]

@Tais993 Yes, would be a good addition.

Right now, the gist feature just throws exceptions and the log-forwarded PR by me simply disables the feature if the webhook in the config throws an exception bc its not an URL.

So my log forwarded explicitly supports this situation, but not in a nice way.

[Tais993]

The singular issue I have with this, is that it just gives an unclear error.

Objects.requireNonNull(token, "The token from the config cannot be null");

While my phrasing is bad here, at least it instantly gets to the point, otherwise it'd still be a little vague and you have to check more than just the error message.

If that's modified, I'm fine with it, but i dont like it how it is rn

[Zabuzard]

The singular issue I have with this, is that it just gives an unclear error.

Objects.requireNonNull(token, "The token from the config cannot be null");

While my phrasing is bad here, at least it instantly gets to the point, otherwise it'd still be a little vague and you have to check more than just the error message.

If that's modified, I'm fine with it, but i dont like it how it is rn

The problem I have with the "proper fix" is that it bloats it even further.

On the good side:

• this check only triggers when you explicitly set something to null. In practice, that is unlikely. When you just leave it out, the required = true from Jackson will give you a direct and helpful error message.

And the stacktrace ofc at least links the exact line that had an issue.

But yeah, maybe Ill just add it...

[Tais993]

I understand the bloat concern very much, I'll accept this PR. The required = true already fixes 99% of the cases

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication