Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/microservices/exceptions/rpc-exceptions-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ export class RpcExceptionsHandler extends BaseRpcExceptionFilter {
exception: T,
host: ArgumentsHost,
): Observable<any> | null {
const filters = this.filters.filter(
filter => filter.exceptionMetatypes?.length === 0,
);
if (filters.length > 0) {
return filters[0].func(exception, host);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why(?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change prioritizes global exception filters (i.e., filters with empty exceptionMetatypes) over more specific ones. Previously, global filters that were intended to catch all exceptions could be skipped because they were processed after specific filters.

The updated logic ensures that if a filter is designed to handle all exception types (as indicated by an empty exceptionMetatypes array), it is invoked first, preserving the intended filter hierarchy and behavior.

Please let me know if I’ve misunderstood any part of this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, global filters that were intended to catch all exceptions could be skipped because they were processed after specific filters.

That's the expected behavior.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification! I've removed the exception filter changes
in commit 6cf7ad5 since the current behavior is indeed the intended
behavior.

if (isEmpty(this.filters)) {
return null;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/platform-socket.io/adapters/io-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ export class IoAdapter extends AbstractWsAdapter {
return { data: payload };
}

public bindClientDisconnect(client: Socket, callback: Function) {
client.on(DISCONNECT_EVENT, (reason: string) => callback(reason));
}

public async close(server: Server): Promise<void> {
if (this.forceCloseConnections && server.httpServer === this.httpServer) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
* @publicApi
*/
export interface OnGatewayDisconnect<T = any> {
handleDisconnect(client: T): any;
handleDisconnect(client: T, reason?: string): any;
}
2 changes: 1 addition & 1 deletion packages/websockets/interfaces/nest-gateway.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
export interface NestGateway {
afterInit?: (server: any) => void;
handleConnection?: (...args: any[]) => void;
handleDisconnect?: (client: any) => void;
handleDisconnect?: (client: any, reason?: string) => void;
}
64 changes: 64 additions & 0 deletions packages/websockets/test/web-sockets-controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,70 @@ describe('WebSocketsController', () => {
instance.subscribeDisconnectEvent(gateway, event);
expect(subscribe.called).to.be.true;
});

describe('when handling disconnect events', () => {
let handleDisconnectSpy: sinon.SinonSpy;

beforeEach(() => {
handleDisconnectSpy = sinon.spy();
(gateway as any).handleDisconnect = handleDisconnectSpy;
});

it('should call handleDisconnect with client and reason when data contains both', () => {
const mockClient = { id: 'test-client' };
const mockReason = 'client namespace disconnect';
const disconnectData = { client: mockClient, reason: mockReason };

let subscriptionCallback: Function | undefined;
event.subscribe = (callback: Function) => {
subscriptionCallback = callback;
};

instance.subscribeDisconnectEvent(gateway, event);

if (subscriptionCallback) {
subscriptionCallback(disconnectData);
}

expect(handleDisconnectSpy.calledOnce).to.be.true;
expect(handleDisconnectSpy.calledWith(mockClient, mockReason)).to.be
.true;
});

it('should call handleDisconnect with only client for backward compatibility', () => {
const mockClient = { id: 'test-client' };

let subscriptionCallback: Function | undefined;
event.subscribe = (callback: Function) => {
subscriptionCallback = callback;
};

instance.subscribeDisconnectEvent(gateway, event);

if (subscriptionCallback) {
subscriptionCallback(mockClient);
}

expect(handleDisconnectSpy.calledOnce).to.be.true;
expect(handleDisconnectSpy.calledWith(mockClient)).to.be.true;
});

it('should handle null/undefined data gracefully', () => {
let subscriptionCallback: Function | undefined;
event.subscribe = (callback: Function) => {
subscriptionCallback = callback;
};

instance.subscribeDisconnectEvent(gateway, event);

if (subscriptionCallback) {
subscriptionCallback(null);
}

expect(handleDisconnectSpy.calledOnce).to.be.true;
expect(handleDisconnectSpy.calledWith(null)).to.be.true;
});
});
});
describe('subscribeMessages', () => {
const gateway = new Test();
Expand Down
16 changes: 12 additions & 4 deletions packages/websockets/web-sockets-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,9 @@ export class WebSocketsController {

const disconnectHook = adapter.bindClientDisconnect;
disconnectHook &&
disconnectHook.call(adapter, client, () => disconnect.next(client));
disconnectHook.call(adapter, client, (reason?: string) =>
disconnect.next({ client, reason }),
);
};
}

Expand All @@ -162,9 +164,15 @@ export class WebSocketsController {

public subscribeDisconnectEvent(instance: NestGateway, event: Subject<any>) {
if (instance.handleDisconnect) {
event
.pipe(distinctUntilChanged())
.subscribe(instance.handleDisconnect.bind(instance));
event.pipe(distinctUntilChanged()).subscribe((data: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont think distinctUntilChanged will work as expected now

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right — thank you for pointing that out.
Since we’re now emitting objects like { client, reason } instead of just the client, distinctUntilChanged() was comparing object references, which caused the deduplication to fail.

I’ve updated the code to include a custom comparison function for distinctUntilChanged(). This handles both the previous format (just client) and the new one ({ client, reason }) by extracting and comparing the actual client values.

Really appreciate you catching this.

be6b50c

// Handle both old format (just client) and new format ({ client, reason })
if (data && typeof data === 'object' && 'client' in data) {
instance.handleDisconnect!(data.client, data.reason);
} else {
// Backward compatibility: if it's just the client
instance.handleDisconnect!(data);
}
});
}
}

Expand Down