Skip to content
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

Add new visitor to get deterministic SARIF log by sorting results #2422

Merged
merged 29 commits into from
Mar 2, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
22ec32b
Add new visitor to get deterministic SARIF log by sorting results
yongyan-gh Jan 17, 2022
44a9f46
Merge branch 'main' into users/yongyan-gh/sortingvisitor
yongyan-gh Jan 17, 2022
3c597ff
Fix dotnet format issue
yongyan-gh Jan 17, 2022
ca5c356
updating format
yongyan-gh Jan 18, 2022
043079f
remove unnecessary using
yongyan-gh Jan 20, 2022
ea82e82
Add Run Comparer to support sorting logs with multiple runs.
yongyan-gh Jan 21, 2022
3961924
Merge branch 'main' into users/yongyan-gh/sortingvisitor
yongyan-gh Jan 27, 2022
d568f85
Add command argument unit tests
yongyan-gh Jan 27, 2022
07e1b4c
Merge branch 'users/yongyan-gh/sortingvisitor' of https://github.com/…
yongyan-gh Jan 27, 2022
cfebb85
use ContainsKey to avoid allocating variable
yongyan-gh Feb 2, 2022
351ef43
Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` art…
eddynaka Jan 31, 2022
9f140e4
`SarifLogger` now emits an artifacts table entry if `artifactLocation…
eddynaka Feb 1, 2022
0727702
Fix `ArgumentException` when recurse is enabled and two file target s…
eddynaka Feb 1, 2022
6306faf
Addressing PR review issues
yongyan-gh Feb 8, 2022
f192c36
Merge branch 'main' into users/yongyan-gh/sortingvisitor
yongyan-gh Feb 9, 2022
bb3ce50
Merge branch 'main' of https://github.com/microsoft/sarif-sdk
yongyan-gh Feb 9, 2022
196cc0a
Merge branch 'main' into users/yongyan-gh/sortingvisitor
yongyan-gh Feb 9, 2022
9fa0ca7
Fix issues in PR review
yongyan-gh Feb 11, 2022
9d1ce42
Add xml comments
yongyan-gh Feb 11, 2022
305570e
Fix test issues
yongyan-gh Feb 14, 2022
4747ec1
fix dotnet format
yongyan-gh Feb 14, 2022
9281b44
Merge branch 'main' into users/yongyan-gh/sortingvisitor
yongyan-gh Feb 14, 2022
314bd53
Addressing review feedbacks
yongyan-gh Feb 18, 2022
d37dd18
Merge branch 'main' into users/yongyan-gh/sortingvisitor
yongyan-gh Feb 18, 2022
eb150f7
Merge branch 'main' into users/yongyan-gh/sortingvisitor
yongyan-gh Feb 18, 2022
7c77019
Fix tests
yongyan-gh Feb 18, 2022
966b7f8
Update extension methods names
yongyan-gh Feb 19, 2022
f07f4af
Change xml doc comments to normal comments
yongyan-gh Feb 20, 2022
f18e005
Merge branch 'main' into users/yongyan-gh/sortingvisitor
michaelcfanning Mar 2, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Sarif.Multitool.Library/RewriteCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ public int Run(RewriteOptions options)
SarifLog reformattedLog = new RemoveOptionalDataVisitor(dataToRemove).VisitSarifLog(actualLog);
reformattedLog = new InsertOptionalDataVisitor(dataToInsert, originalUriBaseIds, insertProperties: options.InsertProperties).VisitSarifLog(reformattedLog);

if (options.SortResults)
{
reformattedLog = new SortingVisitor().VisitSarifLog(reformattedLog);
}

if (options.SarifOutputVersion == SarifVersion.OneZeroZero)
{
var visitor = new SarifCurrentToVersionOneVisitor();
Expand Down
6 changes: 6 additions & 0 deletions src/Sarif.Multitool.Library/RewriteOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,11 @@ namespace Microsoft.CodeAnalysis.Sarif.Multitool
[Verb("rewrite", HelpText = "Enrich a SARIF file with additional data.")]
public class RewriteOptions : SingleFileOptionsBase
{
[Option(
's',
"sort-results",
Copy link
Member

@michaelcfanning michaelcfanning Feb 2, 2022

Choose a reason for hiding this comment

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

results

Why isn't this in a base class? #Pending

Copy link
Member

Choose a reason for hiding this comment

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

Future work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

open issue #2443 for tracking

Default = false,
HelpText = "Sort results in the final output file.")]
public bool SortResults { get; set; }
}
}
56 changes: 56 additions & 0 deletions src/Sarif/Comparers/ArtifactContentSortingComparer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;

