Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
@greptile-apps anything else? |
|
The main critical issues have been covered — the two static field bugs are blocking and must be fixed before merge. A few additional notes: 1. No tests included — The PR adds JUnit/Mockito dependencies in 2. Resource cleanup — In try (Response response = httpClient.newCall(request).execute()) {
ResponseBody body = response.body();
// ... null check and read bytes
}3. Thread safety — Beyond the static field bugs, once those are fixed to instance fields, the driver should be thread-safe (OkHttp's client is thread-safe), but this hasn't been documented or tested. Bottom line: Fix the two critical static field bugs first — they break multi-driver usage entirely. The other issues are important but secondary. |
There was a problem hiding this comment.
Pull request overview
This PR adds an initial Java SDK/driver for the NeuG graph database, including a public API surface, internal HTTP/protobuf plumbing, Maven build configuration, and accompanying tests/docs.
Changes:
- Introduces core driver APIs (
Driver,Session,ResultSet) and factory entry point (GraphDatabase) backed by internal implementations. - Adds utility components for request/response handling over HTTP (OkHttp) with JSON serialization and protobuf response parsing.
- Adds a standalone Maven module (
tools/java_driver) with unit tests and a usage guide.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/java_driver/src/test/java/org/alibaba/neug/driver/QuerySerializerTest.java | Adds unit tests covering query JSON serialization permutations. |
| tools/java_driver/src/test/java/org/alibaba/neug/driver/JsonUtilTest.java | Adds tests to validate singleton ObjectMapper access. |
| tools/java_driver/src/test/java/org/alibaba/neug/driver/InternalResultSetTest.java | Adds tests for protobuf-to-Java type mapping, cursoring, and null bitmap handling. |
| tools/java_driver/src/test/java/org/alibaba/neug/driver/GraphDatabaseTest.java | Adds basic factory creation tests and multi-driver instantiation check. |
| tools/java_driver/src/test/java/org/alibaba/neug/driver/ConfigTest.java | Adds tests for default/custom config builder behavior. |
| tools/java_driver/src/test/java/org/alibaba/neug/driver/ClientTest.java | Adds tests around Client lifecycle and basic connectivity failure behavior. |
| tools/java_driver/src/test/java/org/alibaba/neug/driver/AccessModeTest.java | Adds enum sanity tests for AccessMode. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/ResponseParser.java | Adds protobuf response parsing helper returning ResultSet. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/QuerySerializer.java | Adds JSON request serializer for query/params/access mode. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/JsonUtil.java | Adds shared Jackson ObjectMapper singleton holder. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/Config.java | Adds driver configuration object + builder for timeouts/pool sizing. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/Client.java | Adds OkHttp-based synchronous POST client to /cypher. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/AccessMode.java | Adds access mode enum used in requests. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/internal/InternalSession.java | Implements Session by serializing, posting, and parsing results. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/internal/InternalResultSet.java | Implements ResultSet cursoring and typed getters over protobuf arrays. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/internal/InternalDriver.java | Implements Driver to create sessions and manage the underlying client. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/Session.java | Defines the Session API for executing queries. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/ResultSet.java | Defines the ResultSet cursor and typed access API. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/GraphDatabase.java | Adds factory methods for driver creation with URI/config validation. |
| tools/java_driver/src/main/java/org/alibaba/neug/driver/Driver.java | Defines the top-level Driver interface and lifecycle methods. |
| tools/java_driver/pom.xml | Adds Maven module build, dependencies, protobuf generation, formatting, and test setup. |
| tools/java_driver/USAGE.md | Documents installation and example usage of the new Java driver. |
| proto/response.proto | Adds Java protobuf generation options (java_package, java_outer_classname). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalDriver.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalResultSet.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalResultSet.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/org/alibaba/neug/driver/Session.java
Outdated
Show resolved
Hide resolved
tools/java_driver/src/main/java/com/alibaba/neug/driver/utils/AccessMode.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalResultSet.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/org/alibaba/neug/driver/internal/InternalResultSet.java
Outdated
Show resolved
Hide resolved
tools/java_driver/src/main/java/org/alibaba/neug/driver/internal/InternalResultSet.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalSession.java
Show resolved
Hide resolved
tools/java_driver/src/test/java/com/alibaba/neug/driver/InternalResultSetTest.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalResultSet.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalDriver.java
Show resolved
Hide resolved
tools/java_driver/src/main/java/com/alibaba/neug/driver/internal/InternalResultSet.java
Show resolved
Hide resolved
tools/java_driver/src/test/java/org/alibaba/neug/driver/InternalResultSetTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…alResultSetTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request introduces a new Java driver for the NeuG graph database, including the initial implementation, build configuration, and usage documentation. The main changes establish the driver’s interface, provide factory methods for creating driver instances, set up Maven build and dependency management, and document how to use and integrate the driver in other projects.
Java driver implementation:
Driverinterface inDriver.java, defining methods for session creation, connectivity verification, resource cleanup, and closed-state checking.GraphDatabaseclass inGraphDatabase.javaas the entry point for creating driver instances, including URI validation and support for custom configuration via theConfigclass.Build and dependency configuration:
pom.xmlto manage dependencies (OkHttp, Protocol Buffers, Jackson, SLF4J, JUnit, Mockito), plugins for compilation, testing, code formatting, and Protocol Buffers code generation.Documentation:
USAGE.md, including installation instructions, dependency details, and example code for connecting, querying, and using configuration and parameters.Protocol Buffers integration:
response.protowithjava_packageandjava_outer_classnameoptions to support Java code generation for the driver.Greptile Summary
This PR introduces a new Java driver for the NeuG graph database, providing a clean public API (
Driver,Session,ResultSet) backed by internal implementations that communicate over HTTP using OkHttp and deserialize responses via Protocol Buffers. The overall architecture is sound, but two critical bugs in the connection management layer must be fixed before this can be used safely in any real application.Key issues found:
clientinInternalDriver: TheClientfield isstatic, meaning allInternalDriverinstances share a single client. Creating a second driver silently replaces the first driver's connection, which will cause incorrect behavior and data routing when multiple drivers are used.httpClientinClient: TheOkHttpClientfield is alsostatic, compounding the above problem — every newClientconstruction overwrites the shared HTTP client for all existing instances.NullPointerExceptioninClient.syncPost:response.body()can returnnull(per OkHttp contract) and is dereferenced without a null check.USAGE.mdinstructscd tools/javainstead of the correctcd tools/java_driver.Configmethod: TheUSAGE.mdexample calls.withConnectionTimeout(3000), but the actual builder method is.withConnectionTimeoutMillis(int).}ofGraphDatabaseclass.configparameter inGraphDatabase.driver(String, Config).Confidence Score: 1/5
staticfield bugs will cause silent connection sharing and data routing failures when more than one driver instance is created.clientfield inInternalDriverand the statichttpClientfield inClientare critical correctness bugs that break multi-driver usage entirely. These must be changed to instance fields before the driver is usable in any non-trivial scenario.InternalDriver.javaandClient.javarequire fixes to the static field declarations before merging.Important Files Changed
clientis declaredstatic, causing all driver instances to share the same connection — second instantiation overwrites the first driver's client.httpClientis declaredstatic, sharing one OkHttpClient across all instances; alsoresponse.body()is dereferenced without a null check.configparameter and a stray trailing semicolon after the class closing brace.tools/javainstead oftools/java_driver) and a non-existent Config method (withConnectionTimeoutinstead ofwithConnectionTimeoutMillis).QuerySerializerandResponseParser; no issues found.java_packageandjava_outer_classnameoptions needed for Java code generation; minimal, correct change.Sequence Diagram
sequenceDiagram participant App participant GraphDatabase participant InternalDriver participant InternalSession participant Client participant NeuGServer App->>GraphDatabase: driver(uri, config) GraphDatabase->>InternalDriver: new InternalDriver(uri, config) InternalDriver->>Client: new Client(uri, config) Client-->>InternalDriver: httpClient (OkHttp) InternalDriver-->>App: Driver App->>InternalDriver: session() InternalDriver->>InternalSession: new InternalSession(client) InternalSession-->>App: Session App->>InternalSession: run(query, params, mode) InternalSession->>QuerySerializer: serialize(query, params, mode) QuerySerializer-->>InternalSession: byte[] request (JSON) InternalSession->>Client: syncPost(request) Client->>NeuGServer: HTTP POST /cypher NeuGServer-->>Client: byte[] response (Protobuf) Client-->>InternalSession: byte[] response InternalSession->>ResponseParser: parse(response) ResponseParser->>InternalResultSet: new InternalResultSet(QueryResponse) InternalResultSet-->>App: ResultSet App->>InternalResultSet: next() / getString() / getInt() / ... App->>InternalSession: close() App->>InternalDriver: close() InternalDriver->>Client: close() [evicts connection pool]Comments Outside Diff (5)
tools/java_driver/src/main/java/org/alibaba/neug/driver/internal/InternalDriver.java, line 959-969 (link)Static
clientfield shared across all driver instancesclientis declaredstatic, which means it is shared across allInternalDriverinstances in the JVM. When a second driver is created (e.g., pointing to a different server), its constructor overwrites the static field, causing the first driver to silently use the wrong client. ThetestMultipleDriverInstancestest inGraphDatabaseTestwould actually both close the same (second) client, anddriver1.close()would close the underlying OkHttp pool ofdriver2.This should be an instance field:
tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/Client.java, line 1810-1835 (link)Static
httpClientshared across allClientinstanceshttpClientis declaredstatic, so everyClientinstantiation overwrites it for all existing instances. TwoClientobjects pointing to different servers (with different connection pool sizes or timeouts) will end up sharing the sameOkHttpClient. The field should be an instance variable:tools/java_driver/src/main/java/org/alibaba/neug/driver/utils/Client.java, line 1851-1852 (link)Potential
NullPointerExceptionon response bodyresponse.body()can returnnullfor HTTP responses that carry no body (e.g.,HEADresponses, or certain error conditions). Calling.bytes()on a null body will throw aNullPointerExceptionthat is not handled here.tools/java_driver/src/main/java/org/alibaba/neug/driver/GraphDatabase.java, line 478-479 (link)Stray trailing semicolon after class closing brace
There is a lone
;after the closing}of the class on the next line. While some Java compilers may silently accept this as an empty type declaration, it is non-standard, confusing, and should be removed.tools/java_driver/src/main/java/org/alibaba/neug/driver/GraphDatabase.java, line 468-477 (link)Missing null check for
configparameterThe
driver(String uri, Config config)overload does not validate thatconfigis non-null. Ifnullis passed, aNullPointerExceptionwill be thrown deep insideClient's constructor whenconfig.getMaxConnectionPoolSize()etc. are called, making the error hard to diagnose. A guard matching the style of the existing URI validation would be more user-friendly:Last reviewed commit: 4898637