Fix mechanical CodeQL note findings: reflection, boxing, deprecated closes

[vharseko]

Closes ~76 of the open note-severity CodeQL alerts (no security rating) that are safe, mechanical fixes: java/deprecated-call (Class.newInstance, SQLUtil.closeQuietly, plexus IOUtil.close), java/inefficient-boxed-constructor, java/inefficient-string-constructor, java/ignored-error-status-of-call, java/inefficient-key-set-iterator, java/inefficient-empty-string-test, java/missing-space-in-concatenation, java/unknown-javadoc-parameter, plus the 8 java/uncaught-number-format-exception alerts in AttributeTypeUtil that the boxing change touched anyway.

Class.newInstance() — 20 sites, 16 files

Replaced with the new ReflectionUtil.newInstance(MethodHandles.lookup(), clazz). Class.newInstance() rethrew whatever the constructor threw, unchanged. A bare getDeclaredConstructor().newInstance() wraps it in InvocationTargetException instead, and the catch-and-wrap sites would then turn e.g. a ConfigurationException from a connector constructor into ConnectorException("java.lang.reflect.InvocationTargetException"). The helper invokes the constructor through a MethodHandle, so the exception reaches the caller as is. Access is checked against the caller's lookup, so a package-private class (StdOutLogger from Log) stays instantiable from its own package. Every site is already inside a catch (Exception …)/catch (Throwable …) or declared throws Exception, so the reflective exceptions need no new handling anywhere.

Boxed constructors — 19 sites

new Integer/Long/Boolean/Double/Float/Character(String) → valueOf/parseX in AttributeTypeUtil, RandomGenerator, SQLUtil; new String(x) → x in AttributeTypeUtil. SQLUtil.string2Timestamp/string2Date use Long.parseLong (a primitive is what the Date/Timestamp constructor needs, so no boxing round-trip at all).

AttributeTypeUtil.createInstantiatedObject (xml-connector) also gets the NumberFormatException handling it was missing: a malformed numeric value in the XML data file now fails with a ConnectorException naming the value and the target type instead of a bare NumberFormatException with no indication of which attribute it came from. This closes the 8 java/uncaught-number-format-exception alerts in that class. The parse calls sit in a helper that declares throws NumberFormatException — CodeQL's query (NumberFormatException.ql) only accepts a catch in the same method or a throws clause on the enclosing one, so the first cut with just a wrapper method left all eight alerts open on the PR's own CodeQL run.

Deprecated closes — 26 sites

• SQLUtil.closeQuietly(Connection/Statement/ResultSet) (deprecated in favour of IOUtil.quietClose) is replaced at its 23 call sites, including its own 6 internal uses.
• Found and fixed a real regression along the way: IOUtil.quietClose(Connection) did not check isClosed() before calling close(), unlike the SQLUtil.closeQuietly(Connection) it replaces. Harmless against a real JDBC driver (Connection.close() on an already-closed connection is a spec-mandated no-op), but two tests using a strict-call-sequence mock (DatabaseConnectionTest.testDispose, SQLUtilTests.quietConnectionClose) caught the difference immediately. IOUtil.quietClose(Connection) now checks isClosed() first, matching what it replaces and what its own javadoc promises.
• Plexus IOUtil.close(Closeable) — its javadoc says "deprecated: use try-with-resources instead" — is replaced by try-with-resources at its 3 call sites in the maven plugin.

ignored-error-status-of-call — 3 sites

XMLConfiguration.validate: getParentFile().mkdir()'s return value is checked now, distinguishing "the directory already existed" from a real failure. GroovyDataProvider (×2): an out file that cannot be created (a directory or a dangling symlink at that path) now throws into the existing catch (IOException), which clears the field, so nothing is stored. An already existing file is still used, as before.

Small mechanical ones

PrettyStringBuilder: map.keySet().iterator() + map.get(key) → map.entrySet().iterator(). StringUtil.isEmpty and an XSD annotation check: "".equals(x) / x.equals("") → x.isEmpty(). A missing space in a concatenated log message (ActiveDirectoryChangeLogSyncStrategy) and a stray comma in a @param javadoc tag (MultiOpTests) are fixed.

Tests

New: AttributeTypeUtilTest (21) — each numeric type parses, and a malformed or blank value fails with a ConnectorException naming the value and the type, caused by the NumberFormatException, for each of the 10 numeric types including BigInteger/BigDecimal. ReflectionUtilTests (+5) — the constructor's RuntimeException, checked exception and Error reach the caller unchanged, and a class without a no-arg constructor fails. XMLConfigurationTests (+1) — a file path without a parent directory validates. Everything else is a mechanical replacement with identical behaviour, except the IOUtil.quietClose(Connection) fix, which is proven by the two existing tests that caught the regression and now pass.

