feat: oauth refresh conn ownership - #409
tamassoltesz wants to merge 3 commits into
Conversation
…rage Implements the transaction-aware twin of isOAuthTokenRevokedByGID contributed by plugin-interface (PI-1). The revocation existence check runs on the caller's connection via QueryExecutorTemplate.execute(con, ...) rather than borrowing a new pooled connection, so the non-rotating OAuth refresh exchange holds a single connection for its whole DB lifetime. Part of PLAN-017. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Non-tx overload now delegates to the connection-scoped twin so the count query and result mapping live in one place; trim the unreleased changelog entry to a single line.
…d-gid-transaction feat: add isOAuthTokenRevokedByGID_Transaction (single-connection revocation read)
tamassoltesz
added
agent-review
![supertokens-agent-runner[bot]](https://avatars.githubusercontent.com/u/50478857?s=60&v=4){kind=small}
There was a problem hiding this comment.
Umbrella PR for feat/oauth-refresh-conn-ownership → 9.9. The diff is the already human-approved PR #408 (isOAuthTokenRevokedByGID_Transaction) rolled up cleanly, plus its changelog trim and query dedupe — nothing new landed on top, so no re-litigation of settled points.
Re-verified against the surrounding code: the tx overload threads the caller's TransactionConnection via the repo-standard (Connection) con.getConnection() cast (used 111× elsewhere) and does not close it, while the non-tx overload borrows and closes its own pooled connection via try-with-resources — so the connection-ownership fix does what it advertises and the delegation direction (connection-scoped overload holds the single query/mapping copy) resolves the earlier duplication note. @Override on both methods compiles against the paired plugin-interface branch, confirming signature parity with the interface contract. Semver-wise this is an additive implementation of a newly-added abstract method, coordinated with the PI/core bump; changelog entry is present. LGTM.
One non-blocking observation below on storage-layer test coverage.
| // caller's connection instead of borrowing a new one from the pool. This overload holds the | ||
| // single copy of the query and result mapping; the non-tx overload above delegates to it with a | ||
| // pooled connection. | ||
| public static boolean isOAuthSessionExistsByGID(Start start, Connection con, AppIdentifier appIdentifier, String gid) |
There was a problem hiding this comment.
Non-blocking: there is no postgres-storage-level test exercising this transaction twin directly (e.g. asserting it reads within the caller's open transaction and does not acquire a second pool connection). The revocation-ownership behavior is covered at the core layer — CORE-2's pool-size-1 single-connection test is what surfaced the exchange-path leak — so this is defensible for a thin storage twin, but a small in-plugin test would guard against a future regression where someone re-points this at the pooled overload. Reviewer's call given the core coverage.
There was a problem hiding this comment.
Agreed this is defensible for a thin storage twin, and I'm deliberately not bolting a plugin-level test onto this integration PR — here's the reasoning:
-
Behavior is covered a layer up. The connection-ownership guarantee is exercised deterministically by CORE-2's pool-size-1 single-connection test (#1451), which is precisely what surfaced the original exchange-path leak. That test asserts the revocation read runs on the caller's already-open connection rather than borrowing a second one — i.e. the exact regression this twin exists to prevent.
-
A plugin-level test can't be verified here. The
src/testmodule compiles againstproject(":supertokens-core")andproject(":supertokens-plugin-interface")(build.gradle:41-42). BecauseisOAuthTokenRevokedByGID_Transactionis a newly-added abstract interface method, the module only compiles once the core in-memoryStartalso implements it — that mirror is the downstream CORE-1 change (#1450), which sits after this PR in the trio. So the test class can neither compile/run locally nor via the repo-wide "Run tests" gate (structurally not dispatched on this branch). Adding it now would be an unverifiable addition to an integration PR that otherwise carries only the already-human-approved feat: add isOAuthTokenRevokedByGID_Transaction (single-connection revocation read) #408 diff. -
Once the trio lands (PI 10.1.1 / core 12.3.1 / postgres 9.9.1) the module graph aligns and a small in-plugin test asserting "reads within the caller's transaction, acquires no second pool connection" becomes both writable and CI-runnable — the right home for that regression guard, tracked as a follow-up rather than this PR.
Leaving the umbrella diff unchanged.
|
CI triage for this umbrella PR:
Neither red is introduced by this PR (its diff is the already-human-approved #408 rolled up). Flagging for reviewer visibility rather than chasing. |
1 participant
Summary of change
(A few sentences about this PR)
Related issues
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)
Documentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)
Checklist for important updates
pluginInterfaceSupported.jsonfile has been updated (if needed)build.gradlebuild.gradle, please make sure to add themin
implementationDependencies.json.git tag) in the formatvX.Y.Z, and then find thelatest branch (
git branch --all) whoseX.Yis greater than the latest released tag.OneMillionUsersTestRemaining TODOs for this PR