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

BUGFIX: Adjust Json Serialization field order for ReportingDescriptor and skip emit empty AutomationDetails node #2420

Merged
merged 9 commits into from
Jan 10, 2022
Merged

BUGFIX: Adjust Json Serialization field order for ReportingDescriptor and skip emit empty AutomationDetails node #2420

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0a4b77b
Fix empty AutomationDetails node
shaopeng-gh Jan 7, 2022
f3540a3
set the order to the exact order before issue happened
shaopeng-gh Jan 7, 2022
fd89033
use DataMember.Order property to set json node order
shaopeng-gh Jan 7, 2022
f101a04
test remove SerializeIfNotNull(_run.RunAggregates, "runAggregates");
shaopeng-gh Jan 7, 2022
d7cc31e
trigger the same check when manual write json nodes one by one
shaopeng-gh Jan 7, 2022
d2b47a4
fix baseline according to the new Order
shaopeng-gh Jan 7, 2022
9f45c86
update ReleaseHistory.md
shaopeng-gh Jan 8, 2022
6e6b2e5
update change description
shaopeng-gh Jan 8, 2022
125f31e
check all other run properties are using the shouldSerialize pattern,…
shaopeng-gh Jan 9, 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
11 changes: 7 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 @@ -363,14 +363,17 @@ private void InitializeOutputFile(TOptions analyzeOptions, TContext context, ISe

SarifLogger sarifLogger;

_run = new Run()
_run = new Run();

if (!string.IsNullOrWhiteSpace(analyzeOptions.AutomationId)
|| !string.IsNullOrWhiteSpace(analyzeOptions.AutomationGuid))
{
AutomationDetails = new RunAutomationDetails
_run.AutomationDetails = new RunAutomationDetails
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh Jan 7, 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.

new RunAutomationDetails

The issue is, if there is no AutomationId and AutomationGuid, we don't need to create a instance here.
instance not null with all property null will still get written with current code:
SerializeIfNotNull(_run.AutomationDetails, "automationDetails");

#Closed

{
Id = analyzeOptions.AutomationId,
Guid = analyzeOptions.AutomationGuid
}
};
};
}

if (analyzeOptions.SarifOutputVersion != SarifVersion.OneZeroZero)
{
Expand Down
13 changes: 7 additions & 6 deletions src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Security;
using System.Threading.Channels;
using System.Threading.Tasks;

Expand Down Expand Up @@ -660,14 +658,17 @@ private void InitializeOutputFile(TOptions analyzeOptions, TContext context)

SarifLogger sarifLogger;

_run = new Run()
_run = new Run();

if (!string.IsNullOrWhiteSpace(analyzeOptions.AutomationId)
|| !string.IsNullOrWhiteSpace(analyzeOptions.AutomationGuid))
{
AutomationDetails = new RunAutomationDetails
_run.AutomationDetails = new RunAutomationDetails
{
Id = analyzeOptions.AutomationId,
Guid = analyzeOptions.AutomationGuid
}
};
};
}

if (analyzeOptions.SarifOutputVersion != SarifVersion.OneZeroZero)
{
Expand Down
31 changes: 16 additions & 15 deletions src/Sarif/Autogenerated/ReportingDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,87 +41,88 @@ public SarifNodeKind SarifNodeKind
/// <summary>
/// A stable, opaque identifier for the report.
/// </summary>
[DataMember(Name = "id", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "id", IsRequired = false, EmitDefaultValue = false, Order = 1)]
public virtual string Id { get; set; }
Copy link
Contributor

@marmegh marmegh Jan 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this is covered in oru standard yet; but we probably want to consider if we're explicitly setting an order whether we want that to be how we sort files like this or if we want to do alphabetical, etc. #Closed


/// <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)]
[DataMember(Name = "deprecatedIds", IsRequired = false, EmitDefaultValue = false, Order = 8)]
public virtual IList<string> DeprecatedIds { get; set; }

/// <summary>
/// A unique identifer for the reporting descriptor in the form of a GUID.
/// </summary>
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false, Order = 9)]
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)]
[DataMember(Name = "deprecatedGuids", IsRequired = false, EmitDefaultValue = false, Order = 10)]
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)]
[DataMember(Name = "name", IsRequired = false, EmitDefaultValue = false, Order = 7)]
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)]
[DataMember(Name = "deprecatedNames", IsRequired = false, EmitDefaultValue = false, Order = 11)]
public virtual IList<string> DeprecatedNames { get; set; }

/// <summary>
/// A concise description of the report. Should be a single sentence that is understandable when visible space is limited to a single line of text.
/// </summary>
[DataMember(Name = "shortDescription", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "shortDescription", IsRequired = false, EmitDefaultValue = false, Order = 6)]
public virtual MultiformatMessageString ShortDescription { get; set; }

