[vharseko]

Closes the 16 note-level code scanning alerts that are worth fixing (#10–#15, #32, #46–#53, #58). The remaining notes are either pure @Override/static cosmetics (done separately in #180) or by-design findings to be dismissed.

openig-oauth2 — OAuth2Session

Alert	Change
#32 missing-space-in-concatenation	"…ID token" + "could not be decoded" produced ID tokencould not be decoded in the server_error description.
#53 uncaught-number-format-exception	A string expires_in that is not a number now raises OAuth2ErrorException(E_SERVER_ERROR, "'expires_in' field value is not a number: …"), consistent with the sibling "neither a Number nor a String" branch, instead of a raw NumberFormatException.
#58 local-shadows-field	The merged token response local is renamed mergedResponse; its uses (extractIdToken, the constructor) are pointed at it. No behavior change — testStateRefreshed covers the merge semantics.

openig-core

Alert	Change
#48, #49 uncaught-number-format-exception	ICAPFilter: a connect_timeout / read_timeout that is not an int now fails with a JsonValueException that points at the field (/connect_timeout: Expecting a number of milliseconds that fits in an int, got: soon) instead of NumberFormatException: For input string: "soon".
#12 deprecated-call (Object.finalize)	LdapClient.finalize() removed: the class is a singleton (INSTANCE), so the finalizer never ran.
#11, #13 deprecated-call (Class.newInstance)	Heaplets, JsonValues: getDeclaredConstructor().newInstance(). The InvocationTargetException it adds is unwrapped so that an Error thrown by the constructor still propagates as it did with Class.newInstance(). Otherwise JsonValues.asNewInstance makes the constructor's own exception the cause of a JsonValueException (other reflective failures are caught as ReflectiveOperationException), and Heaplets.getHeaplet logs and ignores it as before. One difference: an ExceptionInInitializerError raised inside the constructor (by touching a class whose static initializer fails) used to be wrapped by asNewInstance's catch (ExceptionInInitializerError …); it now propagates like any other Error, as it already did from Class.forName(name, true, …) for the target class itself and from Heaplets.getHeaplet.
#10 deprecated-call	OpenApiMockResponseHandler: responses.get(ApiResponses.DEFAULT).
#50 uncaught-number-format-exception	Principal: the org.openidentityplatform.openig.websocket.ttl property is parsed once at class load by ttlMillis(String); an invalid value logs a warning and falls back to 180 s instead of throwing on every websocket message. authorize() checks the interval through expired(now). Semantic change: the property used to be re-read on every authorize() call, i.e. it could be changed at runtime with System.setProperty. I assumed that is not a supported use — say so if it is.
#46, #47 uncaught-number-format-exception	MockDataGenerator: latitude/longitude generated with FAKER.number().randomDouble(6, …) instead of parsing a locale-formatted string.

openig-doc — SampleApplication

Alert	Change
#14, #15 deprecated-call	validateConfiguration(true) + createSSLContext() → createSSLContext(true), catching SSLContextConfigurator.GenericStoreException, which grizzly-framework 3.0.1 (the version openig-doc resolves, through opendj-grizzly) throws when the bundled key store cannot be loaded. The configurator is created with SSLContextConfigurator(false), so it no longer reads the javax.net.ssl.* system properties: the sample sets its own key store and passwords, and createSSLContext(true) only logs a missing key store provider, which would have left the HTTPS listener without a key. Exercised by the openig-doc integration tests, which start the sample with HTTPS.
#51, #52 uncaught-number-format-exception	A non-numeric port argument prints Port numbers must be integers: … plus the usage and exits, instead of a stack trace.

Tests

• OAuth2SessionTest: shouldStateAuthorizedWithNonNumericExpiresInFailWithOAuth2Error (failed before with NumberFormatException: For input string: "soon"; asserts the server_error code) and shouldStateAuthorizedWithUndecodableIdTokenFailWithReadableMessage (failed before with ID tokencould).
• ICAPFilterTest (new): configured and default (5000 / 15000 ms) timeouts are applied; a non-numeric connect_timeout / read_timeout (data provider) fails with a JsonValueException naming the field — failed before with a bare NumberFormatException.
• PrincipalTest (new): ttlMillis — seconds to millis, trimming, and the fallback for null / empty / non-numeric values; shouldReauthorizeWithTtlReadAtClassLoad and shouldAuthorizeFromCacheWithTtlReadAtClassLoad pin that the interval is read once at class load, in expired() and through authorize(); shouldExpireWithoutCachedResult pins that a missing result is always refreshed.
• JsonValuesTest, HeapletsTest: an Error thrown by the constructor propagates, including an ExceptionInInitializerError raised in it (asNewInstance); an exception is wrapped in a JsonValueException (asNewInstance) or ignored (getHeaplet).
• MockDataGeneratorTest.generate_returnsCoordinatesInRange, OpenApiMockResponseHandlerTest.bestResponseSchema_fallsBackToDefaultResponse.

Each test added in the review rounds was run against the mutant it targets (per-call TTL read in expired() and in authorize(), res==null dropped from expired(), Error wrapped or swallowed, ExceptionInInitializerError wrapped again, E_INVALID_REQUEST, swapped ICAP defaults, latitude in ±180, return null for default): all ten fail.

Verified, rebased on current master: openig-core 1029 tests, openig-oauth2 160 and openig-doc 11 (integration tests, testGetLoginPageHttps included) under mvn verify, javadoc with failOnWarnings included.

#173, which also touches Heaplets.java (the warn line, left untouched here) and HeapletsTest.java, has been merged; the branch is rebased onto it, and HeapletsTest keeps the tests of both PRs.

[maximthomas]

praise: The NumberFormatException paths now fail with errors that say what is wrong and reach the right handler.

• ICAPFilter.Heaplet.timeout(name, default) reports the bad field's JSON pointer, and ICAPFilterTest's non-numeric data-provider case pins it.
• A non-numeric expires_in now raises OAuth2ErrorException from OAuth2Session.stateAuthorized, which its only caller routes to handleOAuth2ErrorException (OAuth2ClientFilter.java:541-575).

---

issue (non-blocking): PrincipalTest pins only ttlMillis(); nothing fails if authorize() goes back to parsing the property on every call.

openig-core/src/main/java/org/openidentityplatform/openig/websocket/Principal.java:51, :65-66, openig-core/src/test/java/org/openidentityplatform/openig/websocket/PrincipalTest.java:26-37

Both tests call Principal.ttlMillis(String) with literals. authorize() is reached only from websocket/Filter.java:77 and ServerEndPoint.java:51, and no test calls either. So replacing final long ttl=TTL_MILLIS; at :66 with 0, or with the base's Long.parseLong(System.getProperty(...,"180"))*1000 (the NumberFormatException of alert #50), keeps both tests green. PrincipalTest is the only test for #50. authorize() gets its handler from the static HttpFrameworkServlet.getGatewayHttpFrameworkServlet(), so the cheap seam is the interval check. The test below also pins the read-once behaviour that the description calls out.

// Principal.java — both checks in authorize() become expired(System.currentTimeMillis())
boolean expired(long now) {
    return res==null || now-lastAuthorize>=TTL_MILLIS;
}

// PrincipalTest.java (import org.forgerock.http.protocol.Status)
@Test
public void shouldReauthorizeWithTtlReadAtClassLoad() {
    Principal principal = new Principal(null, null);
    principal.res = Status.OK;
    principal.lastAuthorize = 1_000L;
    long ttl = Principal.ttlMillis(System.getProperty(Principal.TTL_PROPERTY));
    System.setProperty(Principal.TTL_PROPERTY, "0");
    try {
        assertThat(principal.expired(1_000L + ttl - 1)).isFalse();
        assertThat(principal.expired(1_000L + ttl)).isTrue();
    } finally {
        System.clearProperty(Principal.TTL_PROPERTY);
    }
}

Pin: this kills ttl = 0 and the per-call read at :66. The ttlMillis(null) mutant at :51 would need a forked JVM with the property set.

---

question (non-blocking): Should an Error thrown by the target constructor still propagate from JsonValues.asNewInstance as before, rather than arrive as the cause of a JsonValueException?

openig-core/src/main/java/org/forgerock/openig/util/JsonValues.java:155-161

Class.newInstance() rethrew the constructor's own throwable. getDeclaredConstructor().newInstance() wraps it in InvocationTargetException, and :158 makes the target the cause of a JsonValueException. A constructor that throws IllegalStateException, NoClassDefFoundError or OutOfMemoryError now reaches the caller as a JsonValueException. Nothing in the repo calls asNewInstance, so only code outside it can notice. Wrapping exceptions adds the JSON pointer, which helps. Wrapping an Error turns a JVM failure into a configuration error. Minor either way. If wrapping Errors is not intended:

} catch (InvocationTargetException e) {
    final Throwable target = e.getTargetException();
    if (target instanceof Error) {
        throw (Error) target;
    }
    throw new JsonValueException(value, target);
}

