Update fixed findings metadata to align with CodeTF spec (#941)

drdavella · web-flow · commit 5754512bbbf7 · 2024-12-04T22:12:21.000Z
* Update Change.findings =&gt; fixedFindings to conform to spec

* Add fixedFindings to ChangeSet per spec

* Fix logic for gathering fixed findings
diff --git a/src/codemodder/codemods/imported_call_modifier.py b/src/codemodder/codemods/imported_call_modifier.py
@@ -81,7 +81,7 @@ def leave_Call(self, original_node: cst.Call, updated_node: cst.Call):
                     Change(
                         lineNumber=line_number,
                         description=self.change_description,
-                        findings=self.file_context.get_findings_for_location(
+                        fixedFindings=self.file_context.get_findings_for_location(
                             line_number
                         ),
                     )
diff --git a/src/codemodder/codemods/libcst_transformer.py b/src/codemodder/codemods/libcst_transformer.py
@@ -126,7 +126,7 @@ def report_change_for_line(
             Change(
                 lineNumber=line_number,
                 description=description or self.change_description,
-                findings=findings
+                fixedFindings=findings
                 or self.file_context.get_findings_for_location(line_number),
             )
         )
diff --git a/src/codemodder/codemods/regex_transformer.py b/src/codemodder/codemods/regex_transformer.py
@@ -40,7 +40,7 @@ def _apply(self, original_lines, file_context, results):
                     Change(
                         lineNumber=lineno + 1,
                         description=self.change_description,
-                        findings=file_context.get_findings_for_location(lineno),
+                        fixedFindings=file_context.get_findings_for_location(lineno),
                     )
                 )
         return changes, updated_lines
@@ -110,7 +110,7 @@ def _apply(self, original_lines, file_context, results):
                     Change(
                         lineNumber=lineno + 1,
                         description=self.change_description,
-                        findings=file_context.get_findings_for_location(lineno),
+                        fixedFindings=file_context.get_findings_for_location(lineno),
                     )
                 )
 
diff --git a/src/codemodder/codemods/test/utils.py b/src/codemodder/codemods/test/utils.py
@@ -224,5 +224,5 @@ def run_and_assert(
 
     def assert_findings(self, changes: list[Change]):
         assert all(
-            x.findings for x in changes
+            x.fixedFindings for x in changes
         ), f"Expected all changes to have findings: {changes}"
diff --git a/src/codemodder/codemods/xml_transformer.py b/src/codemodder/codemods/xml_transformer.py
@@ -96,7 +96,7 @@ def add_change(self, line):
             Change(
                 lineNumber=line,
                 description=self.change_description or None,
-                findings=self.file_context.get_findings_for_location(line),
+                fixedFindings=self.file_context.get_findings_for_location(line),
             )
         )
 
diff --git a/src/codemodder/codetf.py b/src/codemodder/codetf.py
@@ -62,7 +62,7 @@ class Change(BaseModel):
     diffSide: DiffSide = DiffSide.RIGHT
     properties: Optional[dict] = None
     packageActions: Optional[list[PackageAction]] = None
-    findings: Optional[list[Finding]] = None
+    fixedFindings: Optional[list[Finding]] = None
 
     @model_validator(mode="after")
     def validate_lineNumber(self):
@@ -83,7 +83,7 @@ def with_findings(self, findings: list[Finding] | None) -> Change:
             diffSide=self.diffSide,
             properties=self.properties,
             packageActions=self.packageActions,
-            findings=findings,
+            fixedFindings=findings,
         )
 
 
@@ -108,6 +108,8 @@ class ChangeSet(BaseModel):
     ai: Optional[AIMetadata] = None
     strategy: Optional[Strategy] = None
     provisional: Optional[bool] = False
+    # For fixed findings that are not associated with a specific change
+    fixedFindings: Optional[list[Finding]] = None
 
     def with_changes(self, changes: list[Change]) -> ChangeSet:
         return ChangeSet(
@@ -117,6 +119,7 @@ def with_changes(self, changes: list[Change]) -> ChangeSet:
             ai=self.ai,
             strategy=self.strategy,
             provisional=self.provisional,
+            fixedFindings=self.fixedFindings,
         )
 
 
diff --git a/src/codemodder/file_context.py b/src/codemodder/file_context.py
@@ -50,20 +50,22 @@ def add_unfixed_findings(
             ]
         )
 
