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.

Sign up for GitHub

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

Jump to bottom

Fixing AnalyzeCommandBase and MultithreadedAnalyzeCommandBase artifacts generation #2433

Merged
merged 16 commits into from
Jan 31, 2022
Merged

Fixing AnalyzeCommandBase and MultithreadedAnalyzeCommandBase artifacts generation #2433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2247cbe
Fixing multithreaded artifacts generation
eddynaka Jan 28, 2022
6972472
Fixing tests and flags
eddynaka Jan 28, 2022
5d87498
Loosing precision.
eddynaka Jan 28, 2022
af33d03
Applying fix for AnalyzeCommandBase
eddynaka Jan 28, 2022
3869b86
Enabling tests
eddynaka Jan 28, 2022
70dffb5
Updating test case and release history
eddynaka Jan 28, 2022
171133f
Creating const to prevent magical numbers everywhere
eddynaka Jan 28, 2022
d322ee4
Rebaselining tests
eddynaka Jan 28, 2022
e800ead
Merge branch 'users/ednakamu/rebaselining-files' into users/ednakamu/…
eddynaka Jan 28, 2022
fd9bd16
Creating Artifacts flag to keep previous behavior
eddynaka Jan 28, 2022
6996260
Addressing PR feedback.
eddynaka Jan 28, 2022
8a78dd8
Rollback changes
eddynaka Jan 28, 2022
44eb8d2
Update SARIF2012.ProvideRuleProperties_Invalid.sarif
eddynaka Jan 30, 2022
cabbd5f
updating back
eddynaka Jan 30, 2022
a3408b3
Ordering deprecated names
eddynaka Jan 31, 2022
dbaae40
Merge branch 'users/ednakamu/fixing-multithreaded-artifacts' of https…
eddynaka Jan 31, 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
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## Unreleased

* BREAKING: `AnalyzeCommandBase` previously persisted all scan target artifacts to SARIF logs rather than only persisting artifacts referenced by an analysis result, when an option to persist hashes, text file or binary information was set. `MultithreadedAnalyzeCommandBase` previously persisted all scan targets artifacts to SARIF logs in cases when hash insertion was eenabled rather than only persisting artifacts referenced by an analysis result. [#2433](https://github.com/microsoft/sarif-sdk/pull/2433)
* BUGFIX: Adjust Json Serialization field order for ReportingDescriptor and skip emit empty AutomationDetails node. [#2420](https://github.com/microsoft/sarif-sdk/pull/2420)
* BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application, and remove `[Serializable]` from it. Now use of BinaryFormatter on it will result in `SerializationException`: Type `PropertiesDictionary` is not marked as serializable. [#2415](https://github.com/microsoft/sarif-sdk/pull/2415)

Expand Down
9 changes: 5 additions & 4 deletions src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ private void Analyze(TOptions options, AggregatingLogger logger)
targets = ValidateTargetsExist(_rootContext, targets);

// 5. Initialize report file, if configured.
InitializeOutputFile(options, _rootContext, targets);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

InitializeOutputFile(options, _rootContext, targets);

we won't initialize the outputfile with targets because we don't want to emit all targets in the artifacts.

Copy link
Member

@michaelcfanning michaelcfanning Jan 28, 2022 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

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

targets

Revert this change please it's unnecessarily breaking customers. #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.

This API isn't exposed to customers. Only tools that use it will see a difference.

InitializeOutputFile(options, _rootContext);

// 6. Instantiate skimmers.
ISet<Skimmer<TContext>> skimmers = CreateSkimmers(options, _rootContext);
Expand Down Expand Up @@ -294,6 +294,7 @@ protected virtual TContext CreateContext(
else
{
context.Hashes = HashUtilities.ComputeHashes(filePath);
_pathToHashDataMap?.Add(filePath, context.Hashes);
}
}
}
Expand Down Expand Up @@ -352,7 +353,7 @@ protected virtual void InitializeConfiguration(TOptions options, TContext contex
}
}

private void InitializeOutputFile(TOptions analyzeOptions, TContext context, ISet<string> targets)
private void InitializeOutputFile(TOptions analyzeOptions, TContext context)
{
string filePath = analyzeOptions.OutputFilePath;
AggregatingLogger aggregatingLogger = (AggregatingLogger)context.Logger;
Expand Down Expand Up @@ -388,7 +389,7 @@ private void InitializeOutputFile(TOptions analyzeOptions, TContext context, ISe
dataToRemove,
tool: _tool,
run: _run,
analysisTargets: targets,
analysisTargets: null,
quiet: analyzeOptions.Quiet,
invocationTokensToRedact: GenerateSensitiveTokensList(),
invocationPropertiesToLog: analyzeOptions.InvocationPropertiesToLog,
Expand All @@ -404,7 +405,7 @@ private void InitializeOutputFile(TOptions analyzeOptions, TContext context, ISe
dataToRemove,
tool: _tool,
run: _run,
analysisTargets: targets,
analysisTargets: null,
invocationTokensToRedact: GenerateSensitiveTokensList(),
invocationPropertiesToLog: analyzeOptions.InvocationPropertiesToLog,
levels: analyzeOptions.Level,
Expand Down
7 changes: 3 additions & 4 deletions src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public abstract class MultithreadedAnalyzeCommandBase<TContext, TOptions> : Plug
private Channel<int> _resultsWritingChannel;
private Channel<int> _fileEnumerationChannel;
private Dictionary<string, List<string>> _hashToFilesMap;
private IDictionary<string, HashData> _pathToHashDataMap;
private ConcurrentDictionary<int, TContext> _fileContexts;

public Exception ExecutionException { get; set; }
Expand Down Expand Up @@ -485,12 +486,9 @@ private async Task<bool> HashAsync()
_hashToFilesMap[hashData.Sha256] = paths;
}

_run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri },
dataToInsert: _dataToInsert,
hashData: hashData);
Copy link
Collaborator Author

@eddynaka eddynaka Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

removing this to prevent us from generating artifacts for all the files #ByDesign


paths.Add(localPath);
context.Hashes = hashData;
_pathToHashDataMap?.Add(localPath, hashData);
}
Copy link
Collaborator Author

