150: ModelServer API v2

[CamilleLetavernier]

Introduce a new version for the ModelServer API (V2). This version removes some of the EMFisms from the API:

• All Json Model Elements now consistently include $type and $id (In v1, we had "eClass" and no specific id attribute). References still use $ref.
• Edition can be done via EMF CCommands (as in V1) or Json Patch (New in V2). The patch or command needs to be wrapped in an object specifying the format used:

{
    "type": "modelserver.emfcommand"
    "data": { "...v1-command..." }
}

At the moment, supported types are "modelserver.emfcommand" (V1 CCommand) and "modelserver.jsonpatch" (V2 Json Patch).

• A new endpoint has been added to support transactions. This will complement (or even replace) Custom Commands. Clients may start a transaction, and then execute multiple sequential commands. When the transaction is closed, all subscribed clients will be notified.
• Subscription notifications will be sent as a Json Patch (Instead of EMF ChangeDescription) for V2 clients.

Contributed on behalf of STMicroelectronics.

[CamilleLetavernier]

Note: this is a draft PR. Some pieces are still missing (especially, we need to improve the support for Json Patch operations. Currently, only 'replace' is implemented).

The ModelServer should still support the V1 endpoints, but using both V1 and V2 endpoints at the same time (for different clients) may cause unexpected issues (Especially regarding Command execution/notifications).

[CamilleLetavernier]

The PR includes some library for manipulating Json Patch on the server. The library is json-patch (java-json-tools), and some dependencies. I had to repackage them, because even though the original library provides some OSGi metadata, they seem to be incomplete or invaliid (Especially, not all packages are exported, so we can't consume it).

The libraries are included in the lib folder, and are available under a dual license (LGLP v3 or Apache V2). So, license shouldn't be an issue (But we'll need to fill a CQ).

[CamilleLetavernier]

Added implementation & (integration) tests for Add (Create) and Remove operations. Move and Test are still unsupported.

[CamilleLetavernier]

The new commit attaches the Json Patch node to the result, after executing a Command/Patch operation. The "data" field is modified to support both a message and a patch.

V1 Result:

{
    "type": "success",
    "data": "Model 'file:/.../SuperBrewer3000.coffee' successfully updated"
}

V2 Result:

{
    "type": "success",
    "data": {
        "message": "Model 'file:/.../SuperBrewer3000.coffee' successfully updated",
        "patch": [
            {
                "op": "add",
                "path": "/workflows/-",
                "value": {
                    "$id": "//@workflows.7",
                    "name": "New WF"
                }
            }
        ]
    }
}

(Note: the same thing needs to be done for 'undo' and 'redo' endpoints; but at the moment these endpoints are shared between V1 and V2, so they'll need to be split)

[eneufeld]

I think this looks great thank you.
I have mainly comments about creating follow up issues and some possible generic improvements.

[eneufeld]

why not use the project version anymore?

[CamilleLetavernier]

Because of the introduction of external libraries, which are not versioned as "0.7.0", the target platform couldn't resolve. The alternative would be to override the target platform definition for these bundles that use a different version, but I think having a single target platform is more consistent (rather than assuming we'll have a target platform version per bundle version).

[eneufeld]

is this still the case with the move of libs to the lib folder?

[CamilleLetavernier]