-    def get_findings_for_location(self, line_number: int):
+    def get_findings_for_location(self, line_number: int) -> list[Finding]:
         return [
             result.finding
             for result in (self.results or [])
-            if any(
-                location.start.line <= line_number <= location.end.line
-                for location in result.locations
-            )
-            or any(
-                location.start.line <= line_number <= location.end.line
-                for codeflow in result.codeflows
-                for location in codeflow
+            if result.finding is not None
+            and (
+                any(
+                    location.start.line <= line_number <= location.end.line
+                    for location in result.locations
+                )
+                or any(
+                    location.start.line <= line_number <= location.end.line
+                    for codeflow in result.codeflows
+                    for location in codeflow
+                )
             )
-            and result.finding is not None
         ]
 
     def match_findings(self, line_numbers):
diff --git a/src/codemodder/utils/update_finding_metadata.py b/src/codemodder/utils/update_finding_metadata.py
@@ -27,7 +27,7 @@ def update_finding_metadata(
                             if finding.rule.id in tool_rule_map
                             else finding
                         )
-                        for finding in change.findings or []
+                        for finding in change.fixedFindings or []
                     ]
                     or None
                 )
diff --git a/src/core_codemods/file_resource_leak.py b/src/core_codemods/file_resource_leak.py
@@ -229,7 +229,7 @@ def _handle_block(
                     Change(
                         lineNumber=line_number,
                         description=FileResourceLeakTransformer.change_description,
-                        findings=self.file_context.get_findings_for_location(
+                        fixedFindings=self.file_context.get_findings_for_location(
                             line_number
                         ),
                     )
diff --git a/tests/codemods/defectdojo/semgrep/test_avoid_insecure_deserialization.py b/tests/codemods/defectdojo/semgrep/test_avoid_insecure_deserialization.py
@@ -46,9 +46,9 @@ def test_yaml_load(self, tmpdir):
         )
 
         assert changes is not None
-        assert changes[0].changes[0].findings is not None
-        assert changes[0].changes[0].findings[0].id == "1"
-        assert changes[0].changes[0].findings[0].rule.id == RULE_ID
+        assert changes[0].changes[0].fixedFindings is not None
+        assert changes[0].changes[0].fixedFindings[0].id == "1"
+        assert changes[0].changes[0].fixedFindings[0].rule.id == RULE_ID
 
     @mock.patch("codemodder.codemods.api.FileContext.add_dependency")
     def test_pickle_load(self, adds_dependency, tmpdir):
@@ -80,9 +80,9 @@ def test_pickle_load(self, adds_dependency, tmpdir):
         adds_dependency.assert_called_once_with(Fickling)
 
         assert changes is not None
-        assert changes[0].changes[0].findings is not None
-        assert changes[0].changes[0].findings[0].id == "2"
-        assert changes[0].changes[0].findings[0].rule.id == RULE_ID
+        assert changes[0].changes[0].fixedFindings is not None
+        assert changes[0].changes[0].fixedFindings[0].id == "2"
+        assert changes[0].changes[0].fixedFindings[0].rule.id == RULE_ID
 
     @mock.patch("codemodder.codemods.api.FileContext.add_dependency")
     def test_pickle_and_yaml(self, adds_dependency, tmpdir):
@@ -128,12 +128,12 @@ def test_pickle_and_yaml(self, adds_dependency, tmpdir):
         adds_dependency.assert_called_once_with(Fickling)
 
         assert changes is not None
-        assert changes[0].changes[0].findings is not None
-        assert changes[0].changes[0].findings[0].id == "4"
-        assert changes[0].changes[0].findings[0].rule.id == RULE_ID
-        assert changes[0].changes[1].findings is not None
-        assert changes[0].changes[1].findings[0].id == "3"
-        assert changes[0].changes[1].findings[0].rule.id == RULE_ID
+        assert changes[0].changes[0].fixedFindings is not None
+        assert changes[0].changes[0].fixedFindings[0].id == "4"
+        assert changes[0].changes[0].fixedFindings[0].rule.id == RULE_ID
+        assert changes[0].changes[1].fixedFindings is not None
+        assert changes[0].changes[1].fixedFindings[0].id == "3"
+        assert changes[0].changes[1].fixedFindings[0].rule.id == RULE_ID
 
     @mock.patch("codemodder.codemods.api.FileContext.add_dependency")
     def test_pickle_loads(self, adds_dependency, tmpdir):
diff --git a/tests/codemods/defectdojo/semgrep/test_django_secure_set_cookie.py b/tests/codemods/defectdojo/semgrep/test_django_secure_set_cookie.py
@@ -37,9 +37,9 @@ def test_simple(self, tmpdir):
         )
 
         assert changes is not None
