Uuid Changes #2555

shaopeng-gh · 2022-10-13T22:53:45Z

BREAKING: For Guid properties defined in SARIF spec, updated Json schema to use uuid, and updated C# object model to use Guid? instead of string.

This is based on the updated JSchema Code v2.1.0 that supported generate nullable Guid?.
NOTE: Had to update the $schema from draft-04 to latest 2020-12 which has the draw back of VS not able to auto-compete. The Uuid introduced in 2019-09, however since VS also not able to auto-compete in that version, if we are updating might as well update to the latest version.

… `Guid?` instead of `string`.

shaopeng-gh · 2022-10-13T23:04:45Z

src/Sarif/NotYetAutoGenerated/AddressComparer.cs

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


I found a bug here, if both left and right are null, TryReferenceCompares will directly return true and all the other properties are not compared down below.

The fix by adding CompareTonot only fix this bug but also able to remove this file entirely.
#Closed

shaopeng-gh · 2022-10-13T23:17:29Z

src/Sarif/NotYetAutoGenerated/Location.cs

-        public bool ShouldSerializeId()
-        {
-            return Id >= 0;
-        }


The better place for this is to put in core folder, this is not related to this change, moved btw. #ByDesign

Yes! But see my other comment. I wondered why we preserved this should serialize id if we have 'emit default value' == false.

One reason is that someone could set this number to a garbage value like -37, which wouldn't be removed by the serialization attribute.

If that's true, though, we don't really need the default value and emit default value attributes.

But perhaps we do, because someone could emit serialized json where the -1 is explicit (that's legal!) but we don't want to populate in this case.

OK, so talking it all over, I agree with your implementation. :) We need to preserve this file as it is and retain (and move) ShouldSerializeId().

Thanks for listening/. :)

why we preserved this should serialize id if we have 'emit default value' == false.
------ I think we need both because
ShouldSerializeXXX is from Newtonsoft.Json and used in Newtonsoft.Json.JsonConvert
EmitDefaultValue in the DataMember is used in dotnet e.g. DataContractSerializer
I tried removing either one of them will make the corresponding one populate id:-1 value in the file.

github-advanced-security

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

michaelcfanning · 2022-10-17T21:42:16Z

src/Sarif/ExtensionMethods.cs

+        /// <returns>
+        /// Returns 1 if left is greater than right, -1 if left is less than right, 0 if they are equal.
+        /// </returns>
+        internal static int CompareTo<T>(this T? left, T? right) where T : struct, IComparable


Did we just implement Nullable.Compare here? :)

I think so. If I'm correct, let's zap this helper and use the C# built-in.

https://learn.microsoft.com/en-us/dotnet/api/system.nullable.compare?view=net-6.0 #Resolved

Good catch. I have just updated the extension method to use the C# built-in. Let me close this one and reply on the other so no need to look duplicated

michaelcfanning · 2022-10-17T21:44:40Z

src/Sarif/Autogenerated/ExternalPropertiesComparer.cs

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

-            compareResult = string.Compare(left.RunGuid, right.RunGuid);
+            compareResult = left.RunGuid.CompareTo(right.RunGuid);


left.RunGuid.CompareTo(right.RunGuid)

This is where we would prefer Nullable.Compare(left, right), if possible. Would this require an update to JSchema?

Good catch. I have first updated my extension method to utilize the C# built-in method.

Nullable.Compare(left, right) will need to update the JSchema, it will need to be able to identify if a type is nullable, also struct not ref type and generate different code:
from:
left.RunGuid.CompareTo(right.RunGuid);
to:
Nullable.Compare(left.RunGuid., right.RunGuid)

also all those other nullable in the notyetautogenerated folder, in the JSchema we will not be able to know they are going to be replaced manually and we will read them as not nullable there.
By adding an extension method it works with above. e.g. that file that I removed AddressComparer.cs.

let me know your instruction if to keep the extension method.

michaelcfanning · 2022-10-17T21:46:33Z

src/Sarif/Autogenerated/Location.cs

