support neo4j driver for jooby #796
Conversation
jooby-neo4j/README.md
Outdated
| @@ -0,0 +1,175 @@ | |||
| # neo4j driver | |||
jooby-neo4j/README.md
Outdated
| <dependency> | ||
| <groupId>org.jooby</groupId> | ||
| <artifactId>jooby-neo4j</artifactId> | ||
| <version>1.1.1</version> |
jooby-neo4j/README.md
Outdated
| <dependency> | ||
| <groupId>org.jooby</groupId> | ||
| <artifactId>jooby-neo4j</artifactId> | ||
| <version>1.1.1</version> |
|
|
||
| @Override | ||
| public void configure(Env env, Config config, Binder binder) throws Throwable { | ||
| configure(env, config, binder, (remoteClient) -> {}); |
There was a problem hiding this comment.
why consumer callback? We don't need it.
Merge the two configure methods
|
|
||
| @Override | ||
| public Config config() { | ||
| return ConfigFactory.parseResources(Neo4j.class, "org.jooby.neo4j/neo4j.conf"); |
There was a problem hiding this comment.
does it work? It should be: ConfigFactory.parseResources(Neo4j.class, "neo4j.conf");
| properties.put(DBProperties.ARRAY_BLOCK_SIZE, config.getString("arrayBlockSize")); | ||
| } | ||
| properties.put(DBProperties.DATABASE_DIR, | ||
| config.hasPath("databaseDir") ? config.getString("databaseDir"): System.getProperty("java.io.tmpdir")); |
There was a problem hiding this comment.
remove: System.getProperty("java.io.tmpdir") with config.getString("application.tmpdir")
| Properties properties = new Properties(); | ||
|
|
||
| properties.put(DBProperties.SERVER_ROOT_URI, config.getString("uri")); | ||
| if (config.hasPath("arrayBlockSize")) { |
There was a problem hiding this comment.
It is better to dump all the properties from config into the properties object. It simplifies the code (no ifs) but also it is more flexible
| Config $neo4j = config.getConfig("neo4j"); | ||
|
|
||
| if (config.hasPath("neo4j." + db)) { | ||
| $neo4j = config.getConfig("neo4j." + db);//.withFallback($neo4j); |
There was a problem hiding this comment.
we usually fallback to default configuration, why did you comment the fallback?
| } | ||
|
|
||
| private static Pair<GraphDatabaseService, GraphAwareRuntime> getOrCreateGraph(String databaseDir) { | ||
| if (dbService == null && graphRuntime == null) { |
There was a problem hiding this comment.
Feel we should export dbService and graphRuntime from Ne4j module. For example who/when all these services go down?
This two services should be exposes by Neo4j and injected at construction time here in Neo4jSessionStore
There was a problem hiding this comment.
Hi @jknack , The dbService and graphRuntime cannot be exposed from Neo4j module because neo4j module should be used to access a remote neo4j db while the Neo4j session store creates a local neo4j db & tries to access it using the dbService and graphRuntime objects.
The test cases demonstrates how they should be used.
There was a problem hiding this comment.
Got it, but won't be useful too:
- Add support for local db too Neo4j
- Add support for remote db to Neo4jSessionStore
We can probably merge this and then add that in a new pull request.
There was a problem hiding this comment.
sure, I'll add support for them in a new PR.
There was a problem hiding this comment.
Well kind of, we still need to figure it out how to shutdown the local instance created by Neo4jSessionStore
There was a problem hiding this comment.
Right. Something similar to that, but still we need to manage the lifecycle of those objects (like ehcache module does).
Will be nice to have something like:
db = fs
Or local and then if we do:
{
use(new Neo4j());
}
The module exposes those services...but don't like is that module will exposes completely different services BoltDBAccess vs DbService/GraphRuntime :S
It is probably better to write a LocalNeo4j? or EmbeddedNeo4j module?
and add a note to Ne4jSessioStore that depends on EmbeddedNeo4j module.
Thoughts?
There was a problem hiding this comment.
I think yes. Lets first consider the embedded neo4j..I'll change the neo4j module to provide the
dbService & graphRuntime objects.
Plz let me know if you agree..
There was a problem hiding this comment.
Let;s create a new module: EmbeddedNeo4j and leave the existing Neo4j module as it is now.
There was a problem hiding this comment.
Ok..I'll create a new PR for the EmbeddedNeo4j module & leave this PR untocuhed..is it ok for you?
|
Hi @jknack , Thanks for your review & comments. I have fixed most of them. |
|
I changed the project structure and your pull can't be merge it, sorry for that. But don't worry going to pick your commit and merge them my self later (unless you want to do it :) |
|
Hi @jknack , I can make the changes myself..however, do you want me to merge my 2 PRs into 1 module? |
|
That will be awesome! |
|
Hi @jknack , I have merged the modules & updated the PR. Please let me know if this PR can be merged now.. |
This PR is created to propose the addition of
neo4jgraph database driver for jooby.The PR contains the implementation of
The test cases & documentation is also included with this PR.