/// <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)]
[DataMember(Name = "fullDescription", IsRequired = false, EmitDefaultValue = false, Order = 2)]
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)]
[DataMember(Name = "messageStrings", IsRequired = false, EmitDefaultValue = false, Order = 5)]
public virtual IDictionary<string, MultiformatMessageString> MessageStrings { get; set; }

/// <summary>
/// Default reporting configuration information.
/// </summary>
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false, Order = 12)]
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)]
[DataMember(Name = "helpUri", IsRequired = false, EmitDefaultValue = false, Order = 3)]
[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)]
[DataMember(Name = "help", IsRequired = false, EmitDefaultValue = false, Order = 4)]
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)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
[DataMember(Name = "relationships", IsRequired = false, EmitDefaultValue = false, Order = 13)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate, Order = 13)]
public virtual IList<ReportingDescriptorRelationship> Relationships { get; set; }

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

/// <summary>
Expand Down
31 changes: 16 additions & 15 deletions src/Sarif/NotYetAutoGenerated/ReportingDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,87 +41,88 @@ public SarifNodeKind SarifNodeKind
/// <summary>
/// A stable, opaque identifier for the report.
/// </summary>
[DataMember(Name = "id", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "id", IsRequired = false, EmitDefaultValue = false, Order = 1)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1

how I get the current order:

I use the version before issue, and then temp change code set all fields always written even it is null, so that I can get the full list of the ordering, and then use the same ordering.

fyi it was like below:

{
"id": "BA2001",
"fullDescription": {
"text": "64-bit images should"
},
"helpUri": "https://gith",
"help": {
"text": "64-bit ima"
},
"messageStrings": {
"Pass": {
"text": "'{0}' is"
},
"Error": {
"text": "'{0}' is"
},
"NotApplicable_InvalidMetadata": {
"text": "'{0}' was"
}
},
"shortDescription": null,
"name": "LoadImageAboveFourGigabyteAddress",
"deprecatedIds": null,
"guid": null,
"deprecatedGuids": null,
"deprecatedNames": null,
"defaultConfiguration": {},
"relationships": null,
"properties": {
"equivalentBinScopeRuleReadableName": "FourGbCheck"
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for being so thorough here with your explanation. I found this super helpful.

public virtual string Id { get; set; }

/// <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)]
[DataMember(Name = "deprecatedIds", IsRequired = false, EmitDefaultValue = false, Order = 8)]
public virtual IList<string> DeprecatedIds { get; set; }

/// <summary>
/// A unique identifer for the reporting descriptor in the form of a GUID.
/// </summary>
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "guid", IsRequired = false, EmitDefaultValue = false, Order = 9)]
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)]
[DataMember(Name = "deprecatedGuids", IsRequired = false, EmitDefaultValue = false, Order = 10)]
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)]
[DataMember(Name = "name", IsRequired = false, EmitDefaultValue = false, Order = 7)]
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)]
[DataMember(Name = "deprecatedNames", IsRequired = false, EmitDefaultValue = false, Order = 11)]
public virtual IList<string> DeprecatedNames { get; set; }

/// <summary>
/// A concise description of the report. Should be a single sentence that is understandable when visible space is limited to a single line of text.
/// </summary>
[DataMember(Name = "shortDescription", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "shortDescription", IsRequired = false, EmitDefaultValue = false, Order = 6)]
public virtual MultiformatMessageString ShortDescription { get; set; }

/// <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)]
[DataMember(Name = "fullDescription", IsRequired = false, EmitDefaultValue = false, Order = 2)]
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)]
[DataMember(Name = "messageStrings", IsRequired = false, EmitDefaultValue = false, Order = 5)]
public virtual IDictionary<string, MultiformatMessageString> MessageStrings { get; set; }

/// <summary>
/// Default reporting configuration information.
/// </summary>
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false)]
[DataMember(Name = "defaultConfiguration", IsRequired = false, EmitDefaultValue = false, Order = 12)]
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)]
[DataMember(Name = "helpUri", IsRequired = false, EmitDefaultValue = false, Order = 3)]
[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)]
[DataMember(Name = "help", IsRequired = false, EmitDefaultValue = false, Order = 4)]
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)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
[DataMember(Name = "relationships", IsRequired = false, EmitDefaultValue = false, Order = 13)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Order

if JsonProperty tag exists we should also set it to the same because the default is -1 will get the filed at the first line.

[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate, Order = 13)]
public virtual IList<ReportingDescriptorRelationship> Relationships { get; set; }

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

/// <summary>
Expand Down