-
Notifications
You must be signed in to change notification settings - Fork 569
Fix : SameNeighborsBatchAPI diff between inner and community Ver #2825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2825 +/- ##
============================================
- Coverage 41.23% 0.51% -40.73%
+ Complexity 333 87 -246
============================================
Files 747 735 -12
Lines 60163 57945 -2218
Branches 7680 7256 -424
============================================
- Hits 24811 300 -24511
- Misses 32765 57633 +24868
+ Partials 2587 12 -2575 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds the SameNeighborsBatchAPI class to the community version of HugeGraph server to align it with the inner version. The API enables batch processing of same-neighbor queries for multiple vertex pairs, addressing a functional gap between the two versions.
Key Changes:
- Added new REST API endpoint for batch same-neighbor traversal operations
- Implemented request handling for multiple vertex pairs with configurable parameters (direction, label, max_degree, limit)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| String vertexStr = ""; | ||
| try { | ||
| vertexStr = om.writeValueAsString(this.vertexList); | ||
| } catch (Exception e) { |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty catch block silently swallows exceptions. Consider logging the error or at minimum adding a comment explaining why the exception is intentionally ignored. For example: LOG.debug(\"Failed to serialize vertex list: {}\", e.getMessage());
| } catch (Exception e) { | |
| } catch (Exception e) { | |
| LOG.debug("Failed to serialize vertex list: {}", e.getMessage()); |
| } catch (Exception e) { | ||
| } | ||
| return String.format("SameNeighborsBatchRequest{vertex=%s,direction=%s,label=%s," + | ||
| "max_degree=%d,limit=%d", vertexStr, this.direction, |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing closing brace in format string. The string should end with } after limit=%d. Current output will be malformed like limit=10 instead of limit=10}.
| "max_degree=%d,limit=%d", vertexStr, this.direction, | |
| "max_degree=%d,limit=%d}", vertexStr, this.direction, |
| import org.apache.hugegraph.type.define.Directions; | ||
| import com.codahale.metrics.annotation.Timed; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statements are not properly ordered. Standard Java convention and most projects require imports to be grouped and sorted:
- java/javax imports
- Third-party imports
- Project imports
Current order mixes Apache HugeGraph imports with third-party libraries (Codahale, Jackson, Swagger, Jakarta). This violates standard Java conventions.
Suggestion: Reorder imports to follow standard conventions:
// Java standard
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
// Third-party
import com.codahale.metrics.annotation.Timed;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.inject.Singleton;
import jakarta.ws.rs.POST;
// ... other jakarta imports
// Apache HugeGraph
import org.apache.hugegraph.HugeGraph;
import org.apache.hugegraph.api.API;
// ... other project imports| @Singleton | ||
| @Tag(name = "SameNeighborsBatchAPI") | ||
| public class SameNeighborsBatchAPI extends API { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 56 creates a logger for RestServer.class instead of the current class SameNeighborsBatchAPI.class. This is incorrect and will make debugging difficult as log messages will appear to come from the wrong class.
Fix:
private static final Logger LOG = Log.logger(SameNeighborsBatchAPI.class);| @JsonProperty("label") | ||
| private String label; | ||
| @JsonProperty("max_degree") | ||
| public long maxDegree = Long.parseLong(DEFAULT_MAX_DEGREE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 97-98 use Long.parseLong() and Integer.parseInt() on constants that are already defined as strings. This is inefficient and error-prone.
If DEFAULT_MAX_DEGREE and DEFAULT_ELEMENTS_LIMIT are string constants like "10000", consider:
- Defining them as numeric constants directly, OR
- Parsing them once in a static initializer, OR
- If they must remain as strings elsewhere, create numeric constant versions for API usage
Current:
public long maxDegree = Long.parseLong(DEFAULT_MAX_DEGREE);
public int limit = Integer.parseInt(DEFAULT_ELEMENTS_LIMIT);Better approach:
public long maxDegree = DEFAULT_MAX_DEGREE_VALUE; // Define as long constant
public int limit = DEFAULT_ELEMENTS_LIMIT_VALUE; // Define as int constant| ObjectMapper om = new ObjectMapper(); | ||
| String vertexStr = ""; | ||
| try { | ||
| vertexStr = om.writeValueAsString(this.vertexList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 106-107 catch an exception but do nothing with it - not even logging. This is a bad practice that makes debugging impossible.
Issues:
- No logging of the error
- Returns empty string on failure, which may cause confusing behavior
- No indication to caller that serialization failed
Fix:
try {
vertexStr = om.writeValueAsString(this.vertexList);
} catch (Exception e) {
LOG.warn("Failed to serialize vertex list: {}", e.getMessage());
vertexStr = "<serialization failed>";
}| HugeGraph g = graph(manager, graph); | ||
| SameNeighborTraverser traverser = new SameNeighborTraverser(g); | ||
|
|
||
| List<Set<Id>> result = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No null check for req.vertexList before iterating. If the request body is malformed or missing vertex_list, this will throw a NullPointerException.
Add validation:
E.checkArgumentNotNull(req.vertexList, "vertex_list cannot be null");
E.checkArgument(!req.vertexList.isEmpty(), "vertex_list cannot be empty");
for (List<Object> vertexPair : req.vertexList) {
E.checkArgumentNotNull(vertexPair, "vertex pair cannot be null");
E.checkArgument(vertexPair.size() == 2, "vertex_list size must be 2");
// ...
}| try { | ||
| vertexStr = om.writeValueAsString(this.vertexList); | ||
| } catch (Exception e) { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Minor: Typo in format string
Line 109 has an incomplete format string - missing closing brace } after limit=%d.
Fix:
return String.format("SameNeighborsBatchRequest{vertex=%s,direction=%s,label=%s," +
"max_degree=%d,limit=%d}", vertexStr, this.direction,
this.label, this.maxDegree, this.limit);
Purpose of the PR
close #2798
Main Changes
Previously, SameNeighborBatch was included in inner version of Client and Server, and the Community version of Client, not including the Comunity version of Server.
This PR would add this API to avoid the lack of the SameNeighborBatch function.
Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need