@eddynaka eddynaka Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Updating this will enable us to use the code from SarifLogger to create the artifacts #ByDesign


await _fileEnumerationChannel.Writer.WriteAsync(index);
Expand Down Expand Up @@ -705,6 +703,7 @@ private void InitializeOutputFile(TOptions analyzeOptions, TContext context)
kinds: analyzeOptions.Kind,
insertProperties: analyzeOptions.InsertProperties);
}
_pathToHashDataMap = sarifLogger.AnalysisTargetToHashDataMap;
sarifLogger.AnalysisStarted();
aggregatingLogger.Loggers.Add(sarifLogger);
},
Expand Down
26 changes: 13 additions & 13 deletions src/Sarif/Autogenerated/ReportingDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,31 +49,31 @@ public SarifNodeKind SarifNodeKind
/// <summary>
/// An array of stable, opaque identifiers by which this report was known in some previous version of the analysis tool.
/// </summary>
[DataMember(Name = "deprecatedIds", IsRequired = false, EmitDefaultValue = false, Order = 8)]
[DataMember(Name = "deprecatedIds", IsRequired = false, EmitDefaultValue = false, Order = 9)]
public virtual IList<string> DeprecatedIds { get; set; }

/// <summary>
/// A unique identifier for the reporting descriptor in the form of a GUID.
/// </summary>
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false, Order = 9)]
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false, Order = 10)]
public virtual string Guid { get; set; }

/// <summary>
/// An array of unique identifies in the form of a GUID by which this report was known in some previous version of the analysis tool.
/// </summary>
[DataMember(Name = "deprecatedGuids", IsRequired = false, EmitDefaultValue = false, Order = 10)]
[DataMember(Name = "deprecatedGuids", IsRequired = false, EmitDefaultValue = false, Order = 11)]
public virtual IList<string> DeprecatedGuids { get; set; }

/// <summary>
/// A report identifier that is understandable to an end user.
/// </summary>
[DataMember(Name = "name", IsRequired = false, EmitDefaultValue = false, Order = 7)]
[DataMember(Name = "name", IsRequired = false, EmitDefaultValue = false, Order = 2)]
public virtual string Name { get; set; }

