Repository: boardgameio/boardgame.io (ts)

Issue:
Use storage.createGame for implicit match creation
Currently if the game master receives a sync event for a non-existent `matchID` it initialises that match implicitly:

https://github.com/boardgameio/boardgame.io/blob/9a2a6098da9b651a823c7a4f20956ffb6afe89e5/src/master/master.ts#L380-L395

This code persists the game state using the storage layer’s `setState` method. This can create inconsistencies, because the match’s initial state isn’t stored and any other game creation logic a storage layer might implement won’t be triggered. The storage layer’s `createGame` should be used here instead.

This might not always produce errors, but I ran into it [in the Firestore connector today](https://github.com/delucis/bgio-firebase/issues/5#issuecomment-638962576), because I was initialising the log document as an empty array in `createGame` and then appending to it subsequently. Matches created implicitly then caused errors, because we were trying to append to the uninitialized log.

Pull request description:
fix(master): Use `createMatch` for implicit match creation
Closes #722 

This makes implicit match creation in the master consistent with match creation via the Lobby API by using the Storage API’s `createMatch` method and creating basic match metadata even for these implicitly created matches. (It helps that we can now mark these matches as unlisted just in case someone was mixing the Lobby API & implicit match creation.)

Apart from the consistency, this also fixes a bug by ensuring that `initialState` is stored for implicitly created matches. Previously, by only using `setState`, clients couldn’t retrieve `initialState` for matches that had been initialised in this way, because it was never stored.

Patch:
diff --git a/src/master/master.ts b/src/master/master.ts
index 912b85f..bd18965 100644
--- a/src/master/master.ts
+++ b/src/master/master.ts
@@ -23,6 +23,7 @@ import {
   LogEntry,
   PlayerID,
 } from '../types';
+import { createMetadata } from '../server/util';
 import * as StorageAPI from '../server/db/base';
 
 export const getPlayerMetadata = (
@@ -216,13 +217,11 @@ export class Master {
     const key = matchID;
 
     let state: State;
-    let result: StorageAPI.FetchResult<{ state: true }>;
     if (IsSynchronous(this.storageAPI)) {
-      result = this.storageAPI.fetch(key, { state: true });
+      ({ state } = this.storageAPI.fetch(key, { state: true }));
     } else {
-      result = await this.storageAPI.fetch(key, { state: true });
+      ({ state } = await this.storageAPI.fetch(key, { state: true }));
     }
-    state = result.state;
 
     if (state === undefined) {
       logging.error(`game not found, matchID=[${key}]`);
@@ -334,66 +333,53 @@ export class Master {
    * Called when the client connects / reconnects.
    * Returns the latest game state and the entire log.
    */
-  async onSync(matchID: string, playerID: string, numPlayers: number) {
+  async onSync(matchID: string, playerID: string, numPlayers = 2) {
     const key = matchID;
 
-    let state: State;
-    let initialState: State;
-    let log: LogEntry[];
-    let matchData: Server.MatchData;
-    let filteredMetadata: FilteredMetadata;
-    let result: StorageAPI.FetchResult<{
-      state: true;
-      metadata: true;
-      log: true;
-      initialState: true;
-    }>;
+    const fetchOpts = {
+      state: true,
+      metadata: true,
+      log: true,
+      initialState: true,
+    } as const;
+
+    let fetchResult: StorageAPI.FetchResult<typeof fetchOpts>;
 
     if (IsSynchronous(this.storageAPI)) {
-      result = this.storageAPI.fetch(key, {
-        state: true,
-        metadata: true,
-        log: true,
-        initialState: true,
-      });
+      fetchResult = this.storageAPI.fetch(key, fetchOpts);
     } else {
-      result = await this.storageAPI.fetch(key, {
-        state: true,
-        metadata: true,
-        log: true,
-        initialState: true,
-      });
+      fetchResult = await this.storageAPI.fetch(key, fetchOpts);
     }
 
-    state = result.state;
-    initialState = result.initialState;
-    log = result.log;
-    matchData = result.metadata;
-
-    if (matchData) {
-      filteredMetadata = Object.values(matchData.players).map(player => {
-        const { credentials, ...filteredData } = player;
-        return filteredData;
-      });
-    }
+    let { state, initialState, log, metadata } = fetchResult;
 
     // If the game doesn't exist, then create one on demand.
     // TODO: Move this out of the sync call.
     if (state === undefined) {
       initialState = state = InitializeGame({ game: this.game, numPlayers });
-
-      this.subscribeCallback({
-        state,
-        matchID,
+      metadata = createMetadata({
+        game: this.game,
+        unlisted: true,
+        numPlayers,
       });
 
+      this.subscribeCallback({ state, matchID });
+
       if (IsSynchronous(this.storageAPI)) {
-        this.storageAPI.setState(key, state);
+        this.storageAPI.createMatch(key, { initialState, metadata });
       } else {
-        await this.storageAPI.setState(key, state);
+        await this.storageAPI.createMatch(key, { initialState, metadata });
       }
     }
 
+    let filteredMetadata: FilteredMetadata;
+    if (metadata) {
+      filteredMetadata = Object.values(metadata.players).map(player => {
+        const { credentials, ...filteredData } = player;
+        return filteredData;
+      });
+    }
+
     const filteredState = {
       ...state,
       G: this.game.playerView(state.G, state.ctx, playerID),
diff --git a/src/server/api.ts b/src/server/api.ts
index b9ffcd5..54171ab 100644
--- a/src/server/api.ts
+++ b/src/server/api.ts
@@ -15,6 +15,7 @@ import cors from '@koa/cors';
 import { InitializeGame } from '../core/initialize';
 import * as StorageAPI from './db/base';
 import { Server, LobbyAPI, Game } from '../types';
+import { createMetadata } from './util';
 
 /**
  * Creates a new match.
@@ -44,18 +45,7 @@ export const CreateMatch = async ({
 }) => {
   if (!numPlayers || typeof numPlayers !== 'number') numPlayers = 2;
 
-  const metadata: Server.MatchData = {
-    gameName: game.name,
-    unlisted: !!unlisted,
-    players: {},
-    createdAt: Date.now(),
-    updatedAt: Date.now(),
-  };
-  if (setupData !== undefined) metadata.setupData = setupData;
-  for (let playerIndex = 0; playerIndex < numPlayers; playerIndex++) {
-    metadata.players[playerIndex] = { id: playerIndex };
-  }
-
+  const metadata = createMetadata({ game, numPlayers, setupData, unlisted });
   const matchID = uuid();
   const initialState = InitializeGame({ game, numPlayers, setupData });
 
diff --git a/src/server/util.ts b/src/server/util.ts
new file mode 100644
index 0000000..fb556f2
--- /dev/null
+++ b/src/server/util.ts
@@ -0,0 +1,32 @@
+import { Server, Game } from '../types';
+
+/**
+ * Creates a new match metadata object.
+ */
+export const createMetadata = ({
+  game,
+  unlisted,
+  setupData,
+  numPlayers,
+}: {
+  game: Game;
+  numPlayers: number;
+  setupData?: any;
+  unlisted?: boolean;
+}): Server.MatchData => {
+  const metadata: Server.MatchData = {
+    gameName: game.name,
+    unlisted: !!unlisted,
+    players: {},
+    createdAt: Date.now(),
+    updatedAt: Date.now(),
+  };
+
+  if (setupData !== undefined) metadata.setupData = setupData;
+
+  for (let playerIndex = 0; playerIndex < numPlayers; playerIndex++) {
+    metadata.players[playerIndex] = { id: playerIndex };
+  }
+
+  return metadata;
+};


Test patch:
diff --git a/src/master/master.test.ts b/src/master/master.test.ts
index ae5ec01..b64353b 100644
--- a/src/master/master.test.ts
+++ b/src/master/master.test.ts
@@ -7,6 +7,7 @@
  */
 
 import * as ActionCreators from '../core/action-creators';
+import { InitializeGame } from '../core/initialize';
 import { InMemory } from '../server/db/inmemory';
 import {
   Master,
@@ -16,7 +17,7 @@ import {
   isActionFromAuthenticPlayer,
 } from './master';
 import { error } from '../core/logger';
-import { Server } from '../types';
+import { Server, State } from '../types';
 import * as StorageAPI from '../server/db/base';
 import * as dateMock from 'jest-date-mock';
 
@@ -43,7 +44,8 @@ function TransportAPI(send = jest.fn(), sendAll = jest.fn()) {
 
 describe('sync', () => {
   const send = jest.fn();
-  const master = new Master(game, new InMemory(), TransportAPI(send));
+  const db = new InMemory();
+  const master = new Master(game, db, TransportAPI(send));
 
   beforeEach(() => {
     jest.clearAllMocks();
@@ -59,19 +61,21 @@ describe('sync', () => {
   });
 
   test('sync a second time does not create a game', async () => {
+    const fetchResult = db.fetch('matchID', { metadata: true });
     await master.onSync('matchID', '0', 2);
-    expect(send).toHaveBeenCalled();
+    expect(db.fetch('matchID', { metadata: true })).toMatchObject(fetchResult);
   });
 
   test('should not have metadata', async () => {
-    await master.onSync('matchID', '0', 2);
+    db.setState('oldGameID', {} as State);
+    await master.onSync('oldGameID', '0');
     // [0][0] = first call, first argument
-    expect(send.mock.calls[0][0].args[3]).toBeUndefined();
+    expect(send.mock.calls[0][0].args[1].filteredMetadata).toBeUndefined();
   });
 
   test('should have metadata', async () => {
     const db = new InMemory();
-    const dbMetadata = {
+    const metadata = {
       gameName: 'tic-tac-toe',
       setupData: {},
       players: {
@@ -89,7 +93,7 @@ describe('sync', () => {
       createdAt: 0,
       updatedAt: 0,
     };
-    db.setMetadata('matchID', dbMetadata);
+    db.createMatch('matchID', { metadata, initialState: {} as State });
     const masterWithMetadata = new Master(game, db, TransportAPI(send));
     await masterWithMetadata.onSync('matchID', '0', 2);
 
@@ -279,6 +283,42 @@ describe('update', () => {
     const { metadata } = db.fetch(id, { metadata: true });
     expect(metadata.updatedAt).toEqual(updatedAt.getTime());
   });
+
+  test('processes update if there is no metadata', async () => {
+    const id = 'gameWithoutMetadata';
+    const db = new InMemory();
+    const masterWithoutMetadata = new Master(game, db, TransportAPI(send));
+    // Store state manually to bypass automatic metadata initialization on sync.
+    let state = InitializeGame({ game });
+    expect(state.ctx.turn).toBe(1);
+    db.setState(id, state);
+    // Dispatch update to end the turn.
+    const event = ActionCreators.gameEvent('endTurn', null, '0');
+    await masterWithoutMetadata.onUpdate(event, 0, id, '0');
+    // Confirm the turn ended.
+    let metadata: undefined | Server.MatchData;
+    ({ state, metadata } = db.fetch(id, { state: true, metadata: true }));
+    expect(state.ctx.turn).toBe(2);
+    expect(metadata).toBeUndefined();
+  });
+
+  test('processes update if there is no metadata with async DB', async () => {
+    const id = 'gameWithoutMetadata';
+    const db = new InMemoryAsync();
+    const masterWithoutMetadata = new Master(game, db, TransportAPI(send));
+    // Store state manually to bypass automatic metadata initialization on sync.
+    let state = InitializeGame({ game });
+    expect(state.ctx.turn).toBe(1);
+    db.setState(id, state);
+    // Dispatch update to end the turn.
+    const event = ActionCreators.gameEvent('endTurn', null, '0');
+    await masterWithoutMetadata.onUpdate(event, 0, id, '0');
+    // Confirm the turn ended.
+    let metadata: undefined | Server.MatchData;
+    ({ state, metadata } = db.fetch(id, { state: true, metadata: true }));
+    expect(state.ctx.turn).toBe(2);
+    expect(metadata).toBeUndefined();
+  });
 });
 
 describe('playerView', () => {