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

shaopeng-gh · 2022-01-07T08:31:27Z

Description:

when working on fix microsoft/binskim#533
we encounter 2 more issues

json order changed
related to changeset Baseline tools binskim#344
location: file BA2006.BuildWithSecureTools.cs line 104
emit empty AutomationDetails node
related to changeset Enable automation details for command base #2407
location: file Run.cs line 223

Fix:

json order changed ---- the common way json order is set by the field attribute.
and the benefit is when we fix the order in field attribute, now both single thread and multi thread will generate json file with same order. before fix, it is currently generating different order.
emit empty AutomationDetails node --- this is a existing common problem than the AutomationDetails field itself,
whatever code we write in ShouldSerializeXField() will only get invoked if we use default direct whole object Serialization. Those code we use Json Writer to manually write Field by Field will not invoke it, we should use the same logic.

shaopeng-gh · 2022-01-07T09:47:37Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

                        {
-                            AutomationDetails = new RunAutomationDetails
+                            _run.AutomationDetails = new RunAutomationDetails


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

shaopeng-gh · 2022-01-07T09:49:54Z

src/Sarif/NotYetAutoGenerated/ReportingDescriptor.cs

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


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.

shaopeng-gh · 2022-01-07T23:42:14Z

...arif/TestData/Multitool/ValidateCommand/Inputs/SARIF2012.ProvideRuleProperties_Invalid.sarif

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


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.

shaopeng-gh · 2022-01-08T00:32:04Z

src/Sarif/NotYetAutoGenerated/ReportingDescriptor.cs

@@ -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)]


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"
}
}

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

marmegh · 2022-01-08T00:49:20Z

We really have two identical files in two different folders? I don't expect this to be changed this go around, but I'm curious Michael/Eddy: was there a reason for the repetition here?
Edit to clarify: I'm referring to ReportingDescriptor.cs. #Closed

marmegh · 2022-01-08T01:15:23Z

src/Sarif/Autogenerated/ReportingDescriptor.cs

@@ -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; }


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

marmegh · 2022-01-08T01:17:43Z

src/ReleaseHistory.md

@@ -2,6 +2,7 @@

 ## Unreleased

