[master] Json refactoring (step 1/2 - isolating Jackson from our code) by mathieucarbou · Pull Request #1145 · Terracotta-OSS/terracotta-platform

mathieucarbou · 2022-11-22T19:45:54Z

This refactoring is a first step required to switch to Gson, but also anyway a good refactoring to have in order to isolate Jackson from our code and reduce the direct dependencies we have on it.

Introduced Terracotta Json API (currently implemented by Jackson)
Isolate Jackson classes from Terracotta code:
- no direct imports anymore
- The use of Jackson specific mixins and such are isolated in a *.json package
Refactored Sanskrit to get rid of its coupling with Jackson classes
Added a Workaround in NomadChange json serialisation due to a bug in how Jackson is handling polymorphism serialisation

mathieucarbou · 2023-03-28T11:32:32Z

(rebased)

mobasherul · 2023-03-29T09:25:12Z

This refactoring is a first step required to switch to Gson, but also anyway a good refactoring to have in order to isolate Jackson from our code and reduce the direct dependencies we have on it.

Introduced Terracotta Json API (currently implemented by Jackson)

Isolate Jackson classes from Terracotta code:

no direct imports anymore

The use of Jackson specific mixins and such are isolated in a *.json package

Refactored Sanskrit to get rid of its coupling with Jackson classes

Added a Workaround in NomadChange json serialisation due to a bug in how Jackson is handling polymorphism serialisation

@mathieucarbou When moving to some other json library then mixin present in *.json package in all the affected modules will still need to be changed right ?

mathieucarbou · 2023-03-29T09:30:30Z

This refactoring is a first step required to switch to Gson, but also anyway a good refactoring to have in order to isolate Jackson from our code and reduce the direct dependencies we have on it.

Introduced Terracotta Json API (currently implemented by Jackson)

Isolate Jackson classes from Terracotta code:

no direct imports anymore

The use of Jackson specific mixins and such are isolated in a *.json package

Refactored Sanskrit to get rid of its coupling with Jackson classes

Added a Workaround in NomadChange json serialisation due to a bug in how Jackson is handling polymorphism serialisation

@mathieucarbou When moving to some other json library then mixin present in *.json package in all the affected modules will still need to be changed right ?

Yes! This PR is just isolating all the TC code from Jackson. Currently in platform all the Jackson imports are everywhere. TC code is tightly coupled with Jackson.

This PR (1/2) is a first step: it isolates jackson imports in:

the JsonFactory impl
in a Json.Module impl for each module requiring some custom mappings.

Then, when converting to Gson, only rewriting those impl will be required because all the TC code now depends on the new TC Json API I introduced here.

Next PR (2/2) will switch the Json Factory impl and these mpdules to Gson.

GaryWKeim · 2023-03-28T19:04:54Z

+  public String toString() {
+    return "ObjectMapperFactory{" +
+        "pretty=" + pretty +
+        ", modules=" + modules +


Forgot eol.

GaryWKeim · 2023-03-28T19:05:28Z

+    }
+    for (Json.Module module : modules) {
+      if (module instanceof JacksonModule) {
+        ((JacksonModule) module).configure(mapper);


style: can you move this in the for-loop above.

no... modules need to be registered before triggering the configuration because there are some dependencies to resolve ;-) I will add a comment.

GaryWKeim · 2023-03-28T19:54:41Z

+  }
+
+  @Override
+  public void removeKey(String key) {


I think this would be better named as clearValue.

Ahah! That's the catch ;-) It is really a remove. The main object is SanskritObjectImpl. This map is just a visitor collecting the different sanskrit operations. And the removal operation, so that is gets serialized correctly in json, needs to have a representation in the map so that it can be serialized. In sanskrit, a removal is mapped to a key associated to null.

this si completely different with the sanskritobjectimpl which holds the object graph and is mutated by sanskrit operations.

GaryWKeim · 2023-03-28T20:14:07Z

+      setLong(key, (Long) value);
+    } else if (value instanceof SanskritObject) {
+      setObject(key, (SanskritObject) value);
+    } else if (value == null || value instanceof Number) {


Cannot this block just be removed and handled by the final one.

GaryWKeim · 2023-03-28T21:23:10Z

+    private final T body;
+
+    @JsonCreator
+    public DiagnosticResponseMixin(@JsonProperty("body") T body,


Is it OK to have an empty body? That is, should it be required?

Yes, totally. Jackson mixing are just a way to apply Jackson annotations to model classes without touching them.
Model classes will remain as-is. But All this Jackson modules containing mixing and serializers will be converted to Gson mappers at the end.

Here, what is important are the annotations, that are applied to the real object thanks to the mixin declaration above.

GaryWKeim · 2023-03-28T22:20:01Z

+  }
+
+  @Override
+  public <T> T map(SanskritObject src, Class<T> dest, String version) throws SanskritException {


Unused argument version.

No it is used ;-) In the real impl in the persistence module.

GaryWKeim · 2023-03-29T00:12:46Z

        .replaceAll("\"buildId\":\"[^\"]*\"", "\"buildId\":\"Build ID\"")
        .replaceAll("\"version\":\"[^\"]*\"", "\"version\":\"<version>\"")
-        .replaceAll("\"clientId\":\"[0-9]+@[^:]*:([^:]*):[^\"]*\"", "\"clientId\":\"[email protected]:$1:<uuid>\"")
+        .replaceAll("\"clientId\":\"[0-9]+@[^:]*:([^:]*):[^\"]*\"", "\"clientId\": \"[email protected]:$1:<uuid>\"")


I don't see any other spaces-after-semi colons in here. Was this purposeful?

weird! I don't know how it happened...

GaryWKeim · 2023-03-29T00:47:22Z

    try {
-      return mapper.readTree(new File(AbstractTest.class.getResource("/" + file).toURI()));
-    } catch (Exception e) {
+      return mapper.parse(new File(AbstractTest.class.getResource("/" + file).toURI()));


I guess I wonder why not use the parse(URL) version?

I just replaced the method calls ;-) not did a full replace ;-)

