Add File-based Runtime Loaded peanlty file #83

official-sounding · 2018-06-27T02:41:50Z

Uses a Ruleset definition to point to a file in the config directory that defines a list of penalties. Adds an endpoint in the JSON Servlet that supplies that file as a string to the frontend, and overhauled the pt & pt (color) views to use a direct ajax call (easily chachable later on) to load the file. Defaults to the 2018 penalty file, and includes the previous version for posterity. Added a few small unit tests around the SettingsModel.

Rationale for approach vs text string, etc:

The WS-based delivery of penalty codes is over-complicated, the front-end needs to handle receiving repeated codes, etc. hitting an endpoint to download a file is simpler and allows for potential caching
using a JSON file to define the penalty codes makes it easier to edit than a text delimited string, especially when considering defining multiple penalty verbal cues per code, whether a penalty is expel-able, etc.
Distributing an update to penalty codes/verbal cues becomes easier, as a user just needs to drop the file and repoint, instead of updating a potentially error-prone encoded text string in the ruleset editor. My sense is that OffCom may make changes to verbal cues on a more frequent basis.
Allows users to maintain multiple sets of penalties (say, during beta testing of 2019 rules) more efficiently, and potentially allows WFTDA or other governing bodies to distribute new penalty codes more efficiently.

Closes #53

frank-weinberg · 2018-06-27T07:13:09Z

I find that rationale convincing (and hope to have time to review over the weekend).

brian-brazil · 2018-06-27T08:25:32Z

The WS-based delivery of penalty codes is over-complicated, the front-end needs to handle receiving repeated codes, etc. hitting an endpoint to download a file is simpler and allows for potential caching

Adding an entire 2nd way in which configuration gets to some pages is also complicated. I'd much prefer we stick to exactly one way of getting data to WS endpoints. We already have enough data transfer mechanisms without adding yet another one. The data sizes involved are also tiny, so caching is not relevant.

I also suspect this implementation is vulnerable to a directory traversal attack.

whether a penalty is expel-able,

For simplicity all penalties should be considered expelable. The rules have a pretty broad interpretation, so best to avoid getting stuck in that debate.

Distributing an update to penalty codes/verbal cues becomes easier, as a user just needs to drop the file and repoint, instead of updating a potentially error-prone encoded text string in the ruleset editor.

I'm not convinced by this. Our users seem to have difficulty uploading files as-is for team logos and ads. Now they need to figure out how to do that and change the ruleset anyway.

Allows users to maintain multiple sets of penalties (say, during beta testing of 2019 rules) more efficiently, and potentially allows WFTDA or other governing bodies to distribute new penalty codes more efficiently.

I don't see how this changes this. It's a new ruleset one way or the other.

frank-weinberg · 2018-06-27T08:55:47Z

Our users seem to have difficulty uploading files as-is for team logos and ads. Now they need to figure out how to do that and change the ruleset anyway.

Users can grab logos and ads from literally anywhere. New sets of penalty codes would have to be distributed from someone who knows how to prepare them and can thus include instructions how to install them. Plus we can add multiple sets of codes with our distribution and the user can choose from those without difficulty.

I also suspect this implementation is vulnerable to a directory traversal attack.

Giving the file name as a string is also prone to files not being found due to typos. The ruleset editor should only allow the user to select from the detected set of files (presumably through a dropdown).
However, we currently don't support rules that allow the choice from a fixed set of options, except for binary choices. So we would need to add this type of rule (which would probably be useful in other places as well).