Local runs of all 11 touched modules, all green: connector-framework 197, dbcommon 86, connector-test-common 5, connector-framework-internal 499 (2 skipped, same as on master), connector-framework-osgi (compiles), connector-framework-contract 43, maven-plugin 1, connector-framework-server 32, databasetable-connector 78, ldap-connector 159 (embedded OpenDJ), xml-connector 103. (dbcommon, databasetable-connector and ldap-connector are from the first round; the review round did not touch them.)

Left open on purpose

java/missing-override-annotation (1327 — mechanically safe but would touch ~200 files and conflict with the six other open CodeQL PRs), java/deprecated-call on MessagesUtil.*Legacy (67 — the framework's own deprecated API, its only path for script arguments today), java/uncaught-number-format-exception (54 after the 8 closed here — needs a per-site judgment call, not mechanical; #139 takes them), java/unused-parameter / java/constants-only-interface / java/jdk-internal-api-access (public API or no JDK alternative), java/chained-type-tests (needs an actual refactor), java/call-to-object-tostring / java/local-shadows-field / java/confusing-method-signature (judgment calls, not mechanics).

[vharseko]

The red CodeQL check here is the 2 pre-existing java/zipslip (high) alerts in DocBookResourceMojo.java:414 and ConnectorInfoReportMojo.java:343. Both already exist on master (since 2026-07-10) and are unrelated to this PR's diff — they're only flagged because this diff touches nearby lines in the same files.

#126 fixes exactly those two lines (wraps the new File(...) calls in IOUtil.resolveEntry(...)), and CodeQL on #126 is green. Once #126 merges to master and this branch is rebased on top, the check here should clear on its own.

[maximthomas]

praise: The cleanup finds and fixes real behaviour along the way, and each fix has a test.

• IOUtil.quietClose(Connection) gets back the isClosed() guard of the SQLUtil.closeQuietly(Connection) it replaces. DatabaseConnectionTest.testDispose and SQLUtilTests.quietConnectionClose caught the missing guard.
• AttributeTypeUtil.createInstantiatedObject now turns a malformed numeric value into a ConnectorException that names the value and the type. The new AttributeTypeUtilTest pins this for all 8 types CodeQL flagged.

---

issue (blocking): getDeclaredConstructor().newInstance() wraps constructor exceptions in InvocationTargetException, so the catch-and-wrap sites now hide the original exception type.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/ConnectorPoolManager.java:169, OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java:329, OpenICF-java-framework/connector-framework-osgi/src/main/java/org/forgerock/openicf/framework/impl/api/osgi/internal/OsgiConnectorInfoManagerImpl.java:338, OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/osgi/internal/AsyncOsgiConnectorInfoManagerImpl.java:245, OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ConnectorHelper.java:122, :125

Class.newInstance() rethrew whatever the constructor threw, unchanged. Constructor.newInstance() wraps every throwable, Errors included, in the checked InvocationTargetException. Each of these sites ends in catch (Exception e) { throw ConnectorException.wrap(e); } (ContractException.wrap in ConnectorHelper). wrap passes a RuntimeException through and rethrows an Error, but turns the ITE into new ConnectorException(ite).

The result: a ConnectionFailedException or ConfigurationException from a connector or configuration constructor now reaches the caller as a plain ConnectorException with the message java.lang.reflect.InvocationTargetException. A NoClassDefFoundError becomes a RuntimeException. You can see it by running the contract tests without -DconnectorName. That used to fail with GroovyDataProvider's "To run contract tests, you must specify valid [connectorName] ...". It now fails with ContractException("java.lang.reflect.InvocationTargetException"), and that message sits two causes down. ConnectorAPIOperationRunnerProxy:111 is the only site that already unwraps.

} catch (InvocationTargetException e) {
    throw ConnectorException.wrap(e.getCause()); // ContractException.wrap in ConnectorHelper
} catch (Exception e) {
    throw ConnectorException.wrap(e);
}

Apply the same unwrap at the factory singletons (ConnectorFacadeFactory:55/:74, ConnectorInfoManagerFactory:51, ObjectSerializerFactory:54, ConnectorServer:113, TestHelpers:219/:312) to keep the old behaviour there too. Also put it ahead of the catch (RuntimeException e) branches in Log.java:149 and EncryptorFactory.java:43, which no longer see constructor exceptions.

---

issue (non-blocking): ScriptExecutorFactory.getFactoryCache now silently skips a factory whose constructor throws, and logs the cause as Exception:null.

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/script/ScriptExecutorFactory.java:74, :80, :82, :173

The catch (RuntimeException e) { throw e; } at :80 no longer sees constructor exceptions. They arrive as InvocationTargetException in catch (Throwable) at :82, which logs e.getMessage(), and that is null for an ITE. Say JavaScriptExecutorFactory() throws "JavaScript Engine is not found". Every later newInstance("JavaScript") then fails with "Language not supported: JavaScript", and nothing in the log says why. newInstance(language) at :173 now throws RuntimeException(ITE) instead of the original exception.

} catch (Throwable e) {
    Throwable cause = e instanceof InvocationTargetException ? e.getCause() : e;
    logger.ok("ScriptExecutorFactory {0} can not be activated. Exception:{1}", factory, cause);
}

