strimzi / strimzi-kafka-oauth

OAuth2 support for Apache Kafka® to work with many OAuth2 authorization servers
Apache License 2.0
141 stars 89 forks source link

Error handling #147

Closed mstruk closed 2 years ago

mstruk commented 2 years ago

Review the handling of exceptions, particularly the handling of Errors within catch (Throwable t) blocks. Also update SpotBugs to latest version and address all the warnings it issues.

This PR depends on #141.

mstruk commented 2 years ago

I updated the SpotBugs version and it started issuing some new warnings. Specifically the one that says it's bad practice to throw RuntimeException() in your code (rule THROWS_METHOD_THROWS_RUNTIMEEXCEPTION).

I addressed some of the warnings by introducing custom extensions of RuntimeException. But it's clear that I'm basically doing it to silence the warnings. I see no real value in these exceptions and they are quite unnecessary. In some places I explicitly silenced the rule as I find introducing additional runtime exceptions plain harmful there.

The blanket rule for throwing the RuntimeException would best be turned off IMHO. But for now I indulged it.

scholzj commented 2 years ago

@mstruk In the operators repo, we have an exclude file for disabling the rules we don't like: https://github.com/strimzi/strimzi-kafka-operator/blob/main/.spotbugs/spotbugs-exclude.xml

But I guess someone like @tombentley would be better to comment on how useful a specific rule is. I personally would agree that I don't see too much value from a rule like this.

mstruk commented 2 years ago

@mimaison Any comments? @tombentley Any thoughts on this?

mstruk commented 2 years ago

Thanks, this is a nice improvement @mstruk. Is there any value in a single top-level OauthException, rather than having multiple exception types extend RuntimeException?

I considered it but it seemed just another layer, more for style than for anything useful.