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 8 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
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

* 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)

## **v2.4.12** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.12) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.12) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.12) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.12) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.12)
Expand Down
2 changes: 0 additions & 2 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
33 changes: 18 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 @@ -37,91 +37,94 @@ public SarifNodeKind SarifNodeKind

// NOTYETAUTOGENERATED: Jschema needs a mechanism to emit all public methods as virtual
// https://github.com/Microsoft/jschema/issues/97
// NOTYETAUTOGENERATED: Order attribute
// https://github.com/microsoft/jschema/issues/140
//
/// <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
33 changes: 18 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 @@ -37,91 +37,94 @@ public SarifNodeKind SarifNodeKind

// NOTYETAUTOGENERATED: Jschema needs a mechanism to emit all public methods as virtual
// https://github.com/Microsoft/jschema/issues/97
// NOTYETAUTOGENERATED: Order attribute
// https://github.com/microsoft/jschema/issues/140
//
/// <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
23 changes: 18 additions & 5 deletions src/Sarif/Writers/ResultLogJsonWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,18 +318,31 @@ public void CompleteRun()
WriteLogicalLocations(_run.LogicalLocations);
}

SerializeIfNotNull(_run.Graphs, "graphs");
// All ShouldSerialize() will not be triggered automatically when manually write node by node.
// To make sure the same logic applied we should call the same method here, if it is defined.
if (_run.ShouldSerializeGraphs())
{
Serialize(_run.Graphs, "graphs");
}

// Results go here in schema order

SerializeIfNotNull(_run.AutomationDetails, "automationDetails");
if (_run.ShouldSerializeAutomationDetails())
{
Serialize(_run.AutomationDetails, "automationDetails");
}
Copy link
Collaborator

@eddynaka eddynaka Jan 8, 2022 •
edited by shaopeng-gh
Loading

Choose a reason for hiding this comment

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

can you create a test using resultlogjsonwriter and verify if it will emit or not. #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, check if the other run properties are using the shouldSerialize pattern and if yes, we should add the same logic as this.

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. for this one, before this PR, the test we already have
    "AnalyzeCommandBase_ShouldEmitAutomationDetailsWhenIdOrGuidExists()"
    is already going this path and use the resultlogjsonwriter and verify if it is correctly emitted, (can F5 to step though)

the problem was just there is no case for validate the opposite situation when it should not emit,

so I added cases for null to loop in it, and change the test method name to a generic name reflect it tests both cases.

  1. agree, I just go though all fields in the run.cs those have shouldSerialize, and made the same change, updated the PR. the new ones I added are:

ShouldSerializeArtifacts
ShouldSerializeInvocations
ShouldSerializeLogicalLocations

SerializeIfNotNull(_run.RunAggregates, "runAggregates");
SerializeIfNotNull(_run.BaselineGuid, "baselineGuid");
SerializeIfNotNull(_run.RedactionTokens, "redactionTokens");
SerializeIfNotNull(_run.DefaultEncoding, "defaultEncoding");
SerializeIfNotNull(_run.DefaultSourceLanguage, "defaultSourceLanguage");
SerializeIfNotNull(_run.NewlineSequences, "newlineSequences");
SerializeIfNotNull(_run.ColumnKind == ColumnKind.UnicodeCodePoints ? "unicodeCodePoints" : "utf16CodeUnits", "columnKind");
if (_run.ShouldSerializeNewlineSequences())
{
Serialize(_run.NewlineSequences, "newlineSequences");
}
if (_run.ShouldSerializeColumnKind())
{
Serialize(_run.ColumnKind == ColumnKind.UnicodeCodePoints ? "unicodeCodePoints" : "utf16CodeUnits", "columnKind");
}
SerializeIfNotNull(_run.ExternalPropertyFileReferences, "externalPropertyFileReferences");
SerializeIfNotNull(_run.ThreadFlowLocations, "threadFlowLocations");
SerializeIfNotNull(_run.Taxonomies, "taxonomies");
Expand Down
2 changes: 1 addition & 1 deletion ...a/Multitool/ValidateCommand/ExpectedOutputs/SARIF2012.ProvideRuleProperties_Invalid.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@
"index": 0
},
"region": {
"startLine": 21,
"startLine": 22,
"startColumn": 46
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...tool/ValidateCommand/ExpectedOutputs/SARIF2014.ProvideDynamicMessageContent_Invalid.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
"index": 0
},
"region": {
"startLine": 18,
"startLine": 19,
"startColumn": 74
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...tool/ValidateCommand/ExpectedOutputs/SARIF2015.EnquoteDynamicMessageContent_Invalid.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"index": 0
},
"region": {
"startLine": 18,
"startLine": 19,
"startColumn": 91
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...f/TestData/Multitool/ValidateCommand/Inputs/SARIF2012.ProvideRuleProperties_Invalid.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
},
{
"id": "TEST0002",
"name": "This isn't pascal case",
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html",
"name": "This isn't pascal case"
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

test case logic is, read this json input into c# object, and write the object back to json, and then do analyze on it.

when we write the c# object back to json, with the order currently set, helpUri is before name, so the sarif be will differnt than the input file. so the analyze result on top of that will have the line number +1 from 21 to 22.

To make test more readable I changed this input to be the same order so when we see the error is line 22 we can understand which line it is pointing to. Let me know your thoughts.

}
]
}
Expand Down
4 changes: 2 additions & 2 deletions ...ata/Multitool/ValidateCommand/Inputs/SARIF2014.ProvideDynamicMessageContent_Invalid.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@
"fullDescription": {
"text": "This is a test."
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html",
"messageStrings": {
"NoPlaceholders": {
"text": "This message does not contain dynamic content."
}
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
}
]
}
Expand Down
4 changes: 2 additions & 2 deletions ...ata/Multitool/ValidateCommand/Inputs/SARIF2015.EnquoteDynamicMessageContent_Invalid.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@
"fullDescription": {
"text": "This is a test."
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html",
"messageStrings": {
"NotEnquoted": {
"text": "This message contains dynamic content {0} that is not enquoted."
}
},
"helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html"
}
}
]
}
Expand Down
Loading