Or: unwrap as in the blocking issue to restore the old propagation.

---

issue (non-blocking): In GroovyDataProvider, the new !createNewFile() throw only fires on a race or a dangling symlink. When it does, it logs "will not be stored" and the parameters are stored anyway.

OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/data/GroovyDataProvider.java:241, :250, :266

File.createNewFile() returns false only when the path already exists. A real I/O failure already threw IOException into the same catch before this PR. So the new branch fires in only two cases: the file appears between exists() and createNewFile(), or the path is a dangling symlink (exists() false, createNewFile() false). The catch warns "Unable to create ... the test parameters will not be stored" but leaves _queriedPropsOutFile / _propertyOutFile set. The writers check only for null, so they store the parameters anyway, through the symlink. Before this PR, the symlink case reached canWrite() == false, nulled the field and stored nothing.

_queriedPropsOutFile = new File(pOut);
if (!_queriedPropsOutFile.createNewFile() && !_queriedPropsOutFile.isFile()) {
    throw new IOException("Could not create " + _queriedPropsOutFile);
}
// ...
} catch (IOException iOException) {
    _queriedPropsOutFile = null;
    LOG.warn("Unable to create ''{0}'' file, the test parameters will not be stored", pOut);
}

Make the same change for _propertyOutFile at :266.

---

suggestion (non-blocking): AttributeTypeUtilTest checks only the exception class, not the message, and leaves out BigInteger/BigDecimal.

OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/util/AttributeTypeUtilTest.java:45, :51, :55

The test's Javadoc promises an exception that names the value and the target type, but nothing checks the message. Change AttributeTypeUtil.java:50 to throw new ConnectorException(e) and all 16 rows stay green. The BIG_INTEGER/BIG_DECIMAL branches (AttributeTypeUtil.java:107-113) sit behind the same catch, and no test calls them.

@Test(dataProvider = "numericTypes")
public void createInstantiatedObjectWrapsMalformedNumber(String type) {
    try {
        AttributeTypeUtil.createInstantiatedObject("not-a-number", type);
        fail("ConnectorException expected for " + type);
    } catch (ConnectorException e) {
        assertEquals(e.getMessage(), "Value 'not-a-number' is not a valid " + type);
        assertTrue(e.getCause() instanceof NumberFormatException);
    }
}

Pin: the message assertion catches the mutant above. Add the rows { XmlHandlerUtil.BIG_INTEGER } and { XmlHandlerUtil.BIG_DECIMAL } to numericTypes() to cover the Big* branches.

---

suggestion (non-blocking): No test covers the new parentFile != null guard in XMLConfiguration.validate.

OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConfiguration.java:110

Take a relative xmlFilePath with no parent directory (getParentFile() == null). Before this PR it threw NullPointerException, and it now validates. Drop parentFile != null && and the suite stays green. Every setCreateFileIfNotExists(true) test uses a file whose parent directory exists.

@Test
public void shouldValidateXmlFilePathWithoutParentDirectory() throws IOException {
    File xsd = File.createTempFile("schema", ".xsd");
    xsd.deleteOnExit();
    File xml = new File("xml-config-" + System.nanoTime() + ".xml"); // getParentFile() == null
    config.setXsdFilePath(xsd);
    config.setXmlFilePath(xml);
    config.setCreateFileIfNotExists(true);
    config.validate(); // NullPointerException before the guard
    AssertJUnit.assertFalse(xml.exists());
}

Pin: this test fails with NullPointerException as soon as the guard is removed.

[vharseko]

@maximthomas addressed in 7500a98 (the branch is also rebased on current master, so the java/zipslip note above no longer applies — #126 is in):

InvocationTargetException (blocking) and ScriptExecutorFactory. Fixed with one helper rather than a catch at every site: ReflectionUtil.newInstance(MethodHandles.lookup(), clazz) invokes the no-arg constructor through a MethodHandle, so whatever the constructor throws reaches the caller unchanged, as with Class.newInstance(). All 20 sites use it, including JavaClassProperties.createBean2 and ConnectorDocBuilder, which weren't in your list. The caller's lookup matters: a first cut that called Constructor.newInstance() inside the helper broke Log → StdOutLogger (package-private), because the access check then ran against ReflectionUtil; LogTests.checkSystemProperty caught it. ReflectionUtilTests pins RuntimeException, checked exception and Error passing through unchanged. The Class.newInstance paragraph in the description was wrong and is corrected.

GroovyDataProvider. Taken as suggested: !createNewFile() && !isFile(), and the catch clears the field, at both sites.

AttributeTypeUtilTest. It now asserts the message and the NumberFormatException cause, and has BIG_INTEGER/BIG_DECIMAL rows.

XMLConfiguration. Added shouldValidateXmlFilePathWithoutParentDirectory, with cleanup in finally.