---

suggestion (non-blocking): shouldStateAuthorizedWithNonNumericExpiresInFailWithOAuth2Error does not assert the server_error code.

openig-oauth2/src/test/java/org/forgerock/openig/filter/oauth2/client/OAuth2SessionTest.java:182-192

The test is red at the base (a raw NumberFormatException), so the fix is pinned. But "expires_in" matches the description, not the error code. Passing E_INVALID_REQUEST instead of E_SERVER_ERROR keeps the test green, and the description gives consistency with the sibling server_error branch as the reason for the change.

assertThatThrownBy(() -> OAuth2Session.stateNew(time).stateAuthorized(accessTokenResponse))
        .isInstanceOfSatisfying(OAuth2ErrorException.class, e ->
                assertThat(e.getOAuth2Error().getError()).isEqualTo(OAuth2Error.E_SERVER_ERROR))
        .hasMessageContaining("expires_in");

---

suggestion (non-blocking): No test pins the ICAPFilter timeout defaults (5000 / 15000 ms).

openig-core/src/main/java/org/openidentityplatform/openig/filter/ICAPFilter.java:77-78, openig-core/src/test/java/org/openidentityplatform/openig/filter/ICAPFilterTest.java:35-45

shouldConfigureTimeouts sets both fields, and it passes at the base too. The non-numeric case checks only the exception. Swapping the defaults in timeout("connect_timeout", "5000") / timeout("read_timeout", "15000") keeps every test green.

