Repository: sqlfluff/sqlfluff
Pull request: Add rule L051 to forbid lone JOIN
<!--Firstly, thanks for adding this feature! Secondly, please check the key steps against the checklist below to make your contribution easy to merge.-->

<!--Please give the Pull Request a meaningful title (including the dialect this PR is for if it is dialect specific), as this will automatically be added to the release notes, and then the Change Log.-->

### Brief summary of the change made
<!--If there is an open issue for this, then please include `fixes #XXXX` or `closes #XXXX` replacing `XXXX` with the issue number and it will automatically close the issue when the pull request is merged. Alternatively if not fully closed you can say `makes progress on #XXXX` to create a link on that issue without closing it.-->
New rule to forbid the use of lone JOIN. It also provides a fix by replacing lone JOIN with INNER JOIN.
Logic if pretty simple and have included test cases. Fixes #1712 

![Screen-Recording-2021-11-10-at-18 13 13](https://user-images.githubusercontent.com/80432516/141173888-58635ee0-b41f-432d-a98e-0a51bfb7a520.gif)


### Are there any other side effects of this change that we should be aware of?
No

### Pull Request checklist
- [X] Please confirm you have completed any of the necessary steps below.

- Included test cases to demonstrate any code changes, which may be one or more of the following:
  - `.yml` rule test cases in `test/fixtures/rules/std_rule_cases`.
  - `.sql`/`.yml` parser test cases in `test/fixtures/dialects` (note YML files can be auto generated with `python test/generate_parse_fixture_yml.py` or by running `tox` locally).
  - Full autofix test cases in `test/fixtures/linter/autofix`.
  - Other.
- Added appropriate documentation for the change.
- Created GitHub issues for any relevant followup/future enhancements if appropriate.


Problem statement:
Add rule to not allow plain JOIN
Add rule to not allow "naked" joins, i.e. `JOIN`.

```sql
SELECT
  ...
FROM
  customer
  -- Should be an explicit join type, e.g. INNER JOIN or LEFT JOIN
  JOIN orders ON customer.id = orders.customer_id
```

Potentially, you could also add a setting to assume all naked `JOIN`s are `INNER JOIN`s (as most dialects do) and implement that when running `fix`.

```sql
SELECT
  ...
FROM
  customer
  -- Fixed
  INNER JOIN orders ON customer.id = orders.customer_id
```

Hints from issue discussion:
Would be very similar to rule L033 if anyone is looking to implement this and wants to know where to start.
@tunetheweb could I be assigned this one please :)
Sure

We don't really use the assign label that much on this project so feel free to just leave a comment like "I'll look at this one" and then we'll consider it yours!

Commit message: Merge branch 'main' into no_lone_into