No, this isn't required anymore. Although I still think this is better this way :) If we have a single target platform module, there's really no reason to introduce a variable version to it (The problem with ${project.version} is that it references the built project's version, and not the target platform version, so it only works if we assume that all modules use the same version).

[ndoschek]

Just as a small sidetrack here: For us it is in general easier if we stick to the variable when we want to publish a release (candidate) as we can then use our scripts to adapt the versions without having to check manually.

[CamilleLetavernier]

If the target platform itself is not published, you can achieve the same result by using a fixed version (There's no need to ever change the target platform version; you can set it to 1.0.0 or 0.0.1 and forget about it).

Alternatively, you can use an explicit version Property (instead of the predefined ${project.version}), to keep the Target Platform versioned. This way, the Target Platform can still inherit the version from the parent, but projects referencing the target platform may be versioned separately (while still using this shared target platform at a specific version).

The idea is really that the target platform should have its own version, rather than reuse the version from the project referencing it.

[eneufeld]

The PR includes some library for manipulating Json Patch on the server. The library is json-patch (java-json-tools), and some dependencies. I had to repackage them, because even though the original library provides some OSGi metadata, they seem to be incomplete or invaliid (Especially, not all packages are exported, so we can't consume it).

The libraries are included in the lib folder, and are available under a dual license (LGLP v3 or Apache V2). So, license shouldn't be an issue (But we'll need to fill a CQ).

We could use the maven support for target files, but we would need to get rid of the tpd files then ... . I think adding them to the lib folder is fine.

[ndoschek]

Thanks a lot for this valuable contribution! I have some rather general remarks/questions on the PR, please see below.

• Directory libs

  • Why is it necessary to store them in a separate directory? Could we include them in the existing bundles/org.eclipse.emfcloud.modelserver.lib?
  • If they should remain as separate projects, please update their project settings, project files and classpaths according to the README as they cause file changes on import.

• General remarks/questions

1. Please take a look at the new warnings in the workspace and remove them if possible
2. For newly added files, the header should only contain 2022 and not a year range. Also, some of those newly added files have a EclipseSource header and some a STMicroelectronics header. Should they be unified?
3. Why exactly do we need the targetplatform-dev.target? Would the additional dependencies hurt too much in the main targetplatform? However, it makes the development setup a little bit more complex imo, so if it needs to stay, we should at least mention that in the README.
4. Will this story include API documentation as well (@eneufeld/@sdirix)? In general, the current documentation should be improved anyhow. It would be great to offer API documentation for example via OpenAPI and V2 would be definitely a great starting point to do so.

[CamilleLetavernier]

Why is it necessary to store them in a separate directory? Could we include them in the existing bundles/org.eclipse.emfcloud.modelserver.lib?

I didn't realize we already had a lib bundle. The approach I took is probably closer to the typical Orbit approach (keeping the original names), but adding all libs to the same bundle would definitely work, too. And it would be more consistent with the current ModelServer approach.

For newly added files, the header should only contain 2022 and not a year range. Also, some of those newly added files have a EclipseSource header and some a STMicroelectronics header. Should they be unified?

Actually, these files are not "newly added", they're forked/duplicated from existing classes. So, I kept the original Copyright in these cases. I double-checked yesterday, and all copyrights should be consistent: new files have the ST-2022 Copyright; existing and forked files have the ES-xxxx-2022 Copyright.

Why exactly do we need the targetplatform-dev.target? Would the additional dependencies hurt too much in the main targetplatform? However, it makes the development setup a little bit more complex imo, so if it needs to stay, we should at least mention that in the README.

This looks like a mistake. I played with target platforms a bit during development, but the current -dev platform is definitely not necessary. I'll remove it.

Will this story include API documentation as well (@eneufeld/@sdirix)?

That definitely makes sense. However, I think it's still a bit early for that, as some APIs and use cases are not clearly defined yet (Especially URI handling, which Json Patches we support, how responses/notifications are sent, and how to handle multi-resources use cases). But we should probably start working on an initial version soon.

[eneufeld]

Looks good to me, I suggest to remove the draft flag.

[ndoschek]

Thanks again Camille and Christian for the updates! 🎉

Looks good to me so far, however I still have a few minor comments:

1. Some of the org.eclipse.emfcloud.modelserver.emf.tests are failing and they also produce one one warning in the workspace. The *.emf.tests are not executed in the p2 build currently (please see Allow EMF Tests to run in p2 build #167) but in Eclipse and for the m2 build they should be fixed. Could you please have another look? Edit 2021-02-18: This was a problem on my side and I couldn't reproduce it anymore.
2. The README should be updated before we merge this independently form the API documentation. It would be fine by me to create a section with the updated V2 API and put the V1 in a collapsible section or sth like that.

Will this story include API documentation as well?

That definitely makes sense. However, I think it's still a bit early for that, as some APIs and use cases are not clearly defined yet (Especially URI handling, which Json Patches we support, how responses/notifications are sent, and how to handle multi-resources use cases). But we should probably start working on an initial version soon.

That sounds reasonable - could you please create an follow-up issue for the documentation story?

[CamilleLetavernier]

I've merged the Log4J changes from master into the branch to solve conflicts (That was a big batch of conflicts).

I've also fixed the custom 'counter' endpoint example, which was only registered for the v2 API, and not for v1 anymore. Now, it's available on both versions. This adresses a bug reported on the client side (The custom example menus use v1)

[ndoschek]

Thanks a lot Camille for the quick fixes and the rebasing!
Special thanks for the great README update! 🥇
To me it looks good and ready to merge. Still I will add @martin-fleck-at as a final reviewer.

[martin-fleck-at]

In general the change looks good to me and from what I can see V1 still works as expected. So thank you very much for that great work which must not have been easy!

I tested it as far as I could with the v1 Theia client and the v1 Java client but I could not find any good way to test the v2 API. From the code point of view, it looks good but I do have a few comments and one question: How do we plan to proceed with V1? Some code suggests more a dual approach where both APIs can be used independent from each other but other classes mix their implementation together. I thought we could split the V1 vs V2 based on the server modules that are created, i.e., that there will be a V2 DefaultSessionController etc. which are bound in a V2 DefaultModelServerModule so that adopters can either choose that server module as their base or the V1 one. For instance, the current approach leads to the DefaultSessionController having a method called subscribeV2 and two maps that store the listeners based on the version. It just strikes me as a bit odd so I am curious, what was the reasoning behind this?

[martin-fleck-at]

Just from my point of view, UnexecutableCommand feels like the way to go but I am not sure. Maybe we can create a task to reference and track this? I don't think we want test support anyway but who knows what will happen in the future ;-)

[CamilleLetavernier]

I think Exception gives us more flexibility, to generate e.g. a more detailed client reply. "UnexecutableCommand" doesn't say anything about why it didn't work; an Exception can hold more valuable information (And we probably want to return a different result based on this exception: e.g. a specific error code/message, rather than "OK but nothing changed" or a generic "Can't execute. Try something else.")

[martin-fleck-at]

I agree that in general an exception may have more information but this exception really does not ;-) Also currently we do not even through an exception. @CamilleLetavernier Could you maybe add a task to track adding this feature? As otherwise this TODO just seems like we were too lazy to do it instead of consciously postponing the implementation.

[cdamus]

Thanks for your review, Martin! You raise several good questions.

I tested it as far as I could with the v1 Theia client and the v1 Java client but I could not find any good way to test the v2 API. From the code point of view, it looks good but I do have a few comments and one question: How do we plan to proceed with V1? Some code suggests more a dual approach where both APIs can be used independent from each other but other classes mix their implementation together. I thought we could split the V1 vs V2 based on the server modules that are created, i.e., that there will be a V2 DefaultSessionController etc. which are bound in a V2 DefaultModelServerModule so that adopters can either choose that server module as their base or the V1 one. For instance, the current approach leads to the DefaultSessionController having a method called subscribeV2 and two maps that store the listeners based on the version. It just strikes me as a bit odd so I am curious, what was the reasoning behind this?

In the short term, it is expected that applications will continue to use the v1 API if it works for them. Of course, early adopters of v2 can help to shake things out there, but for now it may still undergo some churn as use cases are fleshed out and gaps in the functionality filled. In the longer term, the intent is that the v1 API can be deprecated and eventually phased out.

On the matter of how the API split is realized in the server, there is so much commonality in the shape and behaviour of these two API versions that it seemed most practical to share the bulk of the implementation and distinguish the divergent behaviour where necessary as in this case in the DefaultSessionController. The server needs to continue to provide the API v1 endpoints, so the proposal is not to make applications choose one version or the other by selecting which Guice module to use. If it is anticipated that adopters will need to jettison the v1 API from their applications by such a mechanism now, then perhaps we can have separate modules that bind the existing services as they are to their respective routes, and also a third module that combines the bindings from both, which would be the default module. What do you think?

[cdamus]

Commit 9751305 adds changes to address specific review comments:

• remove unused code in the CommandUtil class
• remove incomplete and unsupported client API for transactions
• document the internal transaction endpoint in the readme

[martin-fleck-at]

@CamilleLetavernier @cdamus I had a final look at the changes and with clarifications you both gave and the additional "removal" of the transactions from the API, I am more than happy with this change. Thank you very much for your great work!

In the short term, it is expected that applications will continue to use the v1 API if it works for them. Of course, early adopters of v2 can help to shake things out there, but for now it may still undergo some churn as use cases are fleshed out and gaps in the functionality filled. In the longer term, the intent is that the v1 API can be deprecated and eventually phased out.
[...]

Thank you very much for the detailed reasoning behind this. In that sense, V2 is more of an extension of V1 than an actual standalone version which is fine for me. We can definitely leave the separation of versions in modules for another day - if ever - since V1 still works and will be deprecated sometime in the (near) future anyway. It just caught me by surprise since in my mental model it was more of an either/or situation.