/// <summary>
/// All Comparer implementations should be replaced by auto-generated codes by JSchema as
/// part of EqualityComparer in a planned comprehensive solution.
/// Tracking by issue: https://github.com/microsoft/jschema/issues/141
/// </summary>
namespace Microsoft.CodeAnalysis.Sarif
{
internal class ArtifactContentSortingComparer : IComparer<ArtifactContent>
{
internal static readonly ArtifactContentSortingComparer Instance = new ArtifactContentSortingComparer();
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jan 20, 2022

Choose a reason for hiding this comment

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

Instance

not sure, should we also start with s_ since it is static #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's a public (internal) member. It should match naming convention: "All visible static member names are pascal cased."


public int Compare(ArtifactContent left, ArtifactContent right)
{
if (ReferenceEquals(left, right))
{
return 0;
}

if (left == null)
{
return -1;
}

if (right == null)
{
return 1;
}

int compareResult = 0;
compareResult = string.Compare(left.Text, right.Text);
if (compareResult != 0)
{
return compareResult;
}

compareResult = string.Compare(left.Binary, right.Binary);
if (compareResult != 0)
{
return compareResult;
}

compareResult = MultiformatMessageStringSortingComparer.Instance.Compare(left.Rendered, right.Rendered);
if (compareResult != 0)
{
return compareResult;
}

return compareResult;
}
}
}
77 changes: 77 additions & 0 deletions src/Sarif/Comparers/ArtifactLocationSortingComparer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;

/// <summary>
/// All Comparer implementations should be replaced by auto-generated codes by JSchema as
/// part of EqualityComparer in a planned comprehensive solution.
/// Tracking by issue: https://github.com/microsoft/jschema/issues/141
/// </summary>
namespace Microsoft.CodeAnalysis.Sarif
{
internal class ArtifactLocationSortingComparer : IComparer<ArtifactLocation>
{
internal static readonly ArtifactLocationSortingComparer Instance = new ArtifactLocationSortingComparer();

public int Compare(ArtifactLocation left, ArtifactLocation right)
{
if (ReferenceEquals(left, right))
{
return 0;
}

if (left == null)
{
return -1;
}

if (right == null)
{
return 1;
}

int compareResult = 0;
if (ReferenceEquals(left.Uri, right.Uri))
{
return 0;
}

if (left.Uri == null)
{
return -1;
}

if (right.Uri == null)
{
return 1;
}

compareResult = string.Compare(left.Uri.OriginalString, right.Uri.OriginalString);
if (compareResult != 0)
{
return compareResult;
}

compareResult = string.Compare(left.UriBaseId, right.UriBaseId);
if (compareResult != 0)
{
return compareResult;
}

compareResult = left.Index.CompareTo(right.Index);
if (compareResult != 0)
{
return compareResult;
}

compareResult = MessageSortingComparer.Instance.Compare(left.Description, right.Description);
if (compareResult != 0)
{
return compareResult;
}

return compareResult;
}
}
}
139 changes: 139 additions & 0 deletions src/Sarif/Comparers/ArtifactSortingComparer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;
using System.Linq;
Copy link
Contributor

@marmegh marmegh Jan 19, 2022

Choose a reason for hiding this comment

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

I see several instances where using System.Linq is greyed out in VS, can you double check where this is actually needed and remove any unnecessary? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is required in this file, checked other files and removed the unnecessary using.


