fix(compiler): drop duplicate alias in neighbors lowering for non-default-PK center nodes (1dfdfe19) · Commits · GitLab.org / orbit / GitLab Knowledge Graph

config/seeds/data_correctness.sql

+4 −4

Original line number	Diff line number	Diff line
		@@ -166,10 +166,10 @@ INSERT INTO gl_note (id, note, noteable_type, noteable_id, confidential, interna
		(3002, repeat('x', 10000), 'MergeRequest', 2000, false, false, NULL, NULL, '1/100/1000/'),
		(3003, 'Robert''); DROP TABLE gl_note;--', 'MergeRequest', 2000, false, false, NULL, NULL, '1/100/1000/');

		INSERT INTO gl_merge_request_diff (id, merge_request_id, state, traversal_path) VALUES
		(5000, 2000, 'collected', '1/100/1000/'),
		(5001, 2000, 'collected', '1/100/1000/'),
		(5002, 2001, 'collected', '1/100/1000/');
		INSERT INTO gl_merge_request_diff (id, merge_request_id, state, head_commit_sha, traversal_path) VALUES
		(5000, 2000, 'collected', 'aaaaaaaa1111', '1/100/1000/'),
		(5001, 2000, 'collected', 'aaaaaaaa2222', '1/100/1000/'),
		(5002, 2001, 'collected', 'bbbbbbbb3333', '1/100/1000/');

		INSERT INTO gl_milestone (id, iid, title, state, traversal_path) VALUES
		(6000, 1, 'Sprint 1', 'active', '1/100/'),

crates/integration-tests/tests/compiler/dialects/clickhouse.rs

+33 −0

Original line number	Diff line number	Diff line
		@@ -475,6 +475,39 @@ fn multi_table_path_finding_scans_all_tables() {
		);
		}

		#[test]
		fn neighbors_non_default_pk_with_non_denorm_filter_no_alias_clash() {
		use ontology::DataType;
		let ontology = ontology::Ontology::new()
		.with_nodes(["File"])
		.with_edges(["DEFINES"])
		.with_fields("File", [("path", DataType::String)])
		.with_default_columns("File", ["path"])
		.with_redaction("File", "project", "project_id");

		let json = r#"{
		"query_type": "neighbors",
		"node": {
		"id": "f",
		"entity": "File",
		"filters": {"path": {"op": "contains", "value": "labkit"}}
		},
		"neighbors": {"node": "f", "direction": "both"}
		}"#;
		let result = compile(json, &ontology, &test_ctx()).unwrap();
		let rendered = result.base.render();

		let gl_file_refs = rendered.matches("gl_file").count();
		assert_eq!(
		gl_file_refs, 2,
		"expected one gl_file scan per direction arm; got {gl_file_refs}\nSQL:\n{rendered}"
		);
		assert!(
		rendered.contains("f.project_id AS project_id"),
		"dedup subquery must surface redaction id column: {rendered}"
		);
		}

		#[test]
		fn multi_table_neighbors_scans_all_tables() {
		let json = r#"{

crates/integration-tests/tests/server/data_correctness/mod.rs

+3 −0

Original line number	Diff line number	Diff line
		@@ -121,6 +121,9 @@ async fn data_correctness() {
		neighbors::neighbors_dynamic_columns_all_returns_properties,
		neighbors::neighbors_center_node_properties_hydrated,
		neighbors::neighbors_both_direction_preserves_edge_direction,
		neighbors::neighbors_non_default_pk_with_non_denorm_filter,
		neighbors::neighbors_non_default_pk_filter_excludes_non_matching,
		neighbors::neighbors_non_default_pk_redaction_uses_merge_request_id,
		// edge cases
		edge_cases::giant_string_survives_pipeline,
		edge_cases::sql_injection_string_preserved,

crates/integration-tests/tests/server/data_correctness/neighbors.rs

+85 −0

Original line number	Diff line number	Diff line
		@@ -215,6 +215,91 @@ pub(super) async fn neighbors_center_node_properties_hydrated(ctx: &TestContext)
		resp.assert_edge_exists("User", 1, "Group", 102, "MEMBER_OF");
		}

		pub(super) async fn neighbors_non_default_pk_with_non_denorm_filter(ctx: &TestContext) {
		let resp = run_query(
		ctx,
		r#"{
		"query_type": "neighbors",
		"node": {
		"id": "d",
		"entity": "MergeRequestDiff",
		"filters": {"head_commit_sha": {"op": "starts_with", "value": "aaaa"}}
		},
		"neighbors": {"node": "d", "direction": "incoming", "rel_types": ["HAS_DIFF"]}
		}"#,
		&allow_all(),
		)
		.await;

		resp.skip_requirement(Requirement::Filter {
		field: "head_commit_sha".into(),
		});
		resp.assert_referential_integrity();
		resp.assert_node_count(3);
		resp.assert_node_ids("MergeRequestDiff", &[5000, 5001]);
		resp.assert_node_ids("MergeRequest", &[2000]);
		resp.assert_edge_set("HAS_DIFF", &[(2000, 5000), (2000, 5001)]);
		}

		pub(super) async fn neighbors_non_default_pk_filter_excludes_non_matching(ctx: &TestContext) {
		let resp = run_query(
		ctx,
		r#"{
		"query_type": "neighbors",
		"node": {
		"id": "d",
		"entity": "MergeRequestDiff",
		"filters": {"head_commit_sha": {"op": "eq", "value": "no-such-sha"}}
		},
		"neighbors": {"node": "d", "direction": "incoming", "rel_types": ["HAS_DIFF"]}
		}"#,
		&allow_all(),
		)
		.await;

		resp.skip_requirement(Requirement::NodeIds);
		resp.skip_requirement(Requirement::Filter {
		field: "head_commit_sha".into(),
		});
		resp.skip_requirement(Requirement::Relationship {
		edge_type: "HAS_DIFF".into(),
		});
		resp.skip_requirement(Requirement::Neighbors);
		resp.assert_node_count(0);
		assert_eq!(resp.edge_count(), 0);
		}

		pub(super) async fn neighbors_non_default_pk_redaction_uses_merge_request_id(ctx: &TestContext) {
		let mut svc = MockRedactionService::new();
		svc.allow("merge_request", &[2001]);

		let resp = run_query(
		ctx,
		r#"{
		"query_type": "neighbors",
		"node": {
		"id": "d",
		"entity": "MergeRequestDiff",
		"filters": {"head_commit_sha": {"op": "starts_with", "value": "bbbb"}}
		},
		"neighbors": {"node": "d", "direction": "incoming", "rel_types": ["HAS_DIFF"]}
		}"#,
		&svc,
		)
		.await;

		resp.skip_requirement(Requirement::Filter {
		field: "head_commit_sha".into(),
		});
		resp.assert_node_count(2);
		resp.assert_node_ids("MergeRequestDiff", &[5002]);
		resp.assert_node_ids("MergeRequest", &[2001]);
		resp.assert_edge_set("HAS_DIFF", &[(2001, 5002)]);
		resp.assert_node_absent("MergeRequestDiff", 5000);
		resp.assert_node_absent("MergeRequestDiff", 5001);
		resp.assert_node_absent("MergeRequest", 2000);
		}

		pub(super) async fn neighbors_both_direction_preserves_edge_direction(ctx: &TestContext) {
		// Group 100 has incoming MEMBER_OF from users and outgoing CONTAINS to
		// projects/subgroups. With direction: "both", edges should preserve their

crates/query-engine/compiler/src/passes/lower/neighbors.rs

+25 −12

Original line number	Diff line number	Diff line
		@@ -46,6 +46,7 @@ pub fn emit_neighbors(
		filters: &[(String, InputFilter)],
		node_ids: &[i64],
		id_range: Option<&InputIdRange>,
		extra_select: &[&str],
		) -> (TableRef, Expr) {
		let mut scan_where = Vec::new();
		for (prop, filter) in filters {
		@@ -57,10 +58,13 @@ pub fn emit_neighbors(
		if let Some(range) = id_range {
		scan_where.push(id_range_predicate(alias, range));
		}
		let select = vec![
		let mut select = vec![
		SelectExpr::col(alias, DEFAULT_PRIMARY_KEY),
		SelectExpr::col(alias, DELETED_COLUMN),
		];
		for col in extra_select {
		select.push(SelectExpr::col(alias, *col));
		}
		dedup_subquery(alias, table, select, scan_where, DEFAULT_PRIMARY_KEY)
		}

		@@ -172,15 +176,22 @@ pub fn emit_neighbors(
		];

		let mut from: TableRef = edge_table_scan(&edge_table, edge_alias);
		let needs_center_table = !center_uses_default_pk;

		// Non-denorm filters: inline latest-row JOIN instead of CTE.
		if has_non_denorm {
		let redaction_col = center_redaction_col.as_str();
		let extra: Vec<&str> = if needs_center_table {
		vec![redaction_col]
		} else {
		Vec::new()
		};
		let (center_subq, deleted_filter) = build_center_dedup(
		&center_id,
		&center_table,
		&center_filters,
		&center_node_ids,
		center_id_range.as_ref(),
		&extra,
		);
		from = TableRef::join(
		JoinType::Inner,
		@@ -200,6 +211,7 @@ pub fn emit_neighbors(
		redaction_id_column(&center_id),
		));
		} else {
		if !has_non_denorm {
		from = TableRef::join(
		JoinType::Inner,
		from,
		@@ -210,6 +222,7 @@ pub fn emit_neighbors(
		),
		);
		where_parts.push(deleted_false(&center_id));
		}
		select.push(SelectExpr::new(
		Expr::col(&center_id, &center_redaction_col),
		redaction_id_column(&center_id),