Eliminate need to specify the SARIF contract resolver. All serializat… #1103

michaelcfanning · 2018-09-27T14:54:17Z

…ion and deserialization now works by default.

This change eliminates the need to specify a contract resolver in a json settings object that is passed to API such as JsonConvert to serialize/deserialize SARIF. We require special converters to perform operations such as converting [Flags] enum values into string arrays in JSON and vice verse. This change was prompted by the observation that several customers fell into the trap of producing SARIF that could not subsequently be deserialized.

The core change is accomplished by using the Jschema AttributeHint to mark relevant types with the JsonConverterAttribute (which passes the type of the converter as an argument). This converter specification cannot be overridden in cases where a containing class is being serialized or deserialized. The attribute itself is not recognized in cases where a user may be serializing/deserializing a specific member directly.

This change was complicated by inconsistencies produced by the mixture of IRule and Rule in our OM. I made the choice to drive IRule into the OM. Moving forward, I believe we should back away from this and eliminate IRule in favor of an overridable Rule class. Similarly, we should look at eliminating ISarifNode in favor of a common abstract base class for all generated SARIF nodes.

I verified this change by completing eliminating the public SarifContractResolver.Instance member that's commonly used for serializing SARIF. Once I had eliminated all uses, I restored this member and marked it as obsolete (but not an error to use). Legacy code should therefore still compile but emit a warning that this resolver is generally no longer required.

…ion and deserialization now works by default. This change also drives IRule into the SARIF OM (replacing use of concrete type Rule in the SARIF rules dictionary).

michaelcfanning · 2018-09-27T21:44:34Z

src/Sarif/CodeGenHints.json

@@ -18,6 +18,14 @@
          "override"
        ]
      }
