Repository: sqlfluff/sqlfluff
Pull request: Don't allow fixes to COPY code from templated regions
<!--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

Fixes #2051 

This PR builds on yesterday's PR #2220:
* Yesterday's PR discards all fixes from a `LintResult` if any of the fixes **modify** templated code.
* This PR discards all fixes from a `LintResult` if any of the fixes **read from** templated code. This required a slight change to any rules which do this. (Fortunately, there are only a handful of such rules.)

<!--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.-->


### Are there any other side effects of this change that we should be aware of?


### 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 `tox -e generate-fixture-yml`).
  - 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:
sqlfluff fix with dbt templater replaces aliases with sources
<!--If this is a parsing or linting issue, please include a minimal SQL example which reproduces the issue, along with the `sqlfluff parse` output, `sqlfluff lint` output and `sqlfluff fix` output when relevant.-->

## Expected Behaviour

No replacement of aliases with the source, because the source database and schema changes between environments.

## Observed Behaviour
```
SELECT t.dv_RepositoryID
FROM {{ source('mydb', 'repo') }} t
```
is replaced with
```
SELECT mydb_dev.repo.dv_repositoryid
FROM {{ source('mydb', 'repo') }} t
```

However `mydb_dev` is the development database.

## Steps to Reproduce

## Dialect
snowflake

## Version
sqlfluff, version 0.8.2
python 3.9



## Configuration

```
[tool.sqlfluff.core]
templater = "dbt"
sql_file_exts = ".sql,.sql.j2,.dml,.ddl"
dialect = "snowflake"
ignore_templated_areas = true

[tool.sqlfluff.indentation]
indented_joins = false
indented_using_on = true
template_blocks_indent = false

[tool.sqlfluff.templater.dbt]
project_dir = "."
profile = "snowflake"
```

Hints from issue discussion:
@tekumara what lint was being raised by the original query? Helps narrow things down 😄 
```
L:   2 | P:  60 | L031 | Avoid aliases in from clauses and join conditions.
```
This makes sense thanks.

So L031 forbids the use of aliases in FROM/JOIN clauses
i.e. instead of
```sql
SELECT t.foo
FROM bar AS t
```
it suggests you should use
```sql
SELECT bar.foo
FROM bar
```
and provides an autofix to correct this.

What's happening in this case is the same but the table name is a templated variable. So in sqlfluff we initially substitute in the templated variables, evaluate the rules, and then substitute back the templates and we have some logic to not apply rules if they impact a templated area as that can corrupt the SQL.
What's different in this case is the fix is using the value from the templated area and applying it elsewhere in the query, hence not triggering our logic to ignore the fix.

I think that L031 needs some extra logic to abandon the fix if the table name it uses is from a template.

I've tested this with Jinja and can confirm I see the same thing so this is a general templating issue.

MRE:
*test.sql*
```sql
SELECT t.foo
FROM {{ tablename }} AS t
```
*.sqlfluff*
```
[sqlfluff:templater:jinja:context]
tablename=bar
```

Run `sqlfluff fix test.sql`

![Screen-Recording-2021-12-07-at-10 09 53](https://user-images.githubusercontent.com/80432516/145012009-0586f91b-2bcc-4569-a61d-7210d1f46b6e.gif)

Interesting issue. I agree that L031 will need changes, but let's try and think about a more general, long-term fix.

My thoughts: when a rule is copying content from one place to another, this should be captured as part of the LintFix object somehow. With this information, we can enhance the logic added in #2117  to prevent copying content out of a templated area.

This could be adding position info to the new/replacement segment(s) or a new field on LintFix. I think a new field will be better, because using position on the segments is potentially ambiguous (sometimes rules move/reuse existing segments, making it hard to tell if the position info is a leftover or a deliberately set value).

Commit message: Merge branch 'main' into bhart-issue_2051_no_copy_code_out_of_template

Patch to review:
diff --git a/src/sqlfluff/core/rules/base.py b/src/sqlfluff/core/rules/base.py
index 3f238d58e94..635532d7e24 100644
--- a/src/sqlfluff/core/rules/base.py
+++ b/src/sqlfluff/core/rules/base.py
@@ -114,11 +114,14 @@ class LintFix:
             the *position* that this fix should be applied at. For deletions
             it represents the segment to delete, for creations it implies the
             position to create at (with the existing element at this position
-            to be moved *after* the edit), for an `edit` it implies the segment
-            to be replaced.
-        edit (:obj:`BaseSegment`, optional): For `edit` and `create` fixes, this
-            hold the segment, or iterable of segments to create or replace at the
+            to be moved *after* the edit), for a `replace` it implies the
+            segment to be replaced.
+        edit (:obj:`BaseSegment`, optional): For `replace` and `create` fixes,
+            this holsd the iterable of segments to create or replace at the
             given `anchor` point.