@@ -45,11 +45,6 @@ public virtual SarifNodeKind SarifNodeKind
        [JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
        public virtual BigInteger Id { get; set; }

-        public bool ShouldSerializeId()


ShouldSerializeId

Why did this get deleted? Is it because we specify -1 as a default value and 'emit default value' is false, so this is redundant?

Can you explain how this code got left behind, if so? Did we take an autogenerator fix? #Closed

closing this, replied in the other one.

michaelcfanning · 2022-10-17T21:48:01Z

src/Sarif/Autogenerated/ReportingDescriptor.cs


        /// <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 = 11)]
-        public virtual IList<string> DeprecatedGuids { get; set; }
+        public virtual IList<Guid> DeprecatedGuids { get; set; }


DeprecatedGuids

Just curious, do we have an explicit test that populates, serializes this list of Guids? I think we do. It just occurs to me this is a pretty specialized code path/utilization, these list of struct Guids... #WontFix

yes we did have, and I made those test data from string to Guid in the list.

michaelcfanning · 2022-10-17T21:50:44Z

src/Sarif/Autogenerated/ReportingDescriptorEqualityComparer.cs

@@ -235,10 +235,7 @@ public int GetHashCode(ReportingDescriptor obj)
                    foreach (var value_5 in obj.DeprecatedGuids)
                    {
                        result = result * 31;
-                        if (value_5 != null)


null

Wouldn't the right test here be

if (value_5 != default)
OR
if (value_5 != Guid.Empty)

or do we require/benefit from processing the hashcode for these empty items?

Note that an empty guid is not a real guid. OTOH, we should regard two lists as unique if the only difference between them is that one contains Guid.Empty and the other does not, right?

So maybe there is no change here...
#ByDesign

I thought about this a well,
this change is auto-generated not hand made, I believe this is correct because deprecatedGuids is a list of valid GUID and null is not a valid GUID so it can not be in the list, so each item in the list is not nullable and then the null check is not needed. So the DeprecatedGuids is defined as C# List<Guid> without the question mark, let me know.
the definistion says "an array of zero or more unique (§3.7.3) GUID-valued strings (§3.5.3)":
A reportingDescriptor object MAY contain a property named deprecatedGuids whose value is an array of zero or more unique (§3.7.3) GUID-valued strings (§3.5.3) each of which was used by a previous version of the tool as the value of the guid property (§3.49.5) for this object.

michaelcfanning · 2022-10-17T21:54:23Z

src/Sarif/Core/Location.cs

@@ -22,5 +22,10 @@ public LogicalLocation LogicalLocation
                }
            }
        }
+
+        public bool ShouldSerializeId()


ShouldSerializeId

Ah! you moved this out of the 'NotYetAutogenerated' directory, now I understand. But isn't it true that this isn't required because of the serialization attributes set on the Location 'Id' property? Or am I mistaken?

If I'm correct, let's spend a little time looking for similar errors, i.e., other unnecessary 'Should' serialization methods... #Closed

I believe it is needed, please see the comment on the place I removed it.

michaelcfanning · 2022-10-17T21:57:02Z

src/Sarif/Core/RunAutomationDetails.cs

@@ -12,8 +12,8 @@ public partial class RunAutomationDetails

        public bool ShouldSerializeId() => !string.IsNullOrWhiteSpace(this.Id);

-        public bool ShouldSerializeGuid() => !string.IsNullOrWhiteSpace(this.Guid);
+        public bool ShouldSerializeGuid() => this.Guid != null;


this.Guid

these comparisons to null are fine, but we could also use this.Guid.HasValue, yes? #Resolved

yes I actually thought about this as well, I believe most people prefers .HasValue,
the only reason I was thinking about consistency with how string compares with null.

Let me know your thoughts?

Below are some random points in internet:

"The compiler replaces null comparisons with a call to HasValue, so there is no real difference. Just do whichever is more readable/makes more sense to you and your colleagues."

"It's pretty silly to complain about being able to set a Nullable to null or compare it to null given that's called Nullable"

"I prefer (a != null) so that the syntax matches reference types."

michaelcfanning · 2022-10-17T21:58:09Z

src/Sarif/NotYetAutoGenerated/AddressComparer.cs

-using System.CodeDom.Compiler;
-using System.Collections.Generic;
-
-namespace Microsoft.CodeAnalysis.Sarif


CodeAnalysis

Excellent to see this code go! #Closed

michaelcfanning · 2022-11-03T19:29:15Z

src/Sarif/Schemata/sarif-2.1.0-rtm.5.json