GaryWKeim · 2023-03-29T00:53:54Z

+  }
+
+  @Override
+  public <T> T map(SanskritObject src, Class<T> dest, String version) throws SanskritException {


Unused argument version.

no, it is really used ;-)

GaryWKeim · 2023-03-29T01:14:11Z

+/**
+ * @author Mathieu Carbou
+ */
+public class TestModule extends SimpleModule implements Json.Module {


Is it possible to consolidate the various TestModules and TestData?

No: TestData are the pojo classes used for testing.

TestModule are the mixin annotations telling the json framework how to serialise and deserialise the objects... So first one is completely json unrelated (normal java classes) and the json module is currently implemented with jackson, but soon wil be implemented with gson.

The TestData classes won't be touched for the Jackson => Gson switch

(there is also no common place for these testing classes)

mathieucarbou · 2023-04-04T14:17:48Z

(rebased)

* Isolate Jackson classes from Terracotta code * Introduced Terracotta Json API (currently implemented by Jackson) * Refactored Sanskrit to get rid of its coupling with Jackson classes

mathieucarbou self-assigned this Nov 22, 2022

mathieucarbou changed the base branch from master to release/5.9 November 22, 2022 19:46

mathieucarbou marked this pull request as ready for review December 15, 2022 09:45

mathieucarbou marked this pull request as draft January 26, 2023 15:32

mathieucarbou changed the base branch from release/5.9 to master March 27, 2023 15:01

mathieucarbou changed the title ~~[draft] Json refactoring~~ [master] Json refactoring Mar 27, 2023

mathieucarbou marked this pull request as ready for review March 28, 2023 11:16

mathieucarbou requested review from GaryWKeim, chrisdennis, mobasherul and tmesic99 March 28, 2023 11:18

mathieucarbou changed the title ~~[master] Json refactoring~~ [master] Json refactoring (step 1 - isolating Jackson from our code) Mar 28, 2023

mathieucarbou changed the title ~~[master] Json refactoring (step 1 - isolating Jackson from our code)~~ [master] Json refactoring (step 1/2 - isolating Jackson from our code) Mar 28, 2023

GaryWKeim reviewed Mar 29, 2023

View reviewed changes

mathieucarbou requested a review from GaryWKeim March 29, 2023 19:43

mobasherul reviewed Mar 30, 2023

View reviewed changes

Comment thread ...on/sanskrit/src/test/java/org/terracotta/persistence/sanskrit/AddLongSanskritChangeTest.java

mathieucarbou requested a review from mobasherul March 30, 2023 07:03

mobasherul reviewed Mar 30, 2023

View reviewed changes

Comment thread common/sanskrit/src/main/java/org/terracotta/persistence/sanskrit/SanskritObject.java

mobasherul approved these changes Mar 30, 2023

View reviewed changes

GaryWKeim approved these changes Apr 1, 2023

View reviewed changes

Json refactoring

3c3ea2b

* Isolate Jackson classes from Terracotta code * Introduced Terracotta Json API (currently implemented by Jackson) * Refactored Sanskrit to get rid of its coupling with Jackson classes

mathieucarbou merged commit 6656468 into Terracotta-OSS:master Apr 25, 2023

mathieucarbou deleted the json branch April 25, 2023 16:20

mathieucarbou mentioned this pull request Apr 25, 2023

Upgrade to platform 5.10.12 ehcache/ehcache3#3156

Merged

Conversation

mathieucarbou commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathieucarbou commented Mar 28, 2023

Uh oh!

mobasherul commented Mar 29, 2023

Uh oh!

mathieucarbou commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathieucarbou Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathieucarbou Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mathieucarbou commented Apr 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mathieucarbou commented Nov 22, 2022 •

edited

Loading

mathieucarbou commented Mar 29, 2023 •

edited

Loading

mathieucarbou Mar 29, 2023 •

edited

Loading

mathieucarbou Mar 29, 2023 •

edited

Loading