Update schema and tests to SARIF TC#25 conclusions, 2.0.0-csd.2.beta.2018-10-10 #1114

michaelcfanning · 2018-10-18T23:27:03Z

Changes in this PR:

CRITICAL FILES TO REVIEW:

Sarif\Schema\sarif-schema.json and

Sarif\Writers\PrereleaseCompatibilityTransformer.cs

Update schema/OM/etc. to schema as of SARIF TC#25, 10-10-2018

originalUriBaseIds is now a dictionary of file locations, not strings.
Suffix invocation.startTime, invocation.endTime, file.lastModifiedTime and notification.time with Utc (startTimeUtc, endTimeUtc, etc.).
threadflowLocation.timestamp renamed to 'executionTimeUtc'.
versionControlDetails.timestamp renamed to 'asOfTimeUtc'.
versionControlDetails.uri renamed to 'repositoryUri'.
exception.message type converted from string to message object.
all the things have property bags
Provide PrereleaseCompatibilityTransformer functionality that automatically upgrades prerelease v2 to current.

NOTE: this mechanism is used to update all existing checked in test files (a significant proof point).
Author new comprehensive prerelease SARIF v1 file to ensure code coverage of all transformations
TODO: author detailed unit tests for this transformer
Remove all inlined schema property bag definitions in preferences of reference to single 'propertyBag' definition

Remove -NoBuildSample from build.

Clean-up alphabetization of CodeGenHints.json

Convert all SARIF v1 <-> v2 tests to operate against checked in files rather than embedded string literals.

Move transformer tests (and test data files) and rename to conform to existing standards.

Delete 'StepValuesMustFormOneBasedSequence' rule. Note to self: the requirement to author a validation rule sometimes a signal of a design flaw. :)

@lgolding @rtaket

…2018-10-10

michaelcfanning · 2018-10-18T23:31:49Z

scripts/BeforeBuild.ps1

@@ -16,8 +16,6 @@
    Do not restore NuGet packages.
 .PARAMETER NoObjectModel
    Do not rebuild the SARIF object model from the schema.
-.PARAMETER NoBuildSample


NoBuildSample [](start = 11, length = 13)

We have too many knobs in our build script and no easy ways to do common scenarios such as (please just rebuild and run tests rather than everything else). We also don't succeed in Debug configurations. #Closed

michaelcfanning · 2018-10-19T00:09:59Z

src/Sarif/CodeGenHints.json

@@ -360,11 +520,6 @@
      "kind": "DictionaryHint"
    }
  ],
-  "Result.Fingerprints": [


Result.Fingerprints [](start = 3, length = 19)

FYI, i realphabetized some content in this file, as it had gotten out of date based on some renaming. This construct has moved earlier in the file, for example. #Closed

michaelcfanning · 2018-10-19T00:30:07Z

src/Sarif.TestUtilities/Utilities.cs

-        private const string SchemaPropertyPattern = @"""\$schema""\s*:\s*""[^""]+""";
-        private static readonly Regex s_SchemaRegex = new Regex(SchemaPropertyPattern, RegexOptions.Compiled);
-
-        public static string UpdateVersionNumberToCurrent(string logContents)


UpdateVersionNumberToCurrent [](start = 29, length = 28)

This utility also previously replaced the run.tool version! Oops. We now use the JParser to make the sarifLog.version update. #ByDesign

I hope this is the last time I ever think I can get away with using a regex to modify a JSON, XML, or HTML file.

In reply to: 226504993 [](ancestors = 226504993)

michaelcfanning · 2018-10-19T00:31:26Z

...tool.FunctionalTests/TestData/UrisMustBeValid/InvalidVersionControlDetailsUri_Expected.sarif

@@ -35,7 +35,7 @@
                },
                "region": {
                  "startLine": 12,
-                  "startColumn": 50
+                  "startColumn": 60


startColumn [](start = 19, length = 11)

introduction of 'repository' to property name offsets startColumn by 10 chars #ByDesign

michaelcfanning · 2018-10-19T00:32:13Z

...itool.FunctionalTests/TestData/UrisMustBeValid/InvalidUriInOriginalUriBaseIds_Expected.sarif

@@ -34,8 +34,8 @@
                  "uri": "file:///C:/Code/sarif-sdk/src/Sarif.Multitool.FunctionalTests/TestData/UrisMustBeValid/InvalidUriInOriginalUriBaseIds.sarif"
                },
                "region": {
-                  "startLine": 10,
-                  "startColumn": 51
+                  "startLine": 11,


startLine [](start = 19, length = 9)

conversion of originalUriBaseIds values from string to file location introduces a new line in the file, hence changes in offsets #ByDesign

michaelcfanning · 2018-10-19T00:34:11Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

@@ -251,8 +251,7 @@ private HashSet<string> CreateTargetsSet(TOptions analyzeOptions)
                var fileSpecifier = new FileSpecifier(normalizedSpecifier, recurse: analyzeOptions.Recurse, fileSystem: FileSystem);
                foreach (string file in fileSpecifier.Files) { targets.Add(file); }
            }
-            Debug.Assert(targets.Count > 0);


