From 7cfa2bac07029c7555a89df2b940343332289828 Mon Sep 17 00:00:00 2001
From: Fabio Beltramini <fabble@google.com>
Date: Wed, 9 Aug 2023 18:46:49 -0400
Subject: [PATCH] Corrected & renamed Rule E2.1 -> E6

---
 README.md              |   3 +-
 __tests__/e2-1.test.js | 186 -----------------------------------------
 __tests__/e6.test.js   | 171 +++++++++++++++++++++++++++++++++++++
 docs/rules.html        |  43 ++++++++++
 rules/e2-1.js          |  34 --------
 rules/e6.js            |  33 ++++++++
 6 files changed, 249 insertions(+), 221 deletions(-)
 delete mode 100644 __tests__/e2-1.test.js
 create mode 100644 __tests__/e6.test.js
 delete mode 100644 rules/e2-1.js
 create mode 100644 rules/e6.js

diff --git a/README.md b/README.md
index a22b475..b7544de 100644
--- a/README.md
+++ b/README.md
@@ -43,10 +43,11 @@ As of LAMS v3, you must opt-in via your `manifest.lkml` file to use the built-in
 #rule: F4{} # Description or hidden
 #rule: E1{} # Join with subst'n operator
 #rule: E2{} # Join on PK for "one" joins
+#rule: E6{} # FK joins are m:1
 #rule: T1{} # Triggers use datagroups 
 #rule: T2{} # Primary keys in DT
 #rule: W1{} # Block indentation
