Basic rules transformation (except for v1 <-> v2 conversion) #1223

michaelcfanning · 2019-01-18T17:41:27Z

@lgolding, core rules array transformation work is done. I haven't made my own scan of this PR yet, wanted you to know that this is definitely landing today. Note that I've disabled the v1 <-> v2 testing for the change.

michaelcfanning · 2019-01-18T17:43:01Z

src/Sarif/Core/PropertyBagHolder.cs

@@ -15,7 +15,7 @@ namespace Microsoft.CodeAnalysis.Sarif
    /// <summary>
    /// Base class for objects that can hold properties of arbitrary types.
    /// </summary>
-    public abstract class PropertyBagHolder : IPropertyBagHolder
+    public class PropertyBagHolder : IPropertyBagHolder


class [](start = 11, length = 5)

not strictly a required change. i am observing odd cases where serialization isn't eliding default values, thought this might help. i don't see a need to retain this class as abstract, though, so left this in. #ByDesign

Sure, fine.

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

michaelcfanning · 2019-01-18T17:44:27Z

src/Sarif/Core/Rule.cs

@@ -13,5 +13,12 @@ public string Format(string messageId, IEnumerable<string> arguments)
        {
            return string.Format(CultureInfo.CurrentCulture, this.MessageStrings[messageId], arguments.ToArray());
        }
+
+        public bool ShouldSerializeDeprecatedIds()


ShouldSerializeDeprecatedIds [](start = 20, length = 28)

the absence of this helper caused a test to fail. somehow the conversion from dictionary to array interacts with json.net default value handling. to be investigated further. #ByDesign

Ok

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

michaelcfanning · 2019-01-18T17:45:02Z

src/Sarif/Core/SarifRewritingVisitor.cs

-    /// <summary>
-    /// Rewriting visitor for the Sarif object model.
-    /// </summary>
-    public abstract partial class SarifRewritingVisitor


SarifRewritingVisitor [](start = 34, length = 21)

yay! all dictionaries converted to arrays, no need for this key visiting thing. unless we decide it's required in future for things like graphs dictionary #ByDesign

michaelcfanning · 2019-01-18T17:46:03Z

            // OriginalUriBaseIds are directories, not files. We'll disable this visit until the

btw - this is the only code that is requiring us to maintain a NotYetGenerated copy #ByDesign

Refers to: src/Sarif/NotYetAutoGenerated/SarifRewritingVisitor.cs:724 in 1707f72. [](commit_id = 1707f72, deletion_comment = False)

michaelcfanning · 2019-01-18T17:47:27Z

...Sarif.UnitTests/TestData/InsertOptionalDataVisitor/ExpectedOutputs/CoreTests_TextFiles.sarif

