bitflicker64 commented on code in PR #3182:
URL: https://github.com/apache/hugegraph/pull/3182#discussion_r3918293213
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -974,14 +982,16 @@ protected Iterator<Edge> queryEdgesByIds(Object[] edgeIds,
* Sort at the lower layer and return directly if there is no
* local vertex and duplicated id.
*/
- Iterator<HugeEdge> it = this.queryEdgesFromBackend(query);
+ Iterator<HugeEdge> it =
+ this.queryValidEdgesFromBackend(query);
@SuppressWarnings({"unchecked", "rawtypes"})
Iterator<Edge> r = (Iterator) it;
return r;
}
query.mustSortByInput(false);
- Iterator<HugeEdge> it = this.queryEdgesFromBackend(query);
+ Iterator<HugeEdge> it =
+ this.queryValidEdgesFromBackend(query);
Review Comment:
‼️ `queryEdgesByIds(Object[], boolean)` builds its own query at line 950,
`new IdQuery(HugeType.EDGE)`, so it carries the `Query` defaults
`showHidden=false` and `showDeleting=false` (Query.java:99-100). Routing it
through `queryValidEdgesFromBackend()` here and at line 986 now drops every
edge whose label is hidden. Before this PR both call sites went straight to
`queryEdgesFromBackend()` and did no such filtering.
Auth relationships use hidden edge labels: `HugeBelong.P.BELONG =
Hidden.hide("belong")` (HugeBelong.java:232) and `HugeAccess.P.ACCESS =
Hidden.hide("access")` (HugeAccess.java:276). `RelationshipManager` reaches the
backend through `this.tx().queryEdges(id)` in `delete(Id)` (line 94), `get(Id)`
(107), `exists(Id)` (120) and `queryById()` (161, behind `list(List<Id>)`). So
`get` now throws `NotFoundException`, `exists` returns false so `update` fails
its check, and `delete` skips its `if (edges.hasNext())` block and returns null
with no error and no commit.
`queryEdgesByIds` receives no caller query, so the fix is either to put
lines 986 and 994 back on `queryEdgesFromBackend()`, or to set
`showHidden(true)` and `showDeleting(true)` on the `IdQuery` at line 950. Note
that `CachedGraphTransactionTest.testQueryByIdsFiltersDeletingLabels`, added by
this PR, asserts the current behaviour, so it needs to change with the fix.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1859,7 +1876,8 @@ private void removeLeftIndexIfNeeded(Map<Id, HugeVertex>
vertices) {
return;
}
IdQuery idQuery = new IdQuery(HugeType.VERTEX, ids);
- Iterator<HugeVertex> results = this.queryVerticesFromBackend(idQuery);
+ Iterator<HugeVertex> results =
+ this.queryValidVerticesFromBackend(idQuery);
Review Comment:
🧹 Same wrapper problem, smaller blast radius. `removeLeftIndexIfNeeded()`
builds `new IdQuery(HugeType.VERTEX, ids)` at line 1878, which is the
`IdQuery(HugeType, Set<Id>)` constructor (IdQuery.java:50) chaining to
`IdQuery(HugeType)`, so `showHidden` stays false. The stored version of a
hidden-label vertex is now filtered out before
`indexTx.updateVertexIndex(existedVertex, true)` can run, so its stale index
entries survive the overwrite. `~user`, `~group`, `~role`, `~target` and
`~project` (HugeUser.java:261, HugeGroup.java:164, HugeRole.java:186,
HugeTarget.java:276, HugeProject.java:256) are all overwritten in place by
`EntityManager.save()`.
Filed as minor because the call is gated by `removeLeftIndexOnOverwrite`
(line 332), backed by `vertex.remove_left_index_at_overwrite`, which defaults
to false (CoreOptions.java:356-362). Whatever fix lands for lines 994 and 381
should cover this site too.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1963,6 +2015,33 @@ private boolean rightResultFromIndexQuery(Query query,
HugeElement elem) {
return false;
}
+ protected static boolean queryNeedsPostFilter(Query query) {
+ while (query != null) {
+ if (query instanceof ConditionQuery) {
+ ConditionQuery cq = (ConditionQuery) query;
+ /*
+ * Search conditions need post-filtering even before query
+ * optimization marks the query with an optimized type.
+ */
+ if (cq.hasSearchCondition() ||
+ conditionQueryNeedsPostFilter(cq)) {
+ return true;
+ }
+ }
+ query = query.originQuery();
+ }
+ return false;
+ }
+
+ private static boolean conditionQueryNeedsPostFilter(ConditionQuery query)
{
Review Comment:
🧹 The `edgeIndexWithLabel` carve-out cannot be reached from
`rightResultFromIndexQuery()`, the predicate's other caller. That method
already returns true for the same shape earlier: the block at lines 1966-1982
fires when `cq.condition(HugeKeys.LABEL) != null && cq.resultType().isEdge()`,
and returns true both when `cq.conditions().size() == 1` and when
`cq.optimized() == OptimizedType.INDEX`. So whenever `edgeIndexWithLabel` is
true, control never reaches line 1985.
The carve-out only changes the cache decision in `queryNeedsPostFilter()`,
but it is spelled as part of a predicate named for post-filtering in general,
which invites the next reader to widen it and quietly weaken
`rightResultFromIndexQuery()`.
Please move the edge-index-with-label exemption into
`queryNeedsPostFilter()` where it is actually used, and leave
`conditionQueryNeedsPostFilter()` as the plain `optimized() !=
OptimizedType.NONE` test that line 1985 needs.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -377,7 +377,8 @@ protected void prepareDeletions(Map<Id, HugeVertex>
removedVertices,
}
// Query all edges of the vertex and remove them
Query query = constructEdgesQuery(v.id(), Directions.BOTH, new
Id[0]);
- Iterator<HugeEdge> vedges = this.queryEdgesFromBackend(query);
+ Iterator<HugeEdge> vedges =
+ this.queryValidEdgesFromBackend(query);
Review Comment:
⚠️ Line 379 binds to `constructEdgesQuery(Id, Directions, Id...)` (line
1306), which delegates to the private `List<Id>` overload at line 1356. That
builds `new ConditionQuery(HugeType.EDGE)` at line 1364 and never touches the
display flags, so the query arrives with `showHidden=false` and
`showDeleting=false`. Through `queryValidEdgesFromBackend()` the deletion path
now skips any adjacent edge whose label is hidden, or whose label status is
`DELETING` at that moment.
`prepareDeletions()` is the only place that collects a removed vertex's
edges into `removedEdges`; the other writer at line 919 is an explicit
`removeEdge()`. A skipped edge therefore keeps its row and its index entries
with no owner vertex, and nothing later in this path revisits it.
`EntityManager.delete()` removing a `~user` vertex that still has `~belong` or
`~access` edges is a concrete instance.
Please leave vertex deletion on the unfiltered `queryEdgesFromBackend()`, or
set `showHidden(true)` and `showDeleting(true)` on the query built at line 379.
--
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]
