Skip to content

Commit b79a0eb

Browse files
committed
Refactor SessionHost to be stricter on out-of-context calls
This distinguishes out-of-context calls as dev errors rather than hiding as user problems. This also stops creating ephemeral `BehaviorSubject`s, which could be a source of logic being lost & swallowed.
1 parent 8146290 commit b79a0eb

File tree

6 files changed

+50
-8
lines changed

6 files changed

+50
-8
lines changed

src/common/session.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export class SessionPipe implements PipeTransform {
4545
constructor(private readonly sessionHost: SessionHost) {}
4646

4747
transform() {
48-
return this.sessionHost.current$.value;
48+
return this.sessionHost.currentMaybe;
4949
}
5050
}
5151

src/components/authentication/session.host.ts

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,20 @@ import { Injectable, type OnModuleDestroy } from '@nestjs/common';
22
import { AsyncLocalStorage } from 'async_hooks';
33
import { BehaviorSubject } from 'rxjs';
44
import { type Session } from '~/common';
5+
import { AsyncLocalStorageNoContextException } from '~/core/async-local-storage-no-context.exception';
56
import { NoSessionException } from './no-session.exception';
67

78
/**
89
* A service holding the current session / user
910
*/
1011
export abstract class SessionHost {
12+
/**
13+
* Retrieve the current session.
14+
*
15+
* If this call is not within a {@link withSession} stack, a server error is thrown.
16+
*
17+
* If there is no current session, a {@link NoSessionException} is thrown.
18+
*/
1119
get current() {
1220
const value = this.current$.value;
1321
if (value) {
@@ -16,8 +24,34 @@ export abstract class SessionHost {
1624
throw new NoSessionException();
1725
}
1826

27+
/**
28+
* Retrieve the current session or undefined.
29+
*
30+
* If this call is not within a {@link withSession} stack, a server error is thrown.
31+
*/
32+
get currentMaybe() {
33+
return this.current$.value;
34+
}
35+
36+
/**
37+
* Retrieve the current session subject.
38+
*
39+
* It must exist, meaning that this call is within a {@link withSession} stack.
40+
* This subject could still not (yet) have an actual session value.
41+
*/
1942
abstract get current$(): BehaviorSubject<Session | undefined>;
2043

44+
/**
45+
* Retrieve the current session or undefined.
46+
*
47+
* This is allowed to be called outside a {@link withSession} stack
48+
* and will just return undefined.
49+
*/
50+
abstract get currentIfInCtx(): Session | undefined;
51+
52+
/**
53+
* Run a function with a given session.
54+
*/
2155
abstract withSession<R>(
2256
session: BehaviorSubject<Session | undefined> | Session | undefined,
2357
fn: () => R,
@@ -30,10 +64,18 @@ export class SessionHostImpl extends SessionHost implements OnModuleDestroy {
3064
BehaviorSubject<Session | undefined>
3165
>();
3266

67+
get currentIfInCtx() {
68+
return this.als.getStore()?.value;
69+
}
70+
3371
get current$() {
34-
return (
35-
this.als.getStore() ?? new BehaviorSubject<Session | undefined>(undefined)
36-
);
72+
const subject = this.als.getStore();
73+
if (!subject) {
74+
throw new AsyncLocalStorageNoContextException(
75+
'A session context has not been declared',
76+
);
77+
}
78+
return subject;
3779
}
3880

3981
withSession<R>(

src/core/data-loader/data-loader.config.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ export class DataLoaderConfig {
2727
// If we have a session, use that as the cache key.
2828
// It will always be created / scoped within the GQL operation.
2929
// This ensures the cached data isn't shared between users.
30-
const session = this.sessionHost.current$.value;
30+
const session = this.sessionHost.currentMaybe;
3131
if (session) return session;
3232

3333
return lifetimeIdFromExecutionContext(context);

src/core/database/database.service.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ export class DatabaseService {
124124
parameters?: Record<string, any>,
125125
): Query<Result> {
126126
const q = this.db.query() as Query<Result>;
127-
q.params.addParam(this.sessionHost.current$.value?.userId, 'currentUser');
127+
q.params.addParam(this.sessionHost.currentIfInCtx?.userId, 'currentUser');
128128
if (query) {
129129
q.raw(query, parameters);
130130
}

src/core/gel/gel.service.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ export class Gel {
115115
const queryNames = getCurrentQueryNames();
116116
const traceName = queryNames?.xray ?? 'Query';
117117

118-
let currentActorId = this.sessionHost.current$.value?.userId;
118+
let currentActorId = this.sessionHost.currentIfInCtx?.userId;
119119
// TODO temporarily check if UUID before applying global.
120120
// Once migration is complete this can be removed.
121121
currentActorId = isUUID(currentActorId) ? currentActorId : undefined;

src/core/graphql/graphql-tracing.plugin.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ export class GraphqlTracingPlugin {
4747

4848
return {
4949
onExecuteDone: () => {
50-
const userId = this.sessionHost.current$.value?.userId;
50+
const userId = this.sessionHost.currentMaybe?.userId;
5151
if (userId) {
5252
segment.setUser?.(userId);
5353
}

0 commit comments

Comments
 (0)