Skip to content

If HDF CodeDesc is empty, use Desc as Message #2635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

candrews
Copy link
Collaborator

@candrews candrews commented Mar 3, 2023

Sometimes, the HDF CodeDesc is empty. Currently, that results in the SARIF Message being set to "." which isn't good.

To fix that issue, if CodeDesc is empty, use the Desc.

I've reported one case where the HDF code_desc is set to the empty string at mitre/saf#1163

@candrews
Copy link
Collaborator Author

candrews commented Mar 7, 2023

@michaelcfanning how does this PR look?

I'm eager to get this merged and see the improvement in a release :-)

@@ -126,7 +126,7 @@ private static (ReportingDescriptor, IList<Result>) SarifRuleAndResultFromHdfCon
RuleId = execJsonControl.Id,
Message = new Message
{
Text = AppendPeriod(controlResult.CodeDesc),
Text = AppendPeriod(string.IsNullOrWhiteSpace(controlResult.CodeDesc) ? execJsonControl.Desc : controlResult.CodeDesc),
Copy link
Member

Choose a reason for hiding this comment

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

IsNullOrWhiteSpace

are you sure this logic is what you want? I think you have the arguments reversed maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are you sure this logic is what you want? I think you have the arguments reversed maybe?

I think this is right.

If controlResult.CodeDesc is null or whitespace, then use execJsonControl.Desc, otherwise use controlResult.CodeDesc

Copy link
Member

Choose a reason for hiding this comment

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

Um, yes. :) Haven't had my coffee yet. The good news is my mistake squeezed a release note improvement out of you. :)

@@ -1,5 +1,6 @@
# SARIF Package Release History (SDK, Driver, Converters, and Multitool)
## **v4.1.0** UNRELEASED
* BUG: If HDF `code_desc` is empty, use `desc` as the SARIF `message`. [#2632](https://github.com/microsoft/sarif-sdk/pull/2632)
Copy link
Member

Choose a reason for hiding this comment

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

BUG

All BUG entries belong together in the list below, since I asked you to review another possible change could you make this one?

Also let's reference HDFConverter to make things a little more explicit:

BUG: If HDFConverter code_desc... etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made that update - how's it look now?

Copy link
Member

Choose a reason for hiding this comment

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

Looks great! I'm going to ship this right now for you via NuGet.

Sometimes, the HDF CodeDesc is empty. Currently, that results in the SARIF Message being set to "." which isn't good.

To fix that issue, if CodeDesc is empty, use the Desc.
@candrews candrews force-pushed the hdf-sarif-code_desc branch from b476302 to a3b0f64 Compare March 7, 2023 16:55
Copy link
Member

@michaelcfanning michaelcfanning left a comment

Choose a reason for hiding this comment

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

:shipit:

@michaelcfanning michaelcfanning merged commit 0cfa257 into main Mar 7, 2023
@michaelcfanning michaelcfanning deleted the hdf-sarif-code_desc branch March 7, 2023 17:28
@michaelcfanning
Copy link
Member

Your changes are published to NuGet. Thanks for the contribution! Come back again. :)

https://www.nuget.org/packages/Sarif.Sdk

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