search

eclipse-sprotty / sprotty-server

Server implementation for the Sprotty diagramming framework
https://eclipse.org/sprotty
Eclipse Public License 2.0
23 stars 19 forks source link

Fixed problem with parsing unknown properties #77

Closed spoenemann closed 3 years ago

spoenemann commented 3 years ago

I added a test that reveals a bug: without the fix to PropertyBasedTypeAdapter, parsing JSON with unknown properties leads to an IllegalStateException.

gitpod-io[bot] commented 3 years ago

spoenemann commented 3 years ago

The error was an IllegalStateException in the Gson parser.

Logging unknown properties could lead to a lot of possibly irrelevant log entries in some cases. I wouldn't do it for now.

JanKoehnlein commented 3 years ago

What do you think are the irrelevant cases? Doesn't an unknown property mean that client and server are out of sync? If it is a new property that came in by the an update of Sprotty (client) framework, it usually makes a lot of sense to also bump up the server to the corresponding version, so I'd say that's relevant.

spoenemann commented 3 years ago

Irrelevant cases are properties that are not used on the receiving side. I stumbled on this error while trying to use PropertyBasedTypeAdapter to parse an SModel in a separate process encapsulating the ELK layout engine, where the domain-specific SModel properties are not necessary.

Even if we add a log message there, it won't catch all such cases because this type adapter is used only for certain JSON classes. I don't know whether Gson's generic parser can report unknown properties, we could check.

Regarding bumping up the server to match the client version: we could introduce a mechanism to exchange some protocol version to check for compatibility. Changes in the protocol do not necessarily involve new properties.