Skip to content

Commit

Permalink
Update fixed findings metadata to align with CodeTF spec (#941)
Browse files Browse the repository at this point in the history
* Update Change.findings => fixedFindings to conform to spec

* Add fixedFindings to ChangeSet per spec

* Fix logic for gathering fixed findings
  • Loading branch information
drdavella authored Dec 4, 2024
1 parent 95bd4b2 commit 5754512
Show file tree
Hide file tree
Showing 17 changed files with 61 additions and 54 deletions.
2 changes: 1 addition & 1 deletion src/codemodder/codemods/imported_call_modifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
),
)
Expand Down
2 changes: 1 addition & 1 deletion src/codemodder/codemods/libcst_transformer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
)
Expand Down
4 changes: 2 additions & 2 deletions src/codemodder/codemods/regex_transformer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
)
)

Expand Down
2 changes: 1 addition & 1 deletion src/codemodder/codemods/test/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
2 changes: 1 addition & 1 deletion src/codemodder/codemods/xml_transformer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
)

Expand Down
7 changes: 5 additions & 2 deletions src/codemodder/codetf.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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,
)


Expand All @@ -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(
Expand All @@ -117,6 +119,7 @@ def with_changes(self, changes: list[Change]) -> ChangeSet:
ai=self.ai,
strategy=self.strategy,
provisional=self.provisional,
fixedFindings=self.fixedFindings,
)


Expand Down
22 changes: 12 additions & 10 deletions src/codemodder/file_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion src/codemodder/utils/update_finding_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down
2 changes: 1 addition & 1 deletion src/core_codemods/file_resource_leak.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
),
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
4 changes: 3 additions & 1 deletion tests/codemods/semgrep/test_semgrep_sql_parametrization.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
)
8 changes: 4 additions & 4 deletions tests/codemods/sonar/test_sonar_django_json_response_type.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions tests/codemods/sonar/test_sonar_django_receiver_on_top.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = """
Expand Down
6 changes: 3 additions & 3 deletions tests/codemods/sonar/test_sonar_fix_assert_tuple.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = """
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = """
Expand Down
16 changes: 8 additions & 8 deletions tests/test_update_finding_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
],
),
Expand All @@ -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]

0 comments on commit 5754512

Please sign in to comment.