Merge pull request #4072 from metabase/allow-reserved-field-names-in-expressions

Case-insensitive aggregation names and allow aggregation names as field names

frontend/src/metabase/lib/expressions/index.js

@@ -6,12 +6,26 @@ import { VALID_OPERATORS, VALID_AGGREGATIONS } from "./tokens";
 
 export { VALID_OPERATORS, VALID_AGGREGATIONS } from "./tokens";
 
+const AGG_NAMES_MAP = new Map(Array.from(VALID_AGGREGATIONS).map(([short, displayName]) =>
+    // case-insensitive
+    [displayName.toLowerCase(), short]
+));
+
+export function getAggregationFromName(name) {
+    // case-insensitive
+    return AGG_NAMES_MAP.get(name.toLowerCase());
+}
+
+export function isReservedWord(word) {
+    return !!getAggregationFromName(word);
+}
+
 export function formatAggregationName(aggregationOption) {
     return VALID_AGGREGATIONS.get(aggregationOption.short);
 }
 
 function formatIdentifier(name) {
-    return /^\w+$/.test(name) ?
+    return /^\w+$/.test(name) && !isReservedWord(name) ?
         name :
         JSON.stringify(name);
 }

frontend/src/metabase/lib/expressions/parser.js

@@ -2,10 +2,9 @@ import { Lexer, Parser, getImage } from "chevrotain";
 
 import _ from "underscore";
 
-import { formatFieldName, formatExpressionName, formatAggregationName } from "../expressions";
+import { formatFieldName, formatExpressionName, formatAggregationName, getAggregationFromName } from "../expressions";
 
 import {
-    VALID_AGGREGATIONS,
     allTokens,
     LParen, RParen,
     AdditiveOperator, MultiplicativeOperator,
@@ -16,8 +15,6 @@ import {
 
 const ExpressionsLexer = new Lexer(allTokens);
 
-const aggregationsMap = new Map(Array.from(VALID_AGGREGATIONS).map(([a,b]) => [b,a]));
-
 class ExpressionsParser extends Parser {
     constructor(input, options = {}) {
         const parserOptions = {
@@ -195,7 +192,7 @@ class ExpressionsParserMBQL extends ExpressionsParser {
         return initial;
     }
     _aggregation(aggregation, lParen, arg, rParen) {
-        const agg = aggregationsMap.get(aggregation.image)
+        const agg = getAggregationFromName(getImage(aggregation));
         return arg == null ? [agg] : [agg, arg];
     }
     _metricReference(metricName, metricId) {
@@ -395,7 +392,7 @@ export function suggest(source, {
             if (!outsideAggregation) {
                 let fields = [];
                 if (startRule === "aggregation" && currentAggregationToken) {
-                    let aggregationShort = aggregationsMap.get(getImage(currentAggregationToken));
+                    let aggregationShort = getAggregationFromName(getImage(currentAggregationToken));
                     let aggregationOption = _.findWhere(tableMetadata.aggregation_options, { short: aggregationShort });
                     fields = aggregationOption && aggregationOption.fields && aggregationOption.fields[0] || []
                 } else if (startRule === "expression") {

frontend/test/unit/lib/expressions/formatter.spec.js

@@ -6,7 +6,9 @@ const mockMetadata = {
             {id: 1, display_name: "A"},
             {id: 2, display_name: "B"},
             {id: 3, display_name: "C"},
-            {id: 10, display_name: "Toucan Sam"}
+            {id: 10, display_name: "Toucan Sam"},
+            {id: 11, display_name: "count"},
+            {id: 12, display_name: "Count"}
         ],
         metrics: [
             {id: 1, name: "foo bar"},
@@ -29,6 +31,11 @@ describe("lib/expressions/parser", () => {
             expect(format(["+", ["/", ["field-id", 1], ["field-id", 10]], ["field-id", 3]], mockMetadata)).toEqual("(A / \"Toucan Sam\") + C");
         });
 
+        it("quotes fields that conflict with reserved words", () => {
+            expect(format(["+", 1, ["field-id", 11]], mockMetadata)).toEqual('1 + "count"');
+            expect(format(["+", 1, ["field-id", 12]], mockMetadata)).toEqual('1 + "Count"');
+        });
+
         it("format aggregations", () => {
             expect(format(["count"], mockMetadata)).toEqual("Count");
             expect(format(["sum", ["field-id", 1]], mockMetadata)).toEqual("Sum(A)");

frontend/test/unit/lib/expressions/parser.spec.js

@@ -7,7 +7,8 @@ const mockMetadata = {
             {id: 1, display_name: "A"},
             {id: 2, display_name: "B"},
             {id: 3, display_name: "C"},
-            {id: 10, display_name: "Toucan Sam"}
+            {id: 10, display_name: "Toucan Sam"},
+            {id: 11, display_name: "count"}
         ],
         metrics: [
             {id: 1, name: "foo bar"},
@@ -91,6 +92,12 @@ describe("lib/expressions/parser", () => {
             expect(() => compile("1 + ", expressionOpts)).toThrow();
         });
 
+        it("should treat aggregations as case-insensitive", () => {
+            expect(compile("count", aggregationOpts)).toEqual(["count"]);
+            expect(compile("cOuNt", aggregationOpts)).toEqual(["count"]);
+            expect(compile("average(A)", aggregationOpts)).toEqual(["avg", ["field-id", 1]]);
+        });
+
         // fks
         // multiple tables with the same field name resolution
     });
@@ -107,7 +114,10 @@ describe("lib/expressions/parser", () => {
         })
         it("should suggest fields after an operator", () => {
             expect(cleanSuggestions(suggest("1 + ", expressionOpts))).toEqual([
+                // quoted because has a space
                 { type: 'fields',      text: '"Toucan Sam" ' },
+                // quoted because conflicts with aggregation
+                { type: 'fields',      text: '"count" ' },
                 { type: 'fields',      text: 'A ' },
                 { type: 'fields',      text: 'B ' },
                 { type: 'fields',      text: 'C ' },
@@ -121,9 +131,16 @@ describe("lib/expressions/parser", () => {
         })
         it("should suggest partial matches in expression", () => {
             expect(cleanSuggestions(suggest("1 + C", expressionOpts))).toEqual([
+                { type: 'fields', text: '"count" ' },
                 { type: 'fields', text: 'C ' },
             ]);
         })
+        it("should suggest partial matches after an aggregation", () => {
+            expect(cleanSuggestions(suggest("average(c", expressionOpts))).toEqual([
+                { type: 'fields',      text: '"count" ' },
+                { type: 'fields',      text: 'C ' }
+            ]);
+        })
     })
 
     describe("compile() in syntax mode", () => {