/// <summary>
/// An array of readable identifiers by which this report was known in some previous version of the analysis tool.
/// </summary>
[DataMember(Name = "deprecatedNames", IsRequired = false, EmitDefaultValue = false, Order = 11)]
[DataMember(Name = "deprecatedNames", IsRequired = false, EmitDefaultValue = false, Order = 3)]
public virtual IList<string> DeprecatedNames { get; set; }

/// <summary>
Expand All @@ -85,46 +85,46 @@ public SarifNodeKind SarifNodeKind
/// <summary>
/// A description of the report. Should, as far as possible, provide details sufficient to enable resolution of any problem indicated by the result.
/// </summary>
[DataMember(Name = "fullDescription", IsRequired = false, EmitDefaultValue = false, Order = 2)]
[DataMember(Name = "fullDescription", IsRequired = false, EmitDefaultValue = false, Order = 4)]
public virtual MultiformatMessageString FullDescription { get; set; }

/// <summary>
/// A set of name/value pairs with arbitrary names. Each value is a multiformatMessageString object, which holds message strings in plain text and (optionally) Markdown format. The strings can include placeholders, which can be used to construct a message in combination with an arbitrary number of additional string arguments.
/// </summary>
[DataMember(Name = "messageStrings", IsRequired = false, EmitDefaultValue = false, Order = 5)]
[DataMember(Name = "messageStrings", IsRequired = false, EmitDefaultValue = false, Order = 6)]
public virtual IDictionary<string, MultiformatMessageString> MessageStrings { get; set; }

/// <summary>
/// Default reporting configuration information.
/// </summary>
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false, Order = 12)]
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false, Order = 13)]
public virtual ReportingConfiguration DefaultConfiguration { get; set; }

/// <summary>
/// A URI where the primary documentation for the report can be found.
/// </summary>
[DataMember(Name = "helpUri", IsRequired = false, EmitDefaultValue = false, Order = 3)]
[DataMember(Name = "helpUri", IsRequired = false, EmitDefaultValue = false, Order = 14)]
[JsonConverter(typeof(Microsoft.CodeAnalysis.Sarif.Readers.UriConverter))]
public virtual Uri HelpUri { get; set; }

/// <summary>
/// Provides the primary documentation for the report, useful when there is no online documentation.
/// </summary>
[DataMember(Name = "help", IsRequired = false, EmitDefaultValue = false, Order = 4)]
[DataMember(Name = "help", IsRequired = false, EmitDefaultValue = false, Order = 5)]
public virtual MultiformatMessageString Help { get; set; }

/// <summary>
/// An array of objects that describe relationships between this reporting descriptor and others.
/// </summary>
[DataMember(Name = "relationships", IsRequired = false, EmitDefaultValue = false, Order = 13)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate, Order = 13)]
[DataMember(Name = "relationships", IsRequired = false, EmitDefaultValue = false, Order = 15)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate, Order = 15)]
public virtual IList<ReportingDescriptorRelationship> Relationships { get; set; }

/// <summary>
/// Key/value pairs that provide additional information about the report.
/// </summary>
[JsonProperty(Order = 14)]
[DataMember(Name = "properties", IsRequired = false, EmitDefaultValue = false, Order = 14)]
[DataMember(Name = "properties", IsRequired = false, EmitDefaultValue = false, Order = 16)]
internal override IDictionary<string, SerializedPropertyInfo> Properties { get; set; }

/// <summary>
Expand Down
26 changes: 13 additions & 13 deletions src/Sarif/NotYetAutoGenerated/ReportingDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,31 +49,31 @@ public SarifNodeKind SarifNodeKind
/// <summary>
/// An array of stable, opaque identifiers by which this report was known in some previous version of the analysis tool.
/// </summary>
[DataMember(Name = "deprecatedIds", IsRequired = false, EmitDefaultValue = false, Order = 8)]
[DataMember(Name = "deprecatedIds", IsRequired = false, EmitDefaultValue = false, Order = 9)]
public virtual IList<string> DeprecatedIds { get; set; }

/// <summary>
/// A unique identifier for the reporting descriptor in the form of a GUID.
/// </summary>
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false, Order = 9)]
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false, Order = 10)]
public virtual string Guid { get; set; }

