maptool
maptool copied to clipboard
RPTools
[Feature]: move from hessian to protobuf
Feature Request
To address security concerns of hessian serialisation and to allow easier interoperability with other tools like apps for initiative tracking or similar stuff it would be nice to move from hessian to protobuf.
A nice site effect is, that we would have a defined representation of the data. So we could store the protobuf objects on disk or in a DB an get rid of java serialisation as on disk format later.
The Solution you'd like
Remove everything hessian related and replace it with protobuf.
Alternatives that you've considered.
No response
Additional Context
No response
Messages to test:
- [x] AddTopology
- [x] BootPlayer
- [x] BringTokensToFront
- [x] ChangeZoneDisplayName
- [x] ClearAllDrawings
- [x] ClearExposedArea
- [x] Draw
- [x] EditToken
- [x] EnforceNotification
- [x] EnforceZone
- [x] EnforceZoneView
- [ ] ExecFunction
- [ ] ExecLink
- [x] ExposeFow
- [ ] ExposePcArea (unused)
- [x] GetAsset
- [ ] GetZone (unused)
- [x] Heartbeat
- [x] HideFow
- [x] HidePointer
- [x] Message
- [x] MovePointer
- [x] PlayerConnected
- [x] PlayerDisconnected
- [x] PutAsset
- [x] PutLabel
- [x] PutToken
- [x] PutZone
- [ ] RemoveAsset (unused)
- [x] RemoveLabel
- [x] RemoveToken
- [x] RemoveTokens
- [x] RemoveTopology
- [x] RemoveZone
- [x] RenameZone
- [x] RestoreZoneView
- [x] SendTokensToBack
- [x] SetBoard
- [x] SetCampaign
- [x] SetCampaignName
- [ ] SetFow (unused)
- [x] SetLiveTypingLabel
- [ ] SetTokenLocation (unused and seems to be remains of some external app)
- [x] SetServerPolicy
- [x] SetVisionType
- [x] SetZoneGridSize
- [x] SetZoneHasFow
- [x] SetZoneVisibility
- [x] ShowPointer
- [x] StartAssetTransfer
- [x] StartTokenMove
- [x] StopTokenMove
- [x] ToggleTokenMoveWaypoint
- [x] UndoDraw
- [x] UpdateAssetTransfer
- [x] UpdateCampaign
- [x] UpdateCampaignMacros
- [x] UpdateDrawing
- [x] UpdateExposedAreaMeta
- [x] UpdateGmMacros
- [x] UpdateInitiative
- [x] UpdateTokenInitiative
- [x] UpdateTokenMove
- [x] UpdateTokenProperty Those used protobuf already so don't need prio testing.
- [x] RemoveAddOnLibrary
- [ ] RemoveAllAddOnLibraries
- [x] AddAddOnLibrary
- [ ] UpdateDataStore
- [x] UpdateData
- [x] UpdateDataNamespace
- [ ] RemoveDataStore
- [ ] RemoveDataNamespace
- [x] RemoveData
Can no longer load existing campaigns into MapTool with current code.
Attempting to run [r: setPC()], [r: setNPC()] and similar macro functions ends up with a stack overflow as the recursive call here can never get out:
https://github.com/RPTools/maptool/blob/60a2de63eec8b7c2d017091806dd6e435879dbff/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java#L648-L650
Can no longer load existing campaigns into MapTool with current code.
I also ran into this. Lots of NullPointerExecptions when converting to DTO. There are a fair number of of object fields that aren't handled in readResolve() and can become unexpectedly null as a result. I dug through each exception I encountered and hacked in null checks / readResolve() extensions as necessary and was able to resolve this problem. I can clean that up and put up a fix, at least for whatever I've found. We'll need to do a pretty detailed search through the model classes to find any potentially unexpected null fields and add readResolve().
Some of the problem fields do have default values but those fields are being overwritten when deserialized. I don't know yet if that's because the XML explicitly contains null values for these fields or because fields are forced to null when missing. If it's the latter, it would be great if there were a way to change the deserializer to use the default value when a field is missing, as that would make a lot of this readResolve() logic superfluous.
There was one weird situation I ran into: Zone.exposedAreaMeta was being handled properly, except that somehow one of my maps has a null key in there. How it got there I don't know but it shouldn't be possible nor should it be possible to ever read that value back out (no tokens should have null exposed area IDs). We may just be able to skip such keys during serialization, which is what my temporary solution is doing right now. (Side note: while going down this rabbit hole I found that there are some places that cause token GUIDs to be added to this map even though it should only contain "exposed area" GUIDs. This may or may not be related to the null key issue, but either way if I can find a way to manifest this as an actual bug I'll open a bug report for it)
Attempting to run [r: setPC()], [r: setNPC()] and similar macro functions ends up with a stack overflow as the recursive call here can never get out:
I can include a fix for this as well.
I did some research into null values vs xstream. Apparently xstream isn't setting these missing fields to null, it instead just skips object construction altogether. Not only does this mean no constructors are called, neither is the object initializer (that surprised me). As a result, all fields are set by Java to their defaults according to type (so null for object types, 0 for numeric, false for boolean). This even applies to final fields, which makes it tricky to use such fields properly. Also it looks like Java does not provide any way (not even a hacky way) to run an object initializer without going through a constructor, so there's not really much to be done about that for the current strategy.
The above describes the default "reflection provider". Xstream has other reflection providers with different behaviour. E.g., there is the PureJavaReflectionProvider that initializes objects by going through a default constructor before setting any values. But there are some constraints on what xstream can do with this strategy. E.g., it can't do non-static inner classes or classes without default cosntructors. Not all of our model types fulfill these constraints, though they could probably be made to if we wanted to go down that rabbit hole. It could reduce the amount of readResolve() we need, which could helpful in general, not just with the current issues of null fields. But there may also be other downsides I haven't grasped yet.
I'm a bit confused how the null values got into the campaign file and how this is related to the protobuf change. EDIT: The problem is probably that I didn't load campaign but only created a new on and tested with this.
The fromDto methods that I added for protobuf serialisation call the constuctors properly. We can add further initialisation there if needed.
The long term plan was to move away from xstream and use protobuf DTOs converted to Json and store them instead of the xstream xml. Maybe even use some nonsql db to store them.
I figured that's what had happened as no previously created campaigns would load.
The fromDto methods that I added for protobuf serialisation call the constuctors properly. We can add further initialisation there if needed.
The fromDto side of things looks fine - it's the toDTO side that has trouble since it currently has to handle model objects that have been deserialized via xstream (and therefore have not had their constructors called). But you're also right in that this isn't really an issue with the protobuf change. It's just that the change exposes (and breaks due to) a fairly pervasive issue in our model objects where we do not account for the peculiarities of xstream.
Note to myself: test if some bigger campaigns can be opened without error.
During testing of #3274 I discovered that the protobuf code can throw this exception for HeroLab tokens:
I ran into some NPEs when loading old campaigns related to this. I'll submit a PR.
I also encountered a few campaigns that threw this when trying to load (originating from Path.java):
java.lang.ClassCastException: class net.rptools.maptool.client.walker.astar.AStarCellPoint cannot be cast to class net.rptools.maptool.model.AbstractPoint (net.rptools.maptool.client.walker.astar.AStarCellPoint and net.rptools.maptool.model.AbstractPoint are in unnamed module of loader 'app') at java.lang.Iterable.forEach(Iterable.java:75) ~[?:?]
Edit: Decided to look into this exception a bit more and have something now. Turns out really old tokens can actually have a lastPath path made up of AStarCellPoint nodes, even though that is not possible or supported now.
Any future issues with old campaigns can go on other issues. Closing for 1.12 release.