eclipse-sirius / sirius-emf-json

JSON-based EMF Resource implementation - part of Eclipse Sirius
https://eclipse.dev/sirius/sirius-web.html
Eclipse Public License 2.0
5 stars 10 forks source link

NPE when loading JSON resources with unknown types #13

Closed pcdavid closed 1 year ago

pcdavid commented 1 year ago

In org.eclipse.sirius.emfjson.utils.GsonEObjectDeserializer.loadObject(JsonObject, boolean):

    private EObject loadObject(JsonObject object, boolean isTopObject) {
        EObject eObject = null;

        JsonElement eClassJsonElement = object.get(IGsonConstants.ECLASS);
        if (!eClassJsonElement.isJsonNull()) {
            EClassifier eClassifier = this.getEClass(object, eClassJsonElement);
            eObject = this.deserializeEClassifier(eClassifier, object, isTopObject);
        }

        if (this.eObjectHandler != null) {
            this.eObjectHandler.processDeserializedContent(eObject, object);
        }

        JsonElement idJsonElement = object.get(IGsonConstants.ID);
        if (idJsonElement != null && this.resource != null) {
            this.resource.setID(eObject, idJsonElement.getAsString());
        }

        return eObject;
    }
  1. eObject is set to null, and then only changed if eClassJsonElement is not "json-null"
  2. when it is set, it is set to the result of this.deserializeEClassifier(), which can return null if the type name in the serialized version is not known/resolvabled at load time.

this.resource.setID(eObject, idJsonElement.getAsString()) is invoked without whecking it eObject has been set to a non-null value, which can lead to NPEs.

This can be relatively easy to trigger when trying to load a JSON resources which were serialized using a different versions of the metamodels used at load time, and depending on where it happens it can make the whole application (e.g. Sirius Web) unusable.

Objects serialized using types which are not known at load-time should probably just be ignored. Ideally the corresponding JonObject could be put in the resources's extended-metadata like EMF does so that the client code cand post-process them after load ("here are the things I ignored because I could not interpret them meaningfully").

pcdavid commented 1 year ago

It's easy to reproduce in Sirius Web:

  1. Create a new domain definition with an entity named A.
  2. In another project, create a document with an instance of A. Close the project with this instance.
  3. In the domain definition, rename A into B.
  4. To to reopen the project with the instance document previously created: the document does not load, the frontend displays a "Exception while fetching data (/editingContextEvent) : null" and the backend log shows a stack trace with the NPE:
java.lang.NullPointerException: null
    at org.eclipse.sirius.components.emf.services.EObjectIDManager.findAdapter(EObjectIDManager.java:108) ~[classes/:na]
    at org.eclipse.sirius.components.emf.services.EObjectIDManager.findId(EObjectIDManager.java:59) ~[classes/:na]
    at org.eclipse.sirius.emfjson.resource.JsonResourceImpl.setID(JsonResourceImpl.java:435) ~[org.eclipse.sirius.emfjson-2.3.1-SNAPSHOT.jar:na]
    at org.eclipse.sirius.emfjson.utils.GsonEObjectDeserializer.loadObject(GsonEObjectDeserializer.java:376) ~[org.eclipse.sirius.emfjson-2.3.1-SNAPSHOT.jar:na]
    at org.eclipse.sirius.emfjson.utils.GsonEObjectDeserializer.deserializeContent(GsonEObjectDeserializer.java:270) ~[org.eclipse.sirius.emfjson-2.3.1-SNAPSHOT.jar:na]
    at org.eclipse.sirius.emfjson.utils.GsonEObjectDeserializer.deserialize(GsonEObjectDeserializer.java:253) ~[org.eclipse.sirius.emfjson-2.3.1-SNAPSHOT.jar:na]
    at org.eclipse.sirius.emfjson.utils.GsonEObjectDeserializer.deserialize(GsonEObjectDeserializer.java:1) ~[org.eclipse.sirius.emfjson-2.3.1-SNAPSHOT.jar:na]
    at com.google.gson.internal.bind.TreeTypeAdapter.read(TreeTypeAdapter.java:69) ~[gson-2.9.0.jar:na]

Technically the NPE is in EObjectIDManager, but EMF JSON should not even try to invoke the id manager on null.

Note that if instead of crashing loadObject can return null, its callers must be updated to consider this new case.