Skip to content

Conversation

@andrecsilva
Copy link
Contributor

No description provided.

@andrecsilva andrecsilva marked this pull request as ready for review May 1, 2025 13:13
Copy link
Member

@drdavella drdavella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the reason for this change? This behavior is intentional.

EDIT: fwiw my main objection is with making finding ID optional. If you are concerned about falling back on rule_id then we should just make it an error to not properly populate finding ID

@andrecsilva
Copy link
Contributor Author

Can you explain the reason for this change? This behavior is intentional.

EDIT: fwiw my main objection is with making finding ID optional. If you are concerned about falling back on rule_id then we should just make it an error to not properly populate finding ID

Finding id is optional as per the codetfv2 spec:

class Finding(BaseModel):
id: Optional[str] = None
rule: Rule

The spec also expects the finding_id to be unique an thus falling back to rule_id may cause problems. Since we expect sarif (and similar formats) to have some unique id (i.e. guid in sarif) the fallback is not really necessary. Honestly, the rule_id fallback seems to be used by internal semgrep codemods, so we can safely ditch it.

I'll implement your suggestion and make it throw an error.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 2, 2025

@andrecsilva andrecsilva added this pull request to the merge queue May 2, 2025
Merged via the queue into main with commit 676de98 May 2, 2025
14 checks passed
@andrecsilva andrecsilva deleted the fix-finding-ids branch May 2, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants