Skip to content

refactor(client): adjust APIs to compatible with 1.7.0 server#685

Merged
imbajin merged 12 commits intoapache:masterfrom
sadwitdastreetz:update-client
Oct 28, 2025
Merged

refactor(client): adjust APIs to compatible with 1.7.0 server#685
imbajin merged 12 commits intoapache:masterfrom
sadwitdastreetz:update-client

Conversation

@sadwitdastreetz
Copy link
Copy Markdown
Contributor

@sadwitdastreetz sadwitdastreetz commented Oct 23, 2025

Purpose of the PR

refactor(client): adjust APIs to compatible with 1.7.0 server

Main Changes

support graphspace for1.7.0 server;
adjust api changes for 1.7.0 server.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

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

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Oct 23, 2025
@github-actions github-actions bot added the client hugegraph-client label Oct 23, 2025
@imbajin imbajin requested a review from Copilot October 24, 2025 04:28
Copy link
Copy Markdown

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 updates the HugeGraph client to maintain compatibility with version 1.7.0 server by adjusting GraphSpace support detection and adding GraphSpace-aware API methods for graph mode and read mode operations.

Key Changes:

  • Changed GraphSpace support detection threshold from version 2.0 to 1.7
  • Added overloaded methods accepting graphSpace parameters for mode and readMode operations
  • Changed default supportGs flag from false to true

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
HugeClient.java Updated version check to enable GraphSpace support starting from version 1.7 instead of 2.0
RestClient.java Changed default supportGs initialization from false to true
GraphsManager.java Added graphSpace-aware wrapper methods for mode and readMode operations
GraphsAPI.java Implemented graphSpace-aware versions of mode() and readMode() methods with proper path construction

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}

public void mode(String graph, String graphSpace, GraphMode mode) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

‼️ Critical - 缺少单元测试

这个PR添加了多个新的API方法(6个重载方法),但没有看到相应的测试。建议:

  1. 为所有新增的graphSpace相关方法添加单元测试
  2. 测试应该覆盖:
    • 正常的mode/readMode获取和设置
    • 无效的GraphMode/GraphReadMode值处理
    • 空graphSpace参数的边界情况
    • API版本兼容性检查

示例测试场景:

@Test
public void testModeWithGraphSpace() {
    GraphsAPI api = new GraphsAPI(client);
    api.mode("testGraph", "testSpace", GraphMode.RESTORING);
    GraphMode mode = api.mode("testGraph", "testSpace");
    assertEquals(GraphMode.RESTORING, mode);
}

this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}

public void mode(String graph, String graphSpace, GraphMode mode) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ API一致性 - 缺少参数校验

新增的graphSpace参数方法缺少参数校验。建议添加:

public void mode(String graph, String graphSpace, GraphMode mode) {
    E.checkArgumentNotNull(graph, "Graph name cannot be null");
    E.checkArgumentNotNull(graphSpace, "GraphSpace name cannot be null");
    E.checkArgumentNotNull(mode, "GraphMode cannot be null");
    
    this.client.put(joinPath(this.path(), graphSpace, graph, MODE), null, mode);
}

这样可以提供更清晰的错误信息,而不是让空指针在URL拼接时才失败。

this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}

public void mode(String graph, String graphSpace, GraphMode mode) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🧹 文档完善

虽然PR标记为Doc - No Need,但建议添加:

  1. JavaDoc注释:新增方法应该有完整的JavaDoc,说明:

    • @param graphSpace参数的含义和用途
    • 与无graphSpace版本的方法区别
    • @since 1.7.0标记新API的引入版本
  2. 迁移指南:在CHANGELOG或升级文档中说明:

    • 如何从旧API迁移到新API
    • graphspace参数何时需要传入

示例:

/**
 * Set the mode for specified graph in graphspace.
 * 
 * @param graph the graph name
 * @param graphSpace the graphspace name (required for server >= 1.7.0)
 * @param mode the graph mode to set
 * @since 1.7.0
 */
public void mode(String graph, String graphSpace, GraphMode mode) {
    // ...
}

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.00%. Comparing base (b066b80) to head (9101077).
⚠️ Report is 56 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #685      +/-   ##
============================================
- Coverage     62.49%   62.00%   -0.50%     
+ Complexity     1903      936     -967     
============================================
  Files           262       93     -169     
  Lines          9541     4572    -4969     
  Branches        886      531     -355     
============================================
- Hits           5963     2835    -3128     
+ Misses         3190     1523    -1667     
+ Partials        388      214     -174     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Oct 27, 2025
@imbajin imbajin requested a review from Copilot October 27, 2025 08:18
Copy link
Copy Markdown

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Enhanced Javadoc comments for GraphMode and GraphReadMode enums to clarify their operational contexts, permissions, and use cases. Refactored GraphsAPI.clear() for cleaner path selection logic.
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 28, 2025
@imbajin imbajin requested review from Thespica and Copilot October 28, 2025 04:28
Copy link
Copy Markdown

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

private String version;
@Getter
@Setter
private boolean supportGs;
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The default value for supportGs has changed from explicit false initialization to implicit false (Java's default for boolean fields). While functionally equivalent, the previous explicit initialization made the intent clearer. Consider restoring the explicit initialization: private boolean supportGs = false;

Suggested change
private boolean supportGs;
private boolean supportGs = false;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@Thespica Thespica left a comment

Choose a reason for hiding this comment

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

LGTM

@imbajin imbajin merged commit 852d76a into apache:master Oct 28, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client hugegraph-client lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants