Skip to content

Conversation

@sadwitdastreetz
Copy link

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

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. api Changes of API labels Jul 3, 2025
@codecov
Copy link

codecov bot commented Jul 6, 2025

Codecov Report

❌ Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.51%. Comparing base (a53af86) to head (d7b10d6).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...ugegraph/api/traversers/SameNeighborsBatchAPI.java 0.00% 30 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (a53af86) and HEAD (d7b10d6). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (a53af86) HEAD (d7b10d6)
3 2
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

This comment was marked as duplicate.

@imbajin imbajin requested a review from Copilot October 20, 2025 11:39
Copy link
Contributor

Copilot AI left a 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) {
Copy link

Copilot AI Oct 20, 2025

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());

Suggested change
} catch (Exception e) {
} catch (Exception e) {
LOG.debug("Failed to serialize vertex list: {}", e.getMessage());

Copilot uses AI. Check for mistakes.
} catch (Exception e) {
}
return String.format("SameNeighborsBatchRequest{vertex=%s,direction=%s,label=%s," +
"max_degree=%d,limit=%d", vertexStr, this.direction,
Copy link

Copilot AI Oct 20, 2025

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}.

Suggested change
"max_degree=%d,limit=%d", vertexStr, this.direction,
"max_degree=%d,limit=%d}", vertexStr, this.direction,

Copilot uses AI. Check for mistakes.
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: Import organization issue

The import statements are not properly ordered. Standard Java convention and most projects require imports to be grouped and sorted:

  1. java/javax imports
  2. Third-party imports
  3. 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 {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: Wrong Logger class usage

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: Inefficient parsing of constants

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:

  1. Defining them as numeric constants directly, OR
  2. Parsing them once in a static initializer, OR
  3. 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: Silent exception swallowing

Lines 106-107 catch an exception but do nothing with it - not even logging. This is a bad practice that makes debugging impossible.

Issues:

  1. No logging of the error
  2. Returns empty string on failure, which may cause confusing behavior
  3. 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<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: Missing validation

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) {
}
Copy link
Member

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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Changes of API inactive size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add /traversers/sameneighborsbatch API endpoint to support client-side batch same neighbors queries

2 participants