Patch to review:
diff --git a/src/sqlfluff/rules/L051.py b/src/sqlfluff/rules/L051.py
new file mode 100644
index 00000000000..e3c5c3fd5b0
--- /dev/null
+++ b/src/sqlfluff/rules/L051.py
@@ -0,0 +1,60 @@
+"""Implementation of Rule L051."""
+from typing import Optional
+from sqlfluff.core.parser.segments.raw import KeywordSegment, WhitespaceSegment
+
+from sqlfluff.core.rules.base import BaseRule, LintResult, LintFix, RuleContext
+from sqlfluff.core.rules.doc_decorators import document_fix_compatible
+
+
+@document_fix_compatible
+class Rule_L051(BaseRule):
+    """INNER JOIN must be fully qualified.
+
+    | **Anti-pattern**
+    | Lone JOIN is used.
+
+    .. code-block:: sql
+       :force:
+
+        SELECT
+            foo
+        FROM bar
+        JOIN baz;
+
+    | **Best practice**
+    | Use INNER JOIN rather than JOIN.
+
+    .. code-block:: sql
+       :force:
+
+        SELECT
+            foo
+        FROM bar
+        INNER JOIN baz;
+    """
+
+    def _eval(self, context: RuleContext) -> Optional[LintResult]:
+        """INNER JOIN must be fully qualified."""
+        # We are only interested in JOIN clauses.
+        if context.segment.type != "join_clause":
+            return None
+
+        # We identify non-lone JOIN by looking at first child segment.
+        if context.segment.segments[0].name != "join":
+            return None
+
+        # Replace lone JOIN with INNER JOIN.
+        return LintResult(
+            context.segment.segments[0],
+            fixes=[
+                LintFix(
+                    "edit",
+                    context.segment.segments[0],
+                    [
+                        KeywordSegment("INNER"),
+                        WhitespaceSegment(),
+                        context.segment.segments[0],
+                    ],
+                )
+            ],
+        )
diff --git a/test/cli/commands_test.py b/test/cli/commands_test.py
index 9adc90fd49d..cae0f43fae3 100644
--- a/test/cli/commands_test.py
+++ b/test/cli/commands_test.py
@@ -149,20 +149,49 @@ def test__cli__command_lint_stdin(command):
     "command",
     [
         # Test basic linting
-        (lint, ["-n", "test/fixtures/cli/passing_b.sql"]),
+        (lint, ["-n", "test/fixtures/cli/passing_b.sql", "--exclude-rules", "L051"]),
         # Original tests from test__cli__command_lint
         (lint, ["-n", "test/fixtures/cli/passing_a.sql"]),
         (lint, ["-n", "-v", "test/fixtures/cli/passing_a.sql"]),
         (lint, ["-n", "-vvvv", "test/fixtures/cli/passing_a.sql"]),
         (lint, ["-vvvv", "test/fixtures/cli/passing_a.sql"]),
         # Test basic linting with very high verbosity
-        (lint, ["-n", "test/fixtures/cli/passing_b.sql", "-vvvvvvvvvvv"]),
+        (
+            lint,
+            [
+                "-n",
+                "test/fixtures/cli/passing_b.sql",
+                "-vvvvvvvvvvv",
+                "--exclude-rules",
+                "L051",
+            ],
+        ),
         # Test basic linting with specific logger
-        (lint, ["-n", "test/fixtures/cli/passing_b.sql", "-vvv", "--logger", "parser"]),
+        (
+            lint,
+            [
+                "-n",
+                "test/fixtures/cli/passing_b.sql",
+                "-vvv",
+                "--logger",
+                "parser",
+                "--exclude-rules",
+                "L051",
+            ],
+        ),
         # Check basic parsing
-        (parse, ["-n", "test/fixtures/cli/passing_b.sql"]),
+        (parse, ["-n", "test/fixtures/cli/passing_b.sql", "--exclude-rules", "L051"]),
         # Test basic parsing with very high verbosity
-        (parse, ["-n", "test/fixtures/cli/passing_b.sql", "-vvvvvvvvvvv"]),
+        (
+            parse,
+            [
+                "-n",
+                "test/fixtures/cli/passing_b.sql",
+                "-vvvvvvvvvvv",
+                "--exclude-rules",
+                "L051",
+            ],
+        ),
         # Check basic parsing, with the code only option
         (parse, ["-n", "test/fixtures/cli/passing_b.sql", "-c"]),
         # Check basic parsing, with the yaml output
@@ -171,8 +200,26 @@ def test__cli__command_lint_stdin(command):
         # Check the profiler and benching commands
         (parse, ["-n", "test/fixtures/cli/passing_b.sql", "--profiler"]),
         (parse, ["-n", "test/fixtures/cli/passing_b.sql", "--bench"]),
-        (lint, ["-n", "test/fixtures/cli/passing_b.sql", "--bench"]),
-        (fix, ["-n", "test/fixtures/cli/passing_b.sql", "--bench"]),
+        (
+            lint,
+            [
+                "-n",
+                "test/fixtures/cli/passing_b.sql",
+                "--bench",
+                "--exclude-rules",
+                "L051",
+            ],
+        ),
+        (
+            fix,
+            [
+                "-n",
+                "test/fixtures/cli/passing_b.sql",
+                "--bench",
+                "--exclude-rules",
+                "L051",
+            ],
+        ),
         # Check linting works in specifying rules
         (lint, ["-n", "--rules", "L001", "test/fixtures/linter/operator_errors.sql"]),
         # Check linting works in specifying multiple rules
diff --git a/test/fixtures/rules/std_rule_cases/L051.yml b/test/fixtures/rules/std_rule_cases/L051.yml
new file mode 100644
index 00000000000..bcde7b2cab7
--- /dev/null
+++ b/test/fixtures/rules/std_rule_cases/L051.yml
@@ -0,0 +1,26 @@
+rule: L051
+
+test_fail_lone_join:
+  fail_str: "SELECT foo.a, bar.b FROM foo JOIN bar;\n"
+  fix_str: "SELECT foo.a, bar.b FROM foo INNER JOIN bar;\n"
+
+test_pass_inner_join:
+  pass_str: "SELECT foo.a, bar.b FROM foo INNER JOIN bar;\n"
+
+test_pass_left_join:
+  pass_str: "SELECT foo.a, bar.b FROM foo LEFT JOIN bar;\n"
+
+test_pass_right_join:
+  pass_str: "SELECT foo.a, bar.b FROM foo RIGHT JOIN bar;\n"
+
+test_pass_full_join:
+  pass_str: "SELECT foo.a, bar.b FROM foo FULL JOIN bar;\n"
+
+test_pass_left_outer_join:
+  pass_str: "SELECT foo.a, bar.b FROM foo LEFT OUTER JOIN bar;\n"
+
+test_pass_right_outer_join:
+  pass_str: "SELECT foo.a, bar.b FROM foo RIGHT OUTER JOIN bar;\n"
+
+test_pass_full_outer_join:
+  pass_str: "SELECT foo.a, bar.b FROM foo FULL OUTER JOIN bar;\n"


Code review comments:
diff_hunk: @@ -0,0 +1,60 @@
+"""Implementation of Rule L051."""
+from typing import Optional
+from sqlfluff.core.parser.segments.raw import KeywordSegment, WhitespaceSegment
+
+from sqlfluff.core.rules.base import BaseRule, LintResult, LintFix, RuleContext
+from sqlfluff.core.rules.doc_decorators import document_fix_compatible
+
+
+@document_fix_compatible
+class Rule_L051(BaseRule):
+    """INNER JOIN must be fully qualified.
+
+    | **Anti-pattern**
+    | Lone JOIN is used.
+
+    .. code-block:: sql
+       :force:
+
+        SELECT
+            foo
+        FROM bar
+        JOIN baz;
+
+    | **Best practice**
+    | Use INNER JOIN rather than JOIN.
+
+    .. code-block:: sql
+       :force:
+
+        SELECT
+            foo
+        FROM bar
+        INNER JOIN baz;
+    """
+
+    def _eval(self, context: RuleContext) -> Optional[LintResult]:
+        """INNER JOIN must be fully qualified."""
+        # We are only interested in JOIN clauses.
+        if context.segment.type != "join_clause":
+            return None
+
+        # We identify non-lone JOIN by looking at first child segment.
+        if context.segment.segments[0].name != "join":
+            return None
+
+        # Replace lone JOIN with INNER JOIN.
+        return LintResult(
+            context.segment.segments[0],
+            fixes=[
+                LintFix(
+                    "edit",
path: src/sqlfluff/rules/L051.py
text: @user1:
I think we want to use `"create"` here rather than `"edit"`.

Reason: `"create"` adds new **sibling** segments, while `"edit"` creates a new level of the parse tree, replacing the specified segment with a new segment of the same type, with the new segments _as children_. This is a subtle bug, because the final SQL **string** looks okay, but the parse **tree** is wrong. This can cause other lint rules to misbehave.

See an excerpt of `sqlfluff parse` output below for two queries, one using `JOIN`, the other using `INNER JOIN`. In the second, you'll see that `INNER` and the whitespace should be at the same level as the `JOIN`.

```
[L:  1, P: 30]      |                    [META] dedent:
[L:  1, P: 30]      |                    join_clause:
[L:  1, P: 30]      |                        keyword:                              'JOIN'
[L:  1, P: 34]      |                        [META] indent:
[L:  1, P: 34]      |                        whitespace:                           ' '
[L:  1, P: 35]      |                        from_expression_element:
[L:  1, P: 35]      |                            table_expression:
[L:  1, P: 35]      |                                table_reference:
[L:  1, P: 35]      |                                    identifier:               'bar'
[L:  1, P: 38]      |                        [META] dedent:
[L:  1, P: 38]      |    newline:                                                  '\n'
```

```
[L:  1, P: 30]      |                    [META] dedent:
[L:  1, P: 30]      |                    join_clause:
[L:  1, P: 30]      |                        keyword:                              'INNER'
[L:  1, P: 35]      |                        whitespace:                           ' '
[L:  1, P: 36]      |                        keyword:                              'JOIN'
[L:  1, P: 40]      |                        [META] indent:
[L:  1, P: 40]      |                        whitespace:                           ' '
[L:  1, P: 41]      |                        from_expression_element:
[L:  1, P: 41]      |                            table_expression:
[L:  1, P: 41]      |                                table_reference:
[L:  1, P: 41]      |                                    identifier:               'bar'
[L:  1, P: 44]      |                        [META] dedent:
[L:  1, P: 44]      |    newline:                                                  '\n'
```

@author:
@user1 Interesting, I wasn't aware of that! Will bear it in mind going forwards.

I've made the switch to using a create LintFix now, unit tests are all passing still and, as you said, the change should prevent future bugs 😄 

@user1:
Thanks! The behavior I described is pretty tricky and not documented. I hope in the future, we can add automated checking inside the linter to flag it if a rule does something to the parse tree that makes it illegal wrt the dialect.