imbajin commented on code in PR #3159:
URL: https://github.com/apache/hugegraph/pull/3159#discussion_r3883994576
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -259,16 +266,19 @@ public void deleteDefaultRole(@Context GraphManager
manager,
E.checkArgument(false, "Invalid role value '%s'", role);
defaultRole = null; // unreachable, satisfies compiler
}
- 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);
}
if (hasGraph) {
authManager.deleteDefaultRole(name, user, defaultRole, graph);
} else {
authManager.deleteDefaultRole(name, user, defaultRole);
Review Comment:
Removed on `6851b1cd`: API 0.72 no longer performs implicit per-graph legacy
observer cleanup. Explicit `ALL_GRAPHS` is the current contract; versioned
legacy migration is recorded in the consolidated TODO.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -2327,7 +2377,9 @@ public TraversalStrategiesProxy(TraversalStrategies
strategies) {
@Override
public List<TraversalStrategy<?>> toList() {
Review Comment:
Fixed on `6851b1cd`: `toList()` materializes `TraversalStrategyProxy`
wrappers and returns an unmodifiable list. Tests verify wrapper preservation
and mutation rejection.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -2414,6 +2466,11 @@ public void apply(Traversal.Admin<?, ?> traversal) {
*/
String caller = Thread.currentThread().getName();
if (!caller.contains(TraversalStrategiesProxy.REST_WORKER)) {
+ for (HugePermission permission :
Review Comment:
Removed on `6851b1cd`: the general Gremlin mutation pre-check is deleted, so
existing GREMLIN EXECUTE permissions are not silently remapped to WRITE/DELETE.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -178,13 +204,27 @@ public static void setRequestGraphSpace(String
graphSpace) {
REQUEST_GRAPH_SPACE.set(graphSpace);
}
- public static Context setAdmin() {
- Context old = getContext();
- AuthContext.useAdmin();
- return old;
+ public static void runAsAdmin(Runnable runnable) {
Review Comment:
Fixed on `6851b1cd`: `runAsAdmin` is filtered from Gremlin reflection and is
used only behind a private internal callback wrapper. All ambient contexts are
restored in `finally`.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -259,16 +266,19 @@ public void deleteDefaultRole(@Context GraphManager
manager,
E.checkArgument(false, "Invalid role value '%s'", role);
defaultRole = null; // unreachable, satisfies compiler
}
- 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);
}
if (hasGraph) {
authManager.deleteDefaultRole(name, user, defaultRole, graph);
} else {
authManager.deleteDefaultRole(name, user, defaultRole);
+ if (defaultRole.equals(HugeDefaultRole.OBSERVER)) {
+ for (String currentGraph : manager.graphs(name)) {
Review Comment:
The implicit scan/migration path was removed from API 0.72. A versioned
legacy-role migration covering deleted/recreated graph names is now explicit in
the consolidated TODO:
https://github.com/apache/hugegraph/pull/3159#issuecomment-5457674257
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -519,6 +519,9 @@ public void clearBackend() {
LockUtil.lock(this.spaceGraphName(), LockUtil.GRAPH_LOCK);
try {
+ if (this.isHstore()) {
+ ((CachedSchemaTransactionV2) this.schemaTransaction()).clear();
Review Comment:
Recorded as a separately scoped lifecycle TODO: deletion must be coordinated
with concurrent schema/data mutations through one documented graph-wide gate,
not a local HStore-only lock. See
https://github.com/apache/hugegraph/pull/3159#issuecomment-5457674257. The
current head preserves the focused HStore metadata-clear bugfix.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransactionV2.java:
##########
@@ -467,6 +467,7 @@ public void clear() {
// Clear schema info firstly
super.clear();
this.clearCache(false);
+ this.notifySchemaCacheClear();
Review Comment:
The direct notification is intentionally retained: when
`task.sync_deletion=true`, the later `STORE_CLEAR` path suppresses its
broadcast, so removing the direct call can leave remote schema caches stale.
Exact-once ownership without losing that correctness guarantee is recorded in
https://github.com/apache/hugegraph/pull/3159#issuecomment-5457674257.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -519,6 +519,9 @@ public void clearBackend() {
LockUtil.lock(this.spaceGraphName(), LockUtil.GRAPH_LOCK);
try {
+ if (this.isHstore()) {
+ ((CachedSchemaTransactionV2) this.schemaTransaction()).clear();
Review Comment:
Fixed on `6851b1cd`: the HStore path now checks that the schema transaction
is `CachedSchemaTransactionV2` before the cast; failure stops provider clear.
The ordering, failure, and unexpected-type paths are covered.
--
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]
