search

microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
191 stars 90 forks source link

SARIF OM changes for nullable int batch 1 #2650

Closed shaopeng-gh closed 1 year ago

shaopeng-gh commented 1 year ago

The reason of this change is, the upgrade of the Microsoft.Json.Schema.* packages exposed an issue that a few int properties should be nullable but it is current not nullable in our C# code.

Due to impact of the 4 line/columns numbers for Region, proposed for a separated PR and will need a confirmation for the details before start.

michaelcfanning commented 1 year ago 
    [DefaultValue(-1)]

why is this still here? :) when will it go away?

In reply to: 1491078338

Refers to: src/Sarif/Autogenerated/Address.cs:43 in fefed9a. [](commit_id = fefed9a7ff09fd0d9a33d1c94a201b14bbe67f04, deletion_comment = False)

michaelcfanning commented 1 year ago 
      "default": -1,

observe that the minimum value here matches the default value!

In reply to: 1491093798

In reply to: 1491093798

Refers to: src/Sarif/Schemata/sarif-2.1.0-rtm.6.json:1740 in fefed9a. [](commit_id = fefed9a7ff09fd0d9a33d1c94a201b14bbe67f04, deletion_comment = False)

github-advanced-security[bot] commented 1 year ago

You have successfully added a new CodeQL configuration .github/workflows/codeql-analysis.yml:analyze. As part of the setup process, we have scanned this repository and found 6 existing alerts. Please check the repository Security tab to see all alerts.

shaopeng-gh commented 1 year ago

Thanks, I see your comments about the changes about the 4 line/column numbers. You are correct that all changes related to them are wrong. The reason is I want to skip these 4 fields and keep them not nullable now, and because they are auto generated, the way I skip them is to add a default value to them. Can be any integer but why I choose 0 which is less than the minimum 1? Because it is actually the effective default, if not most of the tests will fail. Those temporary changes related to them are auto generated which will be auto generated again once we work on these 4 fields. So we can ignore any changes related to them now. Also this will go to DEV branch first so will not get merged to main before we get to these 4 fields. I put the comment where and will close those ones, to avoid duplicated comments to look.

shaopeng-gh commented 1 year ago 
    [DefaultValue(-1)]

Will need to fix this in the future changes, this change set is only those triggered by the new version of JSchema, must act or else will not build.

In reply to: 1491078338

Refers to: src/Sarif/Autogenerated/Address.cs:43 in fefed9a. [](commit_id = fefed9a7ff09fd0d9a33d1c94a201b14bbe67f04, deletion_comment = False)