BREAKING: `Id` field of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) #2463

shaopeng-gh · 2022-03-02T03:04:21Z

Description:

We found that in SARIF spec, there is no definition of the max length of integer type, whether it is 32 bit, 64 bit or no limit.
Our current SDK code assumes 32 bit, which should be wrong assumption, should be no limit since spec does not mention any limit.

Fixes:

Spot fix the Location.Id to use BigInteger which is no limit.

Note that we should also change the jschema to use BigInteger across the solution. created a issue there:
microsoft/jschema#144

…nteger`(unlimited)

shaopeng-gh · 2022-03-02T06:22:53Z

src/Sarif/NotYetAutoGenerated/Location.cs

+    /// </summary>
+    [DataContract]
+    [GeneratedCode("Microsoft.Json.Schema.ToDotNet", "1.1.0.0")]
+    public partial class Location : PropertyBagHolder, ISarifNode


Location

created a copy of the Location.cs to
NotYetAutoGenerated folder to make modification. #Closed

shaopeng-gh · 2022-03-02T06:29:22Z

src/Sarif/Autogenerated/Location.cs

+        public bool ShouldSerializeId()
+        {
+            return Id != -1;
+        }


this is needed because DefaultValue does not work for BigInteger.

then this need 2 things,

in the constructor to set it, which we already have.

ShouldSerializeXXX to handle the serialization. #Closed

michaelcfanning · 2022-03-02T23:33:19Z

src/ReleaseHistory.md

@@ -11,6 +11,7 @@
 * BUGFIX: Fix `Merge` command produces empty SARIF file in Linux when providing file name only without path. [#2408](https://github.com/microsoft/sarif-sdk/pull/2408)
 * BUGFIX: Fix `NullReferenceException` when filing work item with a SARIF file which has no filable results. [#2412](https://github.com/microsoft/sarif-sdk/pull/2412)
 * BUGFIX: Fix missing `endLine` and `endColumn` properties and remove vulnerable packages for ESLint SARIF formatter. [#2458](https://github.com/microsoft/sarif-sdk/pull/2458)
+* BREAKING: `Id` field of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) [#2463](https://github.com/microsoft/sarif-sdk/pull/2463)


field

use 'property' not 'field' as a term. Add something about the exception message you see on deserializing a id value larger than 2^31 #Closed

michaelcfanning · 2022-03-02T23:34:13Z

src/Sarif/Autogenerated/Location.cs

+
+        public bool ShouldSerializeId()
+        {
+            return Id != -1;


return Id != -1

return Id >-1; #Closed

Please add a unit test for this.

michaelcfanning · 2022-03-02T23:38:55Z

src/Test.UnitTests.Sarif/Core/LocationTests.cs

+            location.Id = int.MaxValue;
+            AssertShouldSerializeId(location, true);
+
+            location.Id = long.MaxValue;


long

add ulong max value #Closed

michaelcfanning · 2022-03-02T23:40:15Z

src/Test.UnitTests.Sarif/Core/LocationTests.cs

+            AssertShouldSerializeId(location, false);
+
+            location.Id = -1;
+            AssertShouldSerializeId(location, false);


add test for long.MinValue - 1
add test for long.MinValue
add test for int.MinValue
add test for -2 #Closed

michaelcfanning · 2022-03-02T23:41:36Z

src/Test.UnitTests.Sarif/Core/LocationTests.cs

@@ -85,5 +92,68 @@ public void Location_LogicalLocation_WhenSetToNull_RemovesLogicalLocationsArray(

            location.LogicalLocations.Should().BeNull();
        }
+
+        [Fact]
+        public void Location_SerializeId()


Location_SerializeId

These tests may not provoke round-tripping issues?

#Closed

changed to round trip

michaelcfanning

shaopeng-gh · 2022-03-16T23:15:07Z

src/ReleaseHistory.md

 * BUGFIX: Eliminate dispose of stream and `StreamWriter` arguments passed to `SarifLog.Save` helpers. This would result in `ObjectDisposedException` being raised on attempt to access streams after save.
+* BREAKING: `Id` property of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) to fix `Newtonsoft.Json.JsonReaderException: JSON integer XXXXX is too large or small for an Int32.` [#2463](https://github.com/microsoft/sarif-sdk/pull/2463)


removed the very first part "error ERR999.UnhandledEngineException" which only apply to the SDK validate tool.

the rest error message apply to both SDK validate tool and directly using:
JsonConvert.DeserializeObject<SarifLog>(content)

shaopeng-gh · 2022-03-16T23:16:35Z

src/Test.UnitTests.Sarif/Core/LocationTests.cs

+        }
+
+        [Fact]
+        public void Location_VerifyAbleToDeserializeWithBigIntegerId()


public void Location_VerifyAbleToDeserializeWithBigIntegerId()

added a new test for JsonConvert.DeserializeObject<SarifLog>(content)
from a SARIF file.

shaopeng-gh · 2022-03-18T23:20:23Z

src/Test.UnitTests.Sarif/TestData/JsonConverters/LocationId_BigInteger.sarif

+          "level": "note",
+          "locations": [
+            {
+              "id": 31197130097450771296369962162453149327732752356239421572342053257324632475324,


31197130097450771296369962162453149327732752356239421572342053257324632475324

data in this file contains very long int, 64 int max +1, 32 int max +1 and a few normal ones.

michaelcfanning and others added 4 commits February 16, 2022 12:33

Investigate converting 'location.id' to BigInteger type.

eb47217

fix build and test

9b89197

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

b78fe3d

BREAKING: Id field of Location changed from int(32bit) to `BigI…