@Test
public void shouldApplyDefaultTimeouts() throws Exception {
    ICAPFilter filter = (ICAPFilter) new ICAPFilter.Heaplet().create(
            Name.of("icap"),
            json(object(field("server", "icap://localhost:1344"))),
            HeapUtilsTest.buildDefaultHeap());

    assertThat(filter.icap.getConnectTimeout()).isEqualTo(5000);
    assertThat(filter.icap.getReadTimeout()).isEqualTo(15000);
}

---

suggestion (non-blocking): The latitude/longitude branches of MockDataGenerator have no test.

openig-core/src/main/java/org/forgerock/openig/handler/MockDataGenerator.java:294-297

No test or test resource in any module uses a latitude or longitude field name. A mutant that uses randomDouble(6, -180, 180) for latitude, or returns a String, passes every test.

@Test
public void generate_returnsCoordinatesInRange() {
    Schema<Object> schema = new Schema<>();
    schema.setType("number");
    for (int i = 0; i < 100; i++) {
        final Object latitude = MockDataGenerator.generate("latitude", schema);
        final Object longitude = MockDataGenerator.generate("longitude", schema);
        assertThat(latitude).isInstanceOf(Number.class);
        assertThat(((Number) latitude).doubleValue()).isBetween(-90.0, 90.0);
        assertThat(((Number) longitude).doubleValue()).isBetween(-180.0, 180.0);
    }
}

---

suggestion (non-blocking): No test exercises the default response fallback of bestResponseSchema.

openig-core/src/main/java/org/forgerock/openig/handler/OpenApiMockResponseHandler.java:268-269

responses.get(ApiResponses.DEFAULT) does the same lookup as getDefault(). The bestResponseSchema tests build only 2xx responses, though, so return null; at :269 passes them all.

@Test
public void bestResponseSchema_fallsBackToDefaultResponse() {
    final Operation op = buildOperation("default");
    assertThat(OpenApiMockResponseHandler.bestResponseSchema(op).getDescription())
            .isEqualTo("schema-default");
}

[vharseko]

Addressed in c700163. The branch is rebased onto current master first; the only conflict was the ttl line in Principal.java, which #177 had also touched.

issue — PrincipalTest pins only ttlMillis(): done as suggested. authorize() now checks expired(System.currentTimeMillis()) both times, and shouldReauthorizeWithTtlReadAtClassLoad sets the property to 0 after class load. Two small deviations: the test compares against Principal.TTL_MILLIS (now package-private, like TTL_PROPERTY) instead of recomputing it with ttlMillis(System.getProperty(...)), and it restores a previous property value instead of clearing it.

question — Error from JsonValues.asNewInstance: not intended. The Error is rethrown now (your snippet), which also matches the javadoc's "as if by a new expression"; exceptions stay wrapped with the JSON pointer. The same regression was in Heaplets.getHeaplet: once the Error arrives wrapped in InvocationTargetException, its catch (Exception e) logged and swallowed it, so a NoClassDefFoundError in a heaplet constructor became "no heaplet found". A small instantiate helper unwraps it there; the warn line is left untouched because #173 rewrites it. JsonValuesTest and HeapletsTest pin both sides: an Error propagates, an exception is wrapped (asNewInstance) or ignored as before (getHeaplet).

suggestions: all four added as proposed:

• OAuth2SessionTest asserts E_SERVER_ERROR for a non-numeric expires_in;
• ICAPFilterTest.shouldApplyDefaultTimeouts;
• MockDataGeneratorTest.generate_returnsCoordinatesInRange;
• OpenApiMockResponseHandlerTest.bestResponseSchema_fallsBackToDefaultResponse.

Each new check was run against its mutant (per-call TTL read, Error wrapped in asNewInstance, Error swallowed in getHeaplet, E_INVALID_REQUEST, swapped ICAP defaults, latitude in ±180, return null for default): all seven fail, and all pass on the branch. openig-core (1017 tests) and openig-oauth2 (160) pass under verify, javadoc included.

[maximthomas]

praise: the round-1 points are addressed where the behaviour lives, and the new error paths name what failed.

• ICAPFilter.timeout builds the JsonValueException from config.get(name).defaultTo(...), the JSON value before evaluation, so the message carries /connect_timeout or /read_timeout. ICAPFilterTest can only match the field name through that pointer.
• JsonValues.asNewInstance now rethrows an Error from the constructor and wraps exceptions, as its Javadoc describes ("as if by a new expression").
• responses.get(ApiResponses.DEFAULT) is exactly what getDefault() did in swagger-models 2.2.37 and 2.2.45 (get("default")).

---

issue (non-blocking): PrincipalTest still never calls authorize(), so it does not pin that authorize() uses the interval read at class load.

openig-core/src/test/java/org/openidentityplatform/openig/websocket/PrincipalTest.java:41-56, openig-core/src/main/java/org/openidentityplatform/openig/websocket/Principal.java:66-80

shouldReauthorizeWithTtlReadAtClassLoad only asserts expired(...), and no test in the repo calls authorize(). I reverted authorize() to the BASE per-call Long.parseLong(System.getProperty(TTL_PROPERTY, "180"))*1000 and left expired()/TTL_MILLIS untouched. With that mutant all 3 PrincipalTest methods still pass (JDK 26). The mutant also brings back alert #50's per-message NumberFormatException for an invalid property value. Dropping res==null || from expired() survives too, because the only expired() test sets res = Status.OK first.

    @Test
    public void shouldAuthorizeFromCacheWithTtlReadAtClassLoad() throws Exception {
        final Principal principal = new Principal(null, null);
        principal.res = Status.OK;
        principal.lastAuthorize = System.currentTimeMillis();
        // A per-call read of "0" would re-authorize through the gateway servlet instead of the cache
        final String previous = System.setProperty(Principal.TTL_PROPERTY, "0");
        try {
            assertThat(principal.authorize()).isSameAs(Status.OK);
        } finally {
            if (previous == null) {
                System.clearProperty(Principal.TTL_PROPERTY);
            } else {
                System.setProperty(Principal.TTL_PROPERTY, previous);
            }
        }
    }

    @Test
    public void shouldExpireWithoutCachedResult() {
        assertThat(new Principal(null, null).expired(1_000L)).isTrue();
    }

Pin: the first test fails under the per-call mutant, which leaves the cache and goes to HttpFrameworkServlet.getGatewayHttpFrameworkServlet(). The second test kills the res==null drop. Neither test covers a mutant that ignores the TTL entirely, because that needs a gateway-servlet stand-in. This review did not compile or run the snippet.

---

issue (non-blocking): with -Djavax.net.ssl.keyStoreProvider naming a missing provider, SampleApplication now starts the HTTPS listener with no server key, where it used to skip HTTPS.

openig-doc/src/main/java/org/forgerock/openig/doc/SampleApplication.java:235, :248-256

createSSLContext(true) only logs a key-store NoSuchProviderException, in both grizzly-framework 3.0.1 and 4.0.2. The key manager stays null and SSLContext.init(null, null, null) succeeds. The removed validateConfiguration(true) returned false for this configuration, so BASE logged "Failed to build a valid HTTPS configuration." and added no listener. A probe against both jars confirmed this; the TLS handshake failure that follows was not run. The description says GenericStoreException is "what Grizzly 4.0.2 throws", but openig-doc resolves grizzly-framework 3.0.1, and neither version throws for this input. validateConfiguration(boolean) is deprecated in both versions, so putting it back would bring the alert back.

        SSLContextConfigurator sslContextConfigurator = new SSLContextConfigurator(false);

Or: keep the head and correct the "Grizzly 4.0.2" sentence. SSLContextConfigurator(boolean) is public and not deprecated in 3.0.1 or 4.0.2. It reads no javax.net.ssl.* system property, so a bogus provider never gets in. Whether the sample relies on any other javax.net.ssl.* property was not checked.

