Repository: boardgameio/boardgame.io
Pull request #831 (2020-10-20 00:47:29)

feat(server): Enable game setup to perform validation
Many games have complex rules which affect configuration or setup options. For example, Sushi Go Party ([see rules](https://gamewright.com/pdfs/Rules/SushiGoPartyTM-RULES.pdf)) allows the Edamame card to only be used when there are 3 or more players in the game or the Menu card when there are 6 or fewer players in the game. To enable games to feed back these sorts of validation results to the client, this pull request introduces a new exception type `GameSetupError` which game setup code can throw to indicate an incompatible configuration. If thrown, the exception is communicated back to the client as a HTTP 400 error.

#### Checklist

* [x] Use a separate branch in your local repo (not `master`).
* [x] Test coverage is 100% (or you have a story for why it's ok).


c-w: Not sure why coveralls is failing given that it states `8 of 8 new or added lines in 1 file covered. (100.0%)`.
delucis: Hi @c-w this is a nice idea, but there are a few more places that the setup function can be called, so this might have some unintended consequences.

https://github.com/boardgameio/boardgame.io/blob/051c43fc9abc12bedb26c16716a4a013506853d1/src/client/client.ts#L182

https://github.com/boardgameio/boardgame.io/blob/051c43fc9abc12bedb26c16716a4a013506853d1/src/master/master.ts#L366

I assume that the intent here is to validate `setupData`, which isn’t used in the two above cases. Is validating `setupData` possible using a custom middleware on the server? Something like the following?

```js
const server = Server({ /* options */ });

// Add middleware to the create game route to validate `setupData`.
server.router.use('/games/:name/create', async (ctx, next) => {
  // Read setup data from request.
  const setupData = ctx.request.body.setupData;
  // Validate setup data.
  if (!isConfigValid(setupData)) {
    ctx.throw(400, 'invalid setupData');
  } else {
    await next();
  }
});

server.run(8000);
```

If this works, do you think documenting how to do this could make sense, seeing as this is a server-specific concern, or do you think that the setup logic is likely to be so tangled with validation logic that it’s better to be able to bail with a `GameSetupError` rather than doing the validation first?

> Not sure why coveralls is failing given that it states `8 of 8 new or added lines in 1 file covered. (100.0%)`.

I think it’s a rounding issue in Coveralls — sometimes despite coverage essentially remaining unchanged it indicates a tiny increase or decrease. Nothing wrong on your end as far as I can tell.
c-w: Thanks for the feedback @delucis.

For the games I'm working on, the setupData validation logic changes for each game, so the `isConfigValid` method in the middleware you propose would also take the game instance as an argument. This led me to the following idea: what do you think about adding a new top-level optional key called `validateSetupData` to the game object which is a callable that takes the setupData instance and returns `true` or `false` based on its validity. In this way the createGame endpoint can check whether the validation function exists on the game instance and return a HTTP 400 if the validation returns false. The benefit of this approach is that we decouple the validation implementation (just a pure function on the game object) from the server endpoints. Requiring each user to code up the server middleware to add validation doesn't seem like a very straight forward developer experience to me and also couples the validation more strongly with the server when really it's more of a game concern from my perspective. Thoughts?
delucis: @c-w Yes, that might be a nice way to do this. It would definitely be a better developer experience, although it does add code to the game object that will only run on the server but will also be included on the client.

You can get game instances, number of players etc. from middleware, but it’s true that it’s quite fiddly, so one other option could be to write a middleware that provided a simpler interface for developers.

```js
import { validateSetupData } from 'boardgame.io/server';

server.router.use(validateSetupData({
  validator: ({ numPlayers, game, setupData }) => true,
));
```

That said, I agree that it seems strange to separate this from the game objects themselves, so it’s probably best to go with your suggestion and potentially try to handle `setupData` better everywhere games are initialized in the future.
c-w: @delucis I pushed an update based on our conversation. Let me know what you think!

> although it does add code to the game object that will only run on the server but will also be included on the client.

I believe there'll be value in having the validation logic available on the client too, e.g. on the games I'm currently working on I have a form that the user fills-in to provide the setupData. Having the validation logic available on the game object means that I could validate the form client-side before submitting it but then also have the canonical check done by the server using the same code.


Patch:
diff --git a/docs/documentation/api/Game.md b/docs/documentation/api/Game.md
index 9ff6cc5..ff2406c 100644
--- a/docs/documentation/api/Game.md
+++ b/docs/documentation/api/Game.md
@@ -10,6 +10,12 @@
   // passed through the Game Creation API.
   setup: (ctx, setupData) => G,
 
+  // Optional function to validate the setupData before
+  // matches are created. If this returns a value,
+  // an error will be reported to the user and match
+  // creation is aborted.
+  validateSetupData: (setupData, numPlayers) => 'setupData is not valid!',
+
   moves: {
     // short-form move.
     A: (G, ctx) => {},
diff --git a/src/server/api.ts b/src/server/api.ts
index 54171ab..a5570e6 100644
--- a/src/server/api.ts
+++ b/src/server/api.ts
@@ -124,6 +124,10 @@ export const createRouter = ({
     const game = games.find(g => g.name === gameName);
     if (!game) ctx.throw(404, 'Game ' + gameName + ' not found');
 
+    const setupDataError =
+      game.validateSetupData && game.validateSetupData(setupData, numPlayers);
+    if (setupDataError !== undefined) ctx.throw(400, setupDataError);
+
     const matchID = await CreateMatch({
       db,
       game,
diff --git a/src/types.ts b/src/types.ts
index 2982f85..608e43b 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -231,13 +231,21 @@ interface PhaseMap<G extends any = any, CtxWithPlugins extends Ctx = Ctx> {
   [phaseName: string]: PhaseConfig<G, CtxWithPlugins>;
 }
 
-export interface Game<G extends any = any, CtxWithPlugins extends Ctx = Ctx> {
+export interface Game<
+  G extends any = any,
+  CtxWithPlugins extends Ctx = Ctx,
+  SetupData extends any = any
+> {
   name?: string;
   minPlayers?: number;
   maxPlayers?: number;
   disableUndo?: boolean;
   seed?: string | number;
-  setup?: (ctx: CtxWithPlugins, setupData?: any) => any;
+  setup?: (ctx: CtxWithPlugins, setupData?: SetupData) => any;
+  validateSetupData?: (
+    setupData: SetupData | undefined,
+    numPlayers: number
+  ) => string | undefined;
   moves?: MoveMap<G, CtxWithPlugins>;
   phases?: PhaseMap<G, CtxWithPlugins>;
   turn?: TurnConfig<G, CtxWithPlugins>;


Test patch:
diff --git a/src/server/api.test.ts b/src/server/api.test.ts
index 5ddf70d..72f1286 100644
--- a/src/server/api.test.ts
+++ b/src/server/api.test.ts
@@ -102,6 +102,19 @@ describe('.createRouter', () => {
                 }
               : {},
         },
+        {
+          name: 'validate',
+          setup: (_, setupData) =>
+            setupData
+              ? {
+                  numTokens: setupData.tokens,
+                }
+              : {},
+          validateSetupData: (setupData, numPlayers) =>
+            numPlayers == 2 && setupData.tokens !== 2
+              ? 'Two player games must use two tokens'
+              : undefined,
+        },
       ];
     });
 
@@ -226,6 +239,34 @@ describe('.createRouter', () => {
         });
       });
 
+      describe('with setupData validation', () => {
+        test('creates game if validation passes', async () => {
+          response = await request(app.callback())
+            .post('/games/validate/create')
+            .send({
+              numPlayers: 2,
+              setupData: {
+                tokens: 2,
+              },
+            });
+
+          expect(response.status).toEqual(200);
+        });
+
+        test('returns error if validation fails', async () => {
+          response = await request(app.callback())
+            .post('/games/validate/create')
+            .send({
+              numPlayers: 2,
+              setupData: {
+                tokens: 3,
+              },
+            });
+
+          expect(response.status).toEqual(400);
+        });
+      });
+
       describe('with unlisted option', () => {
         beforeEach(async () => {
           response = await request(app.callback())