-
+#rule: W1{} # Block indentation
 ```
 
 ### Rule Exemptions
diff --git a/__tests__/e2-1.test.js b/__tests__/e2-1.test.js
deleted file mode 100644
index 97bb361..0000000
--- a/__tests__/e2-1.test.js
+++ /dev/null
@@ -1,186 +0,0 @@
-/* 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/e2-1.js');
-const {parse} = require('lookml-parser');
-
-describe('Rules', () => {
-	describe('E2.1', () => {
-		let info = {level: 'info'};
-		let error = {level: 'error'};
-		let e2_1 = {rule: 'E2.1'};
-
-		it('should not match and pass if there are no models', () => {
-			let result = rule(parse(`file: foo{}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 0 matches, 0 matches exempt, and 0 errors"
-			});
-			expect(result).not.toContainMessage(error);
-		});
-
-		it('should not match and pass if there are no explores', () => {
-			let result = rule(parse(`model: foo {}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 0 matches, 0 matches exempt, and 0 errors"
-			});
-			expect(result).not.toContainMessage(error);
-		});
-
-		it('should not match and pass if there are no joins', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 0 matches, 0 matches exempt, and 0 errors"
-			});
-			expect(result).not.toContainMessage(error);
-		});
-
-		it('should not match and pass for joins without foreign_key', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						relationship: many_to_one
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 0 matches, 0 matches exempt, and 0 errors"
-			});
-			expect(result).not.toContainMessage(error);
-		});
-
-		it('should pass for *_to_many joins', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: order_items {
-						foreign_key: order_id
-						relationship: one_to_many
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 0 errors"
-			});
-			expect(result).not.toContainMessage(error);
-		});
-
-		it('should pass if a many_to_one join correctly uses a 1pk foreign key', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						relationship: many_to_one
-						foreign_key: pk1_user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 0 errors"
-			});
-			expect(result).not.toContainMessage(error);
-		});
-
-		it('should pass if a many_to_one join correctly uses an implicit 1pk equality constraint', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						relationship: many_to_one
-						foreign_key: pk_user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 0 errors"
-			});
-			expect(result).not.toContainMessage(error);
-		});
-
-		it('should error if a many_to_one join uses no PK dimensions', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						relationship: many_to_one
-						foreign_key: user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 1 errors"
-			});
-			expect(result).toContainMessage({...e2_1, ...error});
-		});
-
-		it('should error if a many_to_one foreign_key join uses a PK dimensions of non-1 size', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						relationship: many_to_one
-						foreign_key: pk2_user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 1 errors"
-			});
-			expect(result).toContainMessage({...e2_1, ...error});
-		});
-
-		it('should error if a one_to_one join uses no PK dimensions', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						relationship: one_to_one
-						foreign_key: user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 1 errors"
-			});
-			expect(result).toContainMessage({...e2_1, ...error});
-		});
-
-		it('should error if a many_to_one foreign_key join uses a PK dimensions of non-1 size', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						relationship: one_to_one
-						foreign_key: pk2_user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 1 errors"
-			});
-			expect(result).toContainMessage({...e2_1, ...error});
-		});
-
-		it('should error if an unspecified cardinality (implicitly many_to_one) join uses no PK dimensions', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						foreign_key: user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 1 errors"
-			});
-			expect(result).toContainMessage({...e2_1, ...error});
-		});
-
-		it('should error if an unspecified cardinality (implicitly many_to_one) foreign_key join uses a PK dimensions of non-1 size', () => {
-			let result = rule(parse(`model: m {
-				explore: orders {
-					join: users {
-						foreign_key: pk2_user_id
-					}
-				}
-			}`));
-			expect(result).toContainMessage({...e2_1, ...info,
-				description: "Rule E2.1 summary: 1 matches, 0 matches exempt, and 1 errors"
-			});
-			expect(result).toContainMessage({...e2_1, ...error});
-		});
-	});
-});
diff --git a/__tests__/e6.test.js b/__tests__/e6.test.js
new file mode 100644
index 0000000..38a5cb9
--- /dev/null
+++ b/__tests__/e6.test.js
@@ -0,0 +1,171 @@
+/* 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/e6.js');
+const {parse} = require('lookml-parser');
+
+describe('Rules', () => {
+	describe('E6', () => {
+		let info = {level: 'info'};
+		let error = {level: 'error'};
+		let e6 = {rule: 'E6'};
+
+		it('should not match and pass if there are no models', () => {
+			let result = rule(parse(`file: foo{}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 0 matches, 0 matches exempt, and 0 errors"
+			});
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should not match and pass if there are no explores', () => {
+			let result = rule(parse(`model: foo {}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 0 matches, 0 matches exempt, and 0 errors"
+			});
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should not match and pass if there are no joins', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 0 matches, 0 matches exempt, and 0 errors"
+			});
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should not match and pass for joins without foreign_key', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: users {
+						relationship: many_to_one
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 0 matches, 0 matches exempt, and 0 errors"
+			});
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should pass for implicit m:1 relationship', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: users {
+						foreign_key: user_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 0 errors"
+			});
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should pass for explicit m:1 relationship', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: users {
+						relationship: many_to_one
+						foreign_key: user_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 0 errors"
+			});
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should pass when the FK is the base-view\'s PK and uses 1:1 relationship', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: order_derived_columns {
+						relationship: one_to_one
+						foreign_key: pk1_order_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 0 errors"
+			});
+			expect(result).not.toContainMessage(error);
+		});
+
+		it('should error if a *:m relationship is specified', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: users {
+						relationship: one_to_many
+						foreign_key: user_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 1 errors"
+			});
+			expect(result).toContainMessage({...e6, ...error});
+		});
+
+		it('should error if a 1:1 relationship is specified', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: users {
+						relationship: one_to_one
+						foreign_key: user_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 1 errors"
+			});
+			expect(result).toContainMessage({...e6, ...error});
+		});
+
+		it('should error if a *:m relationship is specified for joins from a pk', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: order_derived_columns {
+						relationship: one_to_many
+						foreign_key: pk1_order_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 1 errors"
+			});
+			expect(result).toContainMessage({...e6, ...error});
+		});
+
+		it('should error if a m:1 relationship is specified for joins from a pk', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: order_derived_columns {
+						relationship: many_to_one
+						foreign_key: pk1_order_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 1 errors"
+			});
+			expect(result).toContainMessage({...e6, ...error});
+		});
+
+		it('should error if a m:1 relationship is implicit for joins from a pk', () => {
+			let result = rule(parse(`model: m {
+				explore: orders {
+					join: order_derived_columns {
+						foreign_key: pk1_order_id
+					}
+				}
+			}`));
+			expect(result).toContainMessage({...e6, ...info,
+				description: "Rule E6 summary: 1 matches, 0 matches exempt, and 1 errors"
+			});
+			expect(result).toContainMessage({...e6, ...error});
+		});
+	});
+});
diff --git a/docs/rules.html b/docs/rules.html
index 3a1fd34..cd89495 100644
--- a/docs/rules.html
+++ b/docs/rules.html
@@ -714,6 +714,49 @@ <h1 id="explores">Explores</h1>
 	</code>
 	<p><b>Note:</b> The above strategy relies on enforcement of rule F3. See <a href="https://discourse.looker.com/t/a-cure-for-the-one-to-many-blues/6458">this article</a> for more details.</p>
 	</details>
+<details id="e6"> <!-- foreign_key joins must be *-to-one -->
+	<summary><b><a href="#e6">E6</a></b> If using a <code>foreign_key</code> based join, do not use a *-to-many relationship</summary>
+	<p><b>Note:</b> Specifying join logic using <code>sql_on</code> is usually preferred to using <code>foreign_key</code>, as <code>sql_on</code> is more general-purpose.</p>
+	<p>If you do use <code>foreign_key</code> to specify your join logic, Looker will implicitly join this declared foreign key against the primary key of the joined table. As such, the resulting join should be a many_to_one join (or rarely one_to_one if you use the primary key of the base view as the foreign key in the join)</p>
+	
+	<code class="bad block">
+	explore: orders {
+		join: users {
+			foreign_key: user_id
+			relationship: many_to_many
+		}
+	}
+	</code>
+
+	<code class="good block">
+	explore: orders {
+		join: users {
+			foreign_key: user_id
+			relationship: many_to_one
+		}
+	}
+	</code>
+
+	<code class="good block">
+	# Joins are already implicitly many_to_one by default, so this is ok
+	explore: orders {
+		join: users {
+			foreign_key: user_id
+		}
+	}
+	</code>
+
+	<code class="good block">
+	# In some cases, one_to_one is appropriate
+	explore: orders {
+		join: orders_derived_table {
+			foreign_key: pk1_order_id
+			relationship: one_to_one
+		}
+	}
+	</code>
+
+	</details>
 
 
 
diff --git a/rules/e2-1.js b/rules/e2-1.js
deleted file mode 100644
index 6ada572..0000000
--- a/rules/e2-1.js
+++ /dev/null
@@ -1,34 +0,0 @@
-/* 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');
-// const deepGet = require('../lib/deep-get.js');
-
-module.exports = function(
-	project,
-) {
-	let ruleDef = {
-		$name: 'E2.1',
-		match: `$.model.*.explore.*.join[?(@.foreign_key)]`,
-		ruleFn,
-	};
-	let messages = checkCustomRule(ruleDef, project, {ruleSource: 'internal'});
-
-	return {messages};
-};
-
-function ruleFn(match, path, project) {
-	const join = match;
-	const pkRegex = /^([0-9]+pk|pk[0-9]*)_([a-z0-9A-Z_]+)$/;
-	if(join.relationship === "one_to_many" || join.relationship === "many_to_many"){
-		return true
-	}
-	const pkRegexMatch = join.foreign_key.match(pkRegex)
-	if(!pkRegexMatch){
-		return `${join.$name} is a *-to-one join that does not join on a PK-named dimension` 
-	}
-	const declaredKeySize = pkRegexMatch[1].replace("pk","") || "1"
-	if(declaredKeySize !== "1"){
-		return `${join.$name} is a *-to-one join that uses a single field as a foreign_key whose declared key size is ${declaredKeySize} keys`
-	}
-	return true
-}
diff --git a/rules/e6.js b/rules/e6.js
new file mode 100644
index 0000000..356b462
--- /dev/null
+++ b/rules/e6.js
@@ -0,0 +1,33 @@
+/* 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: 'E6',
+		match: `$.model.*.explore.*.join[?(@.foreign_key)]`,
+		description: "Foreign-key based joins may not be *-to-many",
+		ruleFn,
+	};
+	let messages = checkCustomRule(ruleDef, project, {ruleSource: 'internal'});
+
+	return {messages};
+};
+
+function ruleFn(match, path, project) {
+	const join = match;
+	const describeRelationship = join.relationship || "implicitly many_to_one"
+	const simplePkRegex = /^(1pk|pk1?)_([a-z0-9A-Z_]+)$/;
+	if(join.foreign_key.match(simplePkRegex)){
+		//Foreign key is also the base view's primary key. The relationship must be one_to_one
+		if(join.relationship === "one_to_one"){return true}
+		return `A foreign_key join from a primary key should be one_to_one, but ${join.$name} is ${describeRelationship}`
+	}
+	else{
+		if(join.relationship === "many_to_one"){return true}
+		if(join.relationship === undefined){return true}
+		return `A foreign_key join should be many_to_one, but ${join.$name} is ${describeRelationship}`
+	}
+}