Debug.Assert [](start = 12, length = 12)

this assert prevents debug unit tests from completing #Closed

ghost · 2018-10-19T16:23:31Z

<PreviousVersionSuffix>csd.1.0.3</PreviousVersionSuffix>

Set this to csd.2.beta.2018-09-26 #Closed

Refers to: src/build.props:26 in 7aaa3e9. [](commit_id = 7aaa3e9, deletion_comment = False)

ghost · 2018-10-19T16:25:56Z

src/ReleaseHistory.md

+* BREAKING: versionControlDetails.timestamp renamed to 'asOfTimeUtc'.
+* BREAKING: versionControlDetails.uri renamed to 'repositoryUri'.
+* BREAKING: exception.message type converted from string to message object.


. [](start = 74, length = 1)

Also mention the introduction of result.occurrenceCount (I have a note on my desk that you have a customer waiting for an SDK version that supports occurrenceCount). #Closed

Yes, also neglected file.hashes, run.id and run.aggregateIds and threadFlowLocation.step removal.

In reply to: 226705862 [](ancestors = 226705862)

ghost · 2018-10-19T16:34:54Z

src/Sarif/CodeGenHints.json

+      }
+    }
+  ],
+  "externalFiles": [


externalFiles [](start = 3, length = 13)

This doesn't look right. The run object has a property externalFiles, which is of type "array of externalFile", and the externalFile object is the thing that has a property bag. #ByDesign

Sorry, I got this wrong. The spec says that the value of run.externalFiles is an object, and then it goes on to describe the properties of that object -- but it doesn't give that set of properties a name, unlike every other object in the spec. I filed Issue #266: "Define an object type for the value of run.externalFiles" (CSD.2, bug, editorial, documentation-only).

In reply to: 226708519 [](ancestors = 226708519)

ghost · 2018-10-19T16:38:57Z

src/Sarif/CodeGenHints.json

@@ -512,13 +657,25 @@
  "Run.OriginalUriBaseIds": [
    {
      "kind": "DictionaryHint"
-    },
+    }


} [](start = 4, length = 1)

I assume you omitted the AttributeHint because with the new "chained" design for originalUriBaseIds, the dictionary entries are no longer necessarily URIs. #ByDesign

Right

In reply to: 226709656 [](ancestors = 226709656)

ghost · 2018-10-19T16:42:16Z

src/Sarif/ExtensionMethods.cs

@@ -16,6 +16,40 @@ namespace Microsoft.CodeAnalysis.Sarif
 {
    public static class ExtensionMethods
    {
+        public static string InstanceIdInstanceComponent(this RunAutomationDetails runAutomationDetails)


InstanceIdInstanceComponent [](start = 29, length = 27)

Consider: Make this an extension method on string so it would be used like this:

string instanceComponent = runAutomationDetails.InstanceId.?InstanceComponent;

That seems more natural to me than

string instanceComponent = runAutomationDetails.InstanceIdInstanceComponent;

and it reduces the coupling of this method (you could unit test it without having the definition of RunAutomationDetails).

Same comment for InstanceIdLogicalComponent as well. #ByDesign

Although I suppose you could argue that the current design eliminates the potential error of forgetting to null-check runAutomationDetails.InstanceId. Ok, I withdraw this.

In reply to: 226710524 [](ancestors = 226710524)

ghost · 2018-10-19T16:49:41Z

src/Sarif/ExtensionMethods.cs

+                return String.Empty;
+            }
+
+            return instanceId.Substring(0, instanceId.LastIndexOf('/') + 1);


1 [](start = 71, length = 3)

This includes the final slash. I don't agree with that. If instanceId is x/y, then the "category" (the logical id) is x, not x/. I know you love your trailing slashes on directory-valued URIs, but I don't think it's right here.

I actually intended this, because for this value, the slash is required to indicate the logical component. So, 'logical' on its on in this id would actually be the instance component'. 'logical/' represents a logical only component in the id. I'm glad to change it.

In reply to: 226712702 [](ancestors = 226712702)

IMO it should be changed. When you call Path.GetDirectoryName, whether on dirName or on dirName\fileName, you get back dirName, not dirName\.

You don't need \ on the return value to "indicate the logical component". You know it's the logical component because you got it by calling InstanceIdLogicalComponent.

In reply to: 226844096 [](ancestors = 226844096,226712702)

(Reactivated to call my response to your attention.)

In reply to: 226872911 [](ancestors = 226872911,226844096,226712702)

ghost · 2018-10-19T17:22:18Z

},

You omitted the declaration of the properties property, here and in these other classes that are newly declared as deriving from PropertyBagHolder:

fileChange
fileContent
fileLocation
fix
logicalLocation
message
physicalLocation
rectangle
region
replacement
ruleConfiguration

You did add it to sarifLog, and conversion

This won't affect C# code because PropertyBagHolder itself declares a Properties dictionary. But without the declaration in the schema, an attachement object with a properties property won't validate.
#Closed

Refers to: src/Sarif/Schemata/sarif-schema.json:74 in 7aaa3e9. [](commit_id = 7aaa3e9, deletion_comment = False)

ghost · 2018-10-19T17:49:56Z