Copilot commented on code in PR #3159:
URL: https://github.com/apache/hugegraph/pull/3159#discussion_r3885662308
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -519,6 +519,13 @@ public void clearBackend() {
LockUtil.lock(this.spaceGraphName(), LockUtil.GRAPH_LOCK);
try {
+ if (this.isHstore()) {
+ E.checkState(this.schemaTransaction() instanceof
+ CachedSchemaTransactionV2,
+ "The HStore schema transaction must be %s",
+ CachedSchemaTransactionV2.class.getSimpleName());
+ ((CachedSchemaTransactionV2) this.schemaTransaction()).clear();
+ }
Review Comment:
In clearBackend(), schemaTransaction() is invoked twice (once for the
instanceof check and again for the cast+clear). If schemaTransaction() can
return different instances per call (it delegates to tx.schemaTransaction()),
the type check may not apply to the instance being cleared. Store it in a local
variable to ensure consistency and avoid duplicate work.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java:
##########
@@ -287,20 +287,18 @@ public String checkDefaultRole(@Context GraphManager
manager,
defaultRole = null; // unreachable, satisfies compiler
}
validGraphSpace(manager, graphSpace);
- boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER);
- E.checkArgument(!hasGraph || StringUtils.isNotEmpty(graph),
- "Must set a graph for observer");
+ boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER) &&
StringUtils.isNotEmpty(graph);
if (hasGraph) {
validGraph(manager, graphSpace, graph);
}
boolean result;
if (hasGraph) {
result = authManager.isDefaultRole(graphSpace, graph, user,
- defaultRole);
+ defaultRole) ||
+ authManager.isDefaultRole(graphSpace, user, defaultRole);
Review Comment:
ManagerAPI.checkDefaultRole() does two default-role checks for the
OBSERVER+graph case. In PD/HStore mode
StandardAuthManagerV2.isDefaultRole(graphSpace, graph, ...) already includes
the ALL_GRAPHS fallback, so the second call duplicates work and increases
per-request auth lookups.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -203,20 +200,19 @@ public String checkDefaultRole(@Context GraphManager
manager,
defaultRole.equals(HugeDefaultRole.SPACE)) {
throw new ForbiddenException("Forbidden to check role " + role);
}
- boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER);
- E.checkArgument(!hasGraph || StringUtils.isNotEmpty(graph),
- "Must set a graph for observer");
+ boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER) &&
+ StringUtils.isNotEmpty(graph);
if (hasGraph) {
validGraph(manager, name, graph);
}
boolean result;
if (hasGraph) {
result = authManager.isDefaultRole(name, graph, user,
- defaultRole);
+ defaultRole) ||
+ authManager.isDefaultRole(name, user, defaultRole);
Review Comment:
GraphSpaceAPI.checkDefaultRole() calls isDefaultRole(graphSpace, graph,
user, role) and then also isDefaultRole(graphSpace, user, role). In PD/HStore
mode the underlying StandardAuthManagerV2.isDefaultRole(graphSpace, graph, ...)
already falls back to the ALL_GRAPHS role, so the extra call is redundant and
adds an extra auth lookup per request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