-        assert changes[0].changes[0].findings is not None
-        assert changes[0].changes[0].findings[0].id == "1"
+        assert changes[0].changes[0].fixedFindings is not None
+        assert changes[0].changes[0].fixedFindings[0].id == "1"
         assert (
-            changes[0].changes[0].findings[0].rule.id
+            changes[0].changes[0].fixedFindings[0].rule.id
             == "python.django.security.audit.secure-cookies.django-secure-set-cookie"
         )
diff --git a/tests/codemods/semgrep/test_semgrep_sql_parametrization.py b/tests/codemods/semgrep/test_semgrep_sql_parametrization.py
@@ -195,4 +195,6 @@ def f():
             expexted_output,
             results=json.dumps(results),
         )
-        assert len(changes[0].changes[0].findings) == len(results["runs"][0]["results"])
+        assert len(changes[0].changes[0].fixedFindings) == len(
+            results["runs"][0]["results"]
+        )
diff --git a/tests/codemods/sonar/test_sonar_django_json_response_type.py b/tests/codemods/sonar/test_sonar_django_json_response_type.py
@@ -50,7 +50,7 @@ def foo(request):
             tmpdir, input_code, expected, results=json.dumps(issues)
         )
         assert changes is not None
-        assert changes[0].changes[0].findings is not None
-        assert changes[0].changes[0].findings[0].id == rule_id
-        assert changes[0].changes[0].findings[0].rule.id == rule_id
-        assert changes[0].changes[0].findings[0].rule.name == rule_id
+        assert changes[0].changes[0].fixedFindings is not None
+        assert changes[0].changes[0].fixedFindings[0].id == rule_id
+        assert changes[0].changes[0].fixedFindings[0].rule.id == rule_id
+        assert changes[0].changes[0].fixedFindings[0].rule.name == rule_id
diff --git a/tests/codemods/sonar/test_sonar_django_receiver_on_top.py b/tests/codemods/sonar/test_sonar_django_receiver_on_top.py
@@ -13,8 +13,8 @@ def test_name(self):
 
     def assert_findings(self, changes):
         # For now we can only link the finding to the line with the receiver decorator
-        assert changes[0].findings
-        assert not changes[1].findings
+        assert changes[0].fixedFindings
+        assert not changes[1].fixedFindings
 
     def test_simple(self, tmpdir):
         input_code = """
diff --git a/tests/codemods/sonar/test_sonar_fix_assert_tuple.py b/tests/codemods/sonar/test_sonar_fix_assert_tuple.py
@@ -13,9 +13,9 @@ def test_name(self):
 
     def assert_findings(self, changes):
         # For now we can only link the finding to the first line changed
-        assert changes[0].findings
-        assert not changes[1].findings
-        assert not changes[2].findings
+        assert changes[0].fixedFindings
+        assert not changes[1].fixedFindings
+        assert not changes[2].fixedFindings
 
     def test_simple(self, tmpdir):
         input_code = """
diff --git a/tests/codemods/sonar/test_sonar_remove_assertion_in_pytest_raises.py b/tests/codemods/sonar/test_sonar_remove_assertion_in_pytest_raises.py
@@ -14,7 +14,7 @@ def test_name(self):
         assert self.codemod.name == "remove-assertion-in-pytest-raises"
 
     def assert_findings(self, changes):
