From dd9b16264678f380a1eec1eda87064122fd8e7b8 Mon Sep 17 00:00:00 2001 From: Matt Lyons Date: Thu, 4 Jan 2024 16:34:58 -0600 Subject: [PATCH] Remove redundant sender ID from the "registerRequest" command (#692) --- lib/papi-dts/papi.d.ts | 3 +- src/shared/data/internal-connection.model.ts | 1 - src/shared/services/connection.service.ts | 2 +- src/shared/services/network.service.ts | 91 +++++++++++++------- src/shared/utils/papi-util.ts | 4 +- 5 files changed, 65 insertions(+), 36 deletions(-) diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index b4b4b921a7..15e667406b 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -596,6 +596,8 @@ declare module 'shared/utils/papi-util' { * This type is used as the public-facing interface for requests */ export type ComplexRequest = { + /** The one who sent the request */ + senderId: number; contents: TParam; }; type ComplexResponseSuccess = { @@ -856,7 +858,6 @@ declare module 'shared/data/internal-connection.model' { } /** Request to do something and to respond */ export type InternalRequest = { - senderId: number; requestId: number; } & ComplexRequest; /** Response to a request */ diff --git a/src/shared/data/internal-connection.model.ts b/src/shared/data/internal-connection.model.ts index 803c79f973..621cb0e755 100644 --- a/src/shared/data/internal-connection.model.ts +++ b/src/shared/data/internal-connection.model.ts @@ -59,7 +59,6 @@ export enum ConnectionStatus { /** Request to do something and to respond */ export type InternalRequest = { - senderId: number; requestId: number; } & ComplexRequest; diff --git a/src/shared/services/connection.service.ts b/src/shared/services/connection.service.ts index 62e6cebf70..bce162ef4a 100644 --- a/src/shared/services/connection.service.ts +++ b/src/shared/services/connection.service.ts @@ -4,7 +4,7 @@ * over this. */ // TODO: Refactor into a class and an interface -// TODO: Combine with NetworkSerice? +// TODO: Combine with NetworkService? import { CLIENT_ID_UNASSIGNED, diff --git a/src/shared/services/network.service.ts b/src/shared/services/network.service.ts index e8859c4d95..174067f084 100644 --- a/src/shared/services/network.service.ts +++ b/src/shared/services/network.service.ts @@ -327,11 +327,7 @@ async function registerRequestHandlerUnsafe( // Check with the server if it already has a handler for this requestType if (isClient()) { // If we are the client, try to register with the server because server has all registrations - await requestUnsafe( - serializeRequestType(CATEGORY_SERVER, 'registerRequest'), - requestType, - connectionService.getClientId(), - ); + await requestUnsafe(serializeRequestType(CATEGORY_SERVER, 'registerRequest'), requestType); } // We have successfully checked that this is the first registration for this requestType. Set up @@ -428,47 +424,66 @@ const createNetworkEventEmitterUnsafe = ( * Unregisters a client connection's request handler SERVER-ONLY. This should not be needed on the * client * - * @param requestType The type of request on which to unregister the handler - * @param clientId ClientId of the client who wants to unregister the handler - * @returns True if successfully unregistered, false otherwise + * @param request An array of values, the first of which is the `SerializedRequestType` value that + * defines what request type is being unregistered. Note that the value is an array even though we + * only ever look at the first value because all requests come over the network with an array of + * arguments. Technically if there is more than 1 argument passed to the function, values after + * the first they might not be `SerializedRequestType` values. + * @returns `ComplexResponse` with `success` = True if successfully unregistered, otherwise False + * along with an error message */ const unregisterRemoteRequestHandler = async ( - requestType: SerializedRequestType, - clientId: number, -): Promise => { + request: ComplexRequest<[SerializedRequestType]>, +): Promise> => { + const [requestType] = request.contents; const requestRegistration = requestRegistrations.get(requestType); if (!requestRegistration) - // The request isn't registered - return false; - - if (requestRegistration.registrationType === 'local' || requestRegistration.clientId !== clientId) - // The request handler is not theirs to unregister. Is this egregious enough that we should - // throw here? - return false; + return { + success: false, + errorMessage: `Cannot unregister ${requestType} because it was not registered`, + }; + + if ( + requestRegistration.registrationType === 'local' || + requestRegistration.clientId !== request.senderId + ) + return { + success: false, + errorMessage: `Cannot unregister ${requestType} unless the owner requests it`, + }; // We can unregister this handler! Remove it from the registrations requestRegistrations.delete(requestType); - return true; + return { + contents: undefined, + success: true, + }; }; /** * Registers a client connection's request handler SERVER-ONLY. This should not be needed on the * client * - * @param requestType The type of request on which to register the handler - * @param clientId ClientId of the client who wants to register the handler + * @param request An array of values, the first of which is the `SerializedRequestType` value that + * defines what request type is being registered. Note that the value is an array even though we + * only ever look at the first value because all requests come over the network with an array of + * arguments. Technically if there is more than 1 argument passed to the function, values after + * the first they might not be `SerializedRequestType` values. + * @returns `ComplexResponse` with `success` = True if successfully registered, otherwise False + * along with an error message */ const registerRemoteRequestHandler = async ( - requestType: SerializedRequestType, - clientId: number, -): Promise => { - // TODO: Consider a good way to expose senderId in this scenario instead of just passing it as an argument. - // Maybe create a registerRequestHandlerInternal function that uses InternalRequest and InternalResponse? + request: ComplexRequest<[SerializedRequestType]>, +): Promise> => { + const [requestType] = request.contents; // Check to see if there is already a handler for this requestType if (requestRegistrations.has(requestType)) { - throw new Error(`requestType ${requestType} already has a remote handler registered`); + return { + errorMessage: `requestType ${requestType} already has a remote handler registered`, + success: false, + }; } validateRequestTypeFormatting(requestType); @@ -478,8 +493,13 @@ const registerRemoteRequestHandler = async ( requestRegistrations.set(requestType, { registrationType: 'remote', requestType, - clientId, + clientId: request.senderId, }); + + return { + contents: undefined, + success: true, + }; }; /** @@ -504,7 +524,10 @@ const handleClientDisconnect = ({ clientId }: ClientDisconnectEvent) => { // Remove registrations for this clientId logger.info(`Client ${clientId} disconnected! Unregistering ${requestTypesToRemove.join(', ')}`); requestTypesToRemove.forEach((requestType) => - unregisterRemoteRequestHandler(requestType, clientId), + unregisterRemoteRequestHandler({ + contents: [requestType], + senderId: clientId, + }), ); }; @@ -689,9 +712,13 @@ export const initialize = () => { const registrationUnsubscribers = Object.entries(serverRequestHandlers).map( ([requestType, handler]) => - // Re-assert type after passing through `map`. - // eslint-disable-next-line no-type-assertion/no-type-assertion - registerRequestHandlerUnsafe(requestType as SerializedRequestType, handler), + registerRequestHandlerUnsafe( + // Re-assert type after passing through `map`. + // eslint-disable-next-line no-type-assertion/no-type-assertion + requestType as SerializedRequestType, + handler, + RequestHandlerType.Complex, + ), ); // Wait to successfully register all requests unsubscribeServerRequestHandlers = aggregateUnsubscriberAsyncs( diff --git a/src/shared/utils/papi-util.ts b/src/shared/utils/papi-util.ts index a1eea87fc8..1d1f8f2b05 100644 --- a/src/shared/utils/papi-util.ts +++ b/src/shared/utils/papi-util.ts @@ -81,6 +81,8 @@ export const createSafeRegisterFn = >( * This type is used as the public-facing interface for requests */ export type ComplexRequest = { + /** The one who sent the request */ + senderId: number; contents: TParam; }; @@ -300,7 +302,7 @@ export function deserializeRequestType(requestType: SerializedRequestType): Requ const colonIndex = requestType.indexOf(REQUEST_TYPE_SEPARATOR); if (colonIndex <= 0 || colonIndex >= requestType.length - 1) throw new Error( - `deserializeRequestType: Must have two parts divided by a ${REQUEST_TYPE_SEPARATOR}`, + `deserializeRequestType: Must have two parts divided by a ${REQUEST_TYPE_SEPARATOR} (${requestType})`, ); const category = requestType.substring(0, colonIndex); const directive = requestType.substring(colonIndex + 1);