@@ -1,6 +1,6 @@
 {
-  "$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-11-28",
-  "version": "2.0.0-csd.2.beta.2018-11-28",
+  "$schema": "http://json.schemastore.org/sarif-2.0.0",


http://json.schemastore.org/sarif-2.0.0 [](start = 14, length = 39)

need a version that forces the prerelease transform #ByDesign

Got it.

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

michaelcfanning · 2019-01-18T17:48:05Z

src/Sarif.TestUtilities/FileDiffingTests.cs

        {
-            return Path.GetFullPath(Path.Combine(@"..\..\..\..\..\src\Sarif.UnitTests\TestData", subdirectory));
+            return Path.GetFullPath(Path.Combine($@"..\..\..\..\..\src\{testBinaryName}\TestData", subdirectory));


testBinaryName [](start = 72, length = 14)

baseline file location was previously always getting redirected to sarif.unit tests location. #ByDesign

Nice generalization.

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

ghost · 2019-01-18T18:09:09Z

...estData/PrereleaseCompatibilityTransformer/ExpectedOutputs/ComprehensiveFileProperties.sarif

@@ -29,7 +29,7 @@
      ],
      "results": [
        {
-          "ruleId": "C1",
+          "ruleIndex": 0,


ruleIndex [](start = 11, length = 9)

I don't think this is the desired outcome. I think you should always include ruleId for readability. As your "rule id collisions" test demonstrates, you do mention both ruleId and ruleIndex when there is a collision. I think you should do it even if there is no collision. #Closed

ghost · 2019-01-18T18:10:46Z

...estData/PrereleaseCompatibilityTransformer/ExpectedOutputs/ComprehensiveFileProperties.sarif

-        "rules": {
-          "C1": {
+        "rules": [
+          {


{ [](start = 10, length = 1)

And now the rule object doesn't have an id property. In v1 the key was the id, unless there was a collision, so we should have "id": "C1" here. #Closed

hm, good catch

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

ghost · 2019-01-18T18:17:09Z

            // OriginalUriBaseIds are directories, not files. We'll disable this visit until the

Progress!

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

Refers to: src/Sarif/NotYetAutoGenerated/SarifRewritingVisitor.cs:724 in 1707f72. [](commit_id = 1707f72, deletion_comment = False)

ghost · 2019-01-18T18:21:48Z

src/Sarif.Multitool.FunctionalTests/TestData/UriMustBeAbsolute/Invalid.sarif

@@ -40,7 +40,7 @@
            "runs[0].tool.downloadUri",
            "runs[0].results[0].workItemUris[0]",
            "runs[0].originalUriBaseIds.SRCROOT",
-            "runs[0].resources.rules.TST0001.helpUri",
+            "runs[0].resources.rules[0].helpUri",


runs[0].resources.rules[0].helpUri [](start = 13, length = 34)

I hope you are enjoying the easy maintainability :-) #ByDesign

indeed! much improved. :)

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

ghost · 2019-01-18T18:30:10Z

src/Sarif.Converters/FortifyFprConverter.cs

@@ -32,7 +32,8 @@ internal class FortifyFprConverter : ToolFileConverterBase
        private string _originalUriBasePath;
        private List<Result> _results = new List<Result>();
        private HashSet<FileData> _files;
-        private Dictionary<string, Rule> _ruleDictionary;
+        private List<Rule> _rules;
+        private Dictionary<string, int> _ruleIdToIndexMap;


_ruleIdToIndexMap [](start = 40, length = 17)

Note to self: This only works if Fortify doesn't have rule collisions (if it did, a single rule id could map to multiple indices). #Closed

ghost · 2019-01-18T18:33:39Z

src/Sarif.Converters/FortifyFprConverter.cs

-                {
-                    Message message = rule.ShortDescription ?? rule.FullDescription;
-                    Dictionary<string, string> replacements = null;
+                int ruleIndex = _ruleIdToIndexMap[result.RuleId];


RuleId [](start = 57, length = 6)

Don't you need a test similar to the old code, where you do TryGetValue on _ruleIdToIndexMap? #Closed

don't believe so. i think the previous test was unnecessary, as the fortify report consistently contains rules metadata. will verify.

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

verified this data s.be present, no need for check

In reply to: 249147574 [](ancestors = 249147574,249143447)

ghost · 2019-01-18T18:35:37Z

src/Sarif.Converters/FxCopConverter.cs

                run.Resources = new Resources
                {
-                    Rules = rulesDictionary
+                    Rules = rules


rules [](start = 28, length = 5)

Well, that's a nice simple change! #ByDesign

:)

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

ghost · 2019-01-18T18:37:04Z

src/Sarif.Multitool/Rules/SarifValidationSkimmerBase.cs

@@ -654,9 +654,10 @@ private void Visit(Resources resources, string resourcesPointer)
        {
            string rulesPointer = resourcesPointer.AtProperty(SarifPropertyName.Rules);

-            foreach (string key in resources.Rules.Keys)
+
+            for (int i = 0; i < resources.Rules.Count; ++i)


Remove extra blank line. #Closed

ghost · 2019-01-18T18:56:34Z

src/Sarif.Multitool.FunctionalTests/SarifMultitoolTestBase.cs

@@ -46,38 +46,38 @@ private static string MakeQualifiedFilePath(string testDirectory, string testFil

        private static void SelectiveCompare(SarifLog actualLog, SarifLog expectedLog)
        {
-            Result[] actualResults = SafeListToArray(actualLog.Runs[0].Results);


SafeListToArray [](start = 37, length = 15)

I don't think you can remove this. The spec says that Runs can be null if (for example) the tool couldn't start, or the database query that was supposed to return a set of runs was malformed. Removing this from test code relies on us never having a test with a missing Runs array. #ByDesign

How about actualLog.Runs?[0].Results -- checking to see if that's legal C#.

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

I'm sorry, I'm confused, but I still have a point :-). You don't need the helper, because you're not converting a possibly-null list to an array. BUT! Runs might still be null. I guess it's ok to leave. On the day we add such a test, that will be time enough to test for that.

In reply to: 249150739 [](ancestors = 249150739,249150331)

ghost · 2019-01-18T19:02:46Z

src/Sarif.Multitool.FunctionalTests/SarifMultitoolTestBase.cs

-                    Rule expectedRule;
-                    expectedRules.TryGetValue(key, out expectedRule).Should().BeTrue();
+                    Rule actualRule = actualRules[i];
+                    Rule expectedRule = expectedRules[i];


[i] [](start = 53, length = 3)

Ah, I see. Assuming I wrote this code, it was all based on my misconception that you couldn't index into a list. Bleah. #ByDesign

ghost · 2019-01-18T19:07:10Z

src/Sarif/Writers/SarifLogger.cs

@@ -308,7 +308,16 @@ public void Log(IRule rule, Result result)

            // TODO: we need to finish eliminating the IRule interface from the OM
            // https://github.com/Microsoft/sarif-sdk/issues/1189
-            Rules[result.RuleId] = (Rule)rule;
+            if (!RuleToIndexMap.TryGetValue((Rule)iRule, out int ruleIndex))


if [](start = 12, length = 2)

These 8 lines (311-318) occur twice. Consider DRYing them out with a helper LogRule(Rule rule, out int ruleIndex) #Closed

ghost · 2019-01-18T19:08:00Z

        _serializer.Serialize(_jsonWriter, rules);

Ha ha! The serializer didn't care, no change. #ByDesign

Refers to: src/Sarif/Writers/ResultLogJsonWriter.cs:234 in 1707f72. [](commit_id = 1707f72, deletion_comment = False)

ghost · 2019-01-18T19:10:40Z

src/Sarif/Schemata/sarif-schema.json

@@ -1248,10 +1248,13 @@
          }
        },

+
        "rules": {


rules [](start = 9, length = 5)

Specify "default": [] #Closed

ghost · 2019-01-18T19:12:54Z

src/Sarif.UnitTests/Visitors/UpdateIndicesVisitorTests.cs

@@ -41,7 +41,7 @@ public void UpdateIndicesVisitor_FunctionsWithNullMaps()
        {
            var result = _result.DeepClone();

-            var visitor = new UpdateIndicesVisitor(null, null);
+            var visitor = new UpdateIndicesVisitor(null, null, null);


UpdateIndicesVisitor [](start = 30, length = 20)

Consider: use named parameter syntax so we know what the nulls are. (I see below you did that sometimes but not always.) #WontFix

right, when every argument is null in a test named 'FunctionsWithNullMap', i don't see the need for this readability. the need for naming comes during actual utilization.

things that need to be readable need to be readable. otherwise, they should be compact. still don't care enough about this to push back.

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

I took a look at the fix and it actually, to me, compromises the readability of this test. the test is intended to comprehensively validate a call where all args are null. the call without named parameters makes it easiest to chunk the information allowing this condition to be validated.

In reply to: 249187800 [](ancestors = 249187800,249155287)

michaelcfanning · 2019-01-18T19:16:14Z

        _serializer.Serialize(_jsonWriter, rules);

Yes! if you elide the type argument, magic just happens,

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

Refers to: src/Sarif/Writers/ResultLogJsonWriter.cs:234 in 1707f72. [](commit_id = 1707f72, deletion_comment = False)

ghost · 2019-01-18T19:17:25Z

src/Sarif.Multitool.FunctionalTests/TestData/MessagesShouldEndWithPeriod/Invalid.sarif

-            "runs[0].resources.rules.TST0001.fullDescription.richText",
-            "runs[0].resources.rules.TST0001.messageStrings.Default",
-            "runs[0].resources.rules.TST0001.richMessageStrings.Default"
+            "runs[0].resources.rules[0].shortDescription.text",


rules[0] [](start = 31, length = 8)

I assume this path has an array, even though the rules property above is a dictionary, because this file goes through the PrereleaseCompatibilityTransformer. #ByDesign

that's right. i didn't care to tinker partially with this file. better to tackle all these comprehensively later when appropriate.

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

ghost · 2019-01-18T19:18:35Z

src/Sarif/Visitors/InsertOptionalDataVisitor.cs

                {
+                    // Otherwise, pick it up from the 


the [](start = 50, length = 3)

Unfinished comment. #Closed

ghost · 2019-01-18T19:20:32Z

src/Sarif/Visitors/InsertOptionalDataVisitor.cs

                string formatString = null;
-                if (_ruleId != null &&
-                    _run.Resources?.Rules.TryGetValue(_ruleId, out rule) == true)
+                Rule rule = _ruleIndex != -1 ? _run.Resources.Rules[_ruleIndex] : null;


_ruleIndex [](start = 28, length = 10)

Ugh, right, "lookup for messages in results is special". result.ruleMessageId is lookin' pretty good right now, isn't it? ;-) #Resolved

BUG: Darn it, I don't even think this works. There are messages in sub-objects of a result that are not rule messages. You will incorrectly take this branch, for example, for a code flow location message.

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

One possible fix: As soon as you see _ruleIndex is not -1, then use it and immediately reset it to -1. Don't wait to come back from base.VisitResult.

In reply to: 249166259 [](ancestors = 249166259,249157484)

ghost · 2019-01-18T19:21:43Z

src/Sarif/Visitors/InsertOptionalDataVisitor.cs

-                    _run.Resources?.Rules.TryGetValue(_ruleId, out rule) == true)
+                Rule rule = _ruleIndex != -1 ? _run.Resources.Rules[_ruleIndex] : null;
+
+                if (rule != null && rule.MessageStrings.TryGetValue(node.MessageId, out formatString))


.T [](start = 55, length = 2)

Careful of null: rule.MessageStrings?.TryGetValue. #ByDesign

nice catch. what a fantastic corner case. adding unit test.

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

I don't think this is a sufficient fix. It assumes that a message id that points into a rule's message strings will never be the same as a message id that points into run.resources.messageStrings. It's easy to imagine message ids like "default" or "success" that might occur in both places (and even if it weren't easy to imagine, it might still happen).

What do you think of the fix I suggested above (reset _ruleIndex to -1 as soon as you use it)?

In reply to: 249194811 [](ancestors = 249194811,249157825)

See my comment in the unit tests, where I describe the failure case I'm worried about.

In reply to: 249229167 [](ancestors = 249229167,249194811,249157825)

I don't understand your suggestion. When we are visiting a result with a corresponding resultIndex, we retain it. If we reset this to -1 on first use, as you suggest, no other message visiting in context of the result would have access to this data. This seems like entirely the wrong thing to so. Remember: stacks, codeflows, graphs, fixes, etc. might dip into the rule resources to get format string.

We explicitly designed string look-up to refer to the rule first, then fall back to the global. Maybe your objections will be clearer to me when i look at your unit test remarks.

oasis-tcs/sarif-spec#216
"in the presence of a non-null message.messageId, consult resources.rules by rule id, to attempt to locate the string. if not present, fall back to resources for look-up."

In reply to: 249229167 [](ancestors = 249229167,249194811,249157825)

ghost · 2019-01-18T19:26:48Z

src/Sarif/Visitors/UpdateIndicesVisitor.cs

@@ -10,11 +10,40 @@ public class UpdateIndicesVisitor : SarifRewritingVisitor
    {
        private IDictionary<string, int> _fullyQualifiedLogicalNameToIndexMap;


IDictionary [](start = 16, length = 11)

While you're in here: The three dictionaries can be readonly. #Closed

ghost · 2019-01-18T19:28:29Z

src/Sarif/Visitors/UpdateIndicesVisitor.cs

+
+                    // We need to update the rule id, as it previously referred to a synthesized 
+                    // key that resolved some collision in the resources.rules collection.
+                    node.RuleId = _resources.Rules[ruleIndex].Id;


ruleIndex [](start = 51, length = 9)

Can run.resources fail to exist, or run.resources.rules fail to exist, or run.resources.rules fail to have as many entries as ruleIndex? #ByDesign

if you have a rule index but no entry, you've got bad SARIF. admittedly, we aren't hardened for negative conditions, but these are numerically intimidating. most systems should work like this: validate your SARIF if you need a hardened system, somewhere close to ingestion. from here, code can assume it is in reasonable shape.

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

ok, that sounds like a reasonable workflow to require.

In reply to: 249204481 [](ancestors = 249204481,249159879)

ghost · 2019-01-18T19:30:22Z

src/Sarif/Core/Rule.cs

+        public bool ShouldSerializeDeprecatedIds()
+        {
+            return this.DeprecatedIds != null &&
+                (this.DeprecatedIds.Where((e) => { return e != null; }).Count() == this.DeprecatedIds.Count);


Where [](start = 36, length = 5)

Count can take a predicate (and my goodness, so many extra keystrokes in that lambda :-)):

this.DeprecatedIds.Count(e => e != null) == this.DeprecatedIds.Count() ``` #Closed

But is this the right test? You could just

return this.DeprecatedIds?.Count(e => e != null) > 0;

... and then persist the non-null ones.

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

yes, greatly reduced, like it.

In reply to: 249182441 [](ancestors = 249182441,249160403)

ghost · 2019-01-18T19:34:12Z