-        assert not all(x.findings for x in changes)
+        assert not all(x.fixedFindings for x in changes)
 
     def test_simple(self, tmpdir):
         input_code = """
diff --git a/tests/test_update_finding_metadata.py b/tests/test_update_finding_metadata.py
@@ -12,14 +12,14 @@ def test_update_finding_metdata():
             Change(
                 lineNumber=1,
                 description="foo",
-                findings=[
+                fixedFindings=[
                     Finding(id="rule_id", rule=Rule(id="rule_id", name="other_name"))
                 ],
             ),
             Change(
                 lineNumber=2,
                 description="bar",
-                findings=[
+                fixedFindings=[
                     Finding(id="other_id", rule=Rule(id="other_id", name="other_name"))
                 ],
             ),
@@ -34,10 +34,10 @@ def test_update_finding_metdata():
         tool_rules=[tool_rule], changesets=[changeset]
     )
 
-    assert new_changesets[0].changes[0].findings
-    assert new_changesets[0].changes[0].findings[0].rule.name == "rule_name"
-    assert new_changesets[0].changes[0].findings[0].rule.url == "rule_url"
-    assert new_changesets[0].changes[1].findings
-    assert new_changesets[0].changes[1].findings[0].rule.name == "other_name"
-    assert new_changesets[0].changes[1].findings[0].rule.url is None
+    assert new_changesets[0].changes[0].fixedFindings
+    assert new_changesets[0].changes[0].fixedFindings[0].rule.name == "rule_name"
+    assert new_changesets[0].changes[0].fixedFindings[0].rule.url == "rule_url"
+    assert new_changesets[0].changes[1].fixedFindings
+    assert new_changesets[0].changes[1].fixedFindings[0].rule.name == "other_name"
+    assert new_changesets[0].changes[1].fixedFindings[0].rule.url is None
     assert new_changesets[0].changes[2] == changeset.changes[2]

Original file line number	Diff line number	Diff line change
`@@ -81,7 +81,7 @@ def leave_Call(self, original_node: cst.Call, updated_node: cst.Call):`
`81`	`81`	`Change(`
`82`	`82`	`lineNumber=line_number,`
`83`	`83`	`description=self.change_description,`
`84`		`- findings=self.file_context.get_findings_for_location(`
	`84`	`+ fixedFindings=self.file_context.get_findings_for_location(`
`85`	`85`	`line_number`
`86`	`86`	`),`
`87`	`87`	`)`
Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ def report_change_for_line(`
`126`	`126`	`Change(`
`127`	`127`	`lineNumber=line_number,`
`128`	`128`	`description=description or self.change_description,`
`129`		`- findings=findings`
	`129`	`+ fixedFindings=findings`
`130`	`130`	`or self.file_context.get_findings_for_location(line_number),`
`131`	`131`	`)`
`132`	`132`	`)`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ def _apply(self, original_lines, file_context, results):`
`40`	`40`	`Change(`
`41`	`41`	`lineNumber=lineno + 1,`
`42`	`42`	`description=self.change_description,`
`43`		`- findings=file_context.get_findings_for_location(lineno),`
	`43`	`+ fixedFindings=file_context.get_findings_for_location(lineno),`
`44`	`44`	`)`
`45`	`45`	`)`
`46`	`46`	`return changes, updated_lines`
`@@ -110,7 +110,7 @@ def _apply(self, original_lines, file_context, results):`
`110`	`110`	`Change(`
`111`	`111`	`lineNumber=lineno + 1,`
`112`	`112`	`description=self.change_description,`
`113`		`- findings=file_context.get_findings_for_location(lineno),`
	`113`	`+ fixedFindings=file_context.get_findings_for_location(lineno),`
`114`	`114`	`)`
`115`	`115`	`)`
`116`	`116`
Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,7 @@ def add_change(self, line):`
`96`	`96`	`Change(`
`97`	`97`	`lineNumber=line,`
`98`	`98`	`description=self.change_description or None,`
`99`		`- findings=self.file_context.get_findings_for_location(line),`
	`99`	`+ fixedFindings=self.file_context.get_findings_for_location(line),`
`100`	`100`	`)`
`101`	`101`	`)`
`102`	`102`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ def update_finding_metadata(`
`27`	`27`	`if finding.rule.id in tool_rule_map`
`28`	`28`	`else finding`
`29`	`29`	`)`
`30`		`- for finding in change.findings or []`
	`30`	`+ for finding in change.fixedFindings or []`
`31`	`31`	`]`
`32`	`32`	`or None`
`33`	`33`	`)`
Original file line number	Diff line number	Diff line change
`@@ -229,7 +229,7 @@ def _handle_block(`
`229`	`229`	`Change(`
`230`	`230`	`lineNumber=line_number,`
`231`	`231`	`description=FileResourceLeakTransformer.change_description,`
`232`		`- findings=self.file_context.get_findings_for_location(`
	`232`	`+ fixedFindings=self.file_context.get_findings_for_location(`
`233`	`233`	`line_number`
`234`	`234`	`),`
`235`	`235`	`)`