fix(server): fix npe in non-auth mode#2912
Conversation
dosubot
bot
added
size:L
Tsukilc
force-pushed
the
fix/server-npe
branch
from
a5afd91 to
310a9d1
Compare
dosubot
bot
added
size:S
Tsukilc
force-pushed
the
fix/server-npe
branch
from
310a9d1 to
fbf80ee
Compare
Tsukilc
force-pushed
the
fix/server-npe
branch
from
fbf80ee to
d7bfdb6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2912 +/- ##
============================================
- Coverage 35.57% 29.48% -6.10%
+ Complexity 333 286 -47
============================================
Files 802 802
Lines 67568 67572 +4
Branches 8777 8778 +1
============================================
- Hits 24040 19926 -4114
- Misses 40967 45371 +4404
+ Partials 2561 2275 -286 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (context == null) { | ||
| return "anonymous"; | ||
| } | ||
| return context.user.username(); |
There was a problem hiding this comment.
新增的 username() 方法在非认证模式下返回 "anonymous",但需要考虑:
-
业务逻辑正确性: 在
ManagerAPI.createManager()和GraphSpaceAPI.create()中,这个 username 被用作creator字段。如果在非认证模式下所有创建者都是 "anonymous",会导致无法追踪真实的资源创建者。 -
建议方案:
- 如果非认证模式下不需要追踪创建者,应该返回
null而不是 "anonymous",让调用方明确处理 - 或者在调用方检查认证模式,非认证模式下使用其他标识(如 IP、session ID 等)
- 如果非认证模式下不需要追踪创建者,应该返回
| return context.user.username(); | |
| public static String username() { | |
| Context context = HugeGraphAuthProxy.getContext(); | |
| if (context == null || context.user == null) { | |
| return null; // 让调用方明确处理非认证场景 | |
| } | |
| return context.user.username(); | |
| } |
请确认非认证模式下的业务预期行为。
| } | ||
|
|
||
| public static String username() { | ||
| Context context = HugeGraphAuthProxy.getContext(); |
There was a problem hiding this comment.
虽然修复了 context 为 null 的情况,但没有处理 context.user 为 null 的场景。建议增加防御性检查:
| Context context = HugeGraphAuthProxy.getContext(); | |
| public static String username() { | |
| Context context = HugeGraphAuthProxy.getContext(); | |
| if (context == null || context.user == null) { | |
| return "anonymous"; | |
| } | |
| return context.user.username(); | |
| } |
或者检查一下 Context 的构造函数是否保证 user 字段非空。
| this.hugegraph.updateTime(updateTime); | ||
| } | ||
|
|
||
| public static String username() { |
There was a problem hiding this comment.
这是一个 NPE 修复,应该添加单元测试来验证:
- 非认证模式下调用
username()的行为 getContext()返回 null 时的处理- 各个调用方(ManagerAPI, GraphsAPI, GraphSpaceAPI)在非认证模式下的正确性
建议添加测试类或测试方法覆盖这些场景。
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java
Show resolved
Hide resolved
| validUser(authManager, user); | ||
|
|
||
| String creator = HugeGraphAuthProxy.getContext().user().username(); | ||
| String creator = HugeGraphAuthProxy.username(); |
There was a problem hiding this comment.
建议搜索整个代码库中所有 HugeGraphAuthProxy.getContext().user().username() 的调用点,确保都已替换为新的 username() 方法,避免遗漏其他潜在的 NPE 风险点。
可以使用以下命令检查:
grep -r "HugeGraphAuthProxy.getContext().user().username()" --include="*.java"
dosubot
bot
added
size:L
There was a problem hiding this comment.
Pull request overview
This PR fixes a NullPointerException (NPE) that occurs in non-authentication mode when the authentication context is null. The fix introduces a new username() utility method that safely handles null contexts by returning "anonymous" as the default username, replacing direct calls to getContext().user().username() which would throw NPE when context is null.
- Adds a new static
username()method toHugeGraphAuthProxythat safely handles null authentication contexts - Replaces all direct
getContext().user().username()calls with the newusername()method across API classes - Adds comprehensive unit tests to verify correct behavior with null, valid, and admin contexts
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java | Adds new username() static method that returns "anonymous" when context is null, preventing NPE in non-auth mode |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java | Updates graph space creation to use safe username() method instead of direct context access |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java | Updates graph creation to use safe username() method instead of direct context access |
| hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java | Updates manager operations (create, delete, checkRole) to use safe username() method instead of direct context access |
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/auth/HugeGraphAuthProxyTest.java | Adds comprehensive unit tests covering null context, valid context, admin user, TaskManager integration, and context reset scenarios |
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java | Registers the new HugeGraphAuthProxyTest in the test suite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (context == null) { | ||
| return "anonymous"; | ||
| } | ||
| return context.user.username(); |
There was a problem hiding this comment.
Direct field access context.user is inconsistent with the rest of the codebase. The Context class provides a public user() method that should be used instead. All other usages in the codebase access the user through the method call context.user() (e.g., lines 203, 1171, 1172, 1202, 1458, 1489). Change context.user.username() to context.user().username() to maintain consistency.
| public static String username() { | ||
| Context context = HugeGraphAuthProxy.getContext(); | ||
| if (context == null) { | ||
| return "anonymous"; | ||
| } | ||
| return context.user.username(); | ||
| } |
There was a problem hiding this comment.
The username() method should be placed near other static utility methods (lines 155-204) rather than in the middle of instance methods. This improves code organization and makes it easier to locate static methods. Consider moving this method to be placed after the getContext() method around line 196, which it depends on.
In version 1.7.0, dynamic creation of graphs would cause a NPE. This PR fixes this error.
The current master version and version 1.7.0- do not have this problem.