Fix ConstructorThrow SpotBugs warnings in core module #5785

DomGarguilo · 2025-08-07T18:41:29Z

This PR fixes the warnings from the core module. Here is a description from the ticket:

This warning gets emitted when an exception is thrown within an constructor.

There are several ways to avoid this warning:

make the class final

remove the thrown exception from the constructor

add a finalize() method so it cant be overridden

make the constructor private

There are pros and cons to each fix.

making a class final prohibits inheriting it

removing the thrown exception usually means forgoing validation

adding a finalize() method means we also need to ignore the deprecation warning since finalizer() is deprecated

making the constructor private usually takes a lot of refactoring to move to a different pattern to create the object

I think the best approach here is to work through a couple modules at a time and fix these.

I removed omit block from the root pom and instead moved it into each module so that we can just ignore this spotbugs warning until we fix them in each module.

…CoreModule

ddanielr

Moving this check out from the top-level pom to the modules makes sense.

Do we need to make release notes changes for finalizing any of the classes that are defined in our public API?

DomGarguilo · 2025-08-08T16:28:41Z

Do we need to make release notes changes for finalizing any of the classes that are defined in our public API?

That is a good question. I am not sure.

DomGarguilo · 2025-08-08T16:32:16Z

I think feedback regarding the method of fixing this warning would be helpful (making the class final vs. adding the finalize() method). I tried to only make things final if it was an inner class and/or something that seemed very unlikely to be subclassed, however I just used my best judgement so would like others input on this part.

ddanielr · 2025-08-08T16:35:24Z

Do we need to make release notes changes for finalizing any of the classes that are defined in our public API?

That is a good question. I am not sure.

I think feedback regarding the method of fixing this warning would be helpful (making the class final vs. adding the finalize() method). I tried to only make things final if it was an inner class and/or something that seemed very unlikely to be subclassed, however I just used my best judgement so would like others input on this part.

I don't see an issue with finalizing the default implementation classes since users should be implementing new classes from the interface rather than extending the clientImpl classes.

Since finalize() is deprecated, does this SpotBugs warning go away if we were to switch to a newer java version?

dlmarion · 2025-08-08T19:26:36Z

I think feedback regarding the method of fixing this warning would be helpful (making the class final vs. adding the finalize() method).

I have no issue with making classes final where we can and where it makes sense. I'm not sure adding a finalizer to fix the others is a good long-term approach though it's likely the easiest fix. Finalizers themselves are deprecated and will be removed in a future Java release. I'm wondering if fixing the objects so that they don't throw exceptions from the constructor is the better choice, for example, using an init method on the objects that needs to be called post-construction but before use. There is now a jvm option, --finalization=disabled, in JDK 18 that disables all Finalizers as a step toward removal.

ctubbsii

These are pretty broad suppressions. I think it's nice that you're trying to narrow them down and triage them one at a time, but I'm wondering if this should be considered a WIP, or is this the intended final PR?

assemble/pom.xml

ctubbsii · 2025-08-13T00:39:59Z

core/pom.xml

+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <configuration>
+          <omitVisitors>SharedVariableAtomicityDetector</omitVisitors>


We have other spotbugs filters in src/main/spotbugs/exclude-filter.xml, where we have fine-grained control over things to filter, and instead of omitting entire detectors, we can filter based on the documented bug names. I'm not sure which is better, but it would probably be nice to consolidate the config, so we're doing it all in one place, instead of doing some here in the pom, and others elsewhere in the exclude-filter.xml file.

I would argue that each serves a different purpose. To me it seems the filters in exclude-filter.xml are more or less permanent for code that we don't expect to change where as this new <omitVisitors> approach is a nice way to temporarily omit these detectors while we are fixing them. If we find that we want to permanently omit these detectors than I think it would be a good idea to move things into the xml file but for now this seems to work well.

It's only temporary while it's in the PR. Once it gets merged, it's more or less permanent, or at least as permanent as the exclude-filter.xml files. My review comments are based on the desired end result after this is merged. That is to say, doing it in a different way while you're working on it is fine... but unless there's a good reason to have these configured separately from what's in the exclude-filter.xml files, then it's better that the end result by the time this is merged, has removed these extra configs, or consolidated them with the exclude-filter.xml files, so we don't have config scattered everywhere that basically does similar things.

core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java

ctubbsii · 2025-08-13T00:43:38Z

core/src/main/java/org/apache/accumulo/core/fate/Fate.java

+
+  @Override
+  @SuppressWarnings("deprecation")
+  public final void finalize() {
+    // Prevent finalizer attacks (SpotBugs CT_CONSTRUCTOR_THROW)
+  }


I'd really rather not add these methods in so many places. If this isn't possible to address another way, I think we can just suppress it. Adding this deprecated method is probably the least good workaround.

Yea I agree. Adding the finalize method definitely seems like the worst fix.

I think the best way to handle things is

if its easy and safe to remove the exception thrown from the constructor, do that (usually not possible)

try to make the class final if its obvious that it can be made final

add a @SuppressFBWarnings tag to the place in the code that the warning is coming from

Do you think that sounds good or would do you think its best to suppress this at module/project level?

Avoiding finalize here will be helpful in the future https://openjdk.org/jeps/421

Do you think that sounds good or would do you think its best to suppress this at module/project level?

I think trying those 3 fixes, in that order, are probably best. There may be good reasons to suppress at the module/project level, but it should be a very good reason. We have relatively few module/project-wide exceptions, and they are easily articulable, like the comments in the existing spotbugs/exclude-filter.xml files.

ctubbsii · 2025-08-13T00:45:17Z

hadoop-mapreduce/pom.xml

+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <configuration>
+          <omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>


In addition to previous comments about consolidating spotbugs config rules, this config here makes me wonder what rules are associated with "SharedVariableAtomicityDetector" and "ConstructorThrow", and why we're suppressing them here. It seems that suppression here is just because they are noisy?

I'm not sure i understand your question about why we're suppressing them here. Do you mean within that module specifically or within the poms in general?

I mean, having the config here raises the question "why are we skipping these checks?" without bothering to provide an answer. In the other places, such as the annotation, or the XML file, we generally offer an explanation, but there isn't one here. It's not clear what checks are being skipped, and why, when these visitors are being omitted.

…CoreModule

DomGarguilo · 2025-08-14T16:12:34Z

This PR is no longer a WIP.

To fix these spotbugs warnings I either made the class final (best solution, not possible everywhere) or I added a @SuppressFBWarnings tag to the class.

keith-turner

All the java changes look good, could not really review the maven changes very well though.

keith-turner · 2025-08-28T21:18:14Z

iterator-test-harness/pom.xml

+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <configuration>
+          <omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>
+        </configuration>
+      </plugin>


Could these changes be made in the accumulo parent pom instead of in each pom?

They could but the point of doing it in each pom is to allow us to make these changes incrementally so we don't have to fix the whole codebase at once and instead can go module by module.

It would be much simpler to just set <spotbugs.omitVisitors /> in the property section of the various POMs rather than add the whole plugin config to the pom.

Made this change in 941c2a3 @ctubbsii. Definitely a better approach than what I had before.

…CoreModule

ctubbsii · 2025-09-16T17:54:23Z

core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java

 * file://path/one/jar1.jar,file://path/two/jar2.jar
 */
-public class URLContextClassLoaderFactory implements ContextClassLoaderFactory {
+public final class URLContextClassLoaderFactory implements ContextClassLoaderFactory {


This class actually seems like it might be useful to extend. The check in the constructor probably doesn't need to be there, and is overly restrictive. It's only there as a sanity check to make sure we didn't screw up elsewhere in Accumulo... but having it here isn't exactly the right place for that check, and is overly restrictive.

ctubbsii · 2025-09-16T18:06:53Z

core/src/main/java/org/apache/accumulo/core/iteratorsImpl/conf/ColumnSet.java

+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+@SuppressFBWarnings(value = "CT_CONSTRUCTOR_THROW",
+    justification = "Constructor validation is required for proper initialization")


While we may be reluctant to make final public API classes that the user might have extended, stuff in *Impl are fair game. I'm not sure if it's possible to make them all final (we might extend them ourselves, elsewhere internally), but if it is possible, there shouldn't be any harm doing so.

ctubbsii · 2025-09-16T18:15:34Z

server/base/pom.xml

        <directory>src/test/resources</directory>
      </testResource>
    </testResources>
+    <plugins />


Suggested change

<plugins />

ctubbsii · 2025-09-16T18:15:47Z

iterator-test-harness/pom.xml

+  <build>
+    <plugins />
+  </build>


Suggested change

<build>

<plugins />

</build>

ctubbsii · 2025-09-16T18:16:02Z

server/compactor/pom.xml

        </plugin>
      </plugins>
    </pluginManagement>
+    <plugins />


Suggested change

<plugins />

ctubbsii · 2025-09-16T18:16:13Z

server/gc/pom.xml

+  <build>
+    <plugins />
+  </build>


Suggested change

<build>

<plugins />

</build>

ctubbsii · 2025-09-16T18:16:27Z

server/manager/pom.xml

+  <build>
+    <plugins />
+  </build>


Suggested change

<build>

<plugins />

</build>

ctubbsii · 2025-09-16T18:16:42Z

server/tserver/pom.xml

        </plugin>
      </plugins>
    </pluginManagement>
+    <plugins />


Suggested change

<plugins />

ctubbsii · 2025-09-16T18:16:52Z

shell/pom.xml

+  <build>
+    <plugins />
+  </build>


Suggested change

<build>

<plugins />

</build>

Fix ConstructorThrow SpotBugs warningins in core module

f1144b6

DomGarguilo added this to the 4.0.0 milestone Aug 7, 2025

DomGarguilo self-assigned this Aug 7, 2025

Merge remote-tracking branch 'upstream/main' into fixConstructorThrow…

a14fbd9

…CoreModule

ddanielr reviewed Aug 8, 2025

View reviewed changes

DomGarguilo requested a review from ctubbsii August 8, 2025 16:28

ctubbsii reviewed Aug 13, 2025

View reviewed changes

DomGarguilo changed the title ~~Fix ConstructorThrow SpotBugs warnings in core module~~ WIP - Fix ConstructorThrow SpotBugs warnings in core module Aug 13, 2025

DomGarguilo added 3 commits August 13, 2025 16:03

Remove omit block from assemble pom. Remove redundant finalize method

25506d1

Merge remote-tracking branch 'upstream/main' into fixConstructorThrow…

6c340aa

…CoreModule

Remove finalize() overrides. Add final or suppress instead

2acabf9

DomGarguilo changed the title ~~WIP - Fix ConstructorThrow SpotBugs warnings in core module~~ Fix ConstructorThrow SpotBugs warnings in core module Aug 14, 2025

use suppress instead of final to fix mocking in test

72e636d

keith-turner approved these changes Aug 28, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fixConstructorThrow…

c149a51

…CoreModule

DomGarguilo requested a review from ctubbsii August 29, 2025 16:14

DomGarguilo added 3 commits September 15, 2025 10:35

Merge remote-tracking branch 'upstream/main' into fixConstructorThrow…

e335014

…CoreModule

fix new classes

e608b7a

use pom prop instead of plugin to omit detectors

941c2a3

ctubbsii reviewed Sep 16, 2025

View reviewed changes

Fix ConstructorThrow SpotBugs warnings in core module #5785

Are you sure you want to change the base?

Fix ConstructorThrow SpotBugs warnings in core module #5785

Uh oh!

Conversation

DomGarguilo commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddanielr left a comment

Choose a reason for hiding this comment

Uh oh!

DomGarguilo commented Aug 8, 2025

Uh oh!

DomGarguilo commented Aug 8, 2025

Uh oh!

ddanielr commented Aug 8, 2025

Uh oh!

dlmarion commented Aug 8, 2025

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

DomGarguilo commented Aug 14, 2025

Uh oh!

keith-turner left a comment

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!

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!

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!

Reviewers

Assignees

Labels

DomGarguilo commented Aug 7, 2025 •

edited

Loading

keith-turner Aug 25, 2025 •

edited

Loading