/// <summary>
/// An array of unique identifies in the form of a GUID by which this report was known in some previous version of the analysis tool.
/// </summary>
[DataMember(Name = "deprecatedGuids", IsRequired = false, EmitDefaultValue = false, Order = 10)]
[DataMember(Name = "deprecatedGuids", IsRequired = false, EmitDefaultValue = false, Order = 11)]
public virtual IList<string> DeprecatedGuids { get; set; }

/// <summary>
/// A report identifier that is understandable to an end user.
/// </summary>
[DataMember(Name = "name", IsRequired = false, EmitDefaultValue = false, Order = 7)]
[DataMember(Name = "name", IsRequired = false, EmitDefaultValue = false, Order = 2)]
public virtual string Name { get; set; }

/// <summary>
/// An array of readable identifiers by which this report was known in some previous version of the analysis tool.
/// </summary>
[DataMember(Name = "deprecatedNames", IsRequired = false, EmitDefaultValue = false, Order = 11)]
[DataMember(Name = "deprecatedNames", IsRequired = false, EmitDefaultValue = false, Order = 3)]
public virtual IList<string> DeprecatedNames { get; set; }

/// <summary>
Expand All @@ -85,46 +85,46 @@ public SarifNodeKind SarifNodeKind
/// <summary>
/// A description of the report. Should, as far as possible, provide details sufficient to enable resolution of any problem indicated by the result.
/// </summary>
[DataMember(Name = "fullDescription", IsRequired = false, EmitDefaultValue = false, Order = 2)]
[DataMember(Name = "fullDescription", IsRequired = false, EmitDefaultValue = false, Order = 4)]
public virtual MultiformatMessageString FullDescription { get; set; }

/// <summary>
/// A set of name/value pairs with arbitrary names. Each value is a multiformatMessageString object, which holds message strings in plain text and (optionally) Markdown format. The strings can include placeholders, which can be used to construct a message in combination with an arbitrary number of additional string arguments.
/// </summary>
[DataMember(Name = "messageStrings", IsRequired = false, EmitDefaultValue = false, Order = 5)]
[DataMember(Name = "messageStrings", IsRequired = false, EmitDefaultValue = false, Order = 6)]
public virtual IDictionary<string, MultiformatMessageString> MessageStrings { get; set; }

/// <summary>
/// Default reporting configuration information.
/// </summary>
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false, Order = 12)]
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false, Order = 13)]
public virtual ReportingConfiguration DefaultConfiguration { get; set; }

/// <summary>
/// A URI where the primary documentation for the report can be found.
/// </summary>
[DataMember(Name = "helpUri", IsRequired = false, EmitDefaultValue = false, Order = 3)]
[DataMember(Name = "helpUri", IsRequired = false, EmitDefaultValue = false, Order = 14)]
[JsonConverter(typeof(Microsoft.CodeAnalysis.Sarif.Readers.UriConverter))]
public virtual Uri HelpUri { get; set; }

/// <summary>
/// Provides the primary documentation for the report, useful when there is no online documentation.
/// </summary>
[DataMember(Name = "help", IsRequired = false, EmitDefaultValue = false, Order = 4)]
[DataMember(Name = "help", IsRequired = false, EmitDefaultValue = false, Order = 5)]
public virtual MultiformatMessageString Help { get; set; }

/// <summary>
/// An array of objects that describe relationships between this reporting descriptor and others.
/// </summary>
[DataMember(Name = "relationships", IsRequired = false, EmitDefaultValue = false, Order = 13)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate, Order = 13)]
[DataMember(Name = "relationships", IsRequired = false, EmitDefaultValue = false, Order = 15)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate, Order = 15)]
public virtual IList<ReportingDescriptorRelationship> Relationships { get; set; }

/// <summary>
/// Key/value pairs that provide additional information about the report.
/// </summary>
[JsonProperty(Order = 14)]
[DataMember(Name = "properties", IsRequired = false, EmitDefaultValue = false, Order = 14)]
[DataMember(Name = "properties", IsRequired = false, EmitDefaultValue = false, Order = 16)]
internal override IDictionary<string, SerializedPropertyInfo> Properties { get; set; }

/// <summary>
Expand Down
Loading