search

eclipse-emfcloud / emfcloud-modelserver

Modelserver component
Other
43 stars 21 forks source link

Fix #174 API v2: Injection of JsonPatchHelper #175

Closed cdamus closed 2 years ago

cdamus commented 2 years ago

Inject the JsonPatchHelper also into the resource manager and transaction controller that previously constructed it for themselves.

This requires additionally:

So far, this is tied to json-v2 format exclusively as this is the default format for API v2. If the helper should need to pick up other formats from the request/socket context, then that's an orthogonal issue.

Contributed on behalf of STMicroelectronics.

cdamus commented 2 years ago

Commit d2c041d5a812e054846567b6823a06ab2f134fbf rebases the PR onto the latest state of the #151 branch, which has essentially completed its review.

cdamus commented 2 years ago

Commit d97e467422132c9400c779684d479dd5af02ab6b rebases the branch onto the latest from the master branch to resolve a merge conflict.

That conflict was in the JsonPatchHelper::getCurrentModel(...) method which on the master branch was changed to directly serialize the model using the object mapper from the JsonCodecV2 class. However, this class is substitutable by Guice injection with a codec that uses a different mapper, which the current branch originally accounted for via the CodecsManager registry. So, in resolving the conflict, I've introduced a new internal interface for codecs that does as direct as possible a serialization using that codec's mapper which we can expect to be available on all codecs that extend the DefaultJsonCodec (which in practice should be all of them).

cdamus commented 2 years ago

Should we maybe bind the JSONPatchHelper as part of the default bindings? Or is it already the case and I just missed it?

Guice does not permit self-bindings. As the JsonPatchHelper is a concrete class, it is creatable by default, so it is implicitly bound to itself by Guice. We could make it explicit with an incomplete binding like

   bind(JsonPatchHelper.class);

but I think there are other cases in this project where we decided not to do that (?).

eneufeld commented 2 years ago

Should we maybe bind the JSONPatchHelper as part of the default bindings? Or is it already the case and I just missed it?

Guice does not permit self-bindings. As the JsonPatchHelper is a concrete class, it is creatable by default, so it is implicitly bound to itself by Guice. We could make it explicit with an incomplete binding like

   bind(JsonPatchHelper.class);

but I think there are other cases in this project where we decided not to do that (?).

So if it is autobound I'm happy.

cdamus commented 2 years ago

Commit 9f38ae49488acd34aecce096beafff9a6988f5dd is rebased on the latest updates from the master branch for compatible extensibility refactorings.