diff --git a/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-access-denied-segment.test.ts b/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-access-denied-segment.test.ts new file mode 100644 index 0000000000000..44a23a5908b96 --- /dev/null +++ b/packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-access-denied-segment.test.ts @@ -0,0 +1,81 @@ +import { PostgresQuery } from '../../../src/adapter/PostgresQuery'; +import { prepareJsCompiler } from '../../unit/PrepareCompiler'; + +// When a cube's access policy denies the queried members, RBAC +// (CompilerApi.applyRowLevelSecurity) appends a member-expression segment +// `{ expression: () => '1 = 0', cubeName, name: 'rlsAccessDenied' }`. The rollup +// must still be selected (the `1 = 0` is just a constant filter on top of it). +// This guards that the segment doesn't disqualify pre-aggregation matching. +describe('PreAggregations access-denied segment', () => { + jest.setTimeout(200000); + + const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(` + cube('rls_visitors', { + sql: 'select * from visitors', + sqlAlias: 'rlsv', + + measures: { + count: { + type: 'count' + } + }, + + dimensions: { + id: { + type: 'number', + sql: 'id', + primaryKey: true + }, + status: { + type: 'number', + sql: 'status' + } + }, + + preAggregations: { + statusRollup: { + type: 'rollup', + measures: [CUBE.count], + dimensions: [CUBE.status], + } + } + }); + + view('rls_visitors_view', { + cubes: [ + { + join_path: 'rls_visitors', + includes: '*', + }, + ] + }); + `); + + it('selects the rollup despite the access-denied segment', async () => { + await compiler.compile(); + + const query = new PostgresQuery( + { joinGraph, cubeEvaluator, compiler }, + { + measures: ['rls_visitors_view.count'], + // Byte-for-byte the segment CompilerApi.applyRowLevelSecurity injects on denial. + segments: [ + { + expression: () => '1 = 0', + cubeName: 'rls_visitors', + name: 'rlsAccessDenied', + }, + ], + timezone: 'America/Los_Angeles', + preAggregationsSchema: '', + } + ); + + const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription(); + const [sql] = query.buildSqlAndParams(); + + expect(preAggregationsDescription[0].tableName).toEqual('rlsv_status_rollup'); + expect(sql).toContain('rlsv_status_rollup'); + expect(sql).toContain('1 = 0'); + }); +}); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/dimension_matcher.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/dimension_matcher.rs index 29edda6ca9ef0..ac7b1bf24414e 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/dimension_matcher.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/dimension_matcher.rs @@ -315,7 +315,16 @@ impl<'a> DimensionMatcher<'a> { Ok(result) } FilterItem::Segment(segment) => { - self.try_match_symbol(&segment.member_evaluator(), add_to_matched_dimension) + let symbol = segment.member_evaluator(); + // An ad-hoc member-expression segment that references no members + // (a constant like the `1 = 0` rlsAccessDenied segment RBAC injects + // on access denial) is a filter pushable on top of any rollup, so it + // doesn't need pre-aggregation coverage. Named segments still must be + // covered and fall through to the regular matcher. + if segment.is_member_expression() && symbol.get_dependencies().is_empty() { + return Ok(MatchState::Full); + } + self.try_match_symbol(&symbol, add_to_matched_dimension) } } } @@ -339,26 +348,55 @@ impl<'a> DimensionMatcher<'a> { #[cfg(test)] mod tests { use super::*; + use crate::cube_bridge::member_expression::MemberExpressionExpressionDef; + use crate::cube_bridge::member_sql::MemberSql; + use crate::cube_bridge::options_member::OptionsMember; use crate::logical_plan::optimizers::pre_aggregation::{ PreAggregationFullName, PreAggregationsCompiler, }; - use crate::test_fixtures::cube_bridge::MockSchema; + use crate::test_fixtures::cube_bridge::{ + MockMemberExpressionDefinition, MockMemberSql, MockSchema, + }; use crate::test_fixtures::test_utils::TestContext; use indoc::indoc; + use std::rc::Rc; fn create_test_context() -> TestContext { let schema = MockSchema::from_yaml_file("common/pre_aggregation_matching_test.yaml"); TestContext::new(schema).unwrap() } + /// Builds an ad-hoc member-expression segment (no registered `segments:` + /// path) from constant SQL — mirrors the `rlsAccessDenied` `1 = 0` segment. + fn const_expr_segment(name: &str, cube: &str, sql: &str) -> OptionsMember { + let member_sql: Rc = Rc::new(MockMemberSql::new(sql).unwrap()); + let expr = MockMemberExpressionDefinition::builder() + .expression_name(Some(name.to_string())) + .cube_name(Some(cube.to_string())) + .expression(MemberExpressionExpressionDef::Sql(member_sql)) + .build(); + OptionsMember::MemberExpression(Rc::new(expr)) + } + fn match_pre_agg(ctx: &TestContext, pre_agg_name: &str, query_yaml: &str) -> MatchState { + match_pre_agg_with_segments(ctx, pre_agg_name, query_yaml, vec![]) + } + + fn match_pre_agg_with_segments( + ctx: &TestContext, + pre_agg_name: &str, + query_yaml: &str, + extra_segments: Vec, + ) -> MatchState { let cube_names = vec!["orders".to_string()]; let mut compiler = PreAggregationsCompiler::try_new(ctx.query_tools().clone(), &cube_names).unwrap(); let name = PreAggregationFullName::new("orders".to_string(), pre_agg_name.to_string()); let pre_agg = compiler.compile_pre_aggregation(&name).unwrap(); - let qp = ctx.create_query_properties(query_yaml).unwrap(); + let qp = ctx + .create_query_properties_with_segments(query_yaml, extra_segments) + .unwrap(); let mut matcher = DimensionMatcher::new(ctx.query_tools().query_tools().clone(), &pre_agg); matcher .try_match( @@ -743,6 +781,30 @@ mod tests { ); } + #[test] + fn test_constant_member_expression_segment_full_match() { + let ctx = create_test_context(); + // The ad-hoc `1 = 0` rlsAccessDenied segment references no members, so it's a + // constant filter pushable on top of any rollup and imposes no coverage + // requirement: with both rollup dimensions selected the match stays Full + // (without the fix the bare segment forces NotMatched). + assert_eq!( + match_pre_agg_with_segments( + &ctx, + "main_rollup", + indoc! {" + measures: + - orders.count + dimensions: + - orders.status + - orders.city + "}, + vec![const_expr_segment("rlsAccessDenied", "orders", "1 = 0")], + ), + MatchState::Full, + ); + } + #[test] fn test_segment_not_matched_missing_in_pre_agg() { let ctx = create_test_context(); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/base_segment.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/base_segment.rs index 629ad8aa35fb1..d8003789a9d7b 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/base_segment.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/planner/filter/base_segment.rs @@ -14,6 +14,9 @@ pub struct BaseSegment { member_evaluator: Rc, cube_name: String, name: String, + /// True when this segment is an ad-hoc query-level member expression (no + /// registered `segments:` path), as opposed to a named cube segment. + is_member_expression: bool, } impl PartialEq for BaseSegment { @@ -38,6 +41,7 @@ impl BaseSegment { None, vec![cube_name.clone()], )?; + let is_member_expression = full_name.is_none(); let full_name = full_name.unwrap_or(member_expression_symbol.full_name()); let member_evaluator = MemberSymbol::new_member_expression(member_expression_symbol); @@ -46,8 +50,13 @@ impl BaseSegment { member_evaluator, cube_name, name, + is_member_expression, })) } + + pub fn is_member_expression(&self) -> bool { + self.is_member_expression + } pub fn full_name(&self) -> String { self.full_name.clone() } diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs index 8e87988639238..562db29f75e74 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/test_utils/test_context.rs @@ -1,5 +1,6 @@ use crate::cube_bridge::base_query_options::{BaseQueryOptions, MaskedMemberItem}; use crate::cube_bridge::join_hints::JoinHintItem; +use crate::cube_bridge::options_member::OptionsMember; use crate::logical_plan::PreAggregationUsage; #[cfg(feature = "integration-postgres")] use crate::logical_plan::{PreAggregation, PreAggregationSource, PreAggregationTable}; @@ -347,6 +348,17 @@ impl TestContext { /// /// Panics if YAML cannot be parsed. pub fn create_query_options_from_yaml(&self, yaml: &str) -> Rc { + self.create_query_options_from_yaml_with_segments(yaml, vec![]) + } + + /// Like `create_query_options_from_yaml`, but appends extra (typically + /// member-expression) segments that can't be expressed as YAML member + /// names — e.g. the constant `1 = 0` `rlsAccessDenied` segment RBAC injects. + pub fn create_query_options_from_yaml_with_segments( + &self, + yaml: &str, + extra_segments: Vec, + ) -> Rc { let yaml_options: YamlBaseQueryOptions = serde_yaml::from_str(yaml) .unwrap_or_else(|e| panic!("Failed to parse YAML query options: {}", e)); @@ -360,10 +372,14 @@ impl TestContext { .map(|d| members_from_strings(d)) .filter(|d| !d.is_empty()); - let segments = yaml_options - .segments - .map(|s| members_from_strings(s)) - .filter(|s| !s.is_empty()); + let segments = { + let mut segments = yaml_options + .segments + .map(|s| members_from_strings(s)) + .unwrap_or_default(); + segments.extend(extra_segments); + Some(segments).filter(|s| !s.is_empty()) + }; let order = yaml_options .order @@ -444,7 +460,15 @@ impl TestContext { } pub fn create_query_properties(&self, yaml: &str) -> Result, CubeError> { - let options = self.create_query_options_from_yaml(yaml); + self.create_query_properties_with_segments(yaml, vec![]) + } + + pub fn create_query_properties_with_segments( + &self, + yaml: &str, + extra_segments: Vec, + ) -> Result, CubeError> { + let options = self.create_query_options_from_yaml_with_segments(yaml, extra_segments); QueryPropertiesCompiler::new(self.query_tools.clone()).build(options) } @@ -469,7 +493,15 @@ impl TestContext { &self, query: &str, ) -> Result<(String, Vec), cubenativeutils::CubeError> { - let options = self.create_query_options_from_yaml(query); + self.build_sql_with_used_pre_aggregations_with_segments(query, vec![]) + } + + pub fn build_sql_with_used_pre_aggregations_with_segments( + &self, + query: &str, + extra_segments: Vec, + ) -> Result<(String, Vec), cubenativeutils::CubeError> { + let options = self.create_query_options_from_yaml_with_segments(query, extra_segments); let ctx = self.for_options(options.as_ref())?; let request = QueryPropertiesCompiler::new(ctx.query_tools.clone()).build(options)?; let planner = TopLevelPlanner::new(request, ctx.query_tools.clone(), true); diff --git a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs index db0e234d220bd..83f26924ed5e2 100644 --- a/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs +++ b/rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/sql_generation.rs @@ -3,9 +3,15 @@ //! These tests verify that queries correctly match and use pre-aggregations, //! checking that the generated SQL contains references to pre-aggregation tables. -use crate::test_fixtures::cube_bridge::MockSchema; +use crate::cube_bridge::member_expression::MemberExpressionExpressionDef; +use crate::cube_bridge::member_sql::MemberSql; +use crate::cube_bridge::options_member::OptionsMember; +use crate::test_fixtures::cube_bridge::{ + MockMemberExpressionDefinition, MockMemberSql, MockSchema, +}; use crate::test_fixtures::test_utils::TestContext; use indoc::indoc; +use std::rc::Rc; #[tokio::test(flavor = "multi_thread")] async fn test_basic_pre_agg_sql() { @@ -495,6 +501,46 @@ async fn test_base_and_calculated_measure_parital_match() { // --- Segment matching tests --- +// When a cube's access policy denies the queried members, RBAC +// (CompilerApi.applyRowLevelSecurity) appends a member-expression segment +// `{ expression: () => '1 = 0', cubeName, name: 'rlsAccessDenied' }`. It +// references no members (empty dependencies), so it's a constant filter on top +// of the rollup and must not disqualify pre-aggregation matching. +#[tokio::test(flavor = "multi_thread")] +async fn test_constant_member_expression_segment_keeps_pre_aggregation() { + let schema = MockSchema::from_yaml_file("common/pre_aggregation_matching_test.yaml") + .only_pre_aggregations(&["main_rollup"]); + let ctx = TestContext::new(schema).unwrap(); + + let query_yaml = indoc! {" + measures: + - orders.count + dimensions: + - orders.status + "}; + + let access_denied_segment = { + let sql: Rc = Rc::new(MockMemberSql::new("1 = 0").unwrap()); + let expr = MockMemberExpressionDefinition::builder() + .expression_name(Some("rlsAccessDenied".to_string())) + .cube_name(Some("orders".to_string())) + .expression(MemberExpressionExpressionDef::Sql(sql)) + .build(); + OptionsMember::MemberExpression(Rc::new(expr)) + }; + + let (sql, pre_aggrs) = ctx + .build_sql_with_used_pre_aggregations_with_segments(query_yaml, vec![access_denied_segment]) + .unwrap(); + + assert_eq!(pre_aggrs.len(), 1); + assert_eq!(pre_aggrs[0].name(), "main_rollup"); + assert!( + sql.contains("1 = 0"), + "expected the constant access-denied segment in SQL, got:\n{sql}" + ); +} + #[tokio::test(flavor = "multi_thread")] async fn test_segment_full_match() { let schema = MockSchema::from_yaml_file("common/pre_aggregation_matching_test.yaml")