ThePandemoniumInstitute / botc-release

The Official Blood on the Clocktower App
https://botc.app
35 stars 2 forks source link

[BUG] App does not validate against published script schema #252

Closed AdmiralGT closed 1 month ago

AdmiralGT commented 1 month ago

Description

I'm attempting to add validation support for uploading homebrew and hybrid scripts to botcscript.com. The attached scripts fail to validate against the published schema (character elements don't match against any of the "OneOf" object items because the character objects contain a "flavor" object. The Character type object does not define flavor as an allowable property (additionaProperties is set to false and it is not defined)).

However, both scripts can be imported into the app and games hosted using them.

curtain_call.json fall_of_rome.json

Steps to reproduce

  1. Go to https://www.jsonschemavalidator.net/
  2. Enter the Schema into the left box
  3. Enter the content of the attached scripts in the right box
  4. See Errors
  5. Import scripts into app and host game

Browser

Chrome

Operating System

Windows

Date and Time

Irrelevant

Game Session

No response

Relevant console log output

No response

bra1n commented 1 month ago

Hey Geoff, thanks for the report! The app does validate against the script schema, however it only shows a warning when you try to load an invalid script. It will still do its best to let you play with the script, which I think is preferred to preventing people from playing at all, if their script contains some unexpected properties. In the case of your 2 example scripts, warnings are shown correctly, indicating that a validation does happen.

AdmiralGT commented 1 month ago

If you are attempting to load the script as best as possible, then I don't think you should fail validation to elements that you consider valid. These "invalid" characters are loaded correctly no matter what properties are included in the character object type and so additionalProperties should represent this as being allowable. Once you've validated, you can then view the additionalProperties (which should be displayed as annotations) and warn about any present to indicate that they've been ignored.

bra1n commented 1 month ago

Yes, in an ideal world, that would be a better approach. But considering that we have limited resources to dedicate to app development (of which homebrew is just a small part!) and that currently the most popular homebrew tool outputs invalid JSON by default, I would argue that the current approach is still the best one. If I were to remove the additionalProperties rule then that could be interpreted as encouragement to augment script JSONs with whatever extra data you want, when in reality, those get stripped away internally. (you'll see that any extraneous properties do not get kept when you download a homebrew JSON from the app) On the other hand, if we were to enforce the schema validation, everyone that uses Bloodstar, for example, would HAVE to clean up their JSONs to make them compliant, which is equally bad.