Repository navigation
Add missing @Override annotations and make nested helper classes static - #180
Merged
vharseko merged 1 commit intoOct 15, 2026
Merged
Conversation
Cosmetic CodeQL notes: java/missing-override-annotation in CookieFilter, AdminHttpApplication, StreamPatternMatches and Resolvers; java/non-static-nested-class for HeapImpl.DecorationContext and HeadlessAuthenticationFilter.SsoTokenHolder; java/unused-parameter for the context argument of OAuth2Utils.sessionKey().
vharseko
added a commit
that referenced
this pull request
Oct 23, 2026
Closes code scanning alerts #36–#39 (`java/dereferenced-value-may-be-null`, `ServerEndPoint`), #34 (`java/input-resource-leak`, `SeparatedValuesFile`) and, in a follow-up commit, #23 / #24 (`java/missing-override-annotation`, the two `onMessage` handlers in `ServerEndPoint`). ### `ServerEndPoint` — a real bug behind the warnings `echoTextMessage` / `echoBinaryMessage` wait up to ~5 s for `session_upstream` and then dereference it without a check, relying on the `NullPointerException` to reach the `catch (Throwable)` that closes the client. Writing a test for that path showed the close never happens: `jakarta.websocket.CloseReason` throws `IllegalArgumentException` for reason phrases longer than 123 UTF-8 bytes (checked in `jakarta.websocket-api-2.0.0`), and the helpful NPE message of Java 14+ (`Cannot invoke "jakarta.websocket.Session.getBasicRemote()" because "this.session_upstream" is null`) is longer than that. `new CloseReason(...)` is evaluated as the argument of `close()`, the exception is swallowed by the inner `catch (Throwable e1) {}`, and **the client connection is left open** with the message silently dropped. The same applies to any exception with a long message in all seven `new CloseReason(..., "…: " + e)` sites in the class. This holds wherever `CloseReason` enforces the limit: the Eclipse `jakarta.websocket-api` (the unit-test classpath), Jetty 11+, Tyrus. Tomcat's own `CloseReason` has no length check and `WsSession` truncates the phrase itself, so on the `openig-docker` image (`tomcat:11-jre25`) the client was already closed and only the reason phrase is affected — except for a phrase over 123 bytes with supplementary characters that is short in UTF-16 units, where `WsSession.appendCloseReasonWithTruncation` throws `StringIndexOutOfBoundsException` and no close frame goes out. Changes: - `upstream()`: the wait loop moved into a method that reads the volatile field once per iteration and fails with `IOException("upstream not connected")` when the upstream is still missing/closed. - `closeReason(code, phrase)`: a phrase over the 123-byte limit is cut to 120 bytes and ends with `…` (U+2026), the marker Tomcat's `WsSession` puts on its own cut, so a truncated reason reads the same on every container. The cut never splits a multi-byte character or a surrogate pair. Every `CloseReason` in the class is now built through it. - `closeUpstream(reason)` / `closeAbnormally(client, phrase)`: null-safe close and reset of the upstream, shared by both `@OnMessage` handlers, `@OnClose` and `@OnError`. The upstream-side `MessageHandler`s still close their own `onOpen` session parameter directly: it is never null, and the field is assigned only after `connectToServer()` returns, while the handlers are registered in `onOpen`. - `@OnMessage` handlers call `authorize()` before waiting for the upstream: a revoked client is closed with `access revoked`, as before, and no longer waits ~5 s for a missing upstream first. - `@OnError` closes the upstream before the client. On Tomcat `close()` runs `@OnClose` on the calling thread, so `end()` used to close the upstream with `NORMAL_CLOSURE` first and the `client error: …` reason never reached it. Now the upstream gets `CLOSED_ABNORMALLY` (sent by Tomcat as 1002), and its `onClose` closes the client with the same reason. - `session_upstream` is `volatile` (written in `@OnOpen`, read from `@OnMessage` on other threads). Tests (`ServerEndPointTest`, new): - text and binary message without an upstream → client closed with `CLOSED_ABNORMALLY` / `"upstream not connected"`. Before the fix both failed with `Wanted but not invoked: session.close(...)` — zero interactions with the client session. - `shouldCloseStaleUpstreamWhenItIsNotOpen`: an upstream that is set but no longer open → client closed with `"upstream not connected"`, the stale session closed and the field cleared. - `shouldTruncateLongUpstreamErrorWhenClosingClient`: the upstream fails with a 200-char `IOException` → the client close phrase is cut to 123 bytes ending in `…`, and the same `CloseReason` closes the upstream (pins the `closeAbnormally()` → `closeReason()` routing; no wait). - `shouldCloseRevokedClientWithoutWaitingForUpstreamOnTextMessage` / `…OnBinaryMessage`: `authorize()` returns 403 and there is no upstream → client closed with exactly `"client message error: access revoked"` (no wait). - `shouldCloseUpstreamWithClientErrorOnError`: `client.close()` runs `end()` as Tomcat does → the upstream is closed once, with `CLOSED_ABNORMALLY` / `"client error: java.io.IOException: broken pipe"`. - `shouldTruncateCloseReasonPhraseToProtocolLimit`: ASCII 123 untouched, 500 → 120 + `…`; Cyrillic 200 → 60 + `…` (123 bytes); `"x"` + U+1F600 ×100 → `"x"` + U+1F600 ×29 + `…` and `"a"` ×119 + U+1F600 ×5 → `"a"` ×119 + `…` (no lone high surrogate at the cut); a lone `\uD83D` and `""` → `""`; short phrases untouched. The three tests that go through the wait (no upstream ×2, stale upstream) each take the real ~5 s (about +18 s on the `openig-core` run); I preferred that over adding a test-only hook to shorten the timeout in production code. ### `ServerEndPoint` — #23 / #24 `@Override` added to `onMessage` of the two anonymous `MessageHandler.Whole` classes (second commit, `1a122ed1`). Folded in here rather than into #180 to keep the file in a single open PR. ### `SeparatedValuesFile` (#34) `SeparatedValuesReader` now `implements Closeable` (it already had `close()`), and `getRecord()` opens it in try-with-resources instead of `try/finally`, with the underlying `FileInputStream` as a resource of its own so that it is closed even if the `InputStreamReader` constructor throws. Behavior-preserving; covered by `SeparatedValuesFileTest` (new), which reads records from a real CSV file. `FileAttributesFilterTest` mocks `SeparatedValuesFile` and does not run this code.
vharseko
added a commit
that referenced
this pull request
Oct 23, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the cosmetic
note-level code scanning alerts #16–#22, #27, #28 and #41. No behavior change.java/missing-override-annotationCookieFilter.java@OverrideonCookiePolicy.shouldAcceptjava/missing-override-annotationAdminHttpApplication.java@OverrideongetApiProducerjava/missing-override-annotationStreamPatternMatches.java@Overrideonclosejava/missing-override-annotationResolvers.java@Overrideoniterator/hasNext/next/removejava/non-static-nested-classHeapImpl.javaDecorationContext→staticjava/non-static-nested-classHeadlessAuthenticationFilter.javaSsoTokenHolder→staticjava/unused-parameterOAuth2Utils.javasessionKey(context, uri)→sessionKey(uri)(package-private static, three in-class callers updated)Deferred: #23 / #24 (
ServerEndPoint, two more@Override) until #176 lands, to keep that file out of two open PRs.Verified:
openig-core1002 tests,openig-oauth2158,openig-openam118, javadoc withfailOnWarnings.