@@ -1,5 +1,5 @@
 {
-  "$schema": "http://json-schema.org/draft-04/schema#",
+  "$schema": "https://json-schema.org/draft/2020-12/schema",


$schema

Sorry for the late-breaking comment! You need to produce an rtm.6 version of this file, we can't modify rtm.5, it's an artifact of the standards process. Ask Chris Meyer for information about how to update everything to use a new flavor of schema.

Sorry again for the delay giving you this advice.

michaelcfanning · 2022-11-03T19:30:02Z

"id": "https://raw.githubusercontent.com/schemastore/schemastore/master/src/schemas/json/sarif-external-property-file-2.1.0-rtm.5.json",

This is why you can't just update these files, we need to publish new versions in the schema store.

Refers to: src/Sarif/Schemata/sarif-external-property-file-2.1.0-rtm.5.json:4 in ebaff74. [](commit_id = ebaff74, deletion_comment = False)

michaelcfanning · 2022-11-03T19:31:38Z

src/Test.EndToEnd.Baselining/Extensions/SarifLogExtensions.cs

@@ -59,7 +59,9 @@ public static IEnumerable<Run> EnumerateRuns(this SarifLog log)

        public static Result FindByGuid(this SarifLog log, string guid)


FindByGuid

Should we simply have updated this API to accept a GUID argument?? Something to keep in mind moving forward.

michaelcfanning

michaelcfanning · 2022-11-03T19:34:31Z

@shaopeng-gh, what a fantastic (and enormous!) change. Please see my notes, we need to revert your changes to the rtm.5 schema and create an rtm.6. I didn't want to delay getting this in, we'll fix things up as a follow-on change. @EasyRhinoMSFT is familiar with this process (and actually may be willing to simply take on the work).

shaopeng-gh · 2022-11-03T23:05:54Z

Thanks for the review!
@EasyRhinoMSFT Talked to Michael I will create a clean PR that reverts RTM.5 and adds RTM.6 (in progress version). We won't publish RTM.6 to schemastore.org or elsewhere until finalize it with the next PR with Big Integer changes.

Updated Json schema to use uuid, and updated C# object model to use…

d083029

… `Guid?` instead of `string`.

shaopeng-gh requested review from michaelcfanning, EasyRhinoMSFT, marmegh and cfaucon as code owners October 13, 2022 22:53

shaopeng-gh commented Oct 13, 2022

View reviewed changes

github-advanced-security bot found potential problems Oct 13, 2022

View reviewed changes

shaopeng-gh added 2 commits October 13, 2022 16:31

Add a test

2e70072

Update PR number

846da85

shaopeng-gh requested a review from HulonJenkins October 14, 2022 00:21

michaelcfanning reviewed Oct 17, 2022

View reviewed changes

shaopeng-gh and others added 2 commits October 18, 2022 00:50

Use Nullable.Compare

eda8d1b

Merge branch 'main' into users/shaopeng-gh/useuuidpr

ed4bc10

shaopeng-gh requested a review from michaelcfanning October 18, 2022 08:52

shaopeng-gh and others added 2 commits November 1, 2022 11:38

merge from main

aa68481

Merge branch 'main' into users/shaopeng-gh/useuuidpr

ebaff74

michaelcfanning reviewed Nov 3, 2022

View reviewed changes

michaelcfanning approved these changes Nov 3, 2022

View reviewed changes

michaelcfanning merged commit c0121cb into main Nov 3, 2022

michaelcfanning deleted the users/shaopeng-gh/useuuidpr branch November 3, 2022 19:33

shaopeng-gh mentioned this pull request Nov 4, 2022

SARIF Schema Revert change in rtm.5 and add rtm.6 #2571

Merged

This was referenced Feb 21, 2023

Releasing v3.2.0 #2622

Closed

Releasing 4.0.0 #2623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uuid Changes #2555

Uuid Changes #2555

shaopeng-gh commented Oct 13, 2022

shaopeng-gh Oct 13, 2022 •

edited

shaopeng-gh Oct 13, 2022 •

edited

michaelcfanning Oct 17, 2022

shaopeng-gh Oct 18, 2022

github-advanced-security bot left a comment

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

shaopeng-gh Oct 18, 2022

michaelcfanning Oct 17, 2022

shaopeng-gh Oct 18, 2022 •

edited

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

shaopeng-gh Oct 18, 2022

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

shaopeng-gh Oct 18, 2022

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

shaopeng-gh Oct 18, 2022 •

edited

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

shaopeng-gh Oct 18, 2022

michaelcfanning Oct 17, 2022 •

edited

shaopeng-gh Oct 18, 2022

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

michaelcfanning Nov 3, 2022

michaelcfanning commented Nov 3, 2022

michaelcfanning Nov 3, 2022

michaelcfanning left a comment

michaelcfanning commented Nov 3, 2022

shaopeng-gh commented Nov 3, 2022

		@@ -59,7 +59,9 @@ public static IEnumerable<Run> EnumerateRuns(this SarifLog log)

		public static Result FindByGuid(this SarifLog log, string guid)

Uuid Changes #2555

Uuid Changes #2555

Conversation

shaopeng-gh commented Oct 13, 2022

shaopeng-gh Oct 13, 2022 • edited

Choose a reason for hiding this comment

shaopeng-gh Oct 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

michaelcfanning Oct 17, 2022 • edited by shaopeng-gh

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh Oct 18, 2022 • edited

Choose a reason for hiding this comment

michaelcfanning Oct 17, 2022 • edited by shaopeng-gh

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Oct 17, 2022 • edited by shaopeng-gh

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Oct 17, 2022 • edited by shaopeng-gh

Choose a reason for hiding this comment

shaopeng-gh Oct 18, 2022 • edited

Choose a reason for hiding this comment

michaelcfanning Oct 17, 2022 • edited by shaopeng-gh

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Oct 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Oct 17, 2022 • edited by shaopeng-gh

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Nov 3, 2022

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning commented Nov 3, 2022

shaopeng-gh commented Nov 3, 2022

shaopeng-gh Oct 13, 2022 •

edited

shaopeng-gh Oct 13, 2022 •

edited

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

shaopeng-gh Oct 18, 2022 •

edited

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

shaopeng-gh Oct 18, 2022 •

edited

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh

michaelcfanning Oct 17, 2022 •

edited

michaelcfanning Oct 17, 2022 •

edited by shaopeng-gh