---

suggestion (non-blocking): no test pins that an ExceptionInInitializerError raised inside the constructor now escapes asNewInstance raw.

openig-core/src/main/java/org/forgerock/openig/util/JsonValues.java:157-163

At BASE, Class.newInstance() rethrew such an error and catch (ExceptionInInitializerError | ...) wrapped it. The head rethrows it, in line with the "Error propagates" rule and with Class.forName(name, true, ...) at :102, which already lets a target-class static-init error out raw. The only Error test throws NoClassDefFoundError, which BASE also let out raw, so wrapping this error again would fail no test. A probe on JDK 8 through 26 showed BASE wrapped and head raw.

    @Test(expectedExceptions = ExceptionInInitializerError.class)
    public void shouldAsNewInstancePropagateInitializerErrorRaisedByConstructor() throws Exception {
        asNewInstance(json(ConstructorTouchingBrokenClass.class.getName()), Object.class);
    }

    public static class ConstructorTouchingBrokenClass {
        public ConstructorTouchingBrokenClass() {
            BrokenStaticInitializer.touch();
        }
    }

    static class BrokenStaticInitializer {
        static final int VALUE = Integer.parseInt("boom");

        static int touch() {
            return VALUE;
        }
    }

Pin: the test fails if the Error branch wraps ExceptionInInitializerError again. This review did not compile or run the snippet.

---

nitpick (non-blocking): ICAPFilter.timeout reports "Expecting a number of milliseconds" for a number that does not fit in an int.

openig-core/src/main/java/org/openidentityplatform/openig/filter/ICAPFilter.java:87-96

connect_timeout: "99999999999" fails with "/connect_timeout: Expecting a number of milliseconds, got: 99999999999". The Javadoc also says "when the value is not a number".

		/** Reads a timeout (in milliseconds) from the config, naming the field when the value is not an int. */
		...
				throw new JsonValueException(value, "Expecting a number of milliseconds that fits in an int, got: "+evaluated, e);

[vharseko]

Addressed in d5133ba. The branch is rebased onto current master first; the only conflict was the end of HeapletsTest, where #173 (merged since) also appended tests — both sets are kept.

issue — PrincipalTest never calls authorize(): both tests added as proposed, shouldAuthorizeFromCacheWithTtlReadAtClassLoad and shouldExpireWithoutCachedResult. Run against your mutants: with authorize() reverted to the per-call Long.parseLong(System.getProperty(TTL_PROPERTY, "180"))*1000, the first one fails with the NPE from getGatewayHttpFrameworkServlet(); with res==null || dropped from expired(), the second one fails.

issue — bogus javax.net.ssl.keyStoreProvider in SampleApplication: confirmed. openig-doc resolves grizzly-framework 3.0.1 (through opendj-grizzly), and its createSSLContext(boolean) only logs the key-store NoSuchProviderException, unlike every other branch there. Took your first option, new SSLContextConfigurator(false). The sample sets the key store bytes and both passwords itself; without the system properties the key store type falls back to KeyStore.getDefaultType() as before, the protocol to TLS, and no trust store is needed since client auth is off. Nothing in openig-doc sets a javax.net.ssl.* property. The description now names 3.0.1 instead of 4.0.2, and says that the system properties are no longer read.

suggestion — ExceptionInInitializerError raised by the constructor: test added as proposed, with a note on BrokenStaticInitializer that only its first access raises the ExceptionInInitializerError. It fails when the Error branch wraps ExceptionInInitializerError again. The description said an Error propagates "as it did with Class.newInstance()", which is not true for this one; it now names the difference.

nitpick — ICAPFilter.timeout message: message and Javadoc now say "fits in an int".

openig-core (1029 tests), openig-oauth2 (160) and openig-doc (11 integration tests, testGetLoginPageHttps included) pass under verify, javadoc included.

[maximthomas]

praise: The round-3 tests each fail when their fix is reverted.

• PrincipalTest.shouldAuthorizeFromCacheWithTtlReadAtClassLoad turns red when Principal.authorize() goes back to reading org.openidentityplatform.openig.websocket.ttl on every call, and shouldExpireWithoutCachedResult turns red without res==null in expired(long). Both are green at the head.
• JsonValuesTest.shouldAsNewInstancePropagateInitializerErrorRaisedByConstructor turns red when asNewInstance wraps the ExceptionInInitializerError in a JsonValueException again. It is green at the head.
• SampleApplication.createSslConfiguration now builds new SSLContextConfigurator(false). A deployer's javax.net.ssl.* properties can no longer start the sample's HTTPS listener without a key.