+    },
+    {
+      "kind": "AttributeHint",


AttributeHint [](start = 15, length = 13)

@lgolding FYI, this hint doesn't appear to be operational for a wildcard spec like this one. #Pending

That's disturbing. I filed microsoft/jschema#51, "AttributeHint doesn't work on a wildcard."

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

Now that you know this, I have removed the non-functional hint. I was able to get things to work by providing conversion at a lower level, the SerializedPropertyInfo...

In reply to: 221313782 [](ancestors = 221313782,221087141)

michaelcfanning · 2018-09-27T21:47:17Z

src/Sarif/ExtensionMethods.cs

@@ -16,6 +16,11 @@ namespace Microsoft.CodeAnalysis.Sarif
 {
    public static class ExtensionMethods
    {
+        public static string Format(this IRule rule, string messageId, IEnumerable<string> arguments)


Format [](start = 29, length = 6)

This extension method created as a result of pushing IRule into the OM. Method previously existed on Rule, which is defined as a partial class. Can't do this (partial class) for an interface, obviously #ByDesign

michaelcfanning · 2018-09-27T22:55:15Z

src/Sarif/Autogenerated/Resources.cs

@@ -41,7 +43,8 @@ public SarifNodeKind SarifNodeKind
        /// A dictionary, each of whose keys is a string and each of whose values is a 'rule' object, that describe all rules associated with an analysis tool or a specific run of an analysis tool.
        /// </summary>
        [DataMember(Name = "rules", IsRequired = false, EmitDefaultValue = false)]
-        public IDictionary<string, Rule> Rules { get; set; }
+        [JsonConverter(typeof(Microsoft.CodeAnalysis.Sarif.Readers.RuleDictionaryConverter))]
+        public IDictionary<string, IRule> Rules { get; set; }


IRule [](start = 35, length = 5)

We now emit IRule into the OM. We should either prefer this or eliminate IRule altogether. Probably should prefer the latter. this would be a large breaking change, so should just do it if we are. I think it may be preferable, otherwise we need to figure out how to code gen things like IRuleEqualityComparable. Worse, the rewriting visitor needs special-casing, see other code delta. I suggest we accept this and that I next look at eliminating use of IRule altogether. #Pending

@michaelcfanning I totally agree. Please remind me what the original motivation was for IRule -- I know you just told me recently.

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

michaelcfanning · 2018-09-27T22:57:32Z

src/Sarif/Core/PropertyBagHolder.cs

        }

+        private static readonly JsonSerializerSettings _settingsWithComprehensiveV2ContractResolver = new JsonSerializerSettings


_settingsWithComprehensiveV2ContractResolver [](start = 55, length = 44)

this is how we solve the problem of persisting SARIF types that require special serialization/deserialization to and from the property bag. #ByDesign

…file.

michaelcfanning · 2018-09-27T23:00:12Z

src/Sarif/NotYetAutoGenerated/SarifRewritingVisitor.cs

@@ -594,7 +594,14 @@ public virtual Resources VisitResources(Resources node)
                        var value = node.Rules[key];
                        if (value != null)
                        {
-                            node.Rules[key] = VisitNullChecked(value);
+                            ISarifNode sarifNode = value as ISarifNode;


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

This one-off code required as IRule itself doesn't require ISarifNode to implement. All this means 1) we need special-casing to emit this method. 2) a class that implements IRule needs to also implement ISarifNode in order for the visitor to walk it successfully. Neither is very desirable. #ByDesign

…ted to property bag.

michaelcfanning · 2018-09-28T15:22:04Z

src/Sarif.UnitTests/Baseline/ResultMatching/ResultMatchingBaselinerTests.cs

+                int existingCount = currentLog.Runs[0].Results.Count - 1;
+                calculatedNextBaseline.Runs[0].Results.Where(r => r.BaselineState == BaselineState.Existing).Count().Should().Be(existingCount);
+
+                if (existingCount > 0)


existingCount [](start = 20, length = 13)

This is a fix for a bug in a test that produces random content (that happened to fail intermittently in one of my validation runs) #Resolved

ghost · 2018-09-28T16:23:49Z

src/Sarif.FunctionalTests/PropertyBagConverterTests.cs

-            };
-
-            SarifLog deserializedLog = JsonConvert.DeserializeObject<SarifLog>(originalLog, serializerSettings);
+            var settings = new JsonSerializerSettings { Formatting = Formatting.Indented };


settings [](start = 16, length = 8)

BUG: You created the settings object but then didn't use it. #Resolved

ghost · 2018-09-28T16:25:27Z

src/Sarif.FunctionalTests/SarifLogEqualityComparerTests.cs

-            {
-                ContractResolver = SarifContractResolver.Instance
-            };
+            SarifLog thisLog = JsonConvert.DeserializeObject<SarifLog>(comprehensiveTestSampleContents);


thisLog [](start = 21, length = 7)

I don't like this rename at all. It loses information from what was there before. If you want expectedLog and actualLog, that's fine, but "this" and "that" tell me nothing. #Resolved

ghost · 2018-09-28T16:29:21Z

src/Sarif.UnitTests/Baseline/ResultMatching/ResultMatchingBaselinerTests.cs

@@ -24,7 +24,7 @@ public ResultMatchingBaselinerTests(ITestOutputHelper outputHelper)

        [Fact]
        public void ResultMatchingBaseliner_BaselinesTwoSimpleSarifLogs()
-        {
+        {           


[](start = 9, length = 11)

Superfluous white space added. #Resolved

ghost · 2018-09-28T16:37:52Z

src/Sarif/CodeGenHints.json

    }
  ],
  "codeFlow": [
-    {
+    { 


[](start = 5, length = 1)

Nit: trailing white space. #Resolved

ghost · 2018-09-28T16:49:46Z

src/Sarif/CodeGenHints.json

+      "kind": "AttributeHint",
+      "arguments": {
+        "namespaceName": "Newtonsoft.Json",
+        "typeName": "JsonConverter",


JsonConverter [](start = 21, length = 13)

Does the converter work on the array elements when you apply the attribute to an array? Quick search didn't turn up the answer. Is this tested? #ByDesign

Yes, this works. The UriConverter actually operates against List as well.

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

ghost · 2018-09-28T16:50:49Z

src/Sarif/CodeGenHints.json

+      "kind": "AttributeHint",
+      "arguments": {
+        "namespaceName": "Newtonsoft.Json",
+        "typeName": "JsonConverter",


JsonConverter [](start = 21, length = 13)

Same question: Does the converter work on the object's property values when you apply the attribute to an object? #ByDesign

Ok, I just found the code you wrote to dive into the dictionary.

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

ghost · 2018-09-28T16:51:35Z

src/Sarif/CodeGenHints.json

@@ -445,17 +612,35 @@
      }
    }
  ],
+  "ThreadFlowLocation.Importance": [
+    {
+      "kind": "EnumHint",


EnumHint [](start = 15, length = 8)

So this was just missing -- reveals test gap. #ByDesign

Yep. Found this by updating the comprehensive.sarif example. We should really examine that thing more closely and continue to fill it out. very useful in many test scenarios.

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

ghost · 2018-09-28T16:57:35Z

src/Sarif/Core/PropertyBagHolder.cs

@@ -102,6 +102,12 @@ public T GetProperty<T>(string propertyName)
            return JsonConvert.DeserializeObject<T>(Properties[propertyName].SerializedValue);
        }

+        private static readonly JsonSerializerSettings _settingsWithComprehensiveV2ContractResolver = new JsonSerializerSettings


_settingsWithComprehensiveV2ContractResolver [](start = 55, length = 44)

Nit: Do we use s_ for names of statics? #Closed

ghost · 2018-09-28T16:58:43Z

src/Sarif/Core/IRuleEqualityComparer.cs

+    /// Defines methods to support the comparison of objects of type Rule for equality.
+    /// </summary>
+    [GeneratedCode("Microsoft.Json.Schema.ToDotNet", "0.58.0.0")]
+    internal sealed class IRuleEqualityComparer : IEqualityComparer<IRule>


IRuleEqualityComparer [](start = 26, length = 21)

I assume this is copied from the RuleEqualityComparer so I won't review it. #ByDesign

ghost · 2018-09-28T17:00:25Z

src/Sarif/Autogenerated/SarifRewritingVisitor.cs

@@ -594,7 +594,14 @@ public virtual Resources VisitResources(Resources node)
                        var value = node.Rules[key];
                        if (value != null)
                        {
-                            node.Rules[key] = VisitNullChecked(value);
+                            ISarifNode sarifNode = value as ISarifNode;


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

Don't put this in the Autogenerated code. It will be overwritten on the next build, and you've already copied the visitor to NotYetAutogenerated anyway. #ByDesign

We need this in the autogenerated code. Otherwise, when others sync to this change, the new file will be copied over and everyone will have this delta in their Autogenerated copy. This is just the way things work. :)

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

ghost · 2018-09-28T17:04:21Z

src/Sarif.FunctionalTests/v2/SpecExamples/Comprehensive.sarif

@@ -20,6 +20,15 @@
          "copyright": "Copyright (c) 2016 by Example Corporation. All rights reserved."
        }
      },
+      "versionControlProvenance": [
+        {
+          "uri": "https://github.com/contoso/example",


github [](start = 26, length = 6)

example.com #WontFix

microsoft has created a contoso organization on github.com, do you see an issue using it?

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

I didn't know that.

In reply to: 221400882 [](ancestors = 221400882,221319843)

ghost · 2018-09-28T17:50:22Z

After reviewing this, I can't figure out exactly what forced you to make the Rule-to-IRule change. I asked a specific question along these lines in ResultLogJsonWriter.cs. #Resolved

ghost · 2018-09-28T17:54:20Z

src/Sarif/Writers/ResultLogJsonWriter.cs


-            _jsonWriter.WritePropertyName("rules");
-            _serializer.Serialize(_jsonWriter, rules, typeof(Dictionary<string, IRule>));


IRule [](start = 80, length = 5)

If you had changed this to Rule (because in fact resources.Rules is a Dictionary<string,Rule>), would the rest of the Rule-to-IRule` changes have been needed? #Resolved

ghost

🕐

michaelcfanning · 2018-09-28T22:58:51Z

we have an admixture of abstract and concrete types in our OM, 2) this means that any type that implements IRule has to be converted to a Rule instance when being serialized. You cannot deserialize an alternate IRule provider (you could with a converter/contract resolver, if we took this first change). in any case, i now believe, as you agree, that we should eliminate as many interfaces as possible in preference of well-constructed unsealed base types. Our object hierarchy will start with ISarifNode by providing a kind and deep clone, we will layer property bag holding on type, and then just generate concrete types. this will be a big breaking change for some consumers (like binskim). oh well. in the meantime, i've reverted driving IRule into the OM.

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

ghost · 2018-09-28T23:41:22Z

src/Sarif/Autogenerated/ResourcesEqualityComparer.cs

@@ -4,6 +4,7 @@
 using System;
 using System.CodeDom.Compiler;
 using System.Collections.Generic;
+using Microsoft.CodeAnalysis.Sarif;


Sarif [](start = 29, length = 5)

Odd. Was this a hand-edit you forgot to revert? I don't remember making a code gen change that would have had the effect of generating this superfluos using directive. #Resolved

ghost · 2018-09-28T23:42:16Z

src/Sarif/Autogenerated/SarifRewritingVisitor.cs

@@ -592,6 +592,7 @@ public virtual Resources VisitResources(Resources node)
                    foreach (var key in keys)
                    {
                        var value = node.Rules[key];


var [](start = 24, length = 3)

Extra blank line added by hand. Just revert this auto-generated file. #Resolved

ghost · 2018-09-28T23:43:35Z

src/Sarif/Baseline/ResultMatching/SarifLogMatcher.cs

@@ -157,7 +157,7 @@ private List<ExtractedResult> GetMatchingResultsFromRuns(IEnumerable<Run> sarifR
                {
                    foreach (Result result in run.Results)
                    {
-                        Rule rule = GetRuleFromResources(result, run.Resources.Rules);
+                        IRule rule = GetRuleFromResources(result, run.Resources.Rules);


IRule [](start = 24, length = 5)

I thought we agreed to revert all Rule => IRule changes, and then implement IRule => Rule in a separate PR. #Pending

this rule instance isn't even used, which is how it remained out of sight. @evmaus, why is this code getting called? unnecessary code?

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

ghost · 2018-09-28T23:45:55Z

There are still some left-over Rule => IRule changes. Please revert, then ok to merge.

ghost

michaelcfanning · 2018-09-28T23:53:43Z

Nice catch. Fixed, pushed and will merge after build succeeds. Thanks for the review.

Eliminate need to specify the SARIF contract resolver. All serializat…

2dd4124

…ion and deserialization now works by default. This change also drives IRule into the SARIF OM (replacing use of concrete type Rule in the SARIF rules dictionary).

michaelcfanning commented Sep 27, 2018

View reviewed changes

michaelcfanning added 2 commits September 27, 2018 15:42

Simplify emitted attribute name.

7532972

More type name simplifications in emitted code.

c405958

michaelcfanning commented Sep 27, 2018

View reviewed changes

Update typo. Restore full namespace for UriConverter in codegen hint …

cd8ba1d

…file.

michaelcfanning commented Sep 27, 2018

View reviewed changes

Eliminate conversion of core types (such as DateTime) that are persis…

049f32f

…ted to property bag.

michaelcfanning commented Sep 28, 2018

View reviewed changes

Restore SarifContractResolver instance and mark it as obsolete

742fd9c

ghost reviewed Sep 28, 2018

View reviewed changes

ghost mentioned this pull request Sep 28, 2018

Attribute hint doesn't work on a wildcard microsoft/jschema#51

Open

ghost reviewed Sep 28, 2018

View reviewed changes

ghost suggested changes Sep 28, 2018

View reviewed changes

PR feedback

39b01f7

ghost reviewed Sep 28, 2018

View reviewed changes

ghost approved these changes Sep 28, 2018

View reviewed changes

More PR feedback.

9ede737

michaelcfanning merged commit 47dd2e8 into develop Sep 28, 2018

michaelcfanning deleted the no-contract-resolver branch October 5, 2018 21:35

pascalberger mentioned this pull request Jul 2, 2019

Remove SarifContractResolver from examples #1561

Merged

		}

		private static readonly JsonSerializerSettings _settingsWithComprehensiveV2ContractResolver = new JsonSerializerSettings


		_jsonWriter.WritePropertyName("rules");
		_serializer.Serialize(_jsonWriter, rules, typeof(Dictionary<string, IRule>));

Eliminate need to specify the SARIF contract resolver. All serializat… #1103

Eliminate need to specify the SARIF contract resolver. All serializat… #1103

Conversation

michaelcfanning commented Sep 27, 2018 • edited Loading

michaelcfanning Sep 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Sep 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Sep 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Sep 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Sep 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 28, 2018 • edited by michaelcfanning Loading

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

michaelcfanning commented Sep 28, 2018 • edited Loading

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Sep 28, 2018 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 28, 2018

ghost left a comment

Choose a reason for hiding this comment

michaelcfanning commented Sep 28, 2018

michaelcfanning commented Sep 27, 2018 •

edited

Loading

michaelcfanning Sep 27, 2018 •

edited by ghost

Loading

michaelcfanning Sep 27, 2018 •

edited by ghost

Loading

michaelcfanning Sep 27, 2018 •

edited by ghost

Loading

michaelcfanning Sep 27, 2018 •

edited by ghost

Loading

michaelcfanning Sep 27, 2018 •

edited by ghost

Loading

michaelcfanning Sep 28, 2018 •

edited

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by ghost

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by ghost

Loading

ghost Sep 28, 2018 •

edited by ghost

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost commented Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

michaelcfanning commented Sep 28, 2018 •

edited

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading

ghost Sep 28, 2018 •

edited by michaelcfanning

Loading