Fix the warning-level CodeQL findings across the framework and connectors

[vharseko]

Closes 35 of the 37 open CodeQL alerts of severity warning without a security rating. Not in here: java/constant-comparison #1541 (ObjectPool, already fixed in #126) and java/unsafe-get-resource #1741 (ScriptedConfiguration.getClass().getResource(...): the default customizer path is built from getClass().getPackage() on purpose, so that a subclass ships its own CustomizerScript.groovy next to itself — to be dismissed as intended).

Real defects

• ADLdapUtil (dereferenced-value-may-be-null #1567 #1568): both GUID conversions swallowed the NamingException of attr.get() and then indexed the still-null array — the caller saw a NullPointerException with no clue about the LDAP error. They throw a ConnectorException carrying it now.
• AbstractRemoteConnection.convert (missing-case-in-switch #1538, #1562): no case for RequestType.API, so the method went on to call setHeader on a null request. default throws NotSupportedException.
• ObjectClassRunner.isObjectClassSupported (#1563): a contract test with no required operation iterated a null set; it now falls back to every object class of the schema.
• DocBookResourceMojo (output-resource-leak #1559): the FileWriter of the remote-resources manifest was never closed, i.e. not even reliably flushed. try-with-resources.
• OperationalContext.getConfiguration (unsafe-double-checked-locking-init-order #1553): the configuration bean was stored in the volatile field before its change callback was registered, so another thread could use it without the callback; it is built completely and published last.
• ScriptedConfiguration.getGroovyScriptEngine (#1552): the same pattern, but it can not be fixed by reordering — initializeCustomizer() → getCustomizerClass() → getGroovyScriptEngine() re-enters the getter, and the customizer script gets this. The getter is synchronized for the whole initialisation instead of double-checked, so other threads wait for the customizer rather than see the engine before it ran. Uncontended monitor, not a hot path.

Guards after the dereference / redundant guards

SchemaParser (type.getName() before if (type != null) — now a continue guard at the top, the block dedented; review with -w; a top-level element of a simple type, which threw an NPE before, is skipped with a warning), AttributeTypeUtil (attrInfo.getType() before attrInfo != null), ObjectPool.borrowObject (if (null != rv) after rv.getPooledObject(); borrowObjectNoTest never returns null), MultiOpTests (coBeforeTest starts as an empty map), TstAbstractConnector (null-safe paged-results cookie), CSVFileConnector.generateSyncDelta (explicit IllegalArgumentException when both objects are null).

Housekeeping

• field-masks-super-field #1739 #1740: the OperationMessageListener queues of ICFWebSocket and OpenICFWebSocket shadowed the listeners fields of their superclasses; renamed to messageListeners (the WebSocketListener... constructor parameter is untouched).
• input-resource-leak #1557 / output-resource-leak #1558: IOUtil.getResourceAsString closes the reader (which closes the stream; a null charset is rejected before the stream opens), IOUtil.writeFileUTF8 uses try-with-resources with StandardCharsets.UTF_8.
• non-sync-override #1548 #1547 #1783: getCause() overrides and CompletionListener.start() are synchronized like the methods they override.
• reference-equality-on-strings #1539: SQLParam.equals via Objects.equals.
• non-null-boxed-variable ×9: Boolean/Integer locals that never hold null are primitives (getColumnType never returns null — Types.NULL fallback).
• constant-comparison #1540 #1542 #1543: always-true conditions removed.

Compatibility

OpenICFWebSocketApplication.OpenICFWebSocket is public and non-final, and its protected queue is renamed listeners → messageListeners. A subclass compiled against the old name still links: its getfield listeners resolves to Grizzly's SimpleWebSocket.listeners, a Queue<WebSocketListener>, and fails with a ClassCastException on dispatch. No such subclass is known — inside this repository there is none, and a code search finds it elsewhere only in a fork that carries its own copy of the class — but a custom connector server that subclasses it has to be rebuilt.

Tests

• ADLdapUtilTests: an Attribute whose get() throws — both GUID conversions throw a ConnectorException carrying the NamingException (an NPE with the old empty catch).
• AbstractRemoteConnectionTest: a read request reporting RequestType.API fails the promise with NotSupportedException (an InternalServerErrorException wrapping the NPE without the default branch).
• SchemaParserTests.parseSchemaShouldSkipSimpleTypedElement: the test schema plus a simple-typed top-level element parses to the same schema (an NPE without the guard).

The rest are dead conditions, types, monitors and ordering with no behaviour a test can observe.

Local runs of all 13 touched modules, all green: framework 186, dbcommon 86, internal 469, contract 43, server 29, grizzly 34, csvfile 78, databasetable 43, groovy 125, ldap 159 (embedded OpenDJ), xml 81. After the review round, rebased on master: framework 192, internal 457 (without RemoteConnectorInfoManager*Tests, whose fixed port was taken by a parallel run), groovy 126, ldap 161, xml 82.

CSVFileConnector's header gets the same line #131 adds, so the two merge cleanly.

[maximthomas]

praise: The real defects are fixed where they occur, and the publication-order fixes hold up.

• ADLdapUtil.objectGUIDtoDashedString / objectGUIDtoString (ADLdapUtil.java:81, :119) now throw a ConnectorException that carries the NamingException, instead of indexing a null array.
• OperationalContext.getConfiguration builds the bean, registers addChangeCallback(this), and only then assigns the volatile configuration.
• The now-synchronized ScriptedConfiguration.getGroovyScriptEngine() adds no lock-ordering hazard: ScriptedRESTConfiguration.getHttpClient already calls it inside synchronized (this), which is the same reentrant monitor.

---

issue (non-blocking): getResourceAsString(Class, String, Charset) no longer closes the resource stream when the reader constructor throws.

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java:298-305

new InputStreamReader(ins, charset) now runs before the try, and the finally closes rdr rather than ins. When a caller of this public overload passes charset == null, the constructor throws an NPE while ins is already open, so the stream leaks. At the base, the finally closed ins. The in-repo callers all use the 2-arg UTF-8 overload, so only external bundles can reach this. Checking the charset before the stream opens keeps the reader-closing pattern that CodeQL asked for:

        assert clazz != null && StringUtil.isNotBlank(res);
        Assertions.nullCheck(charset, "charset");
        String ret = null;
        final InputStream ins = getResourceAsStream(clazz, res);

---

issue (non-blocking): parseSchema() still throws an NPE for a top-level simple-typed element in the target namespace.

OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/xsdparser/SchemaParser.java:107-108

The rewritten guard covers type, but the next dereference has no guard. For <xsd:element name="x" type="xsd:string"/>, type.getType().asComplexType() returns null (xsom SimpleTypeImpl), so xsCompType.getAnnotation() throws an NPE out of XMLConnector.schema()/init. This bug predates the PR, so it is fine as a follow-up:

            XSComplexType xsCompType = type.getType().asComplexType();
            if (xsCompType == null) {
                continue;
            }

---

question (non-blocking): Is OpenICFWebSocket meant to be subclassed outside this repo, for example by OpenIDM or by custom connector servers?

OpenICF-java-framework/connector-server-grizzly/src/main/java/org/forgerock/openicf/framework/server/grizzly/OpenICFWebSocketApplication.java:169

The class is public and non-final, and the rename listeners → messageListeners does not break the link for an old subclass binary. Its getfield ...listeners:Ljava/util/Queue; now resolves to Grizzly's SimpleWebSocket.listeners, a Queue<WebSocketListener>. An OperationMessageListener put there then fails with a ClassCastException on dispatch, not with a link error. If no downstream subclass exists, nothing changes. If one does, the release notes should name the rename. Inside this repo there is no subclass, add()/remove() are final and use messageListeners, and ICFWebSocket is private.

---

suggestion (non-blocking): Pin the NamingException → ConnectorException wrap in ADLdapUtil. No test reaches either catch today.

OpenICF-ldap-connector/src/main/java/org/identityconnectors/ldap/ADLdapUtil.java:81-83, :119-122

No test references ADLdapUtil, and the embedded OpenDJ has no attribute whose get() throws. A mutant that restores the empty catch therefore survives. This is the one fix in the PR that changes what a caller sees. The stub that the description offers is small enough to add:

package org.identityconnectors.ldap;

import javax.naming.NamingException;
import javax.naming.directory.BasicAttribute;

import org.identityconnectors.framework.common.exceptions.ConnectorException;
import org.testng.Assert;
import org.testng.annotations.Test;

public class ADLdapUtilTests {

    private static BasicAttribute unreadableGuid() {
        return new BasicAttribute("objectGUID") {
            private static final long serialVersionUID = 1L;

            @Override
            public Object get() throws NamingException {
                throw new NamingException("unreadable");
            }
        };
    }

    @Test
    public void testDashedStringCarriesNamingException() {
        try {
            ADLdapUtil.objectGUIDtoDashedString(unreadableGuid());
            Assert.fail("ConnectorException expected");
        } catch (ConnectorException e) {
            Assert.assertTrue(e.getCause() instanceof NamingException);
        }
    }

    @Test
    public void testStringCarriesNamingException() {
        try {
            ADLdapUtil.objectGUIDtoString(unreadableGuid());
            Assert.fail("ConnectorException expected");
        } catch (ConnectorException e) {
            Assert.assertTrue(e.getCause() instanceof NamingException);
        }
    }
}

Pin: with the empty catch restored, both cases fail with an NPE instead of a ConnectorException.

---

suggestion (non-blocking): No test reaches the new default branch of AbstractRemoteConnection.convert().

OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/crest/AbstractRemoteConnection.java:496-497

No groovy test references RequestType.API, so deleting the branch survives the suite, and rq.setHeader at :502 then throws an NPE again. The branch can only be reached through a Request whose getRequestType() returns API.

Pin: wrap a ReadRequest so that getRequestType() returns RequestType.API, pass it to readAsync, and assert the failure is a NotSupportedException. Without the branch, the same case fails with an NPE.

[vharseko]

All five addressed in 14c92b0, on top of a rebase onto current master.

IOUtil — fixed: getResourceAsString(Class, String, Charset) checks charset with Assertions.nullCheck before the stream opens. The caller still gets a NullPointerException, now with the parameter name, and the reader-closing finally stays as CodeQL wants it.

SchemaParser — fixed here rather than as a follow-up, since the PR already rewrote that block: a top-level element whose type is not complex is skipped with a warning (it can not be an object class). SchemaParserTests.parseSchemaShouldSkipSimpleTypedElement adds <xsd:element name="note" type="xsd:string"/> to the test schema and checks that the parsed schema equals the one without it; without the guard it fails with the NPE.

OpenICFWebSocket subclasses — none that I can find: a code search over GitHub finds OpenICFWebSocket only in OpenICF itself and in the WrenSecurity fork, which carries its own copy of the class rather than subclassing ours; OpenIDM does not reference it. Your analysis of the old binary is right (the getfield resolves to SimpleWebSocket.listeners), so the rename is now named under "Compatibility" in the PR description.

Tests — both added:

• ADLdapUtilTests: your stub, as is. With the empty catch restored both cases fail with an NPE.
• AbstractRemoteConnectionTest: a ReadRequest proxy reporting RequestType.API goes through readAsync, and the promise fails with NotSupportedException. A Proxy over a real Requests.newReadRequest rather than a Mockito mock, so the URI builder gets a real path and fields. Without the default branch it fails with an InternalServerErrorException wrapping the NPE.