/// <summary>
/// All Comparer implementations should be replaced by auto-generated codes by JSchema as
/// part of EqualityComparer in a planned comprehensive solution.
/// Tracking by issue: https://github.com/microsoft/jschema/issues/141
/// </summary>
namespace Microsoft.CodeAnalysis.Sarif
{
internal class ArtifactSortingComparer : IComparer<Artifact>
{
internal static readonly ArtifactSortingComparer Instance = new ArtifactSortingComparer();

public int Compare(Artifact left, Artifact right)
{
if (ReferenceEquals(left, right))
{
return 0;
}

if (left == null)
{
return -1;
}

if (right == null)
{
return 1;
}

int compareResult = 0;
compareResult = MessageSortingComparer.Instance.Compare(left.Description, right.Description);
if (compareResult != 0)
{
return compareResult;
}

compareResult = ArtifactLocationSortingComparer.Instance.Compare(left.Location, right.Location);
if (compareResult != 0)
{
return compareResult;
}

compareResult = left.ParentIndex.CompareTo(right.ParentIndex);
if (compareResult != 0)
{
return compareResult;
}

compareResult = left.Offset.CompareTo(right.Offset);
if (compareResult != 0)
{
return compareResult;
}

compareResult = left.Length.CompareTo(right.Length);
if (compareResult != 0)
{
return compareResult;
}

compareResult = left.Roles.CompareTo(right.Roles);
if (compareResult != 0)
{
return compareResult;
}

compareResult = string.Compare(left.MimeType, right.MimeType);
if (compareResult != 0)
{
return compareResult;
}

compareResult = ArtifactContentSortingComparer.Instance.Compare(left.Contents, right.Contents);
if (compareResult != 0)
{
return compareResult;
}

compareResult = string.Compare(left.Encoding, right.Encoding);
if (compareResult != 0)
{
return compareResult;
}

compareResult = string.Compare(left.SourceLanguage, right.SourceLanguage);
if (compareResult != 0)
{
return compareResult;
}

if (!object.ReferenceEquals(left.Hashes, right.Hashes))
{
if (left.Hashes == null)
{
return -1;
}

if (right.Hashes == null)
{
return 1;
}

compareResult = left.Hashes.Count.CompareTo(right.Hashes.Count);
if (compareResult != 0)
{
return compareResult;
}

for (int i = 0; i < left.Hashes.Count; i++)
{
compareResult = string.Compare(left.Hashes.ElementAt(i).Key, right.Hashes.ElementAt(i).Key);
if (compareResult != 0)
{
return compareResult;
}

compareResult = string.Compare(left.Hashes.ElementAt(i).Value, right.Hashes.ElementAt(i).Value);
Copy link
Member

@michaelcfanning michaelcfanning Feb 2, 2022

Choose a reason for hiding this comment

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

left.Hashes.ElementAt(i)

is this an n^2 access pattern? ouch. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed. new implementation in ComparerHelper.CompareDictionary(...)

if (compareResult != 0)
{
return compareResult;
}
}
}

compareResult = left.LastModifiedTimeUtc.CompareTo(right.LastModifiedTimeUtc);
if (compareResult != 0)
{
return compareResult;
}

return compareResult;
}
}
}
74 changes: 74 additions & 0 deletions src/Sarif/Comparers/CodeFlowSortingComparer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;

/// <summary>
/// All Comparer implementations should be replaced by auto-generated codes by JSchema as
/// part of EqualityComparer in a planned comprehensive solution.
/// Tracking by issue: https://github.com/microsoft/jschema/issues/141
/// </summary>
namespace Microsoft.CodeAnalysis.Sarif
{
internal class CodeFlowSortingComparer : IComparer<CodeFlow>
{
internal static readonly CodeFlowSortingComparer Instance = new CodeFlowSortingComparer();

public int Compare(CodeFlow left, CodeFlow right)
{
if (ReferenceEquals(left, right))
{
return 0;
}

if (left == null)
{
return -1;
}

if (right == null)
{
return 1;
}

int compareResult = 0;

compareResult = MessageSortingComparer.Instance.Compare(left.Message, right.Message);
if (compareResult != 0)
{
return compareResult;
}

if (!object.ReferenceEquals(left.ThreadFlows, right.ThreadFlows))
{

if (left.ThreadFlows == null)
{
return -1;
}

if (right.ThreadFlows == null)
{
return 1;
}

compareResult = left.ThreadFlows.Count.CompareTo(right.ThreadFlows.Count);
if (compareResult != 0)
{
return compareResult;
}

for (int index_1 = 0; index_1 < left.ThreadFlows.Count; ++index_1)
Copy link
Member

@michaelcfanning michaelcfanning Feb 2, 2022

Choose a reason for hiding this comment

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

index_1

index_1? let's leave underscores out of this. :) #Resolved

{
compareResult = ThreadFlowSortingComparer.Instance.Compare(left.ThreadFlows[index_1], right.ThreadFlows[index_1]);
if (compareResult != 0)
{
return compareResult;
}
}
}

return compareResult;
}
}
}
Loading