From a1095b6415f8c4546f2242657b5e75fe8ffc6637 Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 09:28:58 -0500 Subject: [PATCH 01/12] update the logic for extracting parts --- rules/f1.js | 56 +++++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/rules/f1.js b/rules/f1.js index eaaa547..e1cf057 100644 --- a/rules/f1.js +++ b/rules/f1.js @@ -44,34 +44,40 @@ function ruleFn(match, path, project) { continue; } // TODO: Currently only checking the first reference in each block/container, should loop through them all - let regexMatch = referenceContainer + let regexMatch = referenceContainer .replace(/\b\d+\.\d+\b/g, '') // Remove decimals - .match(/(\$\{|\{\{|\{%)\s*(([^.{}]+)(\.[^.{}]+)+)\s*(%\}|\})/); - let parts = ((regexMatch || [])[2] || '').split('.').map((p)=>p.trim()).filter(Boolean); - if (!parts.length) { + .match(/(\$\{|\{\{|\{%)\s*(([^.{}]+)(\.[^.{}]+)+)\s*(%|\}\})/); + let partsList = ((regexMatch || [])[2] || '').split(' ').filter(str => str.includes('.')).map((p)=>p.split('.')).filter(Boolean); + + if (!partsList.length) { continue; } - // Don't treat references to TABLE or to own default alias as cross-view - if (parts[0] === 'TABLE' || parts[0] === view.$name) { - parts.shift(); - } - // Don't treat references to special properties as cross-view - // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields - if ([ - 'SQL_TABLE_NAME', - '_sql', - '_value', - '_name', - '_filters', - '_parameter_value', - '_rendered_value', - '_label', - '_link', - ].includes(parts[parts.length - 1]) - ) { - parts.pop(); - } - if (parts.length > 1) { + + filteredPartsList = partsList.filter((parts) => { + if (parts[0] === 'TABLE' || parts[0] === view.$name) { + return false; + } + // Don't treat references to TABLE or to own default alias as cross-view + // Don't treat references to special properties as cross-view + // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields + if ([ + 'SQL_TABLE_NAME', + '_sql', + '_value', + '_name', + '_filters', + '_parameter_value', + '_rendered_value', + '_label', + '_link', + ].includes(parts[parts.length - 1]) + ) { + return false; + } + return true; + }); + + if (filteredPartsList.length > 1) { messages.push({ level: 'error', description: `${field.$name} references another view, ${parts[0]}, via ${parts.join('.')}`, From dfa62cc2ce492a2d3bc8d57bf67901a0c393a744 Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 09:32:22 -0500 Subject: [PATCH 02/12] format --- rules/f1.js | 52 ++++++++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/rules/f1.js b/rules/f1.js index e1cf057..680781b 100644 --- a/rules/f1.js +++ b/rules/f1.js @@ -44,38 +44,38 @@ function ruleFn(match, path, project) { continue; } // TODO: Currently only checking the first reference in each block/container, should loop through them all - let regexMatch = referenceContainer + let regexMatch = referenceContainer .replace(/\b\d+\.\d+\b/g, '') // Remove decimals .match(/(\$\{|\{\{|\{%)\s*(([^.{}]+)(\.[^.{}]+)+)\s*(%|\}\})/); - let partsList = ((regexMatch || [])[2] || '').split(' ').filter(str => str.includes('.')).map((p)=>p.split('.')).filter(Boolean); + let partsList = ((regexMatch || [])[2] || '').split(' ').filter(str => str.includes('.')).map((p)=>p.split('.')).filter(Boolean); if (!partsList.length) { continue; } - - filteredPartsList = partsList.filter((parts) => { - if (parts[0] === 'TABLE' || parts[0] === view.$name) { - return false; - } - // Don't treat references to TABLE or to own default alias as cross-view - // Don't treat references to special properties as cross-view - // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields - if ([ - 'SQL_TABLE_NAME', - '_sql', - '_value', - '_name', - '_filters', - '_parameter_value', - '_rendered_value', - '_label', - '_link', - ].includes(parts[parts.length - 1]) - ) { - return false; - } - return true; - }); + + filteredPartsList = partsList.filter((parts) => { + if (parts[0] === 'TABLE' || parts[0] === view.$name) { + return false; + } + // Don't treat references to TABLE or to own default alias as cross-view + // Don't treat references to special properties as cross-view + // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields + if ([ + 'SQL_TABLE_NAME', + '_sql', + '_value', + '_name', + '_filters', + '_parameter_value', + '_rendered_value', + '_label', + '_link', + ].includes(parts[parts.length - 1]) + ) { + return false; + } + return true; + }); if (filteredPartsList.length > 1) { messages.push({ From 1ba1c3bfdcfac29c2083eeed4415f974f86dbe4c Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 23:06:09 -0500 Subject: [PATCH 03/12] refactor the f1 to support matching all fields --- rules/f1.js | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/rules/f1.js b/rules/f1.js index 680781b..ebec196 100644 --- a/rules/f1.js +++ b/rules/f1.js @@ -43,20 +43,30 @@ function ruleFn(match, path, project) { if (!referenceContainer || !referenceContainer.replace) { continue; } - // TODO: Currently only checking the first reference in each block/container, should loop through them all - let regexMatch = referenceContainer - .replace(/\b\d+\.\d+\b/g, '') // Remove decimals - .match(/(\$\{|\{\{|\{%)\s*(([^.{}]+)(\.[^.{}]+)+)\s*(%|\}\})/); - let partsList = ((regexMatch || [])[2] || '').split(' ').filter(str => str.includes('.')).map((p)=>p.split('.')).filter(Boolean); - if (!partsList.length) { + referenceContainer = referenceContainer.replace(/\b\d+\.\d+\b/g, ''); // Remove decimals + + let regexPattern = /(\$\{|\{\{|\{%)\s*(([^.{}]+)(\.[^.{}]+)+)\s*(%\}|\})/g; + + let matches = []; + let match; + while ((match = regexPattern.exec(referenceContainer)) !== null) { + matches.push(...match[2].trim().split(' ').filter((str) => str.includes('.')).filter(Boolean)); + } + // remove duplicates + let fieldPaths = matches.filter((item, index) => matches.indexOf(item) === index); + + if (!fieldPaths.length) { continue; } - - filteredPartsList = partsList.filter((parts) => { + + // filter out field paths that don't reference another view + let crossViewFieldPaths = fieldPaths.filter((fieldPath) => { + let parts = fieldPath.split('.'); if (parts[0] === 'TABLE' || parts[0] === view.$name) { return false; } + // Don't treat references to TABLE or to own default alias as cross-view // Don't treat references to special properties as cross-view // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields @@ -77,10 +87,12 @@ function ruleFn(match, path, project) { return true; }); - if (filteredPartsList.length > 1) { + if (crossViewFieldPaths.length > 0) { + // only report the first cross-view reference error + let parts = crossViewFieldPaths[0].split('.'); messages.push({ level: 'error', - description: `${field.$name} references another view, ${parts[0]}, via ${parts.join('.')}`, + description: `${field.$name} references another view, ${parts[0]}, via ${crossViewFieldPaths[0]}`, }); } } From 45fdc25793b8c29cea0405e4d4ce17caae026cca Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 23:10:14 -0500 Subject: [PATCH 04/12] simply remove duplicates logic --- rules/f1.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/f1.js b/rules/f1.js index ebec196..aa4a725 100644 --- a/rules/f1.js +++ b/rules/f1.js @@ -53,8 +53,8 @@ function ruleFn(match, path, project) { while ((match = regexPattern.exec(referenceContainer)) !== null) { matches.push(...match[2].trim().split(' ').filter((str) => str.includes('.')).filter(Boolean)); } - // remove duplicates - let fieldPaths = matches.filter((item, index) => matches.indexOf(item) === index); + + let fieldPaths = [...new Set(matches)]; // remove duplicates if (!fieldPaths.length) { continue; From 93dbfa01700cccf18c6d367fc3dfe43a213dda0b Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 23:29:50 -0500 Subject: [PATCH 05/12] add test cases --- __tests__/f1.test.js | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/__tests__/f1.test.js b/__tests__/f1.test.js index a1e9726..f828ee2 100644 --- a/__tests__/f1.test.js +++ b/__tests__/f1.test.js @@ -542,6 +542,44 @@ describe('Rules', () => { expect(result).not.toContainMessage(error); }); + it('should not error for fields with multiple references in html {% %}', () => { + let result = rule(parse(`model: my_model { + view: foo { + sql_table_name: foo ;; + dimension: bar { + html: {% if bat._value == "usd" or bat._value == "cad" %} +

