refactor: own the HTTP transport lifecycle in the notification path#391
Merged
Conversation
Every build-status notification leaked transport: the Bearer and Basic consumers built a CloseableHttpClient per call and never closed it (an orphaned connection manager with its sockets each time), the returned raw HttpResponse left entity release to the callers (the server visitor's Bearer branch never consumed it and logged the entity stream object), and the OAuth2 consumer never closed its scribejava service or response. The consumers now fully own the lifecycle and return a dead value, the BitBucketPPRApiResponse record (status code and body). Bearer and Basic wrap the client in try-with-resources and execute through a ResponseHandler, the form that consumes the entity and releases the connection before returning; both build the client with HttpClients.createSystem(), one idiom now that the Basic consumer no longer needs a credentials provider. The OAuth2 consumer closes service and response with try-with-resources. The visitors just log statusCode and body of the returned value; the server Bearer branch now logs the actual body instead of the stream's toString. Existing behavior is otherwise unchanged: same verbs, same throws contract, same catch clauses. The Basic parity test moves to the new contract and also pins the returned body.
… point The client layer modeled a 2x2 matrix (Bitbucket flavor x credential type) with five types: a client interface with accept(), two client wrappers, a visitor interface and two visitors doing instanceof dispatch. It was a strategy spelled as a pseudo-visitor: no element hierarchy, no double dispatch, one operation, and cross-cutting policy (the non-https warning) duplicated across both visitors. BitBucketPPRClient is now a concrete class holding flavor and context, dispatching once with a pattern-matching switch on the credential type: username/password goes to Basic auth on both flavors, a Secret-text credential to OAuth2 on Cloud and Bearer on Server. The factory reduces to a constructor call, kept so the two call sites in BitBucketPPRHandlerTemplate stay untouched. warnIfNotHttps is called in one place instead of two. Deleted: BitBucketPPRServerClient, BitBucketPPRCloudClient, BitBucketPPRClientVisitor, BitBucketPPRClientServerVisitor, BitBucketPPRClientCloudVisitor. Behavior is preserved: same consumer per flavor/credential combination, null or unknown credentials still raise NotImplementedException, same catch clauses and log levels; the result log now comes from the BitBucketPPRClient logger instead of the per-visitor loggers.
Consolidates the Basic and Bearer consumers: their send() bodies were verbatim identical except for the Authorization header value, so request building and client lifecycle move to a shared package-private BitBucketPPRApiRequest helper, with RequestBuilder collapsing the duplicated POST/DELETE branches. New tests pin the DELETE path (used in production to revoke an approval when a build fails, previously untested): both consumers must carry the Authorization and X-Atlassian-Token headers on DELETE, and unsupported verbs still raise NoSuchMethodException. A rejected notification is no longer invisible: HTTP 400 or higher now logs a warning with status and response body via BitBucketPPRUtils.warnOnHttpError, deduplicated per origin and status code with the same anti-flooding rule as warnIfNotHttps. Transport failures are logged with the full exception instead of only its message, both in BitBucketPPRClient and in callClient. BitBucketPPRClientFactory, reduced to a pass-through by the topology collapse, is inlined at its two call sites and deleted. The BitBucketPPRClient constructor now rejects a null flavor with requireNonNull: type == CLOUD would otherwise silently route a Secret-text credential to the Bearer consumer. The test helper nonHttpsWarnings generalizes to warningsContaining to serve both warning kinds.
Addresses Christian's review of the branch. The topology collapse had removed five public types and turned the BitBucketPPRClient interface into a class: a binary break (potential IncompatibleClassChangeError / NoClassDefFoundError for bytecode compiled against 4.0.0) that does not belong in a 4.0.1. The interface is restored as it was, the dispatch implementation is now DefaultBitBucketPPRClient, the factory returns the interface again and is back in use at the two call sites, and the visitor interface, the two visitor implementations and the two client wrappers are restored as functional @deprecated types, scheduled for removal in the next major release. accept() becomes a deprecated default no-op on the interface: the dispatch is internal now. warnOnHttpError is reworked from a permanent once-per-origin-and- status set to per-episode state: the first non-successful status per origin warns, a repeat of the same status stays silent, a status change warns again, and a 2xx clears the state, so an incident after a recovery is not suppressed forever. Success is now strictly 2xx: a 3xx (wrong base URL, redirecting proxy that may not preserve method and credentials) warns too. The warning carries only origin and status; the body, stripped of control characters (no forged multi-line log entries) and truncated, moves to FINE. The response body read is capped at 16 KiB in BitBucketPPRApiResponse.from, which now also decodes with the declared charset and falls back to UTF-8 instead of ISO-8859-1. Tests cover the episode semantics including re-arming after a success, redirects, and the body sanitization.
Addresses the second review round on the branch. The consumer classes in client.api are public API too: their send() return type is part of the JVM method descriptor, so changing it (HttpResponse and scribejava Response to BitBucketPPRApiResponse) breaks external bytecode with NoSuchMethodError just as surely as a deleted class. All three consumers are restored with their original signatures and behavior and deprecated; the new contract moves to BitBucketPPRNotificationSender, a @restricted(NoExternalUse) internal class that owns the transport lifecycle (the shared RequestBuilder path absorbs the former package-private helper). The factory is restored exactly as released: it builds the legacy Cloud/Server clients and wires their visitors, so external code using accept() with a custom visitor, or casting to the concrete client types, keeps its behavior instead of being silently ignored. The plugin itself constructs DefaultBitBucketPPRClient directly at the two internal call sites. On the interface, accept() is deprecated but abstract again; the default implementation documents that it ignores it. The 16 KiB body cap now holds on the OAuth2 path too: a from(Response) overload reads at most the cap from the scribejava stream, mirroring the gzip decompression that Response.getBody() would apply. Tests cover the cap on both transports and the gzip case. The production-path TLS, DELETE-header and unsupported-verb tests move to BitBucketPPRNotificationSenderTest; the restored consumer tests keep guarding the legacy contract, including the SECURITY-3856 regression on the deprecated Bearer path. The BEL character in the sanitization test is now a visible unicode escape.
The changelog no longer overpromises: the lifecycle ownership belongs to the plugin's notification path, not to the deprecated consumers that intentionally keep their live-response contract, and the preserved legacy surface is described as keeping its method signatures and dispatch semantics rather than its full behavior (the TLS and credentials-provider fixes of this release apply to it too). The scribejava materializer handles a Response built directly from a String body (getStream() is null there): the same 16 KiB budget is applied to the string. The log sanitization covers Unicode line and paragraph separators besides ASCII control characters, matching what the comment promises. A new test freezes the legacy compatibility contract: the factory keeps returning the legacy concrete types and a custom visitor installed through accept() keeps being invoked by send(), so a future refactoring cannot silently reintroduce the no-op. InterruptedException from the OAuth2 path now restores the interrupt flag before reaching the broad handler upstream. The invisible U+2028 in the sanitization test is a visible unicode escape.
…olish The episode state of the non-success warning is keyed on the origin alone (the URL carries no job or credential identity): on a Bitbucket host shared by many jobs, any job's success re-arms a broken job's episode (which then warns roughly once per build, a tolerated signal for a job that really fails every build) and concurrent same-status incidents collapse into one episode. Documented as a known limitation in the code comment and the changelog; a per-origin cooldown remains a possible future refinement. The interrupt-flag restoration moves next to the blocking OAuth2 calls in the sender, so every caller inherits the invariant; the catch in DefaultBitBucketPPRClient stays as a harmless second line. The stale javadoc in the two restored consumer tests now points at the dispatch layer instead of the visitor layer. TlsTestSupport caches the two self-signed keystores per JVM (localhostKeystore and mismatchedHostKeystore): keytool spawns a subprocess per generation and every consumer needs the identical certificate, so the suite drops from eight spawns to two with no coverage change.
The scribejava materializer declared the stream wrapper as a single try-with-resources resource with the GZIPInputStream construction inline: that constructor reads the gzip header eagerly and throws on a malformed body, and in that case the resource is never initialized and the raw stream never closed. Response.close() would not close it either, since scribejava tracks the stream separately from its closeables. The raw stream is now its own resource ahead of the wrapper, so Java closes it when initializing the second resource fails. A test with a close-tracking stream and invalid gzip data pins the behavior.
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
1 participant
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.
What changed
DefaultBitBucketPPRClient(a pattern-matching switch on the credential type: username/password to Basic on both flavors, Secret text to OAuth2 on Cloud and Bearer on Server), backed by the internal@RestrictedBitBucketPPRNotificationSenderthat owns the shared request building, so POST and DELETE cannot drift apart.BitBucketPPRClientinterface, the factory, the visitors, the client wrappers and the per-credential consumers) is fully preserved with its original method signatures and dispatch semantics, deprecated and scheduled for removal in the next major release. A dedicated test freezes the legacy contract: the factory keeps returning the legacy concrete types and a custom visitor installed through accept() keeps being invoked.Nine commits, iteratively reviewed (maintainer review rounds plus multi-agent code reviews); full suite green with 339 tests on JDK 25.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TCtAXyF7RSWNxpHpDjDAhK