Validation Markers 'Could not retrieve element with id' (glsp-node-server/glsp-vscode-integration)

[jmenzies12]

Hey there,

We've implemented live validation based on AbstractModelValidator.

Running into an issue where if we edit a diagram where one or more nodes have validation markers, and then make some combination of editing/saving/undoing in the diagram we get a 'Could not retrieve element with id' error.

It took me a while but because the IDs are not 32 character hashes like we use for the rest of the diagram elements I realized that it seems to be the validation marker element that is failing to be retrieved, when the ComputedBoundsHandler is triggered.

These elements are never added to the GModelIndex, which makes sense as they are created by the validation action and then added on as feedback. I'm assuming they usually get cleared at some point and should never make it as far as this.

Would you be able to clarify how this is supposed to work by default? We've done a bunch of changes to loading and saving to work with our integration between GLSP and Langium so it's very likely we've done something that has resulted in this issue.

I'm also wondering if it's possible to just add the IssueMarkers from the GModelFactory (and it still display in the same way)? Our validations come from the Langium side anyway and we refresh the diagram when the document changes, so it might make sense for us to remove these from the live validation altogether.

Cheers!
Jonathan

[tortmayr]

@martin-fleck-at Could you please have a look at this?

[martin-fleck-at]

I will look at this maybe tomorrow, latest on Wednesday.

[martin-fleck-at]

Hi Jonathan,

Before I go into any detail, a few questions, because I am not yet sure that what I found is really what you are running into.

1. Which version of @eclipse-glsp/client are you using?
2. Does an export happen anywhere in that flow, maybe triggered automatically rather than by the user?
3. Do you see any error in the webview developer console around the time the diagram breaks, in particular one coming from a bounds or layout pass?
4. In your Langium refresh path, do you build a RequestBoundsAction yourself, or forward a ComputedBoundsAction to the server from your own code?

The reason I am asking is the following. Validation markers are pure client-side feedback. A SetMarkersAction is handled by the SetMarkersActionHandler, which registers an ApplyMarkersAction as feedback, and the ApplyMarkersCommand then adds a GIssueMarker as a child of the marked element. Since it is registered feedback, it is re-established after every model update, so the server never learns about it. We also never assign an id to that marker, so sprotty's ModelIndexImpl.add generates a random 8-character one when it is added to the client-side model, which is probably why those ids looked so unfamiliar to you. One small correction to your assumption though, the markers actually are part of the client-side index, they are just never part of the server-side GModelIndex.

A GIssueMarker is also a GDecoration and therefore has the boundsFeature, so the hidden bounds updater measures it like any other element and puts its id into the ComputedBoundsAction. That is usually harmless, because for a server-triggered RequestBoundsAction the hidden model is created from the schema the server just sent and therefore has no markers in it, while a client-triggered LocalRequestBoundsAction is marked as a local action and is never forwarded to the server.

So as far as I can tell, a marker id should only be able to reach the server if bounds data from one hidden rendering leaks into the next one, which is something we fixed only recently in eclipse-glsp/glsp-client#525 for #1717. Until that fix, the collected bounds were only cleared on the path that actually sends them, so anything that skipped that path left them behind for the next run. There are two ways that can happen. The first one is a hidden rendering that is not a bounds request at all, which today means an export, and that is the case the issue was originally about. The second one is a bounds pass that fails somewhere in between, and that one is more interesting for you, because a LocalRequestBoundsAction renders the live client model with the markers in it. Those local requests happen quite often during normal editing, for example on every resize through the change bounds tool or through the SetBoundsFeedbackCommand, so if one of them fails, the marker bounds stay behind and are sent along with the next server request. I put both cases into a quick test against the version before the fix and could reproduce the leaked marker id in both, so I would think this is a good candidate for what you are seeing, although I may of course be wrong about the trigger. Markers are a particularly good victim here, because unlike a deleted element their ids are never known to the server at all, so every following bounds request fails rather than just one.

If you are on 2.7.0 or older, the quickest check is probably to try 2.8.0-next and see whether the error disappears. If you cannot move versions right now, you should be able to backport the cleanup locally by rebinding the hidden bounds updater.

@injectable()
export class MyHiddenBoundsUpdater extends GLSPHiddenBoundsUpdater {
    override postUpdate(cause?: Action): void {
        try {
            super.postUpdate(cause);
        } finally {
            // backport of https://github.com/eclipse-glsp/glsp-client/pull/525
            this.getElement2BoundsData().clear();
            this.element2route = [];
            this.root = undefined;
        }
    }
}

// in your diagram module
rebind(GLSPHiddenBoundsUpdater).to(MyHiddenBoundsUpdater).inSingletonScope();

Independently of that leak, it probably makes sense for you to keep the marker bounds away from the server entirely, since the server has no use for them anyway.

@injectable()
export class MyHiddenBoundsUpdater extends GLSPHiddenBoundsUpdater {
    override postUpdate(cause?: Action): void {
        if (ServerAction.is(cause)) {
            const data = this.getElement2BoundsData();
            data.forEach((_boundsData, element) => element instanceof GIssueMarker && data.delete(element));
        }
        super.postUpdate(cause);
    }
}

Local bounds requests stay untouched that way, so the marker placement keeps working as before. I think something along these lines is probably what we should do in the framework itself.

On your last question, adding the markers in the GModelFactory should work as well, and for your setup that may actually be the nicer option. There is a GIssueMarker in @eclipse-glsp/graph that uses the same marker type as the client-side one.

GNode.builder()
    .id(nodeId)
    .addChildren(GIssueMarker.builder().id(`${nodeId}_marker`).addIssue('Task name must be unique', 'error').build())
    .build();

The client already registers that type with the GIssueMarkerView, the GlspDecorationPlacer takes care of the placement so you do not need to provide a position, and marker navigation should keep working since it simply collects the GIssueMarker instances from the model. And because the marker is then part of the server index, the error you are seeing cannot really happen anymore. Two things to keep in mind, the markers would no longer show up in the Problems view, since that goes through the ExternalMarkerManager which is only triggered by a SetMarkersAction, and if you keep live validation running in parallel, getOrCreateGIssueMarker would reuse the marker from the factory and add the same issues a second time, so I would disable one of the two paths.

Depending on what you find out, I would probably like to open an issue to filter client-only decorations from the bounds we send to the server and to make applyBounds a bit more tolerant against unknown ids, because a single stale id should not bring down a whole model update.

Cheers,
Martin