(I'm keeping out of the debate about data transmission between backend and frontend as this is not my area of expertise.)

brian-brazil · 2018-06-27T09:02:50Z

Plus we can add multiple sets of codes with our distribution and the user can choose from those without difficulty.

That argument makes sense, and applies for any approach we come up with.

The ruleset editor should only allow the user to select from the detected set of files

It's important that this sort of thing be a backend check. Security is not that important for the scoreboard currently, but allowing anyone with access to the scoreboard to poke around the files of what is likely a personal laptop seems unwise. The safest way to handle this would be to use something that filters out/ignores any ../ in paths.

frank-weinberg · 2018-06-27T09:11:54Z

That argument makes sense, and applies for any approach we come up with.

It's at the very least a lot harder to do, if the set of penalties is stored as one or more StringRules.

The safest way to handle this would be to use something that filters out/ignores any ../ in paths.

I would use the more generic approach of having a rule type that presents a set of options and checks any selction from the frontend against these options. (And the set of options is obviously populated in the backend.)

official-sounding · 2018-06-27T13:42:42Z

I agree that file uploads pose difficulties to users, but I agree with frank-weinberg that using some format stored in a StringRule is going to be harder to update in practice for the set of folks who would be interested in updating penalty codes.

Using a format like JSON over something custom gets us the benefit of being able to use external editors for validating syntax, and using a file reference instead of the contents of a string rule means that we avoid a class of errors (copy-and-paste mishaps leading to corrupted penalty definitions), let users/ruleset maintainers store the penalties in an easier to edit file instead of a one-line string, and (more weakly) allows for more consistency between installs. Updating a file location is (in my opinion) easier for someone to update than putting all the definitions directly into a ruleset item. Users could also tell much more simply what set of penalties a given ruleset is actually using, especially when there are subtle differences; thinking here of JRDA, which uses all of the WFTDA penalties, plus one additional one. It might be hard to see if that penalty is present from the ruleset editor if the penalties are all in a string.

From there, actually delivering the definition to the front-end, I understand the trepidation pulling this out of the WS feed, but I think keeping them in the WS feed adds significantly more complexity than delivering a standalone file.

We'd have to add backend logic to parse the file contents, put everything into the WS feed, then pull it back out on the front-end, potentially introducing issues, for example, the current line format used by the websocket for penalties will break if a verbal cue has a '-' in it. I have a WS-based approach in 816cc96 on this PR, and it requires a lot of extra "work" on the backend to deliver the penalties. The added complexity of the ajax call is outweighed by the simplification of the rest of the process.

I agree with the presence of a directory traversal attack, and I will make some updates to account for that. Additionally, I agree that it'll just be simpler to allow all penalties to be expellable.

brian-brazil · 2018-06-27T14:08:39Z

that using some format stored in a StringRule is going to be harder to update in practice for the set of folks who would be interested in updating penalty codes.

JSON is pretty finicky, in particular around trailing commas. I wouldn't presume this for non-technical users.

one-line string,

It doesn't have to be one line.

It might be hard to see if that penalty is present from the ruleset editor if the penalties are all in a string.

It'd be far harder if it's in a file, which wouldn't provide a list of the penalites in the ruleset editor at all.

From there, actually delivering the definition to the front-end, I understand the trepidation pulling this out of the WS feed, but I think keeping them in the WS feed adds significantly more complexity than delivering a standalone file.

This may be easier here, but I think is worse for the scoreboard overall. It's an entirely new data flow to consider, and we've already seen the fun that inventing new independent dataflows has caused in areas such as penalties. I'd rather not see the same mistake made again.

I believe this should follow our existing standard ways of doing things. That means that all state lives inside the XML, and that copying the XML autosave around is sufficient to continue on a game. In addition we should be passing scoreboard state via the existing XML/WS methods rather than adding a brand new one - and one in particular that hasn't a way to update it inflight that XML/WS has. If we go for a new approach all that work needs to be reinvented and debugged, compared to taking advantage of the architecture that already exists.

Long term I'd like to consolidate to having the WS be the sole API for both internal and external users.

the current line format used by the websocket for penalties will break if a verbal cue has a '-' in it.

That sounds like a bug we should be fixing, team and skater names can contain hyphens.

I have a WS-based approach in 816cc96 on this PR, and it requires a lot of extra "work" on the backend to deliver the penalties.

That seems to be a similar number of lines of code as what you're proposing here. Rewriting the penalty code handling frontend code isn't free.

use jackson to load a penalty json file to hydrate the penalty information into the format the JS understands. Made the definition object itself responsible for loading from json, which seems fitting based on the old-fashioned patterns in the Java in the application.

official-sounding · 2018-06-28T02:16:57Z

Okay, I've updated the approach here somewhat:

Penalty codes are delivered via WS, instead of ajax
Penalty codes are still stored in a json file, but are parsed on the backend and delivered directly as JS objects via WS (I initially did this on accident, actually surprised this worked as well as it did)
removed the regex parsing of penalty types, actually simplifying the front-end instead of just changing it.

This resolves the "different special dataflow" issues, and I think should also resolve security issues of blindly delivering a file to the front-end.

JSON is pretty finicky, in particular around trailing commas. I wouldn't presume this for non-technical users.
It doesn't have to be one line.

I agree JSON can be finicky, I'm still of the opinion that being able to edit a json file in an editor that has syntax highlighting is going to be easier than trying to edit it directly in the ruleset editor.

I think a JSON file is a good middle ground between no runtime penalties and a proper structured editor for penalty codes in the ruleset editor. I envision that as being a fair bit off though, based on what I saw in the current ruleset code. I am not prepared to add even multi-line configs to the current ruleset code/ruleset editor page without a lot more unit test coverage.

That sounds like a bug we should be fixing, team and skater names can contain hyphens.

This is a specific bug with verbal cues, hypens were used to separate different verbal cues under the same code.

…directly instead of text Assume all penalties are expellable, simplifies the font-end code without introducing additional delivery mechanisms

brian-brazil · 2018-07-01T14:31:00Z

.classpath

@@ -37,5 +37,8 @@
 	<classpathentry kind="lib" path="test-libs/junit-4.11.jar"/>
 	<classpathentry kind="lib" path="test-libs/hamcrest-core-1.3.jar"/>
 	<classpathentry kind="lib" path="test-libs/mockito-all-1.9.5.jar"/>
+	<classpathentry kind="lib" path="lib/deps/jackson-annotations-2.9.5.jar"/>


We should use only one json library, there's already another one 4 lines up.

I agree with this wholeheartedly, and my next project is to strip out org.json (both the seemingly unused vendored source code and the jar & uses in the scoreboard. org.json has a weird license and is much lower level than we should really be using.

That said, I think removing org.json is a bit of an undertaking that is outside of the scope of this PR. if you'd prefer me to rewrite PenaltyCodeManager to use org.json constructs for now I can, but I would rather not do that just to undo it in a few weeks.

Also, after doing a bit of reading (and realizing that I had added a 1.3MB dependency), I swapped jackson-databind for jackson-jr, that is much lighter weight but still gives us a much nicer org.json replacement.

It's not the best of libraries alright.

I would rather not do that just to undo it in a few weeks.

I'd hold off on that until I've finished removing the Game directory, as that's going to have json-y impacts. Does jackson-jr permit building up json by hand like the currently library does?

yes, jackson-jr provides a fluent api that works very similarly to how org.json manipulates json objects.

brian-brazil · 2018-07-01T14:31:15Z

config/penalties/wftda2016.json

+{
+	"penalties": [
+	{ "code": "B", "verbalCues": ["Back Block"]},
+{ "code": "A", "verbalCues": ["High Block"]},


Pretty print this please.

brian-brazil · 2018-07-01T14:32:36Z

src/com/carolinarollergirls/scoreboard/json/ScoreBoardJSONListener.java

+            PenaltiesDefinition penalties = pm.loadFromJSON();
+            for(PenaltyCode p : penalties.getPenalties()) {
+
+                updateMap.put("PenaltyCode.("+p.getCode()+")", p);


While you're changing all this, would you mind moving this under ScoreBoard.? That way a WS client will (eventually) be able to register Scoreboard. and get everything.

src/com/carolinarollergirls/scoreboard/json/ScoreBoardJSONListener.java

+
+        try {
+            PenaltiesDefinition penalties = pm.loadFromJSON();
+            for(PenaltyCode p : penalties.getPenalties()) {


brian-brazil · 2018-07-01T14:36:33Z

src/com/carolinarollergirls/scoreboard/json/ScoreBoardJSONListener.java

@@ -280,6 +281,25 @@ private void processPosition(String path, Position p, boolean remove) {
 		updateMap.put(path + "." + Position.EVENT_SKATER, p.getSkater() == null ? null : p.getSkater().getId());
 		updateMap.put(path + "." + Position.EVENT_PENALTY_BOX, p.getPenaltyBox());
 	}
+
+	private void processPenalties(Settings s) {


PenaltyCodes

This could be confused with the actual penalties.

brian-brazil · 2018-07-01T14:36:43Z

src/com/carolinarollergirls/scoreboard/penalties/PenaltiesManager.java

+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class PenaltiesManager {


PenaltyCode

brian-brazil · 2018-07-01T14:40:04Z

src/com/carolinarollergirls/scoreboard/json/ScoreBoardJSONListener.java

 		// Process Settings
 		processSettings("ScoreBoard", sb.getSettings());
+
+		processPenalties(sb.getSettings());


This code will only get called once at startup as far as I can see, can the codes actually be changed during a run/game?

official-sounding · 2018-07-02T03:45:38Z

fixed style/naming convention issues from code review, and added some code to actually correctly handle updating which penalty definition file is used while the scoreboard is running.

jackson-jr is a much lighter jar than jackson-jr, it also provides a fluent interface that is very similar to the org.json interface that the scoreboard uses to generate JSON elsewhere in the system. It will make it easier to strip out the org.json dependency in a later PR.

brian-brazil · 2018-07-02T13:05:41Z

src/com/carolinarollergirls/scoreboard/json/ScoreBoardJSONListener.java

+
+	private void processPenaltyCodes(Settings s) {
+		PenaltyCodesManager pm = new PenaltyCodesManager(s);
+		updateMap.put("ScoreBoard.PenaltyCode", null);


I don't think you can both clear a subtree like this and give it entries in one go, as the updateMap can be processes in any order as maps are unordered.

Since updateMap is a LinkedHashMap, the iteration order is stable in insertion order.

There's two problems with that. Firstly a entry could be nulled and later have a value put in, and the subtree wouldn't get cleared. Secondly we don't iterate over the updates in WS.updateState, we get the set of keys and iterate over that.

We'd need to change that code somehow to make this work correctly in general.

Firstly a entry could be nulled and later have a value put in, and the subtree wouldn't get cleared.

I agree on this in the general case, I think the subtree state deletion basically only works on accident. In this case though I'm never putting anything into the top-level key. I'd probably rather update WS to provide an explicit "removeState/deleteState" function to give us more explicit control over what is in the state object.

If you have an alternative approach in mind, we can break out that discussion to the FB or slack. Unless you think it's too big of a change for this PR, I can incorporate it with these changes.

Secondly we don't iterate over the updates in WS.updateState, we get the set of keys and iterate over that.

It's original-key-insertion-order specifically. That's definitely more nuanced than just insertion order, but since the updateMap is emptied after handing off to WS.updateState, that doesn't become a problem in practice all that much.

Pondering this more, this design as-is is just not safe in general. A hash map is the wrong data structure, as it loses ordering. This may not be an issue for a single deletion and then adding it back, however there's the notion of batching so you could have delete+add+delete+add which would end up with a mixture of the two adds rather than only the last add being present.

I don't believe existing code is affected, as skaters don't change attributes from update to update; and the set of policies is fixed. By contrast penalty codes can be added and removed.

Without getting into a full-transactional design, I think switching the updateMap to a list should do the trick. That way the updates always happen in order, and will still get squashed down before being sent out to clients.

brian-brazil · 2018-07-02T13:09:48Z

html/controls/pt/index.js

-			div.detach();
-			return;
+
+		if(code !== 'FO') {


Maybe we should do the FO code handling entirely in the frontend? It feels odd to list it as a penalty code.

…o WS Ensures ordering of scoreboard updates, even across batches. This update preserves the semantics of the state updates themselves (eg value of null = delete).

…m regular codes in a better way

official-sounding · 2018-07-04T18:06:59Z

Updated the updates to use a List instead of a Map, moved the FO stuff to the front-end, and made it slightly clearer whether the user is editing an expulsion code vs a regular code.

brian-brazil

This looks good, just 2 nits.

brian-brazil · 2018-07-04T19:57:49Z

src/com/carolinarollergirls/scoreboard/jetty/WS.java

 import com.carolinarollergirls.scoreboard.model.ScoreBoardModel;

 public class WS extends WebSocketServlet {
+	private static final long serialVersionUID = 1L;


This is unused

Not having it in a serializable class will generate a warning, even if we don't serialize.

We don't have this in any other class. Where are you seeing these warnings?

Also we shouldn't be using Java serialisation anywhere.

Eclipse gives the warning for 17 files that all implement java.io.Serializable (inherited through some other interface).

I don't think we should be cluttering the code due to false-positive warnings from one editor.

The regular javac used by ant also gives this warning (if you don't configure ant to hide all warnings).

I see no warnings from the default build, Java 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12. If there are such warnings we should disable them.

Setting nowarn to false and xlint:all in build.xml gives 99 warnings with javac 1.8.0_172
And I'm against disabling warnings. If these classes don't need to be serializable, don't implement that interface. But if you implement it, do it properly. (That being said: using 1 as UID is also improper. )

So it's not a warning, it's a lint. If we're going to enable those we should do so as a separate PR, not piecemeal. We should also make sure that any warnings are actually issues, and not false positives like this one.

Serialisation is a very old java feature that noone should be using these days, I don't think our code should suffer due to its legacy.

And looks like it's easy to disable that particular lint warning.

I'm not against useful warnings, but I also like reasonably self-contained changes.

brian-brazil · 2018-07-04T20:00:36Z

src/com/carolinarollergirls/scoreboard/jetty/WS.java

-						if (v != null)
-							state.put(k, new State(stateID, v));
-					} else if (!v.equals(s.value)) {
+					State s= state.get(update.getKey());


official-sounding force-pushed the bugfix/exp-penalty-codes-real branch 2 times, most recently from 5b26e4e to abe6805 Compare June 28, 2018 01:54

official-sounding force-pushed the bugfix/exp-penalty-codes-real branch from 98e6701 to 49fcd10 Compare June 28, 2018 02:13

return to approach 1: deliver penalty codes via WS, but send in JSON …

9fa32e3

…directly instead of text Assume all penalties are expellable, simplifies the font-end code without introducing additional delivery mechanisms

official-sounding force-pushed the bugfix/exp-penalty-codes-real branch from 49fcd10 to 9fa32e3 Compare June 28, 2018 02:20

brian-brazil reviewed Jul 1, 2018

View reviewed changes

official-sounding added 2 commits July 1, 2018 22:59

fixes from code review

dcd858c

official-sounding force-pushed the bugfix/exp-penalty-codes-real branch from fdbe919 to aba1092 Compare July 2, 2018 04:00

brian-brazil reviewed Jul 2, 2018

View reviewed changes

official-sounding added 3 commits July 4, 2018 12:34

convert from LinkedHashMap to LinkedList to pass scoreboard updates t…

9745934

…o WS Ensures ordering of scoreboard updates, even across batches. This update preserves the semantics of the state updates themselves (eg value of null = delete).

Move "FO" code to front-end only.

43ba0c4

changed presentation of expulsion codes slightly, so its distinct fro…

191936f

…m regular codes in a better way

brian-brazil reviewed Jul 4, 2018

View reviewed changes

code review fixes

5258ce1

brian-brazil merged commit d17d0a6 into rollerderby:dev Jul 5, 2018

official-sounding deleted the bugfix/exp-penalty-codes-real branch July 5, 2018 23:50

Add File-based Runtime Loaded peanlty file #83

Add File-based Runtime Loaded peanlty file #83

Conversation

official-sounding commented Jun 27, 2018 • edited Loading

frank-weinberg commented Jun 27, 2018

brian-brazil commented Jun 27, 2018

frank-weinberg commented Jun 27, 2018

brian-brazil commented Jun 27, 2018

frank-weinberg commented Jun 27, 2018

official-sounding commented Jun 27, 2018

brian-brazil commented Jun 27, 2018

official-sounding commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

official-sounding commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

official-sounding commented Jul 4, 2018

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

official-sounding commented Jun 27, 2018 •

edited

Loading