Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): Enable game setup to perform validation #831

Merged
merged 2 commits into from Oct 21, 2020
Merged

feat(server): Enable game setup to perform validation #831

merged 2 commits into from Oct 21, 2020

Conversation

c-w
Copy link
Contributor

@c-w c-w commented Oct 20, 2020

Many games have complex rules which affect configuration or setup options. For example, Sushi Go Party (see rules) 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

  • Use a separate branch in your local repo (not master).
  • Test coverage is 100% (or you have a story for why it's ok).

@c-w c-w changed the title Enable game setup to perform validation feat(server): Enable game setup to perform validation Oct 20, 2020
@c-w
Copy link
Contributor Author

c-w commented Oct 20, 2020

Not sure why coveralls is failing given that it states 8 of 8 new or added lines in 1 file covered. (100.0%).

@delucis
Copy link
Member

delucis commented Oct 20, 2020

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.

this.initialState = InitializeGame({ game: this.game, numPlayers });

initialState = state = InitializeGame({ game: this.game, numPlayers });

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?

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
Copy link
Contributor Author

c-w commented Oct 20, 2020

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
Copy link
Member

delucis commented Oct 20, 2020

@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.

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
Copy link
Contributor Author

c-w commented Oct 21, 2020

@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 while the user is filling it in to provide quick feedback, but then also have the canonical check done by the server using the same code to ensure that no invalid games are ever created.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

I think this looks like a good approach. Here are a couple of small comments.

src/types.ts Show resolved Hide resolved
docs/documentation/api/Game.md Outdated Show resolved Hide resolved
@delucis delucis merged commit d071e98 into boardgameio:master Oct 21, 2020
1 check passed
@c-w c-w deleted the c-w/game-setup-error branch Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants