feat: enforce only aggregate expressions are used with group by (#19)

Author: YoEight

src/analysis.rs

@@ -653,6 +653,9 @@ impl<'a> Analysis<'a> {
             if let Some(expr) = &group_by.predicate {
                 self.analyze_expr(&mut ctx, expr, Type::Bool)?;
             }
+
+            ctx.allow_agg_func = true;
+            ctx.use_agg_funcs = true;
         }
 
         let project = self.analyze_projection(&mut ctx, &query.projection)?;
@@ -742,7 +745,12 @@ impl<'a> Analysis<'a> {
 
                 ctx.allow_agg_func = true;
                 let tpe = self.analyze_expr(ctx, expr, Type::Unspecified)?;
-                self.check_projection_on_record(&mut CheckContext::default(), record.as_slice())?;
+                let mut chk_ctx = CheckContext {
+                    use_agg_func: ctx.use_agg_funcs,
+                    ..Default::default()
+                };
+
+                self.check_projection_on_record(&mut chk_ctx, record.as_slice())?;
                 Ok(tpe)
             }
 
@@ -752,14 +760,24 @@ impl<'a> Analysis<'a> {
                 let tpe = self.analyze_expr(ctx, expr, Type::Unspecified)?;
 
                 if ctx.use_agg_funcs {
-                    self.check_projection_on_field_expr(&mut CheckContext::default(), expr)?;
+                    let mut chk_ctx = CheckContext {
+                        use_agg_func: ctx.use_agg_funcs,
+                        ..Default::default()
+                    };
+
+                    self.check_projection_on_field_expr(&mut chk_ctx, expr)?;
                 } else {
                     self.reject_constant_func(&expr.attrs, app)?;
                 }
 
                 Ok(tpe)
             }
 
+            Value::Id(_) if ctx.use_agg_funcs => Err(AnalysisError::ExpectAggExpr(
+                expr.attrs.pos.line,
+                expr.attrs.pos.col,
+            )),
+
             Value::Id(id) => {
                 if let Some(tpe) = self.scope.entries.get(id.as_str()).cloned() {
                     Ok(tpe)
@@ -772,6 +790,11 @@ impl<'a> Analysis<'a> {
                 }
             }
 
+            Value::Access(_) if ctx.use_agg_funcs => Err(AnalysisError::ExpectAggExpr(
+                expr.attrs.pos.line,
+                expr.attrs.pos.col,
+            )),
+
             Value::Access(access) => {
                 let mut current = &access.target.value;
 

src/tests/analysis.rs

@@ -235,3 +235,24 @@ fn test_analyze_accept_group_by_with_order_by_with_agg() {
     .unwrap();
     insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default()));
 }
+
+#[test]
+fn test_analyze_reject_group_by_no_agg() {
+    let query = parse_query(include_str!("./resources/reject_group_by_no_agg.eql")).unwrap();
+    insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default()));
+}
+
+#[test]
+fn test_analyze_reject_group_by_no_agg_in_rec() {
+    let query = parse_query(include_str!(
+        "./resources/reject_group_by_no_agg_in_rec.eql"
+    ))
+    .unwrap();
+    insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default()));
+}
+
+#[test]
+fn test_analyze_accept_group_by_with_agg_rec() {
+    let query = parse_query(include_str!("./resources/accept_group_by_with_agg_rec.eql")).unwrap();
+    insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default()));
+}

src/tests/resources/accept_group_by_with_agg_rec.eql

@@ -0,0 +1,7 @@
+
+FROM e IN events
+GROUP BY e.data.department
+PROJECT INTO {
+	department: UNIQUE(e.data.department),
+	cost: AVG(e.data.salary)
+}

src/tests/resources/reject_group_by_no_agg.eql

@@ -0,0 +1,3 @@
+FROM e IN events
+GROUP BY e.data.department
+PROJECT INTO e.data.salary

src/tests/resources/reject_group_by_no_agg_in_rec.eql

@@ -0,0 +1,5 @@
+FROM e IN events
+GROUP BY e.data.department
+PROJECT INTO {
+	cost: e.data.salary
+}

src/tests/snapshots/eventql_parser__tests__analysis__analyze_accept_group_by_with_agg_rec.snap

@@ -0,0 +1,123 @@
+---
+source: src/tests/analysis.rs
+expression: "query.run_static_analysis(&Default::default())"
+---
+Ok:
+  attrs:
+    pos:
+      line: 2
+      col: 1
+  sources:
+    - binding:
+        name: e
+        pos:
+          line: 2
+          col: 6
+      kind:
+        Name: events
+  predicate: ~
+  group_by:
+    expr:
+      attrs:
+        pos:
+          line: 3
+          col: 10
+      value:
+        Access:
+          target:
+            attrs:
+              pos:
+                line: 3
+                col: 10
+            value:
+              Access:
+                target:
+                  attrs:
+                    pos:
+                      line: 3
+                      col: 10
+                  value:
+                    Id: e
+                field: data
+          field: department
+    predicate: ~
+  order_by: ~
+  limit: ~
+  projection:
+    attrs:
+      pos:
+        line: 4
+        col: 14
+    value:
+      Record:
+        - name: department
+          value:
+            attrs:
+              pos:
+                line: 5
+                col: 14
+            value:
+              App:
+                func: UNIQUE
+                args:
+                  - attrs:
+                      pos:
+                        line: 5
+                        col: 21
+                    value:
+                      Access:
+                        target:
+                          attrs:
+                            pos:
+                              line: 5
+                              col: 21
+                          value:
+                            Access:
+                              target:
+                                attrs:
+                                  pos:
+                                    line: 5
+                                    col: 21
+                                value:
+                                  Id: e
+                              field: data
+                        field: department
+        - name: cost
+          value:
+            attrs:
+              pos:
+                line: 6
+                col: 8
+            value:
+              App:
+                func: AVG
+                args:
+                  - attrs:
+                      pos:
+                        line: 6
+                        col: 12
+                    value:
+                      Access:
+                        target:
+                          attrs:
+                            pos:
+                              line: 6
+                              col: 12
+                          value:
+                            Access:
+                              target:
+                                attrs:
+                                  pos:
+                                    line: 6
+                                    col: 12
+                                value:
+                                  Id: e
+                              field: data
+                        field: salary
+  distinct: false
+  meta:
+    project:
+      Record:
+        cost: Number
+        department: Unspecified
+    aggregate: true

src/tests/snapshots/eventql_parser__tests__analysis__analyze_reject_group_by_no_agg.snap

@@ -0,0 +1,9 @@
+---
+source: src/tests/analysis.rs
+expression: "query.run_static_analysis(&Default::default())"
+---
+Err:
+  Analysis:
+    ExpectAggExpr:
+      - 3
+      - 14

src/tests/snapshots/eventql_parser__tests__analysis__analyze_reject_group_by_no_agg_in_rec.snap

@@ -0,0 +1,9 @@
+---
+source: src/tests/analysis.rs
+expression: "query.run_static_analysis(&Default::default())"
+---
+Err:
+  Analysis:
+    UnallowedAggFuncUsageWithSrcField:
+      - 4
+      - 8