Repository navigation
Make safe-to-static nested classes static and drop dead locals - #143
Conversation
vharseko
added
java
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: Where a dropped local's right-hand side had a side effect, the call is kept and only the binding goes.
VlvIndexSearchStrategykeepsreader.readInteger(), so the ASN.1 reader still steps past the offset field.ClientRemoteConnectorInfoManagerkeepsnew RemoteConnectionContext(conn, connectionInfo)and says in a comment that the constructor registers itself on the connection.OpenICFServerAdapter.initialiseEncryptor()is left to #126 rather than edited into a second conflict.
issue (blocking): The head does not merge into current master: ConnectionManager.java conflicts in its copyright header.
OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/client/ConnectionManager.java:14
Master (be3ae4a, via #123/#138) added * Portions Copyrighted 2026 3A Systems, LLC after the 2015 ForgeRock line; this PR adds * Portions Copyrighted 2026 3A Systems LLC. in the same place. git merge-tree --write-tree origin/master e3f989e7 reports CONFLICT (content) in this file, with markers only around those two lines, and GitHub shows the PR as CONFLICTING. The green CI ran on a merge with the old base fc18556, not on current master. ClientRemoteConnectorInfoManager and OpenICFServerAdapter, also changed on master, merge cleanly.
* Portions Copyrighted 2015 ForgeRock AS.
* Portions Copyrighted 2026 3A Systems, LLC
*/Rebase onto master and keep master's line.
suggestion (non-blocking): PROP_FACADE_LIFETIME in the grizzly Main is now an unread constant, and the server ignores connectorserver.maxFacadeLifeTime without saying so.
OpenICF-java-framework/connector-server-grizzly/src/main/java/org/forgerock/openicf/framework/server/Main.java:64
This predates the PR, as its description says: the grizzly server disposes facades after a fixed 120 minutes (ConnectorFramework.java:225), while the legacy Main applies the key (connector-framework-internal Main.java:249-250). The description calls the constant a "documented-but-unimplemented setting", but no file in the repo documents it, so nothing tells an operator the key has no effect. A comment on the constant makes that true in the code; porting the setting stays the follow-up the description names.
// Not applied by the grizzly server: facade lifetime is fixed in ConnectorFramework.
private static final String PROP_FACADE_LIFETIME = "connectorserver.maxFacadeLifeTime";nitpick (non-blocking): The new copyright lines use three spellings, none of them the repo's canonical one.
OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/client/ConnectionManager.java:14, OpenICF-java-framework/connector-server-grizzly/src/main/java/org/forgerock/openicf/framework/server/Main.java:23, OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/TstAbstractConnector.java:24, OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/TstConnector.java:23, OpenICF-ldap-connector/src/main/java/org/identityconnectors/ldap/AppendingAttributes.java:23, OpenICF-ldap-connector/src/main/java/org/identityconnectors/ldap/LdapConnection.java:24, OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java:17
Master uses * Portions Copyrighted 2026 3A Systems, LLC in 170+ headers, including the other seven files this PR touches. The PR adds 3A Systems LLC. in five files, 3A Systems LLC in LdapConnection, and Portions Copyright 2026 3A Systems, LLC. in CSVFileConnector. A grep for the canonical line misses these files, and the different spelling is what causes the blocking conflict above. VlvIndexSearchStrategy follows that file's own quoted style, which is fine.
* Portions Copyrighted 2026 3A Systems, LLC
e3f989e to
c8087ce
Compare
vharseko
left a comment
vharseko
left a comment
There was a problem hiding this comment.
Blocking — merge conflict in ConnectionManager.java: rebased onto current master (e6bad43). The header conflict is resolved by keeping master's * Portions Copyrighted 2026 3A Systems, LLC line; the rest of the commit applied cleanly, including the auto-merged ClientRemoteConnectorInfoManager and OpenICFServerAdapter.
PROP_FACADE_LIFETIME: added the suggested comment on the constant, so the code itself now says the grizzly server does not apply the key. Porting the setting stays a separate follow-up.
Copyright spelling: Main, TstAbstractConnector, TstConnector, AppendingAttributes and LdapConnection now use the canonical * Portions Copyrighted 2026 3A Systems, LLC. CSVFileConnector keeps Portions Copyright 2026 3A Systems, LLC.. Its header is written in the Portions Copyright <year> <owner> form throughout: the template on line 12 and the two existing Portions Copyright 2011 … lines. This is the same file-own-style reason you accepted for VlvIndexSearchStrategy.
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The rebase and the follow-up commit close every round-1 item without widening the change.
ConnectionManager.javanow carries master's* Portions Copyrighted 2026 3A Systems, LLCline, so the head merges cleanly into masterc7648cfd.PROP_FACADE_LIFETIMEin grizzlyMainnow states in place that the server does not apply the key (Main.java:64).- Each dropped local keeps its side effect:
reader.readInteger()inVlvIndexSearchStrategy,new RemoteConnectionContext(conn, connectionInfo)inClientRemoteConnectorInfoManager,PingMessage.parseFrom(bytes)inOpenICFServerAdapter.
c8087ce to
d106fd8
Compare
|
Rebased onto current Both commits conflicted only on the license header of |
- Add static to 6 nested classes that never reference their enclosing instance (TstConnector.BatchUseCase2Processor, TstAbstractConnector.BatchUseCase3Processor, AppendingAttributes.AppendingEnumeration, AbstractRemoteConnection.InternalFutureCallback, ConnectorEventSubscriptionApiOpImpl.InternalRequestFactory, CSVFileConnector.OptionalTrim). - Drop 9 local variables that are assigned but never read. Where the assignment's call has a real side effect (ASN.1 cursor advance in VlvIndexSearchStrategy, protobuf validation in OpenICFServerAdapter's onPing/onPong, self-registration in RemoteConnectionContext's constructor) the call stays, only the unused binding goes; the rest (LdapConnection.getAnonymousContext, SQLUtil.getDriverMangerConnection, UpdateApiOpTests.testUpdateToNull's redundant pre-update fetch, ConnectionManager.doClose's commented-out-since-2015 close loop) were fully dead and are removed outright. - Leave OpenICFServerAdapter.initialiseEncryptor()'s dead encryptor local alone: that whole method is removed by OpenIdentityPlatform#126.
d106fd8 to
4cbfa0b
Compare
… NumberFormatException (#139) Closes 48 of the 54 remaining `java/uncaught-number-format-exception` alerts (the 8 in `AttributeTypeUtil` were already fixed in #134). Dismisses the remaining 6 as unreachable in practice, with the reasoning recorded on each alert. ## AD/LDAP cluster — 26 of 28 sites `ADUserAccountControl`, `ADGroupType` and `ActiveDirectoryChangeLogSyncStrategy` share one root cause: Active Directory bitmask attributes (`userAccountControl`, `groupType`) and sync tokens are parsed with plain `Integer.parseInt`, nothing around it. `ADLdapUtil` gets `parseADInteger`/`parseADLong` (new, unit-tested), and all three classes route through it — a corrupted AD attribute, or a sync token tampered with by whoever calls the connector's `sync()` operation, now fails with a `ConnectorException` naming the value instead of a bare `NumberFormatException` three frames deep with no context. The sync strategy's USNs (`uSNChanged`, `highestCommittedUSN`) are 64-bit in AD, so they go through `parseADLong` and the changes are ordered in a `TreeMap<Long, SyncDelta>`; before, a domain controller past `Integer.MAX_VALUE` could not sync at all. `PagedSearchStrategy`'s paged-results cookie is `<base64 cookie>:<context index>`; the base64 half was already inside a `try`/`catch(RuntimeException)` reporting `ConnectorException`, the numeric half was one line below it, outside. Moved in, and an index outside `baseDNs` (`AAAA:5`, `AAAA:-1`, and `AAAA:1` against a single base DN) is now rejected the same way instead of escaping as an `IndexOutOfBoundsException`. Left alone and dismissed as unreachable (`ADLdapUtil.binarySIDtoString`, 2 sites): the hex strings it parses are built two lines above via `String.format("%02X...")` from raw bytes, so they are always valid hex. ## Elsewhere — 22 sites, same idiom as what already surrounds them - `PropertyBag.castValue` (maven-plugin, 8): POM `<configuration>` values converted to typed connector config properties at build time; the conversion chain is wrapped as a whole, reporting through the `MojoExecutionException` the method already uses two lines above for the equivalent blank-value case. - `SQLUtil.attribute2jdbcValue` (dbcommon, 6): converts an attribute value to a JDBC-bindable value for a numeric SQL column; wrapped with `ConnectorException`, already imported and used elsewhere in the file. - `XmlObjectDecoder` (connector-framework-internal, 5): the five primitive decoders of the XML wire protocol — the one genuinely network-facing trust boundary in this batch. Wrapped with `ConnectorException`, matching `decodeClass`'s existing convention two methods above; an empty element (no text node, which `#PCDATA` allows) is rejected the same way instead of an NPE. - `RemoteWSFrameworkConnectionInfo.loadSystemProxy` (1): a typo in `-Dhttp.proxyPort` now fails clearly at startup instead of with a bare NFE. - `ConnectorHelper` / `AuthenticationApiOpTests` (connector-framework-contract, 2): contract-test configuration (`-DserverPort`, a test-suite attribute) now reports through `ContractException`, the framework's own convention, instead of a bare NFE. ## Dismissed as unreachable — 6 - `ADLdapUtil.binarySIDtoString` ×2 (see above). - `TstStatefulConnectorConfig` (testbundlev1): parses a revision the same test connector generated two lines above via `AtomicInteger.getAndIncrement()`/`String.valueOf()` — no external input. - `JavaScriptExecutorFactory`: parses the JVM's own `java.specification.version`, guaranteed valid by the JDK. - `CSVFileConnector` ×2: parses a substring the regex `\.[0-9]{13}$` just matched immediately before — always exactly 13 ASCII digits. ## Tests New: `ADLdapUtilTest` (8), `ADUserAccountControlTest` (2), `ADGroupTypeTest` (2), `PagedSearchStrategyTest` (4), `SQLUtilTests` (+3), `XmlObjectDecoderTest` (11, corrupts or empties a value inside a real serialized document rather than hand-building the XML schema), `RemoteWSFrameworkConnectionInfoTest` (1), `PropertyBagTests` (2, through the public `mergeConfigurationProperties`) — 33 new/changed tests. Each is RED before its fix and green after, except four that are green on master by design: `decodesAValidInt`, `isAccountDisabledReadsTheBitmask` and `isScopeGlobalReadsTheBitmask` are positive controls for the parse path, and `convertsAnIntWithoutTheUnsupportedWarning` guards against the round-1 regression where a split `if` chain logged "Cast to targetType ... is not supported" for every numeric property. The wrapping tests (`ADLdapUtilTest`, `XmlObjectDecoderTest`, `SQLUtilTests`, `RemoteWSFrameworkConnectionInfoTest`, `PropertyBagTests`) pin the exact message and the `NumberFormatException` cause, so a catch that drops either fails. `ConnectorHelper`/`AuthenticationApiOpTests` are contract-test configuration parsers with no dedicated unit tests of their own; verified by compiling and by the module's existing suite. Local runs after rebasing onto current master (`1abfe741`, through #143), one reactor build of every touched module with its dependencies, all green: connector-framework 197, dbcommon 89, connector-framework-internal 520 (2 skipped, same as on master), connector-framework-contract 43, maven-plugin 3, connector-framework-server 33, ldap-connector 179 (embedded OpenDJ). The counts are higher than in earlier rounds because master gained tests meanwhile (e.g. `ADUserAccountControlTests` from #141).
…nterned-string locks, racy lazy init (#132) Closes the remaining 10 open CodeQL alerts of severity `error` that carry no security rating: `java/contradictory-type-checks` #1536 #1537, `java/dereferenced-value-is-always-null` #1560, `java/sync-on-boxed-types` #1554 #1555 #1556, `java/unsafe-double-checked-locking` #1550 #1551, `java/unsynchronized-getter` #1544 #1545. The eleventh, #1549 (`ScriptedConfiguration`), was closed by #133. Fixes #148 Fixes #149 | Where | Finding | Change | |---|---|---| | `SQLUtil.setParam` | `else if (val instanceof Integer)` appears twice; the second branch can never run | second branch removed | | `ContractITCase.createInstances` | `IObjectFactory objectFactory = null` is dereferenced for any test class with a `(String)` constructor. None of the default classes has one today, so the code only worked because the `NoSuchMethodException` path was always taken | the `(String)` constructor is called through plain reflection, so the method no longer depends on TestNG's internal `ObjectFactoryImpl`, which TestNG 7.5 moved and whose `IObjectFactory` 7.10.2 removed. The unused nested `ContractTestFactory` with its three never-read fields is gone | | `BatchRemoteCache` (testbundlev1) | `synchronized (resultLock)` on the interned literal `"resultLock"`, a monitor shared with any other code in the JVM that synchronises on the same string | `new Object()` | | `WebSocketConnectionGroup.operationContext`, `TstStatefulConnectorConfig.executorService` | double-checked lazy initialisation reads a non-volatile field outside the lock, so a thread may see a partially constructed object | fields declared `volatile`, which makes the pattern correct under the JMM | | `ScriptedConfiguration.getGroovyScriptEngine()` | #133 made the getter `synchronized` for the whole initialisation. That closed the double-checked-locking alert and #149 (the getter and `release()` share the monitor now), and concurrent callers wait for the customizer. But the getter still stored the engine before its customizer ran, so a failed customizer left an uncustomized engine in place for good (#148) | on top of #133's synchronised getter, the engine is published only after `initializeCustomizer()` succeeds. The customizer's own re-entrant calls get the engine under construction from a second lock-guarded field. If the customizer fails, nothing is published and the next call retries; the failed customizer class is removed with `InvokerHelper.removeClass`, so the metaclass the REST, CREST and SSH configurations register on it does not keep one class loader alive per retry. The customizer runs under the configuration's monitor, so it must not wait for another thread that uses the engine; the comment on `initializeCustomizer()` says so | | `customize` in `SSHConfiguration`, `ScriptedCRESTConfiguration` | with the retry above, a failed attempt that registered `release {}` left its closure for `release()` to run after a successful retry that registered none: the SSH `customize` never reset it, and CREST's `release = null` wrote a private field nothing reads | both reset it with `setReleaseClosure(null)`, as REST's `customize` already does; CREST's unread field is removed | | `FrameworkUtil.getFrameworkVersion()` | unsynchronised lazy init while `setFrameworkVersion` is synchronised | `static synchronized` (not a hot path). The `(ClassLoader)` overload reads `connectors-framework.properties` rather than the field and is only used by `FrameworkUtilTests`, so it is renamed to `readFrameworkVersion` and the getter/setter pairing no longer applies to it | `ScriptedConfigurationTest` covers #148: a customizer that fails once is retried on the next call, a second thread blocks until the customizer has finished, and a failed customizer class has no metaclass left registered. Each test fails when its part of the fix is reverted (engine published before the customizer, no `removeClass`, getter not synchronised). `ScriptedConfigurationTest` (CREST) and `SSHConfigurationCustomizerTest` (SSH) cover the release closure: a first attempt registers `release {}` and fails, the retry registers none, and `release()` must not run the stale closure; both fail without the `customize` reset. #149 is fixed on master by #133; it was a window between two adjacent field reads that no test can hold open, so it has no test. The other changes have no behaviour a unit test can observe: dead branches, `volatile`, a private monitor and a synchronised getter. `createInstances` takes its classes from a fixed list with no `(String)` constructors, so its reflective path cannot be reached from a test. The class of a customizer that **succeeded** is still never unregistered, so `release()` keeps its loader alive; master behaves the same, and that is tracked separately in #154. Local reactor run (rebased on current master, after #131 #133 #134 #135 #136 #139 #141 #142 #143 #145) of all touched modules and everything they depend on: connector-framework 200, dbcommon 89, framework-internal 524 (2 skipped), contract 43, connector-framework-server 33, groovy-connector 130 (42 skipped as on master, 4 new), ssh-connector 97 (82 skipped, 1 new), testbundlev1 compiles — all green.
2 participants
Summary
Closes out two more CodeQL
note-severity categories:non-static-nested-class(6 alerts) andlocal-variable-is-never-read(10 alerts, 9 fixed here).staticto 6 nested classes confirmed to never reference their enclosing instance (each checked individually — some call outer-looking methods that actually resolve to an inherited superclass method, not the enclosing instance):TstConnector.BatchUseCase2Processor,TstAbstractConnector.BatchUseCase3Processor,AppendingAttributes.AppendingEnumeration,AbstractRemoteConnection.InternalFutureCallback,ConnectorEventSubscriptionApiOpImpl.InternalRequestFactory,CSVFileConnector.OptionalTrim.VlvIndexSearchStrategy:reader.readInteger()still advances the ASN.1 cursor past the offset field even though the offset itself isn't needed.OpenICFServerAdapter.onPing/onPong:PingMessage.parseFrom(bytes)still validates the payload (a malformed frame is caught and logged) even though the parsed message carries no payload we use.ClientRemoteConnectorInfoManager:RemoteConnectionContext's constructor registers itself on theConnectionas a side effect (set(c, this)), so the object still needs constructing even though nothing reads the local.LdapConnection.getAnonymousContext()'s unusedctx,SQLUtil.getDriverMangerConnection()'s unusedretarray,UpdateApiOpTests.testUpdateToNull()'s redundant pre-updategetObject()fetch (never asserted on, unlike the post-update fetch), andConnectionManager.doClose()'sforloop whose body has been a commented-outgroup.close()since the file's first commit in 2015.OpenICFServerAdapter.initialiseEncryptor()'s unusedencryptorlocal (alert on line 500) untouched: that whole method is dead (guaranteed NPE onmessage.getPublicKey()sincemessageis hardcodednull, and it's never called from anywhere) and is already deleted by Refuse archive entries that escape their target directory and drop dead ECIES code #126 — fixing it here would just conflict with that PR.Worth a follow-up look (not fixed here, out of scope for this pass)
ConnectionManager.doClose()'s commented-outgroup.close()not running for over a decade may be deliberate, possibly tied to the knownWebSocketConnectionGroupNPE (NullPointerException in WebSocketConnectionGroup.shutdown() when a client is closed while a request is registered but not yet sent #124) rather than an oversight — worth its own investigation before touching.Main.loadProperties()readsconnectorserver.maxFacadeLifeTimefrom properties, but grizzly's ownConnectorServerhas nosetMaxFacadeLifeTime()at all (unlike the framework variant'sMain.java, which does apply it) — the feature was never ported to the grizzly server. Removed the now-dead local read; left the property-name constant in place, with a comment saying the grizzly server does not apply it (facade lifetime is fixed at 120 minutes inConnectorFramework).Test plan
mvn installonOpenICF-csvfile-connector,OpenICF-dbcommon,OpenICF-groovy-connector,connector-framework-contract,connector-framework-server,connector-server-grizzly,testbundlev1,OpenICF-ldap-connector(incl. their existing test suites, LDAP/OpenDJ integration tests included) — all green.mvn test-compileon the same modules (with-am) — green.