70b392e

…nteger`(unlimited)

shaopeng-gh changed the title ~~testing build for bigint branch~~ BREAKING: Id field of Location changed from int(32bit) to BigInteger(unlimited) [#2463](https://github.com/microsoft/sarif-sdk/pull/2463) Mar 2, 2022

shaopeng-gh changed the title ~~BREAKING: Id field of Location changed from int(32bit) to BigInteger(unlimited) [#2463](https://github.com/microsoft/sarif-sdk/pull/2463)~~ BREAKING: Id field of Location changed from int(32bit) to BigInteger(unlimited) Mar 2, 2022

shaopeng-gh marked this pull request as ready for review March 2, 2022 06:20

shaopeng-gh requested review from eddynaka and michaelcfanning as code owners March 2, 2022 06:20

shaopeng-gh requested review from yongyan-gh and marmegh March 2, 2022 06:20

shaopeng-gh commented Mar 2, 2022

View reviewed changes

michaelcfanning reviewed Mar 2, 2022

View reviewed changes

michaelcfanning approved these changes Mar 2, 2022

View reviewed changes

shaopeng-gh marked this pull request as draft March 9, 2022 22:13

shaopeng-gh and others added 10 commits March 11, 2022 11:29

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

6eb4230

update history.md

061f246

fix comments

8a9b86a

Change test to round trip for location id

32bb4b0

use Fluent Assertions

4b3a441

adding debug info

efe5116

debug code

6b0db9f

debug code 2

c0717b2

debug test 3

7efb48c

debug test

257a413

Fix test VerifyIdRoundTripFromObject

af8f2fc

shaopeng-gh marked this pull request as ready for review March 12, 2022 00:58

shaopeng-gh added 3 commits March 16, 2022 16:01

add SARIF file test

e6ee5a0

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

6571aee

fix var name

1734dfe

shaopeng-gh marked this pull request as draft March 16, 2022 23:10

shaopeng-gh commented Mar 16, 2022

View reviewed changes

shaopeng-gh marked this pull request as ready for review March 16, 2022 23:17

shaopeng-gh commented Mar 18, 2022

View reviewed changes

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

fb46218

michaelcfanning enabled auto-merge (squash) March 24, 2022 16:24

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

1b1aa3a

michaelcfanning merged commit ed38f39 into main Mar 24, 2022

michaelcfanning deleted the users/shaopeng-gh/bigint branch March 24, 2022 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: `Id` field of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) #2463

BREAKING: `Id` field of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) #2463

shaopeng-gh commented Mar 2, 2022 •

edited

Loading

shaopeng-gh Mar 2, 2022 •

edited

Loading

shaopeng-gh Mar 2, 2022 •

edited

Loading

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

shaopeng-gh Mar 11, 2022

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Mar 2, 2022

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

shaopeng-gh Mar 11, 2022

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

shaopeng-gh Mar 11, 2022 •

edited

Loading

michaelcfanning left a comment

shaopeng-gh Mar 16, 2022

shaopeng-gh Mar 16, 2022

shaopeng-gh Mar 18, 2022

		* BUGFIX: Eliminate dispose of stream and `StreamWriter` arguments passed to `SarifLog.Save` helpers. This would result in `ObjectDisposedException` being raised on attempt to access streams after save.
		* BREAKING: `Id` property of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) to fix `Newtonsoft.Json.JsonReaderException: JSON integer XXXXX is too large or small for an Int32.` [#2463](https://github.com/microsoft/sarif-sdk/pull/2463)

BREAKING: Id field of Location changed from int(32bit) to BigInteger(unlimited) #2463

BREAKING: Id field of Location changed from int(32bit) to BigInteger(unlimited) #2463

Conversation

shaopeng-gh commented Mar 2, 2022 • edited Loading

Description:

Fixes:

shaopeng-gh Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

michaelcfanning Mar 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Mar 11, 2022

Choose a reason for hiding this comment

michaelcfanning Mar 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

michaelcfanning Mar 2, 2022

Choose a reason for hiding this comment

michaelcfanning Mar 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

michaelcfanning Mar 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Mar 11, 2022

Choose a reason for hiding this comment

michaelcfanning Mar 2, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

shaopeng-gh Mar 16, 2022

Choose a reason for hiding this comment

shaopeng-gh Mar 16, 2022

Choose a reason for hiding this comment

shaopeng-gh Mar 18, 2022

Choose a reason for hiding this comment

BREAKING: `Id` field of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) #2463

BREAKING: `Id` field of `Location` changed from `int`(32bit) to `BigInteger`(unlimited) #2463

shaopeng-gh commented Mar 2, 2022 •

edited

Loading

shaopeng-gh Mar 2, 2022 •

edited

Loading

shaopeng-gh Mar 2, 2022 •

edited

Loading

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Mar 2, 2022 •

edited by shaopeng-gh

Loading

shaopeng-gh Mar 11, 2022 •

edited

Loading