+        source (:obj:`BaseSegment`, optional): For `replace` and `create` fixes,
+            this holds iterable of segments that provided code. IMPORTANT: The
+            linter uses this to prevent copying material from templated areas.
 
     """
 
@@ -127,6 +130,7 @@ def __init__(
         edit_type: str,
         anchor: BaseSegment,
         edit: Optional[Iterable[BaseSegment]] = None,
+        source: Optional[Iterable[BaseSegment]] = None,
     ) -> None:
         if edit_type not in (
             "create_before",
@@ -163,6 +167,7 @@ def __init__(
             # Once stripped, we shouldn't replace any markers because
             # later code may rely on them being accurate, which we
             # can't guarantee with edits.
+        self.source = [seg for seg in source if seg.pos_marker] if source else []
 
     def is_trivial(self):
         """Return true if the fix is trivial.
@@ -224,24 +229,33 @@ def delete(cls, anchor_segment: BaseSegment) -> "LintFix":
 
     @classmethod
     def replace(
-        cls, anchor_segment: BaseSegment, edit_segments: Iterable[BaseSegment]
+        cls,
+        anchor_segment: BaseSegment,
+        edit_segments: Iterable[BaseSegment],
+        source: Optional[Iterable[BaseSegment]] = None,
     ) -> "LintFix":
         """Replace supplied anchor segment with the edit segments."""
-        return cls("replace", anchor_segment, edit_segments)
+        return cls("replace", anchor_segment, edit_segments, source)
 
     @classmethod
     def create_before(
-        cls, anchor_segment: BaseSegment, edit_segments: Iterable[BaseSegment]
+        cls,
+        anchor_segment: BaseSegment,
+        edit_segments: Iterable[BaseSegment],
+        source: Optional[Iterable[BaseSegment]] = None,
     ) -> "LintFix":
         """Create edit segments before the supplied anchor segment."""
-        return cls("create_before", anchor_segment, edit_segments)
+        return cls("create_before", anchor_segment, edit_segments, source)
 
     @classmethod
     def create_after(
-        cls, anchor_segment: BaseSegment, edit_segments: Iterable[BaseSegment]
+        cls,
+        anchor_segment: BaseSegment,
+        edit_segments: Iterable[BaseSegment],
+        source: Optional[Iterable[BaseSegment]] = None,
     ) -> "LintFix":
         """Create edit segments after the supplied anchor segment."""
-        return cls("create_after", anchor_segment, edit_segments)
+        return cls("create_after", anchor_segment, edit_segments, source)
 
     def has_template_conflicts(self, templated_file: TemplatedFile) -> bool:
         """Does this fix conflict with (i.e. touch) templated code?"""
@@ -274,10 +288,28 @@ def has_template_conflicts(self, templated_file: TemplatedFile) -> bool:
                 slice(anchor_slice.stop, anchor_slice.stop + 1),
             ]
             check_fn = all
-        fix_slices: Set[RawFileSlice] = set()
+        fix_slices = self._raw_slices_from_templated_slices(
+            templated_file, templated_slices
+        )
+
+        # We have the fix slices. Now check for conflicts.
+        result = check_fn(fs.slice_type == "templated" for fs in fix_slices)
+        if result or not self.source:
+            return result
+
+        # Fix slices were okay. Now check template safety of the "source" field.
+        templated_slices = [source.pos_marker.templated_slice for source in self.source]
+        raw_slices = self._raw_slices_from_templated_slices(
+            templated_file, templated_slices
+        )
+        return any(fs.slice_type == "templated" for fs in raw_slices)
+
+    @staticmethod
+    def _raw_slices_from_templated_slices(templated_file, templated_slices):
+        raw_slices: Set[RawFileSlice] = set()
         for templated_slice in templated_slices:
             try:
-                fix_slices.update(
+                raw_slices.update(
                     templated_file.raw_slices_spanning_source_slice(
                         templated_file.templated_slice_to_source_slice(templated_slice)
                     )
@@ -288,9 +320,7 @@ def has_template_conflicts(self, templated_file: TemplatedFile) -> bool:
                 # it is the correct action, because the other (anchor) slice
                 # is still valid.
                 pass
-
-        # Check the result from checking the fix slices.
-        return check_fn(fs.slice_type == "templated" for fs in fix_slices)
+        return raw_slices
 
 
 EvalResultType = Union[LintResult, List[LintResult], None]
diff --git a/src/sqlfluff/rules/L031.py b/src/sqlfluff/rules/L031.py
index 4fe277b229f..9914f58811a 100644
--- a/src/sqlfluff/rules/L031.py
+++ b/src/sqlfluff/rules/L031.py
@@ -199,7 +199,11 @@ def _lint_aliases_in_join(
                     for d in [alias_info.alias_exp_ref, alias_info.whitespace_ref]
                 ],
                 *[
-                    LintFix.replace(alias, [alias.edit(alias_info.table_ref.raw)])
+                    LintFix.replace(
+                        alias,
+                        [alias.edit(alias_info.table_ref.raw)],
+                        source=[alias_info.table_ref],
+                    )
                     for alias in [alias_info.alias_identifier_ref, *ids_refs]
                 ],
             ]
diff --git a/src/sqlfluff/rules/L043.py b/src/sqlfluff/rules/L043.py
index abb85081a73..ce5e4d75911 100644
--- a/src/sqlfluff/rules/L043.py
+++ b/src/sqlfluff/rules/L043.py
@@ -102,6 +102,7 @@ def _coalesce_fix_list(
                     coalesce_arg_2,
                     SymbolSegment(")", name="end_bracket", type="end_bracket"),
                 ],
+                source=[coalesce_arg_2],
             ),
         ] + (
             # Segments to delete -- i.e. all child segments at both levels EXCEPT:
diff --git a/src/sqlfluff/rules/L045.py b/src/sqlfluff/rules/L045.py
index 5f3cedc7b96..a1fd078d567 100644
--- a/src/sqlfluff/rules/L045.py
+++ b/src/sqlfluff/rules/L045.py
@@ -3,10 +3,8 @@
 
 from sqlfluff.core.rules.base import BaseRule, LintResult, RuleContext
 from sqlfluff.core.rules.analysis.select_crawler import Query, SelectCrawler
-from sqlfluff.core.rules.doc_decorators import document_fix_compatible
 
 
-@document_fix_compatible
 class Rule_L045(BaseRule):
     """Query defines a CTE (common-table expression) but does not use it.
 
diff --git a/src/sqlfluff/rules/L058.py b/src/sqlfluff/rules/L058.py
index 6127619f4ff..c3ba12c0d61 100644
--- a/src/sqlfluff/rules/L058.py
+++ b/src/sqlfluff/rules/L058.py
@@ -76,11 +76,9 @@ def _eval(self, context: RuleContext) -> LintResult:
             create_after_last_when = nested_clauses.apply(
                 lambda seg: [NewlineSegment(), WhitespaceSegment(indent_str), seg]
             )
+            segments = [item for sublist in create_after_last_when for item in sublist]
             fixes.append(
-                LintFix.create_after(
-                    case1_last_when.get(),
-                    [item for sublist in create_after_last_when for item in sublist],
-                )
+                LintFix.create_after(case1_last_when.get(), segments, source=segments)
             )
 
             # Delete the outer "else" clause.
diff --git a/src/sqlfluff/testing/rules.py b/src/sqlfluff/testing/rules.py
index 90155733dbe..97281657a5c 100644
--- a/src/sqlfluff/testing/rules.py
+++ b/src/sqlfluff/testing/rules.py
@@ -85,12 +85,8 @@ def assert_rule_fail_in_sql(code, sql, configs=None, line_numbers=None):
                     line_numbers, actual_line_numbers
                 )
             )
-    # The query should already have been fixed if possible so just return the raw.
-    if linted.num_violations(fixable=True) > 0:
-        fixed, _ = linted.fix_string()
-        return fixed
-    else:
-        return linted.tree.raw
+    fixed, _ = linted.fix_string()
+    return fixed
 
 
 def assert_rule_pass_in_sql(code, sql, configs=None):
diff --git a/test/fixtures/rules/std_rule_cases/L031.yml b/test/fixtures/rules/std_rule_cases/L031.yml
index e363b09204a..a7a90b5c999 100644
--- a/test/fixtures/rules/std_rule_cases/L031.yml
+++ b/test/fixtures/rules/std_rule_cases/L031.yml
@@ -193,3 +193,19 @@ issue_1639:
   configs:
     core:
       dialect: tsql
+
+test_fail_no_copy_code_out_of_template:
+  # The rule wants to replace "t" with "foobar", but
+  # LintFix.has_template_conflicts() correctly prevents it copying code out
+  # of the templated region. Hence, fix_str and fail_str are the same.
+  fail_str: |
+    SELECT t.repo_id
+    FROM {{ source_table }} AS t
+  fix_str: |
+    SELECT t.repo_id
+    FROM {{ source_table }} AS t
+  configs:
+    templater:
+      jinja:
+        context:
+          source_table: foobar
diff --git a/test/fixtures/rules/std_rule_cases/L043.yml b/test/fixtures/rules/std_rule_cases/L043.yml
index 228f34e862b..21f2035eac4 100644
--- a/test/fixtures/rules/std_rule_cases/L043.yml
+++ b/test/fixtures/rules/std_rule_cases/L043.yml
@@ -199,3 +199,33 @@ test_fail_unnecessary_case_8:
         foo,
         coalesce(bar, '123') as test
     from baz;
+
+test_fail_no_copy_code_out_of_template:
+  # The rule wants to replace the case statement with coalesce(), but
+  # LintFix.has_template_conflicts() correctly prevents it copying code out
+  # of the templated region. Hence, fix_str and fail_str are the same.
+  fail_str: |
+    select
+        foo,
+        case
+            when
+                bar is null then {{ result }}
+            else bar
+        end as test
+    from baz;
+  fix_str: |
+    select
+        foo,
+        case
+            when
+                bar is null then {{ result }}
+            else bar
+        end as test
+    from baz;
+  configs:
+    core:
+      ignore_templated_areas: False
+    templater:
+      jinja:
+        context:
+          result: "'123'"
diff --git a/test/fixtures/rules/std_rule_cases/L058.yml b/test/fixtures/rules/std_rule_cases/L058.yml
index 47e5a71c84a..d12f5f95e8a 100644
--- a/test/fixtures/rules/std_rule_cases/L058.yml
+++ b/test/fixtures/rules/std_rule_cases/L058.yml
@@ -188,3 +188,37 @@ test_double_nesting_2:
             WHEN species = 'Hyena' THEN 'Cackle'
         END AS sound
     FROM mytable
+
+test_fail_no_copy_code_out_of_template:
+  # The rule wants to replace the case statement with coalesce(), but
+  # LintFix.has_template_conflicts() correctly prevents it copying code out
+  # of the templated region. Hence, fix_str and fail_str are the same.
+  fail_str: |
+    SELECT
+        c1,
+        CASE
+            WHEN species = 'Rat' THEN 'Squeak'
+            ELSE
+                CASE
+                    {{ inner_when }}
+                END
+        END AS sound
+    FROM mytable
+  fix_str: |
+    SELECT
+        c1,
+        CASE
+            WHEN species = 'Rat' THEN 'Squeak'
+            ELSE
+                CASE
+                    {{ inner_when }}
+                END
+        END AS sound
+    FROM mytable
+  configs:
+    core:
+      ignore_templated_areas: False
+    templater:
+      jinja:
+        context:
+          inner_when: "WHEN species = 'Dog' THEN 'Woof'"


Code review comments:
diff_hunk: @@ -114,11 +114,14 @@ class LintFix:
             the *position* that this fix should be applied at. For deletions
             it represents the segment to delete, for creations it implies the
             position to create at (with the existing element at this position
-            to be moved *after* the edit), for an `edit` it implies the segment
-            to be replaced.
-        edit (:obj:`BaseSegment`, optional): For `edit` and `create` fixes, this
-            hold the segment, or iterable of segments to create or replace at the
+            to be moved *after* the edit), for a `replace` it implies the
+            segment to be replaced.
+        edit (:obj:`BaseSegment`, optional): For `replace` and `create` fixes,
+            this holsd the iterable of segments to create or replace at the
path: src/sqlfluff/core/rules/base.py
text: @user1:
```suggestion
            this holds the iterable of segments to create or replace at the
```

@author:
Good catch -- fixed!