Json Jackson dependency in GH core?

Developers
1

Issues like 1176 could be cleaner if we would be able to use json deserialization directly in the core module. IMO it is a Java limitation and a json lib will be sooner or later in the JDK.

I did this in 1112 too, to be able to deserialize json config used for the graph storage (ie. a serializable EncodingManager) instead of the storage properties.

What do you think about moving all json stuff into GH core, removing the reader-json module and making the jackson implementation the default but allow a simple replacement via a new constructor GraphHopper(GHJson) for Android or iOS or something?

1 Like
2

Seems json is needed more and more everywhere, still when possible prefer to not load core modules with third-party libs.
e.g. I try that in VTM where we use Jackson too.
So my thoughts are little shared. :slightly_smiling_face:

allow a simple replacement via a new constructor GraphHopper(GHJson) for Android or iOS

Do you mean GraphHopper will have a json interface to plug Jackson or Gson or …?
But mention also to have Jackson as dependency in core?

3

Do you mean GraphHopper will have a json interface to plug Jackson or Gson or …?

Yes, it already has a simple GHJson interface and jackson implementation. Gson is easy to do.

But mention also to have Jackson as dependency in core?

Yes, would be my preference at the moment. Not sure as one can it always make working but the design complexity might be not worth the trouble. It would be easy to exclude in Android and replace with Gson IMO

4

Generally I like the idea. We face too many issues with reading json files from the web module and injecting it in the core etc.

I would also vote for having a common interface, so that we can switch the implementation to a different library if we might see issues with Jackson in future.

Furthermore I would like to consider keeping the Json in it’s own module but having a dependency from the core to the json module. This would help a bit with keeping the json logic out of the core and maybe make it easier for others to switch to a different json client, without changing code of the core.

5

Do you think that the existing GHJson is sufficient?

Furthermore I would like to consider keeping the Json in it’s own module but having a dependency from the core to the json module

Switching to a different json implementation should be easy via excluding jackson even without such extra dependency as we hide GHJsonJackson. Or what could be the problems with that?

I would like to have it like a first class citizen and I hope they will at some point provide a json implementation in the JDK that we would then use, but we’ll probably have to maintain backward compatibility to jackson a long time.

6

Have implemented like I thought about it: https://github.com/graphhopper/graphhopper/compare/json_refactoring?expand=1

IMO this looks now cleaner: the spatial rule handling as well as the landmark splitting.

All tests pass except one alternative route and potential the android module might be broken, although I have implemented a simple AndroidJsonFactory. Maybe @devemux86 you can try it on Android if this makes problems or e.g. the jar is significantly larger due to some missing exclusions? Or maybe the exclusion trouble is not worth the effort as jackson can be used on Android too?

7

Looking quickly the code changes:

  • Thought that would be possible to not have any json dependency at all if only want just regular routing?
  • If that’s not possible, then going with default implementation (Jackson) could be preferred instead of replace with Gson. Or you did it only to demonstrate the option?
8

This question here is exactly about this: should we have a json dependency in GH core to simplify code :slight_smile: ?

And yes, the Gson shows the option, I thought it is more wide spread on Android, but it is easy to remove this of course and use jackson there too.

9

I meant more like the current exclusions on mobile - if that’s possible.
They are needed for specific GH works (e.g. graph creation) and not for routing which is the sample use.

10

At the moment this is possible, yes. But the plan is to use json for configuration and for EncodingManager (de)serialization and so we would need json stuff for normal routing too, not just for import or post-processing.

11

Avoiding the 2 jackson dependencies in the core is possible, but the price is that we would need two more modules IMO: one json module defining the GHJson interface and another json-jackson module with the implementation (GHJsonFactory, GHJsonJackson) that can use GHJson but is then used in core tests only and core sources use the GHJson interface. But I do not see why this is necessary at the moment. Also it will always require a constructor argument for GraphHopper. Another solution could be the Java service loading mechanism but as this is a library we should not use it and I do not like it much :wink:

12

I tested both ways on Android, the default Jackson and with Gson hook, all seem work ok.

BTW better there is some comment that Gson hook is not mandatory, but just an example.

13

Thanks for testing!

If both work I’ll probably drop gson from the PR

14

Yes, probably you are right :). I just thought it would be easier to switch out the dependencies it would be in a different project/module, but probably it doesn’t matter, as long as we provide an easy way to switch out the GHJson implementation.

Probably for now, if we need more we can always extend it. I think it would be nice to have our own annotations as well, but this is probably not easily possible with our own implementation and might break compatibility with other libraries.

15

Why should we do this?

16

I was just thinking it would be nice to have, since we use the annotations. It’s probably not worth the effort and might not even be easily possible. This was just a “nice to have thought”.