From 412fc522bc8a14827b0548e33d1879939d4dd409 Mon Sep 17 00:00:00 2001 From: "Michael C. Fanning" Date: Sun, 20 Jan 2019 17:02:31 -0800 Subject: [PATCH 1/2] Add notification.ruleIndex and increase flatten messages testing --- .../InsertOptionalDataVisitorTests.cs | 238 ++++++++++++++---- src/Sarif/Autogenerated/Notification.cs | 21 +- .../NotificationEqualityComparer.cs | 6 + src/Sarif/Schemata/sarif-schema.json | 7 + .../Visitors/InsertOptionalDataVisitor.cs | 9 + 5 files changed, 228 insertions(+), 53 deletions(-) diff --git a/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs b/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs index ff6f0e8de..3d5971248 100644 --- a/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs +++ b/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs @@ -110,101 +110,241 @@ public void InsertOptionalDataVisitor_PersistsAll() OptionallyEmittedData.FlattenedMessages); } - [Fact] - public void InsertOptionalDataVisitorTests_FlattensGlobalMessageString() - { - string ruleId = nameof(ruleId); - string globalMessageId = nameof(globalMessageId); - string globalMessageValue = nameof(globalMessageValue); + private const int RuleIndex = 0; + private const string RuleId = nameof(RuleId); + private const string NotificationId = nameof(NotificationId); + + private const string SharedMessageId = nameof(SharedMessageId); + private const string SharedKeyRuleMessageValue = nameof(SharedKeyRuleMessageValue); + private const string SharedKeyGlobalMessageValue = nameof(UniqueGlobalMessageValue); + + private const string UniqueRuleMessageId = nameof(UniqueRuleMessageId); + private const string UniqueRuleMessageValue = nameof(UniqueRuleMessageValue); + + private const string UniqueGlobalMessageId = nameof(UniqueGlobalMessageId); + private const string UniqueGlobalMessageValue = nameof(UniqueGlobalMessageValue); + private static Run CreateBasicRunForMessageStringLookupTesting() + { + // Returns a run object that defines unique string instances both + // for an individual rule and in the global strings object. Also + // defines values for a key that is shared between the rule object + // and the global table. Used for evaluating string look-up semantics. var run = new Run { - Results = new List - { - new Result - { - RuleId = ruleId, - RuleIndex = 0, - Message = new Message - { - MessageId = globalMessageId - } - } - }, + Results = new List { }, // add non-null collections for convenience + Invocations = new List { new Invocation { } }, Resources = new Resources { MessageStrings = new Dictionary { - [globalMessageId] = globalMessageValue + [UniqueGlobalMessageId] = UniqueGlobalMessageValue, + [SharedMessageId] = SharedKeyGlobalMessageValue }, Rules = new List { new Rule { - Id = ruleId + Id = RuleId, + MessageStrings = new Dictionary + { + [UniqueRuleMessageId] = UniqueRuleMessageValue, + [SharedMessageId] = SharedKeyRuleMessageValue + } } } } }; + run.Invocations[0].ToolNotifications = new List(); + run.Invocations[0].ConfigurationNotifications = new List(); + + return run; + } + + [Fact] + public void InsertOptionalDataVisitorTests_FlattensMessageStringsInResult() + { + Run run = CreateBasicRunForMessageStringLookupTesting(); + + run.Results.Add( + new Result + { + RuleId = RuleId, + RuleIndex = RuleIndex, + Message = new Message + { + MessageId = UniqueGlobalMessageId + } + }); + + run.Results.Add( + new Result + { + RuleId = RuleId, + RuleIndex = RuleIndex, + Message = new Message + { + MessageId = UniqueRuleMessageId + } + }); + + + run.Results.Add( + new Result + { + RuleId = RuleId, + RuleIndex = RuleIndex, + Message = new Message + { + MessageId = SharedMessageId + } + }); + + + var visitor = new InsertOptionalDataVisitor(OptionallyEmittedData.FlattenedMessages); + visitor.Visit(run); + + run.Results[0].Message.Text.Should().Be(UniqueGlobalMessageValue); + run.Results[1].Message.Text.Should().Be(UniqueRuleMessageValue); + + // Prefer rule-specific value in the event of a message id collision + run.Results[2].Message.Text.Should().Be(SharedKeyRuleMessageValue); + } + + [Fact] + public void InsertOptionalDataVisitorTests_FlattensMessageStringsInNotification() + { + Run run = CreateBasicRunForMessageStringLookupTesting(); + + IList toolNotifications = run.Invocations[0].ToolNotifications; + IList configurationNotifications = run.Invocations[0].ConfigurationNotifications; + + // Shared message id with no overriding rule id + toolNotifications.Add( + new Notification + { + Id = NotificationId, + Message = new Message { MessageId = SharedMessageId} + }); + configurationNotifications.Add(toolNotifications[0]); + + // Shared message id with an overriding rule id + run.Invocations[0].ToolNotifications.Add( + new Notification + { + Id = NotificationId, + RuleIndex = RuleIndex, + Message = new Message { MessageId = SharedMessageId } + }); + configurationNotifications.Add(toolNotifications[1]); + + run.Invocations[0].ToolNotifications.Add( + new Notification + { + Id = NotificationId, + RuleIndex = RuleIndex, + Message = new Message { MessageId = UniqueGlobalMessageId } + }); + configurationNotifications.Add(toolNotifications[2]); + + run.Invocations[0].ToolNotifications.Add( + new Notification + { + Id = NotificationId, + RuleIndex = RuleIndex, + Message = new Message { MessageId = UniqueRuleMessageId } + }); + configurationNotifications.Add(toolNotifications[3]); + + var visitor = new InsertOptionalDataVisitor(OptionallyEmittedData.FlattenedMessages); visitor.Visit(run); - run.Results[0].Message.Text.Should().Be(globalMessageValue); + toolNotifications[0].Message.Text.Should().Be(SharedKeyGlobalMessageValue); + configurationNotifications[0].Message.Text.Should().Be(SharedKeyGlobalMessageValue); + + toolNotifications[1].Message.Text.Should().Be(SharedKeyRuleMessageValue); + configurationNotifications[1].Message.Text.Should().Be(SharedKeyRuleMessageValue); + + toolNotifications[2].Message.Text.Should().Be(UniqueGlobalMessageValue); + configurationNotifications[2].Message.Text.Should().Be(UniqueGlobalMessageValue); + + toolNotifications[3].Message.Text.Should().Be(UniqueRuleMessageValue); + configurationNotifications[3].Message.Text.Should().Be(UniqueRuleMessageValue); } [Fact] - public void InsertOptionalDataVisitorTests_FlattensFixMessage() + public void InsertOptionalDataVisitorTests_FlattensMessageStringsInFix() { - string ruleId = nameof(ruleId); - string globalMessageId = nameof(globalMessageId); - string globalMessageValue = nameof(globalMessageValue); + Run run = CreateBasicRunForMessageStringLookupTesting(); - var run = new Run - { - Results = new List + run.Results.Add( + new Result { - new Result + RuleId = RuleId, + RuleIndex = RuleIndex, + Message = new Message + { + Text = "Some testing occurred." + }, + Fixes = new List { - RuleId = ruleId, - RuleIndex = 0, - Message = new Message + new Fix { - Text = "Some testing occurred." + Description = new Message + { + MessageId = UniqueGlobalMessageId + } }, - Fixes = new List + new Fix { - new Fix + Description = new Message { - Description = new Message - { - MessageId = globalMessageId - } + MessageId = UniqueRuleMessageId + } + }, + new Fix + { + Description = new Message + { + MessageId = SharedMessageId } } } - }, - Resources = new Resources + }); + run.Results.Add( + new Result { - MessageStrings = new Dictionary + RuleId = "RuleWithNoRulesMetadata", + Message = new Message { - [globalMessageId] = globalMessageValue + Text = "Some testing occurred." }, - Rules = new List + Fixes = new List { - new Rule + new Fix { - Id = ruleId + Description = new Message + { + MessageId = SharedMessageId + } } } - } - }; + }); var visitor = new InsertOptionalDataVisitor(OptionallyEmittedData.FlattenedMessages); visitor.Visit(run); - run.Results[0].Fixes[0].Description.Text.Should().Be(globalMessageValue); + run.Results[0].Fixes[0].Description.Text.Should().Be(UniqueGlobalMessageValue); + run.Results[0].Fixes[1].Description.Text.Should().Be(UniqueRuleMessageValue); + + // Prefer rule-specific value in the event of a message id collision + run.Results[0].Fixes[2].Description.Text.Should().Be(SharedKeyRuleMessageValue); + + // Prefer global value in the event of no rules metadata + run.Results[1].Fixes[0].Description.Text.Should().Be(SharedKeyGlobalMessageValue); } [Fact] diff --git a/src/Sarif/Autogenerated/Notification.cs b/src/Sarif/Autogenerated/Notification.cs index 250ad29ec..6a4f82df6 100644 --- a/src/Sarif/Autogenerated/Notification.cs +++ b/src/Sarif/Autogenerated/Notification.cs @@ -46,6 +46,14 @@ public SarifNodeKind SarifNodeKind [DataMember(Name = "ruleId", IsRequired = false, EmitDefaultValue = false)] public string RuleId { get; set; } + /// + /// The index within the run resources array of the rule object associated with this notification. + /// + [DataMember(Name = "ruleIndex", IsRequired = false, EmitDefaultValue = false)] + [DefaultValue(-1)] + [JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)] + public int RuleIndex { get; set; } + /// /// The file and region relevant to this notification. /// @@ -97,6 +105,7 @@ public SarifNodeKind SarifNodeKind /// public Notification() { + RuleIndex = -1; Level = NotificationLevel.Warning; } @@ -109,6 +118,9 @@ public Notification() /// /// An initialization value for the property. /// + /// + /// An initialization value for the property. + /// /// /// An initialization value for the property. /// @@ -130,9 +142,9 @@ public Notification() /// /// An initialization value for the property. /// - public Notification(string id, string ruleId, PhysicalLocation physicalLocation, Message message, NotificationLevel level, int threadId, DateTime timeUtc, ExceptionData exception, IDictionary properties) + public Notification(string id, string ruleId, int ruleIndex, PhysicalLocation physicalLocation, Message message, NotificationLevel level, int threadId, DateTime timeUtc, ExceptionData exception, IDictionary properties) { - Init(id, ruleId, physicalLocation, message, level, threadId, timeUtc, exception, properties); + Init(id, ruleId, ruleIndex, physicalLocation, message, level, threadId, timeUtc, exception, properties); } /// @@ -151,7 +163,7 @@ public Notification(Notification other) throw new ArgumentNullException(nameof(other)); } - Init(other.Id, other.RuleId, other.PhysicalLocation, other.Message, other.Level, other.ThreadId, other.TimeUtc, other.Exception, other.Properties); + Init(other.Id, other.RuleId, other.RuleIndex, other.PhysicalLocation, other.Message, other.Level, other.ThreadId, other.TimeUtc, other.Exception, other.Properties); } ISarifNode ISarifNode.DeepClone() @@ -172,10 +184,11 @@ private ISarifNode DeepCloneCore() return new Notification(this); } - private void Init(string id, string ruleId, PhysicalLocation physicalLocation, Message message, NotificationLevel level, int threadId, DateTime timeUtc, ExceptionData exception, IDictionary properties) + private void Init(string id, string ruleId, int ruleIndex, PhysicalLocation physicalLocation, Message message, NotificationLevel level, int threadId, DateTime timeUtc, ExceptionData exception, IDictionary properties) { Id = id; RuleId = ruleId; + RuleIndex = ruleIndex; if (physicalLocation != null) { PhysicalLocation = new PhysicalLocation(physicalLocation); diff --git a/src/Sarif/Autogenerated/NotificationEqualityComparer.cs b/src/Sarif/Autogenerated/NotificationEqualityComparer.cs index a34248759..28b0314ac 100644 --- a/src/Sarif/Autogenerated/NotificationEqualityComparer.cs +++ b/src/Sarif/Autogenerated/NotificationEqualityComparer.cs @@ -38,6 +38,11 @@ public bool Equals(Notification left, Notification right) return false; } + if (left.RuleIndex != right.RuleIndex) + { + return false; + } + if (!PhysicalLocation.ValueComparer.Equals(left.PhysicalLocation, right.PhysicalLocation)) { return false; @@ -113,6 +118,7 @@ public int GetHashCode(Notification obj) result = (result * 31) + obj.RuleId.GetHashCode(); } + result = (result * 31) + obj.RuleIndex.GetHashCode(); if (obj.PhysicalLocation != null) { result = (result * 31) + obj.PhysicalLocation.ValueGetHashCode(); diff --git a/src/Sarif/Schemata/sarif-schema.json b/src/Sarif/Schemata/sarif-schema.json index 4d53761c4..97701daab 100644 --- a/src/Sarif/Schemata/sarif-schema.json +++ b/src/Sarif/Schemata/sarif-schema.json @@ -1006,6 +1006,13 @@ "type": "string" }, + "ruleIndex": { + "description": "The index within the run resources array of the rule object associated with this notification.", + "type": "integer", + "default": -1, + "minimum": -1 + }, + "physicalLocation": { "description": "The file and region relevant to this notification.", "$ref": "#/definitions/physicalLocation" diff --git a/src/Sarif/Visitors/InsertOptionalDataVisitor.cs b/src/Sarif/Visitors/InsertOptionalDataVisitor.cs index 8be754b9d..4e8b7334f 100644 --- a/src/Sarif/Visitors/InsertOptionalDataVisitor.cs +++ b/src/Sarif/Visitors/InsertOptionalDataVisitor.cs @@ -149,6 +149,15 @@ public override FileData VisitFileData(FileData node) return base.VisitFileData(node); } + public override Notification VisitNotification(Notification node) + { + _ruleIndex = node.RuleIndex; + node = base.VisitNotification(node); + _ruleIndex = -1; + + return node; + } + public override Result VisitResult(Result node) { _ruleIndex = node.RuleIndex; From 8ddd493035403be082c3cd27a8d36dc625f687fa Mon Sep 17 00:00:00 2001 From: "Michael C. Fanning" Date: Mon, 21 Jan 2019 12:02:01 -0800 Subject: [PATCH 2/2] Notification message tests + add notification.ruleIndex to schema --- .../InsertOptionalDataVisitorTests.cs | 23 +++++-------------- .../Visitors/InsertOptionalDataVisitor.cs | 10 +------- 2 files changed, 7 insertions(+), 26 deletions(-) diff --git a/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs b/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs index 3d5971248..c7ed9620e 100644 --- a/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs +++ b/src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs @@ -229,8 +229,9 @@ public void InsertOptionalDataVisitorTests_FlattensMessageStringsInNotification( }); configurationNotifications.Add(toolNotifications[0]); - // Shared message id with an overriding rule id - run.Invocations[0].ToolNotifications.Add( + // Shared message id with an overriding rule id. This message + // should still be retrieved from the global strings table. + toolNotifications.Add( new Notification { Id = NotificationId, @@ -239,7 +240,7 @@ public void InsertOptionalDataVisitorTests_FlattensMessageStringsInNotification( }); configurationNotifications.Add(toolNotifications[1]); - run.Invocations[0].ToolNotifications.Add( + toolNotifications.Add( new Notification { Id = NotificationId, @@ -248,15 +249,6 @@ public void InsertOptionalDataVisitorTests_FlattensMessageStringsInNotification( }); configurationNotifications.Add(toolNotifications[2]); - run.Invocations[0].ToolNotifications.Add( - new Notification - { - Id = NotificationId, - RuleIndex = RuleIndex, - Message = new Message { MessageId = UniqueRuleMessageId } - }); - configurationNotifications.Add(toolNotifications[3]); - var visitor = new InsertOptionalDataVisitor(OptionallyEmittedData.FlattenedMessages); visitor.Visit(run); @@ -264,14 +256,11 @@ public void InsertOptionalDataVisitorTests_FlattensMessageStringsInNotification( toolNotifications[0].Message.Text.Should().Be(SharedKeyGlobalMessageValue); configurationNotifications[0].Message.Text.Should().Be(SharedKeyGlobalMessageValue); - toolNotifications[1].Message.Text.Should().Be(SharedKeyRuleMessageValue); - configurationNotifications[1].Message.Text.Should().Be(SharedKeyRuleMessageValue); + toolNotifications[1].Message.Text.Should().Be(SharedKeyGlobalMessageValue); + configurationNotifications[1].Message.Text.Should().Be(SharedKeyGlobalMessageValue); toolNotifications[2].Message.Text.Should().Be(UniqueGlobalMessageValue); configurationNotifications[2].Message.Text.Should().Be(UniqueGlobalMessageValue); - - toolNotifications[3].Message.Text.Should().Be(UniqueRuleMessageValue); - configurationNotifications[3].Message.Text.Should().Be(UniqueRuleMessageValue); } diff --git a/src/Sarif/Visitors/InsertOptionalDataVisitor.cs b/src/Sarif/Visitors/InsertOptionalDataVisitor.cs index 4e8b7334f..edf399012 100644 --- a/src/Sarif/Visitors/InsertOptionalDataVisitor.cs +++ b/src/Sarif/Visitors/InsertOptionalDataVisitor.cs @@ -22,6 +22,7 @@ public InsertOptionalDataVisitor(OptionallyEmittedData dataToInsert, IDictionary { _dataToInsert = dataToInsert; _originalUriBaseIds = originalUriBaseIds; + _ruleIndex = -1; } public override Run VisitRun(Run node) @@ -149,15 +150,6 @@ public override FileData VisitFileData(FileData node) return base.VisitFileData(node); } - public override Notification VisitNotification(Notification node) - { - _ruleIndex = node.RuleIndex; - node = base.VisitNotification(node); - _ruleIndex = -1; - - return node; - } - public override Result VisitResult(Result node) { _ruleIndex = node.RuleIndex;