\${{rendered_value}}

+ {% elsif bat._value == "eur" %} +

€{{rendered_value}}

+ {% else %} +

{{rendered_value}}

+ {% endif %} ;; + } + } + }`)); + expect(result).toContainMessage(summary(1, 0, 0)); + expect(result).toContainMessage({...F1}); + }); + + it('should error for 3-part cross-view special-suffix with multiple references in html {% %}', () => { + let result = rule(parse(`model: my_model { + view: foo { + sql_table_name: foo ;; + dimension: bar { + html: {% if bat._value == "usd" or baz.bat._value == "cad" %} +

\${{rendered_value}}

+ {% elsif bat._value == "eur" %} +

€{{rendered_value}}

+ {% else %} +

{{rendered_value}}

+ {% endif %} ;; + } + } + }`)); + expect(result).toContainMessage(summary(1, 0, 1)); + expect(result).toContainMessage({...F1, ...error}); + }); + // TODO: Expose assembleModels from parser to test extensions here without full parseFiles & dummy project test case // it('should not error for a view with extension:required', () => { // let result = rule(parse(`model: my_model { From 5a6758b206225e7c0d54cdf739c4179c61f098cc Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 23:38:30 -0500 Subject: [PATCH 06/12] only check when parts len = 2 --- rules/f1.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/f1.js b/rules/f1.js index aa4a725..d6a58f6 100644 --- a/rules/f1.js +++ b/rules/f1.js @@ -70,7 +70,7 @@ function ruleFn(match, path, project) { // Don't treat references to TABLE or to own default alias as cross-view // Don't treat references to special properties as cross-view // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields - if ([ + if (parts.length == 2 && [ 'SQL_TABLE_NAME', '_sql', '_value', From d010f77b316a578435769afcb63a18c7c6f17bd6 Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 23:41:25 -0500 Subject: [PATCH 07/12] format test cases --- __tests__/f1.test.js | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/__tests__/f1.test.js b/__tests__/f1.test.js index f828ee2..237c26f 100644 --- a/__tests__/f1.test.js +++ b/__tests__/f1.test.js @@ -547,14 +547,14 @@ describe('Rules', () => { view: foo { sql_table_name: foo ;; dimension: bar { - html: {% if bat._value == "usd" or bat._value == "cad" %} -

\${{rendered_value}}

- {% elsif bat._value == "eur" %} -

€{{rendered_value}}

- {% else %} -

{{rendered_value}}

- {% endif %} ;; - } + html: {% if bat._value == "usd" or bat._value == "cad" %} +

\${{rendered_value}}

+ {% elsif bat._value == "eur" %} +

€{{rendered_value}}

+ {% else %} +

{{rendered_value}}

+ {% endif %} ;; + } } }`)); expect(result).toContainMessage(summary(1, 0, 0)); @@ -566,14 +566,14 @@ describe('Rules', () => { view: foo { sql_table_name: foo ;; dimension: bar { - html: {% if bat._value == "usd" or baz.bat._value == "cad" %} -

\${{rendered_value}}

- {% elsif bat._value == "eur" %} -

€{{rendered_value}}

- {% else %} -

{{rendered_value}}

- {% endif %} ;; - } + html: {% if bat._value == "usd" or baz.bat._value == "cad" %} +

\${{rendered_value}}

+ {% elsif bat._value == "eur" %} +

€{{rendered_value}}

+ {% else %} +

{{rendered_value}}

+ {% endif %} ;; + } } }`)); expect(result).toContainMessage(summary(1, 0, 1)); From a631e6fb48c27e4fcc2b476a2249d48210bc5fd9 Mon Sep 17 00:00:00 2001 From: Eric Yang Date: Wed, 24 Jan 2024 23:56:56 -0500 Subject: [PATCH 08/12] update comment --- rules/f1.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/f1.js b/rules/f1.js index d6a58f6..1d142af 100644 --- a/rules/f1.js +++ b/rules/f1.js @@ -60,7 +60,7 @@ function ruleFn(match, path, project) { continue; } - // filter out field paths that don't reference another view + // find all of the field paths that uses cross view references let crossViewFieldPaths = fieldPaths.filter((fieldPath) => { let parts = fieldPath.split('.'); if (parts[0] === 'TABLE' || parts[0] === view.$name) { @@ -70,7 +70,7 @@ function ruleFn(match, path, project) { // Don't treat references to TABLE or to own default alias as cross-view // Don't treat references to special properties as cross-view // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields - if (parts.length == 2 && [ + if (parts.length == 2 && [ // only matches the fields that have two parts 'SQL_TABLE_NAME', '_sql', '_value', From 0b3e21d188b731ffbbc78c9c3b383918475ac032 Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Fri, 5 Apr 2024 14:30:28 -0400 Subject: [PATCH 09/12] move comment back to right location --- rules/f1.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rules/f1.js b/rules/f1.js index 1d142af..d2b1869 100644 --- a/rules/f1.js +++ b/rules/f1.js @@ -63,11 +63,12 @@ function ruleFn(match, path, project) { // find all of the field paths that uses cross view references let crossViewFieldPaths = fieldPaths.filter((fieldPath) => { let parts = fieldPath.split('.'); + + // Don't treat references to TABLE or to own default alias as cross-view if (parts[0] === 'TABLE' || parts[0] === view.$name) { return false; } - // Don't treat references to TABLE or to own default alias as cross-view // Don't treat references to special properties as cross-view // Note: view._in_query,_is_filtered,_is_selected should not be allowed in fields if (parts.length == 2 && [ // only matches the fields that have two parts From 920d6603be7acef60344de22805c8da6aa5e0f6e Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Fri, 5 Apr 2024 14:42:38 -0400 Subject: [PATCH 10/12] add notes for release --- docs/release-notes/v3.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/release-notes/v3.md b/docs/release-notes/v3.md index 8653de6..5600226 100644 --- a/docs/release-notes/v3.md +++ b/docs/release-notes/v3.md @@ -76,3 +76,4 @@ Patches: - v3.1.2 - Fix license metadata to consistently reflect MIT license - Update vulnerable dependencies (both in root package and tools/rule-sandbox) + - v3.1.3 - Fix rule F1 logic to check all references when there are multiple in a field ([Issue 167](https://github.com/looker-open-source/look-at-me-sideways/issues/167) [PR 168](https://github.com/looker-open-source/look-at-me-sideways/pull/168)) From e031f52b345d168ea0355b7215c6ef2687a69707 Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Fri, 5 Apr 2024 14:43:52 -0400 Subject: [PATCH 11/12] 3.1.3 --- npm-shrinkwrap.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 5ce02a4..46194ed 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,12 +1,12 @@ { "name": "@looker/look-at-me-sideways", - "version": "3.1.2", + "version": "3.1.3", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@looker/look-at-me-sideways", - "version": "3.1.2", + "version": "3.1.3", "license": "MIT", "dependencies": { "dot": "^1.1.3", diff --git a/package.json b/package.json index 70fe93c..7174b4a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@looker/look-at-me-sideways", - "version": "3.1.2", + "version": "3.1.3", "description": "A judgmental little LookML linter >_>", "main": "index.js", "directories": { From 52fd1f0abebb097035d472592a0d5be62213de92 Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Fri, 5 Apr 2024 14:51:18 -0400 Subject: [PATCH 12/12] post-publish npm shrinkwrap --- npm-shrinkwrap.dev.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/npm-shrinkwrap.dev.json b/npm-shrinkwrap.dev.json index 5ce02a4..46194ed 100644 --- a/npm-shrinkwrap.dev.json +++ b/npm-shrinkwrap.dev.json @@ -1,12 +1,12 @@ { "name": "@looker/look-at-me-sideways", - "version": "3.1.2", + "version": "3.1.3", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@looker/look-at-me-sideways", - "version": "3.1.2", + "version": "3.1.3", "license": "MIT", "dependencies": { "dot": "^1.1.3",