Fixed AvoidCatchingNPE and AvoidThrowingRawExceptionTypes issues found by PMD #5318

VelinYordanov · 2018-03-27T08:57:34Z

Fixed pmd issues found is smarthome - AvoidCatchingNPE and AvoidThrowingRawExceptionTypes.

Signed-off-by: VelinYordanov [email protected]

htreu

Most changes are unexpected formatter changes. Please check your formatter setting. I expect no JavaDoc & switch/case reformattings in this PR.

htreu · 2018-03-28T06:22:17Z

...eclipse/smarthome/automation/module/script/internal/handler/AbstractScriptModuleHandler.java

@@ -29,20 +29,28 @@
 import org.slf4j.LoggerFactory;

 /**
- * This is an abstract class that can be used when implementing any module handler that handles scripts.
+ * This is an abstract class that can be used when implementing any module


Please check your formatter settings. The maximum line width for ESH is 120.

htreu · 2018-03-28T06:24:16Z

...eclipse/smarthome/automation/module/script/internal/handler/AbstractScriptModuleHandler.java

                    String.format("Type is missing in the configuration of module '%s'.", module.getId()));
-        } else if (script == null || !(script instanceof String) || ((String) script).trim().isEmpty()) {
-            throw new RuntimeException(
-                    String.format("Script is missing in the configuration of module '%s'.", module.getId()));


With this removal the individual error description gets lost.

htreu · 2018-03-28T06:28:26Z

...io.net/src/main/java/org/eclipse/smarthome/io/net/http/internal/SecureHttpClientFactory.java

@@ -174,7 +174,7 @@ public Void run() {
                if (cause instanceof RuntimeException) {
                    throw (RuntimeException) cause;
                } else {
-                    throw new RuntimeException(cause);


With the lines above this looks like the RuntimeException is thrown on purpose here. @sobimibos can you clarify here what we can do alternatively? Thanks.

I discarded the changes and added an exception for this class for now.

htreu · 2018-03-28T06:29:15Z

...io.net/src/main/java/org/eclipse/smarthome/io/net/http/internal/SecureHttpClientFactory.java

@@ -232,7 +232,7 @@ public HttpClient run() {
            if (cause instanceof RuntimeException) {
                throw (RuntimeException) cause;
            } else {
-                throw new RuntimeException(cause);


Same as above.

htreu · 2018-03-28T06:29:51Z

.../org.eclipse.smarthome.io.rest/src/main/java/org/eclipse/smarthome/io/rest/JSONResponse.java

@@ -164,7 +164,7 @@ private Response createResponse(Status status, Object entity) {
            PipedInputStream in = new PipedInputStream(out);
            rp.entity(in);
        } catch (IOException e) {
-            throw new RuntimeException(e);
+            throw new IllegalStateException("I/O error occurred"+e);


please add the original exception as a parameter here.

htreu · 2018-03-28T06:33:51Z

...me.binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/handler/HueBridgeHandler.java

-     * @param state1 Reference state
-     * @param state2 State which is checked for equality.
+     * @param state1
+     *            Reference state


this line break is also not standard with the ESH code formatter.

htreu · 2018-03-28T06:41:55Z

...marthome.binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/internal/HueBridge.java

@@ -161,24 +168,25 @@ public boolean isAuthenticated() {
        String lastScan = safeFromJson(result.getBody(), NewLightsResponse.class).lastscan;

        switch (lastScan) {
-            case "none":
+        case "none":


this is also not ESH formatter conform.

htreu

Thanks, lgtm. Lets wait for Travis' opinion on this before merging.

maggu2810

Added my review comments.

maggu2810 · 2018-03-28T12:28:56Z

...eclipse/smarthome/automation/module/script/internal/handler/AbstractScriptModuleHandler.java

        } else {
            this.type = (String) type;
            this.script = (String) script;
        }
    }
+
+    private boolean isNotValid(Object parameter) {


I see that we would like to check if the parameter is not valid in the cases above.
But I don't think that methods that check that something is not true should be used.
If we want to check if a collection is not empty, we call !collection.isEmpty() instead of collection.isNotEmpty().
I would also prefer to provide a method isValid(Object parameter) and use !isValid(type) above.

maggu2810 · 2018-03-28T12:29:24Z

...eclipse/smarthome/automation/module/script/internal/handler/AbstractScriptModuleHandler.java

        } else {
            this.type = (String) type;
            this.script = (String) script;
        }
    }
+
+    private boolean isNotValid(Object parameter) {
+        return (parameter == null) || !(parameter instanceof String) || ((String) parameter).trim().isEmpty();


Why do we add braces that are not necessary? At least the one around parameter == null could be removed.

maggu2810 · 2018-03-28T12:30:16Z

...marthome.binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/internal/HueBridge.java

@@ -873,6 +873,10 @@ private void handleErrors(Result result) throws IOException, ApiException {
                }

                for (ErrorResponse error : errors) {
+                    if(error == null) {


It seems the formatter is still not correctly applied to all changes.

maggu2810 · 2018-03-28T12:31:10Z

tools/static-code-analysis/pmd/suppressions.properties

@@ -3,3 +3,8 @@ org.eclipse.smarthome.io.console.karaf.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.CommandWrapper=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.core.internal.events.ThreadedEventHandler=EmptyIfStmt
+org.eclipse.smarthome.config.xml.ConfigDescriptionConverter=AvoidCatchingNPE


Is it still necessary to catch NPE?
Can you point me to the places where it cannot be checked before?

I think the check should be suppressed for ModelRepositoryImpl and JavaTest. The only reason for the rest is that removing the catching of NPE would lead to longer and a bit uglier code. I could refactor them as well if you prefer.

Sorry for the delay...

The only reason for the rest is that removing the catching of NPE would lead to longer and a bit uglier code. I could refactor them as well if you prefer.

I cannot judge without having a look at.
Test for null and prevent NPEs in front of instead of catching it normally result into a cleaner (perhaps this depends on the POV) code. I will have a short look at that classes and try to understand why the code gets "uglier".

VelinYordanov · 2018-03-30T08:31:41Z

I also fixed all SimplifyBooleanExpressions and MoreThanOneLogger issues.

maggu2810 · 2018-04-03T07:38:43Z

tools/static-code-analysis/pmd/suppressions.properties

+org.eclipse.smarthome.config.xml.ConfigDescriptionConverter=AvoidCatchingNPE
+org.eclipse.smarthome.io.rest.core.internal.thing.ThingResource=AvoidCatchingNPE
+org.eclipse.smarthome.test.java.JavaTest=AvoidCatchingNPE
+org.eclipse.smarthome.model.core.internal.ModelRepositoryImpl=AvoidCatchingNPE


WRT #3335 and the Xtext magic, I don't care -- it's black magic for me 😉

maggu2810 · 2018-04-03T07:41:46Z

tools/static-code-analysis/pmd/suppressions.properties

@@ -3,3 +3,8 @@ org.eclipse.smarthome.io.console.karaf.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.CommandWrapper=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.core.internal.events.ThreadedEventHandler=EmptyIfStmt
+org.eclipse.smarthome.config.xml.ConfigDescriptionConverter=AvoidCatchingNPE
+org.eclipse.smarthome.io.rest.core.internal.thing.ThingResource=AvoidCatchingNPE
+org.eclipse.smarthome.test.java.JavaTest=AvoidCatchingNPE


Perhaps okay for relaxed test writings, but hey, we are using pure Java tests and no Groovy, so should be easy to fix (you could also do it in a separate PR and create an tracker issue now if too much tests needs to be fixed).

maggu2810 · 2018-04-03T07:42:52Z

tools/static-code-analysis/pmd/suppressions.properties

@@ -3,3 +3,8 @@ org.eclipse.smarthome.io.console.karaf.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.CommandWrapper=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.core.internal.events.ThreadedEventHandler=EmptyIfStmt
+org.eclipse.smarthome.config.xml.ConfigDescriptionConverter=AvoidCatchingNPE
+org.eclipse.smarthome.io.rest.core.internal.thing.ThingResource=AvoidCatchingNPE


Shouldn't this be easy to fix? At which places do we expect that an NPE is thrown?

maggu2810 · 2018-04-03T07:44:14Z

tools/static-code-analysis/pmd/suppressions.properties

@@ -3,3 +3,8 @@ org.eclipse.smarthome.io.console.karaf.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.CommandWrapper=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.core.internal.events.ThreadedEventHandler=EmptyIfStmt
+org.eclipse.smarthome.config.xml.ConfigDescriptionConverter=AvoidCatchingNPE


This one is rather easy to fix.
The URI constructor will throw a NPE if the provided text is null only (AFAIK).

Shouldn't this already do the job?

@@ -62,12 +62,15 @@ public class ConfigDescriptionConverter extends GenericUnmarshaller<ConfigDescri // the URI could be overridden by a context field if it could be // automatically extracted uriText = (String) context.get("config-description.uri"); + if (uriText == null) { + throw new ConversionException("Missing URI."); + } } URI uri = null; try { uri = new URI(uriText); - } catch (NullPointerException | URISyntaxException ex) { + } catch (URISyntaxException ex) { throw new ConversionException( "The URI '" + uriText + "' in node '" + reader.getNodeName() + "' is invalid!", ex); }

Yes, I updated the PR with the required changes.

SimplifyBooleanExpressions, MoreThanOneLogger and AvoidThrowingRawExceptionTypes. Signed-off-by: VelinYordanov <[email protected]>

maggu2810 · 2018-04-03T09:01:59Z

tools/static-code-analysis/pmd/suppressions.properties

@@ -3,3 +3,5 @@ org.eclipse.smarthome.io.console.karaf.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.CommandWrapper=SystemPrintln
 org.eclipse.smarthome.io.console.rfc147.internal.OSGiConsole=SystemPrintln
 org.eclipse.smarthome.core.internal.events.ThreadedEventHandler=EmptyIfStmt
+org.eclipse.smarthome.model.core.internal.ModelRepositoryImpl=AvoidCatchingNPE
+org.eclipse.smarthome.io.net.http.internal.SecureHttpClientFactory=AvoidThrowingRawExceptionTypes


One question remaining about that one (so, this is perhaps more addresses to the author of that file and other ones caring about the API):

Why not using a more specific exception instead of encapsulate all ones into RuntimeException (at the relevant places)?

Why not using a checked exception instead of an unchecked? If there is a potential error the caller should handle it correctly. Now, the caller gets not informed (without reading the called function implementation) that he needs to handle that error.

maggu2810

Thank you for your fixed.
I have just one remaining question that addresses the SecureHttpClientFactory implementation (so it address not directly your changed, but you point me to that as you introduced a suppression).

VelinYordanov · 2018-04-03T09:11:20Z

I added a suppression for the SecureHttpClientFactory in regards to @htreu 's review

With the lines above this looks like the RuntimeException is thrown on purpose here. @sobimibos can you clarify here what we can do alternatively? Thanks.

maggu2810 · 2018-04-03T09:14:49Z

Why not using a more specific exception instead of encapsulate all ones into RuntimeException (at the relevant places)?

Why not using a checked exception instead of an unchecked? If there is a potential error the caller should handle it correctly. Now, the caller gets not informed (without reading the called function implementation) that he needs to handle that error.

@sobimibos can you please give us some details?

maggu2810 · 2018-04-03T09:26:32Z

@VelinYordanov I added another issue, so this one could be merged as soon as Travis CI succeeded.

...which were introduced by eclipse-archived#5318. Signed-off-by: Simon Kaufmann <[email protected]>

...which were introduced by eclipse-archived#5318. fixes eclipse-archived#5373 Signed-off-by: Simon Kaufmann <[email protected]>

...which were introduced by #5318. fixes #5373 Signed-off-by: Simon Kaufmann <[email protected]>

...which were introduced by eclipse-archived#5318. fixes eclipse-archived#5373 Signed-off-by: Simon Kaufmann <[email protected]>

htreu suggested changes Mar 28, 2018

View reviewed changes

VelinYordanov force-pushed the pmd-fixes branch from f8c8ee6 to 9e72e13 Compare March 28, 2018 12:15

htreu approved these changes Mar 28, 2018

View reviewed changes

maggu2810 reviewed Mar 28, 2018

View reviewed changes

VelinYordanov force-pushed the pmd-fixes branch 2 times, most recently from 934dafc to a6f05b6 Compare March 30, 2018 08:30

VelinYordanov force-pushed the pmd-fixes branch 2 times, most recently from 4a28914 to 1a77092 Compare March 30, 2018 11:24

maggu2810 reviewed Apr 3, 2018

View reviewed changes

Fixed pmd issues found is smarthome - AvoidCatchingNPE,

f2e9427

SimplifyBooleanExpressions, MoreThanOneLogger and AvoidThrowingRawExceptionTypes. Signed-off-by: VelinYordanov <[email protected]>

VelinYordanov force-pushed the pmd-fixes branch from 1a77092 to f2e9427 Compare April 3, 2018 08:51

maggu2810 reviewed Apr 3, 2018

View reviewed changes

maggu2810 approved these changes Apr 3, 2018

View reviewed changes

maggu2810 mentioned this pull request Apr 3, 2018

SecureHttpClientFactory: avoid throwing RuntimeException #5340

Closed

maggu2810 merged commit 9828c8f into eclipse-archived:master Apr 3, 2018

maggu2810 mentioned this pull request Apr 4, 2018

adds kW·h as Unit #5346

Closed

sjsf mentioned this pull request Apr 5, 2018

fixed logger name in ExpressionThreadPoolManager #5358

Merged

cweitkamp mentioned this pull request Apr 6, 2018

[hue] An error occurred while calling method 'ThingHandler.handleCommand()' on ... (NPE) #5373

Closed

sjsf pushed a commit to sjsf/smarthome that referenced this pull request Apr 9, 2018

[hue] fix NPEs

85770bf

...which were introduced by eclipse-archived#5318. Signed-off-by: Simon Kaufmann <[email protected]>

sjsf pushed a commit to sjsf/smarthome that referenced this pull request Apr 9, 2018

[hue] fix NPEs

99c02c8

...which were introduced by eclipse-archived#5318. fixes eclipse-archived#5373 Signed-off-by: Simon Kaufmann <[email protected]>

sjsf mentioned this pull request Apr 9, 2018

[hue] fix NPEs #5390

Merged

kaikreuzer pushed a commit that referenced this pull request Apr 9, 2018

[hue] fix NPEs (#5390)

69842f8

...which were introduced by #5318. fixes #5373 Signed-off-by: Simon Kaufmann <[email protected]>

afuechsel pushed a commit to afuechsel/smarthome that referenced this pull request Apr 13, 2018

[hue] fix NPEs (eclipse-archived#5390)

ba689ce

...which were introduced by eclipse-archived#5318. fixes eclipse-archived#5373 Signed-off-by: Simon Kaufmann <[email protected]>

htreu added this to the 0.10.0 milestone Oct 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed AvoidCatchingNPE and AvoidThrowingRawExceptionTypes issues found by PMD #5318

Fixed AvoidCatchingNPE and AvoidThrowingRawExceptionTypes issues found by PMD #5318

VelinYordanov commented Mar 27, 2018

htreu left a comment

htreu Mar 28, 2018

htreu Mar 28, 2018

htreu Mar 28, 2018

VelinYordanov Mar 28, 2018

htreu Mar 28, 2018

htreu Mar 28, 2018

htreu Mar 28, 2018

htreu Mar 28, 2018

htreu left a comment

maggu2810 left a comment

maggu2810 Mar 28, 2018

maggu2810 Mar 28, 2018

maggu2810 Mar 28, 2018

maggu2810 Mar 28, 2018

VelinYordanov Mar 29, 2018

maggu2810 Apr 3, 2018

VelinYordanov commented Mar 30, 2018 •

edited

Loading

maggu2810 Apr 3, 2018

maggu2810 Apr 3, 2018

maggu2810 Apr 3, 2018

maggu2810 Apr 3, 2018

maggu2810 Apr 3, 2018

VelinYordanov Apr 3, 2018

maggu2810 Apr 3, 2018

maggu2810 left a comment

VelinYordanov commented Apr 3, 2018

maggu2810 commented Apr 3, 2018

maggu2810 commented Apr 3, 2018

Fixed AvoidCatchingNPE and AvoidThrowingRawExceptionTypes issues found by PMD #5318

Fixed AvoidCatchingNPE and AvoidThrowingRawExceptionTypes issues found by PMD #5318

Conversation

VelinYordanov commented Mar 27, 2018

htreu 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

htreu left a comment

Choose a reason for hiding this comment

maggu2810 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

VelinYordanov commented Mar 30, 2018 • edited Loading

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

maggu2810 left a comment

Choose a reason for hiding this comment

VelinYordanov commented Apr 3, 2018

maggu2810 commented Apr 3, 2018

maggu2810 commented Apr 3, 2018

VelinYordanov commented Mar 30, 2018 •

edited

Loading