+* BUGFIX: Revert Json Serialization field order change and skip emit empty AutomationDetails node. [#2420](https://github.com/microsoft/sarif-sdk/pull/2420)


BUGFIX: Revert Json Serialization field order change and skip emit empty AutomationDetails node.

I was under the impression that the order change reversion was just until the other PR is approved and then we'll settle on a final order and consolidate the two changes.

Would this be better described accordingly:
"Standardizing Json Serialization field order and..."? #Resolved

I would consider this to be my only blocking comment at the moment.

agree, let me change to word like "Adjust" instead of Standardizing I fear people think it is a whole solution standardizing by looking the title

updated the title.

Nice work! Looks like you've addressed Mary's comment, so I'm going to assume approval there and merge. :) Mary, if you see a retroactive adjustment you'd like, of course we can do that next.

shaopeng-gh · 2022-01-08T01:24:37Z

add my input:
we have those classes auto generated by tool https://github.com/microsoft/jschema
(this tool is also own my our team)
since there are still some attributes/capability we are not yet supporting, for those class we make a copy from autogenerated folder to notyet folder.
and make changes needed in the notyet folder. when build, script will copy from notyet folder to autogenerated folder.
(but I suggest please keep them insync and check in both because it will be easier for local debugging)

In reply to: 1007846306

shaopeng-gh · 2022-01-08T02:23:42Z

Your question reminds me of I think I should add a note for not yet auto generated comment in code. Added.
// NOTYETAUTOGENERATED: Order attribute

In reply to: 1007856536

eddynaka · 2022-01-08T12:31:09Z

src/Sarif/Writers/ResultLogJsonWriter.cs

+            if (_run.ShouldSerializeAutomationDetails())
+            {
+                Serialize(_run.AutomationDetails, "automationDetails");
+            }


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

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

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.

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

… add the same logic as this.

eddynaka

michaelcfanning

shaopeng-gh added 3 commits January 6, 2022 23:03

Fix empty AutomationDetails node

0a4b77b

set the order to the exact order before issue happened

f3540a3

use DataMember.Order property to set json node order

fd89033

shaopeng-gh commented Jan 7, 2022

View reviewed changes

shaopeng-gh added 3 commits January 7, 2022 11:10

test remove SerializeIfNotNull(_run.RunAggregates, "runAggregates");

f101a04

trigger the same check when manual write json nodes one by one

d7cc31e

fix baseline according to the new Order

d2b47a4

shaopeng-gh marked this pull request as ready for review January 7, 2022 23:28

shaopeng-gh requested review from eddynaka and michaelcfanning as code owners January 7, 2022 23:28

shaopeng-gh commented Jan 7, 2022

View reviewed changes

shaopeng-gh commented Jan 8, 2022

View reviewed changes

shaopeng-gh changed the title ~~Revert Json Serialization order change~~ BUGFIX: Revert Json Serialization field order change and skip emit empty AutomationDetails node Jan 8, 2022

update ReleaseHistory.md

9f45c86

marmegh reviewed Jan 8, 2022

View reviewed changes

shaopeng-gh changed the title ~~BUGFIX: Revert Json Serialization field order change and skip emit empty AutomationDetails node~~ BUGFIX: Adjust Json Serialization field order for ReportingDescriptor and skip emit empty AutomationDetails node Jan 8, 2022

update change description

6e6b2e5

eddynaka reviewed Jan 8, 2022

View reviewed changes

check all other run properties are using the shouldSerialize pattern,…

125f31e

… add the same logic as this.

eddynaka approved these changes Jan 10, 2022

View reviewed changes

michaelcfanning approved these changes Jan 10, 2022

View reviewed changes

michaelcfanning merged commit 4959f73 into main Jan 10, 2022

michaelcfanning deleted the users/shaopeng-gh/fixjsonorder branch January 10, 2022 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shaopeng-gh commented Jan 7, 2022 •

edited

Loading

shaopeng-gh Jan 7, 2022 •

edited by eddynaka

Loading

shaopeng-gh Jan 7, 2022

shaopeng-gh Jan 7, 2022 •

edited

Loading

shaopeng-gh Jan 8, 2022

marmegh Jan 8, 2022

marmegh commented Jan 8, 2022 •

edited

Loading

marmegh Jan 8, 2022 •

edited

Loading

marmegh Jan 8, 2022 •

edited by shaopeng-gh

Loading

marmegh Jan 8, 2022

shaopeng-gh Jan 8, 2022

shaopeng-gh Jan 8, 2022

michaelcfanning Jan 10, 2022

shaopeng-gh commented Jan 8, 2022

shaopeng-gh commented Jan 8, 2022

eddynaka Jan 8, 2022 •

edited by shaopeng-gh

Loading

eddynaka Jan 8, 2022

shaopeng-gh Jan 9, 2022

eddynaka left a comment

michaelcfanning left a comment

		@@ -2,6 +2,7 @@

		## Unreleased

		* BUGFIX: Revert Json Serialization field order change and skip emit empty AutomationDetails node. [#2420](https://github.com/microsoft/sarif-sdk/pull/2420)

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

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

Conversation

shaopeng-gh commented Jan 7, 2022 • edited Loading

Description:

Fix:

shaopeng-gh Jan 7, 2022 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh commented Jan 8, 2022 • edited Loading

marmegh Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

marmegh Jan 8, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh commented Jan 8, 2022

shaopeng-gh commented Jan 8, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka left a comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

shaopeng-gh commented Jan 7, 2022 •

edited

Loading

shaopeng-gh Jan 7, 2022 •

edited by eddynaka

Loading

shaopeng-gh Jan 7, 2022 •

edited

Loading

marmegh commented Jan 8, 2022 •

edited

Loading

marmegh Jan 8, 2022 •

edited

Loading

marmegh Jan 8, 2022 •

edited by shaopeng-gh

Loading

eddynaka Jan 8, 2022 •

edited by shaopeng-gh

Loading