Implement K8 (PK's used by primary_key). Add corner-case for K7 (pk0 …

…keys)
looker-open-source · Aug 10, 2023 · c8cefc7 · c8cefc7
1 parent b35c2f6
commit c8cefc7
Show file tree

Hide file tree

Showing 4 changed files with 193 additions and 0 deletions.
diff --git a/__tests__/k7.test.js b/__tests__/k7.test.js
@@ -61,6 +61,17 @@ describe('Rules', () => {
 			expect(result).toContainMessage({...K7, ...error});
 		});
 
+		it('should pass if no pk is defined, but a pk0 is declared', () => {
+			let result = rule(parse(`model: my_model {
+				view: foo {
+					sql_table_name: foo ;;
+					dimension: pk0_foo {}
+				}
+			}`));
+			expect(result).toContainMessage(summary(1, 0, 0));
+			expect(result).not.toContainMessage({...K7, ...error});
+		});
+
 		it('should fail if no pk is defined, including primary_key:no', () => {
 			let result = rule(parse(`model: my_model {
 				view: foo {

diff --git a/__tests__/k8.test.js b/__tests__/k8.test.js
@@ -0,0 +1,131 @@
+/* Copyright (c) 2018 Looker Data Sciences, Inc. See https://github.com/looker-open-source/look-at-me-sideways/blob/master/LICENSE.txt */
+
+require('../lib/expect-to-contain-message');
+
+const rule = require('../rules/k8');
+const {parse} = require('lookml-parser');
+
+
+let K8 = {rule: 'K8'};
+let error = {level: 'error'};
+
+let summary = (m=1, ex=0, er=1) => ({
+	...K8,
+	level: 'info',
+	description: `Rule K8 summary: ${m} matches, ${ex} matches exempt, and ${er} errors`,
+});
+
+describe('Rules', () => {
+	describe('K8', () => {
+		it('should not match and pass if there are no models', () => {
+			let result = rule(parse(`file: foo{}`));
+			expect(result).toContainMessage(summary(0, 0, 0));
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should not match and pass if there are no views', () => {
+			let result = rule(parse(`model: foo {}`));
+			expect(result).toContainMessage(summary(0, 0, 0));
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should not match and pass if there are no primary_key dimensions', () => {
+			let result = rule(parse(`model: foo { view: bar { dimension: baz {}}}`));
+			expect(result).toContainMessage(summary(0, 0, 0));
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should pass if the primary_key dimension is a 1-PK-named dimension', () => {
+			let result = rule(parse(`model: my_model {
+				view: foo {
+					sql_table_name: foo ;;
+					dimension: pk1_foo_id { primary_key: yes }
+				}
+			}`));
+			expect(result).toContainMessage(summary(1, 0, 0));
+			expect(result).not.toContainMessage({...K8, ...error});
+		});
+
+		it('should pass if the primary_key dimension uses all PK-named dimensions', () => {
+			let result = rule(parse(`model: my_model {
+				view: foo {
+					sql_table_name: foo ;;
+					dimension: pk3_foo_id {}
+					dimension: pk3_bar_id {}
+					dimension: pk3_baz_id {}
+					dimension: id {
+						primary_key: yes
+						sql: \${pk3_foo_id} || \${pk3_bar_id} || \${pk3_baz_id} ;;
+					}
+				}
+			}`));
+			expect(result).toContainMessage(summary(1, 0, 0));
+			expect(result).not.toContainMessage({...K8, ...error});
+		});
+
+		// // Not sure if anyone would ever do anything like this, or if it needs to be caught by this rule,
+		// // but at the moment, we let anything named like pk1_* pass, no matter how silly
+		//
+		// it('should fail if the primary_key dimension is a 1-PK-named dimension referencing a PK dimension', () => {
+		// 	let result = rule(parse(`model: my_model {
+		// 		view: foo {
+		// 			sql_table_name: foo ;;
+		// 			dimension: pk1_foo_id { primary_key: yes sql: \${pk1_bar_id} ;;}
+		// 		}
+		// 	}`));
+		// 	expect(result).toContainMessage(summary(1, 0, 1));
+		// 	expect(result).toContainMessage({...K8, ...error});
+		// });
+
+		it('should fail if the primary_key dimension does not reference PK dimensions', () => {
+			let result = rule(parse(`model: my_model {
+				view: foo {
+					sql_table_name: foo ;;
+					dimension: pk3_foo_id {}
+					dimension: pk3_bar_id {}
+					dimension: pk3_baz_id {}
+					dimension: id {
+						primary_key: yes
+						sql: \${TABLE}.id ;;
+					}
+				}
+			}`));
+			expect(result).toContainMessage(summary(1, 0, 1));
+			expect(result).toContainMessage({...K8, ...error});
+		});
+
+		it('should fail if the primary_key dimension references inconsistent PK dimension sizes', () => {
+			let result = rule(parse(`model: my_model {
+				view: foo {
+					sql_table_name: foo ;;
+					dimension: pk3_foo_id {}
+					dimension: pk3_bar_id {}
+					dimension: pk3_baz_id {}
+					dimension: id {
+						primary_key: yes
+						sql: \${pk1_foo_id} || \${pk2_bar_id} || \${pk3_baz_id} ;;
+					}
+				}
+			}`));
+			expect(result).toContainMessage(summary(1, 0, 1));
+			expect(result).toContainMessage({...K8, ...error});
+		});
+
+		it('should fail if the primary_key dimension does not reference enough PK dimensions', () => {
+			let result = rule(parse(`model: my_model {
+				view: foo {
+					sql_table_name: foo ;;
+					dimension: pk3_foo_id {}
+					dimension: pk3_bar_id {}
+					dimension: pk3_baz_id {}
+					dimension: id {
+						primary_key: yes
+						sql: \${pk3_foo_id} || \${pk3_baz_id} ;;
+					}
+				}
+			}`));
+			expect(result).toContainMessage(summary(1, 0, 1));
+			expect(result).toContainMessage({...K8, ...error});
+		});
+	});
+});
diff --git a/rules/k7.js b/rules/k7.js
@@ -15,6 +15,8 @@ module.exports = function(
 	return {messages};
 };
 
+const zeroPkRegex = /^(0pk|pk0)_[a-z0-9A-Z_]+$/;
+
 function ruleFn(match){
 	const view = match
 
@@ -28,6 +30,9 @@ function ruleFn(match){
 	const dimensions = Object.values(view.dimension)
 	const pkDims = dimensions.filter(d=>d.primary_key)
 	if(pkDims.length === 0){
+		if(dimensions.some(d=>d.$name.match(zeroPkRegex))){
+			return true
+		}
 		return "No primary_key:yes dimensions provided"
 	}
 	if(pkDims.length > 1){

diff --git a/rules/k8.js b/rules/k8.js
@@ -0,0 +1,46 @@
+/* Copyright (c) Looker Data Sciences, Inc. See https://github.com/looker-open-source/look-at-me-sideways/blob/master/LICENSE.txt */
+
+const checkCustomRule = require('../lib/custom-rule/custom-rule.js');
+
+module.exports = function(
+	project,
+) {
+	let ruleDef = {
+		$name: 'K8',
+		match: `$.model.*.view.*.dimension[?(@.primary_key===true)]`,
+		ruleFn
+	};
+	let messages = checkCustomRule(ruleDef, project, {ruleSource: 'internal'});
+
+	return {messages};
+};
+
+const simplePkRegex = /^(1pk|pk1?)_[a-z0-9A-Z_]+$/;
+const pkReferencesRegex = /\$\{\s*([0-9]+pk|pk[0-9]*)_[a-z0-9A-Z_]+\s*}/g;
+const unique = (x, i, arr) => arr.indexOf(x)===i;
+const min = (a, b) => a<b?a:b;
+const max = (a, b) => a>b?a:b;
+
+function ruleFn(match){
+	const dim = match
+	if(dim.$name.match(simplePkRegex)){
+		return true
+	}
+	const sql = dim.sql || "${TABLE}."+dim.$name
+	const pksReferenced = sql.match(pkReferencesRegex)
+		.map(match=>match.match(/[a-z0-9A-Z_]+/)[0])
+		.filter(unique);
+	if (pksReferenced.length===0) {
+		return `primary_key dimension is not PK-named and does not reference any PK-named fields`
+	}
+	const pkSizeDeclarations = pksReferenced.map((pk)=>parseInt(pk.match(/\d+/)||'1'))
+	const maxDeclaration = pkSizeDeclarations.reduce(max);
+	const minDeclaration = pkSizeDeclarations.reduce(min);
+	if (minDeclaration !== maxDeclaration) {
+		return `Composite primary_key's PK-named field references specify different column counts (${minDeclaration}, ${maxDeclaration})`
+	}
+	if (pksReferenced.length !== maxDeclaration) {
+		return `The number of PKs used (${pksReferenced.length}) does not match the declared number of PK columns (${maxDeclaration})`
+	}
+	return true
+}