Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddCommand created with constructor can be used only as root #108

Open
vhemery opened this issue Jun 8, 2022 · 1 comment
Open

AddCommand created with constructor can be used only as root #108

vhemery opened this issue Jun 8, 2022 · 1 comment
Labels
bug Something isn't working modelserver EMF.cloud Model Server Theia

Comments

@vhemery
Copy link
Contributor

vhemery commented Jun 8, 2022

  1. Create an AddCommand with one of the constructors of class
    export class AddCommand extends ModelServerCommand {
  2. Then, try and insert it in a CompoundCommand with constructor of
    export class CompoundCommand extends ModelServerCommand {
  3. When the command is sent to the model server, the model server will try and fail at resolving the references for #objectValues in method org.eclipse.emfcloud.jackson.databind.deser.ReferenceEntry.Base.resolve(DatabindContext, URIHandler).

The cause is that the following line :

(o, i) => new ModelServerReferenceDescription(o.eClass, `//@objectsToAdd.${i}`)

assigns the #objectValues, assuming that the command is the resource's root, with an absolute URI.

In my particular use case, where command was at index 1 (2nd), post-treating the references like this did the trick :

addCommand.objectValues?.forEach(v => {
     v.$ref = v.$ref.replace('//@objectsToAdd.', '//@commands.1/@objectsToAdd.');
});

but this fix is too specific to my use case.

In the general case, the AddCommand constructor can't possibly know where the command will be located in the resource. So I'm not sure what the best fix would be.
Possibilities I can think of :

  • Force the user to indicate where the command will be located, so that we directly insert the correct model reference.
  • Patching references automatically :
    1. leave the constructor as is, with a big shouting warning that the reference may be incorrect
    2. when constructing the CompoundCommand, patching all the model references in contained commands to prepend with the command uri (and also a big shouting warning)
  • A kind of mix, with the possibility to use relative uri fragments or placeholders, which will be patched later...
    Not sure which approach is the best.
@vhemery vhemery added bug Something isn't working modelserver EMF.cloud Model Server Theia labels Jun 8, 2022
@vhemery vhemery changed the title AddCommand created with constructor can not be used only as root AddCommand created with constructor can be used only as root Jun 8, 2022
@CamilleLetavernier
Copy link
Member

Hi Vincent,

The fact that Typescript applications don't know much about the concept of Resources, URI and EMF in general is one of the reasons why, in V2, we're focusing more on Json and less on EMF, for Typescript client applications.

With V2, using Json Patches, you wouldn't have this kind of issue: all paths are computed based on their position in the Json Tree (Or, alternatively, based on their EMF URI Fragment, as specified by the Model Server - no guess work involved)

V2 is still not 100% ready for adoption (Especially, there are still a few inconsistencies related to the differences between Json and EMF, or how Json Patches are defined compared to EMF Commands).

For commands that need to reference newly created objects, we now rely on the concept of Transactions, so we never have to reference an object that doesn't exist yet (We first execute the initial command to create the object, then we create and execute the command that needs to reference the new element - all encapsulated in a single Transaction). However, at the moment, Transactions are only supported through the modelserver-node Facade (The Transactions are implemented in the Java Model Server, but this is an internal endpoint that isn't officially part of the API for now).

I'm not sure what the exact plan for XMI/EMF Commands in Typescript Applications will be, but we might start deprecating them (at least when V2 is stable enough).

So this would lean towards this "solution", I guess:

leave the constructor as is, with a big shouting warning that the reference may be incorrect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working modelserver EMF.cloud Model Server Theia
Projects
None yet
Development

No branches or pull requests

2 participants