Fix some unnecessary use of boxed types and incorrect format strings spotted by lgtm. by chrisgavin · Pull Request #4474 · apache/druid

chrisgavin · 2017-06-27T20:00:55Z

This pull request fixes some small problems identified by lgtm. You can find more details here.

The first commit changes some boxed types which can never be null to unboxed types.

The second commit fixes some format strings where the number of arguments doesn't match the number of placeholders in the format string.

jihoonson · 2017-06-30T00:27:53Z

@chrisgavin thanks for your patch. Please check the compilation error.

chrisgavin · 2017-07-03T09:59:15Z

Thanks. I have fixed it. :)

leventov · 2017-07-03T18:40:31Z

@chrisgavin please in the future, don't amend commits in your PR, always add new commits on top, including merge commits.

chrisgavin · 2017-07-03T19:00:55Z

@leventov Sorry about that. I could revert to the previous HEAD of this PR and then make the compilation fix as a new commit if that would be helpful.

leventov · 2017-07-03T19:12:29Z

@chrisgavin not needed now, it's for the future.

Also could you please add inspections for this kind of errors?

Add to Druid.xml:

    <inspection_tool class="MalformedFormatString" enabled="true" level="ERROR" enabled_by_default="true">
      <option name="additionalClasses" value="io.druid.java.util.common.StringUtils,io.druid.java.util.common.logger.Logger" />
      <option name="additionalMethods" value="trace,debug,info,warn,error,wtf,format,nonStrictFormat" />
    </inspection_tool>

Add to checkstyle.xml:

    <module name="Regexp">
      <property name="format" value="com\.metamx\.common\.logger"/>
      <property name="illegalPattern" value="true"/>
      <property name="message" value="Use io.druid.java.util.common.logger.Logger instead"/>
    </module>

I cannot find an inspection for unecessary use for boxed types (there is "unnecessary boxing" but it doesn't spot the places that you have found), but maybe there is some.

leventov · 2017-07-03T19:25:51Z

Or more generally, you may prohibit any non-druid loggers by regex like

(?<!io.druid.java.util.common.logger)\.Log(ger)?[^A-Za-z0-9]

But a few exceptions will be needed, that you may guard with //CHECKSTYLE.OFF: Regexp and //CHECKSTYLE.ON: Regexp comments.

leventov · 2017-07-04T14:22:28Z

@chrisgavin it wasn't supposed to be clean, please fix inspection and checkstyle failures now.

…er is not simple.

… to format strings.

leventov · 2017-07-05T23:52:02Z

 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
+//CHECKSTYLE.OFF: Regexp


Why needs to use old logger?

leventov · 2017-07-05T23:52:18Z

 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
+//CHECKSTYLE.OFF: Regexp


Why needs to use old logger?

leventov · 2017-07-05T23:53:12Z

    log.warn(message);
  }

+  /** @noinspection MalformedFormatString */


Use SuppressWarnings

leventov · 2017-07-05T23:53:46Z


 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableMap;
+//CHECKSTYLE.OFF: Regexp


Why needs to use old logger?

leventov · 2017-07-05T23:53:55Z

 import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
+//CHECKSTYLE.OFF: Regexp


Why needs to use old logger?

chrisgavin · 2017-07-06T00:41:12Z

@leventov The places that still use the old logger do so because the logger is passed into a LoggingEmitter constructor, which only accepts a metamx logger.

leventov · 2017-07-06T05:27:18Z

@chrisgavin 16 more malformed string format violations reported: https://teamcity.jetbrains.com/viewLog.html?buildId=1108816&tab=Inspection&buildTypeId=OpenSourceProjects_Druid_InspectionsPullRequests

…ct string is intentional.

leventov · 2017-07-07T04:27:24Z

              public Result<T> apply(Cursor input)
              {
-                log.debug("Running over cursor[%s]", adapter.getInterval(), input.getTime());
+                log.debug("Running over cursor[%s]", input.getTime());


Strange logging "cursor[input.getTime()]". Also maybe just remove this debugging line

leventov · 2017-07-07T04:27:38Z

      }
      catch (Exception e) {
-        log.warn(e, "Could not close aggregator, skipping.", aggregator);
+        log.warn(e, "Could not close aggregator, skipping.");


Why not print the aggregator?

leventov · 2017-07-07T04:27:52Z

              public Result<TopNResultValue> apply(Cursor input)
              {
-                log.debug("Running over cursor[%s]", adapter.getInterval(), input.getTime());
+                log.debug("Running over cursor[%s]", input.getTime());


Same as above

leventov · 2017-07-07T04:30:50Z

@chrisgavin also please fill the CLA http://druid.io/community/cla.html

chrisgavin force-pushed the boxed-variables-and-format-strings branch from 57bc4ae to 1f659c5 Compare June 30, 2017 17:27

Remove some unnecessary use of boxed types.

bc77b04

chrisgavin force-pushed the boxed-variables-and-format-strings branch from 1f659c5 to dc014e6 Compare July 1, 2017 10:17

Fix some incorrect format strings.

7e9a322

chrisgavin force-pushed the boxed-variables-and-format-strings branch from dc014e6 to 7e9a322 Compare July 2, 2017 22:37

leventov added the Bug label Jul 3, 2017

chrisgavin added 2 commits July 4, 2017 09:17

Enable IDEA's MalformedFormatString inspection.

5f33a2e

Add a Checkstyle check for finding uses of incorrect logging packages.

05596e5

chrisgavin added 5 commits July 5, 2017 15:26

Fix some incorrect usages of the metamx logger.

bc49dca

Bypass incorrect logger Checkstyle check where using the correct logg…

e832e7e

…er is not simple.

Fix some more places where the wrong number of arguments are provided…

cc9c0d2

… to format strings.

Suppress MalformedFormatString inspection on legacy logging test.

54d29e4

Merge branch master into boxed-variables-and-format-strings.

d3cf2fa

leventov reviewed Jul 5, 2017

View reviewed changes

Use @SuppressWarnings rather than a noinspection suppression comment.

58ad436

chrisgavin added 2 commits July 6, 2017 10:22

Fix some more incorrect format strings.

26746d1

Suppress some more incorrect format string warnings where the incorre…

89a3245

…ct string is intentional.

leventov reviewed Jul 7, 2017

View reviewed changes

chrisgavin added 2 commits July 7, 2017 17:41

Log the aggregator when closing it fails.

878b393

Remove some unneeded log lines.

b151c98

Merge branch master into boxed-variables-and-format-strings.

74fb254

leventov approved these changes Jul 7, 2017

View reviewed changes

gianm added this to the 0.11.0 milestone Jul 13, 2017

gianm approved these changes Jul 13, 2017

View reviewed changes

gianm merged commit 960cb07 into apache:master Jul 13, 2017

chrisgavin deleted the boxed-variables-and-format-strings branch July 14, 2017 20:08

leventov added the Area - Automation/Static Analysis label Aug 19, 2019

leventov mentioned this pull request Aug 19, 2019

Statically prohibit missing format parameters in Druid's exception classes #8333

Open

Conversation

chrisgavin commented Jun 27, 2017

Uh oh!

jihoonson commented Jun 30, 2017

Uh oh!

chrisgavin commented Jul 3, 2017

Uh oh!

leventov commented Jul 3, 2017

Uh oh!

chrisgavin commented Jul 3, 2017

Uh oh!

leventov commented Jul 3, 2017

Uh oh!

leventov commented Jul 3, 2017

Uh oh!

leventov commented Jul 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisgavin commented Jul 6, 2017

Uh oh!

leventov commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented Jul 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants