Merge 3d4fbc0 into dc6cd45

src/core/action-creators.ts

@@ -149,3 +149,11 @@ export const plugin = (
   type: Actions.PLUGIN as typeof Actions.PLUGIN,
   payload: { type, args, playerID, credentials },
 });
+
+/**
+ * Private action used to strip transient metadata (e.g. errors) from the game
+ * state.
+ */
+export const stripTransients = () => ({
+  type: Actions.STRIP_TRANSIENTS as typeof Actions.STRIP_TRANSIENTS,
+});

src/core/action-types.ts

@@ -15,3 +15,4 @@ export const UNDO = 'UNDO';
 export const UPDATE = 'UPDATE';
 export const PATCH = 'PATCH';
 export const PLUGIN = 'PLUGIN';
+export const STRIP_TRANSIENTS = 'STRIP_TRANSIENTS';

src/core/errors.ts

@@ -0,0 +1,23 @@
+export enum UpdateErrorType {
+  // The action’s credentials were missing or invalid
+  UnauthorizedAction = 'update/unauthorized_action',
+  // The action’s matchID was not found
+  MatchNotFound = 'match_not_found',
+  // Could not apply Patch operation (rfc6902).
+  PatchFailed = 'patch_failed',
+}
+
+export enum ActionErrorType {
+  // The action contained a stale state ID
+  StaleStateId = 'action/stale_state_id',
+  // The requested move is unknown or not currently available
+  UnavailableMove = 'action/unavailable_move',
+  // The move declared it was invalid (INVALID_MOVE constant)
+  InvalidMove = 'action/invalid_move',
+  // The player making the action is not currently active
+  InactivePlayer = 'action/inactive_player',
+  // The game has finished
+  GameOver = 'action/gameover',
+  // The requested action is disabled (e.g. undo/redo, events)
+  ActionDisabled = 'action/action_disabled',
+}

src/core/reducer.test.ts

@@ -131,11 +131,16 @@ test('valid patch', () => {
 
 test('invalid patch', () => {
   const originalState = { _stateID: 0, G: 'patch' } as State;
-  const state = reducer(
+  const { transients, ...state } = reducer(
     originalState,
     patch(0, 1, [{ op: 'replace', path: '/_stateIDD', value: 1 }], [])
   );
   expect(state).toEqual(originalState);
+  expect(transients.error.type).toEqual('patch_failed');
+  // It's an array.
+  expect(transients.error.payload.length).toEqual(1);
+  // It looks like the standard rfc6902 error language.
+  expect(transients.error.payload[0].toString()).toContain('/_stateIDD');
 });
 
 test('reset', () => {

src/core/reducer.ts

@@ -14,13 +14,16 @@ import { INVALID_MOVE } from './constants';
 import type {
   ActionShape,
   Ctx,
+  ErrorType,
   Game,
   LogEntry,
-  State,
-  Move,
   LongFormMove,
+  Move,
+  State,
+  TransientState,
   Undo,
 } from '../types';
+import { ActionErrorType, UpdateErrorType } from './errors';
 import { applyPatch } from 'rfc6902';
 
 /**
@@ -97,7 +100,7 @@ function initializeDeltalog(
   state: State,
   action: ActionShape.MakeMove | ActionShape.Undo | ActionShape.Redo,
   move?: Move
-): State {
+): TransientState {
   // Create a log entry for this action.
   const logEntry: LogEntry = {
     action,
@@ -121,6 +124,28 @@ function initializeDeltalog(
   };
 }
 
+/**
+ * WithError
+ *
+ * Augment a State instance with transient error information.
+ */
+function WithError<PT extends any = any>(
+  state: State,
+  errorType: ErrorType,
+  payload?: PT
+): TransientState {
+  const error = {
+    type: errorType,
+    payload,
+  };
+  return {
+    ...state,
+    transients: {
+      error,
+    },
+  };
+}
+
 /**
  * CreateGameReducer
  *
@@ -142,8 +167,23 @@ export function CreateGameReducer({
    * @param {object} state - The state before the action.
    * @param {object} action - A Redux action.
    */
-  return (state: State | null = null, action: ActionShape.Any): State => {
+  return (
+    state: TransientState | null = null,
+    action: ActionShape.Any
+  ): TransientState => {
     switch (action.type) {
+      case Actions.STRIP_TRANSIENTS: {
+        // This action indicates that transient metadata in the state has been
+        // consumed and should now be stripped from the state..
+        const {
+          transients,
+          // Revert the game state to before the error so existing consumers do
+          // not see garbage.
+          ...strippedState
+        } = state;
+        return strippedState;
+      }
+
       case Actions.GAME_EVENT: {
         state = { ...state, deltalog: [] };
 
@@ -158,7 +198,7 @@ export function CreateGameReducer({
         // Disallow events once the game is over.
         if (state.ctx.gameover !== undefined) {
           error(`cannot call event after game end`);
-          return state;
+          return WithError(state, ActionErrorType.GameOver);
         }
 
         // Ignore the event if the player isn't active.
@@ -167,11 +207,11 @@ export function CreateGameReducer({
           !game.flow.isPlayerActive(state.G, state.ctx, action.payload.playerID)
         ) {
           error(`disallowed event: ${action.payload.type}`);
-          return state;
+          return WithError(state, ActionErrorType.InactivePlayer);
         }
 
         // Execute plugins.
-        state = plugins.Enhance(state, {
+        state = plugins.Enhance(state as State, {
           game,
           isClient: false,
           playerID: action.payload.playerID,
@@ -200,7 +240,7 @@ export function CreateGameReducer({
         );
         if (move === null) {
           error(`disallowed move: ${action.payload.type}`);
-          return state;
+          return WithError(state, ActionErrorType.UnavailableMove);
         }
 
         // Don't run move on client if move says so.
@@ -211,7 +251,7 @@ export function CreateGameReducer({
         // Disallow moves once the game is over.
         if (state.ctx.gameover !== undefined) {
           error(`cannot make move after game end`);
-          return state;
+          return WithError(state, ActionErrorType.GameOver);
         }
 
         // Ignore the move if the player isn't active.
@@ -220,7 +260,7 @@ export function CreateGameReducer({
           !game.flow.isPlayerActive(state.G, state.ctx, action.payload.playerID)
         ) {
           error(`disallowed move: ${action.payload.type}`);
-          return state;
+          return WithError(state, ActionErrorType.InactivePlayer);
         }
 
         // Execute plugins.
@@ -238,7 +278,8 @@ export function CreateGameReducer({
           error(
             `invalid move: ${action.payload.type} args: ${action.payload.args}`
           );
-          return state;
+          // TODO(#723): Marshal a nice error payload with the processed move.
+          return WithError(state, ActionErrorType.InvalidMove);
         }
 
         const newState = { ...state, G };
@@ -294,12 +335,15 @@ export function CreateGameReducer({
 
         if (game.disableUndo) {
           error('Undo is not enabled');
-          return state;
+          return WithError(state, ActionErrorType.ActionDisabled);
         }
 
         const { _undo, _redo } = state;
 
         if (_undo.length < 2) {
+          // TODO(#723): Add an error case here.
+          //  error(`No moves to undo`);
+          //  return WithError(state, ActionErrorType.ActionDisabled);
           return state;
         }
 
@@ -311,6 +355,9 @@ export function CreateGameReducer({
           actionHasPlayerID(action) &&
           action.payload.playerID !== last.playerID
         ) {
+          // TODO(#723): Add an error case here.
+          //  error(`Cannot undo other players' moves`);
+          //  return WithError(state, ActionErrorType.ActionDisabled);
           return state;
         }
 
@@ -321,6 +368,9 @@ export function CreateGameReducer({
           last.playerID
         );
         if (!CanUndoMove(state.G, state.ctx, lastMove)) {
+          // TODO(#723): Add an error case here.
+          //  error(`Move cannot be undone`);
+          //  return WithError(state, ActionErrorType.ActionDisabled);
           return state;
         }
 
@@ -342,12 +392,15 @@ export function CreateGameReducer({
 
         if (game.disableUndo) {
           error('Redo is not enabled');
-          return state;
+          return WithError(state, ActionErrorType.ActionDisabled);
         }
 
         const { _undo, _redo } = state;
 
         if (_redo.length == 0) {
+          // TODO(#723): Add an error case here.
+          //  error(`No moves to redo`);
+          //  return WithError(state, ActionErrorType.ActionDisabled);
           return state;
         }
 
@@ -358,6 +411,9 @@ export function CreateGameReducer({
           actionHasPlayerID(action) &&
           action.payload.playerID !== first.playerID
         ) {
+          // TODO(#723): Add an error case here.
+          //  error(`Cannot redo other players` moves`);
+          //  return WithError(state, ActionErrorType.ActionDisabled);
           return state;
         }
 
@@ -375,6 +431,10 @@ export function CreateGameReducer({
       }
 
       case Actions.PLUGIN: {
+        // TODO(#723): Expose error semantics to plugin processing.
+        // DevNote: The upcast to State is reasonable both from a typing
+        // and functionality perspective because plugin actions can only be
+        // invoked after any transient artifacts like errors have been cleaned.
         return plugins.ProcessAction(state, action, { game });
       }
 
@@ -385,7 +445,7 @@ export function CreateGameReducer({
         const hasError = patchError.some((entry) => entry !== null);
         if (hasError) {
           error(`Patch ${JSON.stringify(action.patch)} apply failed`);
-          return oldState;
+          return WithError(oldState, UpdateErrorType.PatchFailed, patchError);
         } else {
           return newState;
         }

src/master/master.test.ts

@@ -16,6 +16,7 @@ import { Auth } from '../server/auth';
 import * as StorageAPI from '../server/db/base';
 import * as dateMock from 'jest-date-mock';
 import { PlayerView } from '../core/player-view';
+import { INVALID_MOVE } from '../core/constants';
 
 jest.mock('../core/logger', () => ({
   info: jest.fn(),
@@ -83,6 +84,12 @@ function TransportAPI(send = jest.fn(), sendAll = jest.fn()) {
   return { send, sendAll };
 }
 
+function validateNotTransientState(state: any) {
+  expect(state).toEqual(
+    expect.not.objectContaining({ transients: expect.anything() })
+  );
+}
+
 describe('sync', () => {
   const send = jest.fn();
   const db = new InMemory();
@@ -533,6 +540,9 @@ describe('patch', () => {
         stages: {
           A: {
             moves: {
+              Invalid: () => {
+                return INVALID_MOVE;
+              },
               A: {
                 client: false,
                 move: (G, ctx: Ctx) => {
@@ -560,6 +570,9 @@ describe('patch', () => {
   const action = ActionCreators.gameEvent('endTurn');
 
   beforeAll(async () => {
+    master.subscribe(({ state }) => {
+      validateNotTransientState(state);
+    });
     await master.onSync('matchID', '0', undefined, 2);
   });
 
@@ -624,14 +637,25 @@ describe('patch', () => {
     );
   });
 
-  test('invalid move', async () => {
+  test('disallowed move', async () => {
     await master.onUpdate(ActionCreators.makeMove('move'), 1, 'matchID', '0');
     expect(sendAll).not.toHaveBeenCalled();
     expect(error).toHaveBeenCalledWith(
       `move not processed - canPlayerMakeMove=false - playerID=[0] - action[move]`
     );
   });
 
+  test('invalid move', async () => {
+    await master.onUpdate(
+      ActionCreators.makeMove('Invalid', null, '0'),
+      1,
+      'matchID',
+      '0'
+    );
+    expect(sendAll).toHaveBeenCalled();
+    expect(error).toHaveBeenCalledWith('invalid move: Invalid args: null');
+  });
+
   test('valid matchID / stateID / playerID', async () => {
     await master.onUpdate(action, 1, 'matchID', '0');
     expect(sendAll).toHaveBeenCalled();
@@ -720,6 +744,9 @@ describe('connectionChange', () => {
 
   beforeEach(() => {
     sendAllReturn = undefined;
+    master.subscribe(({ state }) => {
+      validateNotTransientState(state);
+    });
     jest.clearAllMocks();
   });
 

src/master/master.ts

@@ -6,6 +6,7 @@
  * https://opensource.org/licenses/MIT.
  */
 
+import * as ActionCreators from '../core/action-creators';
 import { CreateGameReducer } from '../core/reducer';
 import { ProcessGameConfig, IsLongFormMove } from '../core/game';
 import { UNDO, REDO, MAKE_MOVE } from '../core/action-types';
@@ -266,6 +267,12 @@ export class Master {
     // Update server's version of the store.
     store.dispatch(action);
     state = store.getState();
+    if ('transients' in state) {
+      // TODO(#723): Where appropriate, extract the reported error and notify
+      // the caller.
+      store.dispatch(ActionCreators.stripTransients());
+      state = store.getState();
+    }
 
     this.subscribeCallback({
       state,