Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Notification rule index #1229

Merged
merged 3 commits into from
Jan 22, 2019
Merged

Notification rule index #1229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
412fc52
Add notification.ruleIndex and increase flatten messages testing
michaelcfanning Jan 21, 2019
8ddd493
Notification message tests + add notification.ruleIndex to schema
michaelcfanning Jan 21, 2019
f38d521
Merge branch 'files-array' into notification-rule-index
michaelcfanning Jan 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 178 additions & 49 deletions src/Sarif.UnitTests/Visitors/InsertOptionalDataVisitorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,101 +110,230 @@ 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);
Copy link

@ghost ghost Jan 22, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UniqueGlobalMessageValue [](start = 66, length = 24)

I assume this is a copy/paste error and you intended nameof(SharedKeyGlobalMessageValue). Now I have to figure out why it doesn't make your tests fail. #Closed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. The test succeeds because it just requires the rule-scoped value to be different from the global-scoped value, so you can tell which you have. Should still fix so the next schlub doesn't have to figure it out.

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


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()
Copy link

@ghost ghost Jan 22, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateBasicRunForMessageStringLookupTesting [](start = 27, length = 43)

Love the method name. #Closed

{
// Returns a run object that defines unique string instances both
Copy link

@ghost ghost Jan 22, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns [](start = 15, length = 7)

This is a good comment.

Ultra-nit: IMO it is a method summary comment so it belongs outside the method body. (I am not suggested XML docs.) #Closed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering how good the method name is, you probably considered it self-documenting, and considered this more of an implementation comment. I can get on board with that.

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

// 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<Result>
{
new Result
{
RuleId = ruleId,
RuleIndex = 0,
Message = new Message
{
MessageId = globalMessageId
}
}
},
Results = new List<Result> { }, // add non-null collections for convenience
Invocations = new List<Invocation> { new Invocation { } },
Resources = new Resources
{
MessageStrings = new Dictionary<string, string>
{
[globalMessageId] = globalMessageValue
[UniqueGlobalMessageId] = UniqueGlobalMessageValue,
[SharedMessageId] = SharedKeyGlobalMessageValue
},
Rules = new List<Rule>
{
new Rule
{
Id = ruleId
Id = RuleId,
MessageStrings = new Dictionary<string, string>
{
[UniqueRuleMessageId] = UniqueRuleMessageValue,
[SharedMessageId] = SharedKeyRuleMessageValue
}
}
}
}
};

run.Invocations[0].ToolNotifications = new List<Notification>();
run.Invocations[0].ConfigurationNotifications = new List<Notification>();
Copy link

@ghost ghost Jan 22, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigurationNotifications [](start = 31, length = 26)

Object initialization syntax FTW. On Line 136 above:

    Invocations = new List<Invocation>
    {
        new Invocation
        {
            ToolNotifications = new List<Notification>(),
            ConfigurationNotifications = new List<Notification>()
        }
    },
``` #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this introduces 6 new lines of vertical space, which is why I avoided it. Why doesn't this provoke your 'chunking' need? :) I did take the note, btw

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is a tradeoff. The question is, what do I get back from those 6 lines? For me, in this case, the answers are:

  1. Conceptual grouping: The object initialization happens entirely within the object initializer braces. There are no surprises later ("Oh! It wasn't completely initialized after all!")
  2. Locality of reference: As the code stands, we create the Invocation object on L136, but we don't initialize it until LL159-160.
  3. DRY: We don't want to repeat the prefix run.Invocations[0].
  4. Readability: The reader doesn't have to parse the nested member access expressions to discover that they really are just part of the object initialization.

In reply to: 249836127 [](ancestors = 249836127,249608580)


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);
}
Copy link

@ghost ghost Jan 22, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yes, perfect test. #ByDesign


[Fact]
public void InsertOptionalDataVisitorTests_FlattensMessageStringsInNotification()
{
Run run = CreateBasicRunForMessageStringLookupTesting();

IList<Notification> toolNotifications = run.Invocations[0].ToolNotifications;
IList<Notification> 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. This message
Copy link

@ghost ghost Jan 22, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared message id with an overriding rule id. [](start = 15, length = 45)

How about this for the first sentence: "Notification that refers to a rule that contains a message with the same id as the specified notification message id." The second sentence still works. #Resolved

// should still be retrieved from the global strings table.
toolNotifications.Add(
new Notification
{
Id = NotificationId,
RuleIndex = RuleIndex,
Message = new Message { MessageId = SharedMessageId }
});
configurationNotifications.Add(toolNotifications[1]);

toolNotifications.Add(
Copy link

@ghost ghost Jan 22, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add [](start = 30, length = 3)

Comment: "Notification that refers to a rule that does not contain a message with the same id as the specified notification id. In this case it is no surprise that the message comes from the global string table."

... and then I think the test reads most naturally if you reverse the order of notifications 1 and 2. #Resolved

new Notification
{
Id = NotificationId,
RuleIndex = RuleIndex,
Message = new Message { MessageId = UniqueGlobalMessageId }
});
configurationNotifications.Add(toolNotifications[2]);


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(SharedKeyGlobalMessageValue);
configurationNotifications[1].Message.Text.Should().Be(SharedKeyGlobalMessageValue);

toolNotifications[2].Message.Text.Should().Be(UniqueGlobalMessageValue);
configurationNotifications[2].Message.Text.Should().Be(UniqueGlobalMessageValue);
}


[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<Result>
run.Results.Add(
new Result
{
new Result
RuleId = RuleId,
RuleIndex = RuleIndex,
Message = new Message
{
Text = "Some testing occurred."
},
Fixes = new List<Fix>
{
RuleId = ruleId,
RuleIndex = 0,
Message = new Message
new Fix
{
Text = "Some testing occurred."
Description = new Message
{
MessageId = UniqueGlobalMessageId
}
},
Fixes = new List<Fix>
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<string, string>
RuleId = "RuleWithNoRulesMetadata",
Message = new Message
{
[globalMessageId] = globalMessageValue
Text = "Some testing occurred."
},
Rules = new List<Rule>
Fixes = new List<Fix>
{
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);
}
Copy link

@ghost ghost Jan 22, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Another perfect test. #Closed


[Fact]
Expand Down
21 changes: 17 additions & 4 deletions src/Sarif/Autogenerated/Notification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ public SarifNodeKind SarifNodeKind
[DataMember(Name = "ruleId", IsRequired = false, EmitDefaultValue = false)]
public string RuleId { get; set; }

/// <summary>
/// The index within the run resources array of the rule object associated with this notification.
/// </summary>
[DataMember(Name = "ruleIndex", IsRequired = false, EmitDefaultValue = false)]
[DefaultValue(-1)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
public int RuleIndex { get; set; }

/// <summary>
/// The file and region relevant to this notification.
/// </summary>
Expand Down Expand Up @@ -97,6 +105,7 @@ public SarifNodeKind SarifNodeKind
/// </summary>
public Notification()
{
RuleIndex = -1;
Level = NotificationLevel.Warning;
}

Expand All @@ -109,6 +118,9 @@ public Notification()
/// <param name="ruleId">
/// An initialization value for the <see cref="P:RuleId" /> property.
/// </param>
/// <param name="ruleIndex">
/// An initialization value for the <see cref="P:RuleIndex" /> property.
/// </param>
/// <param name="physicalLocation">
/// An initialization value for the <see cref="P:PhysicalLocation" /> property.
/// </param>
Expand All @@ -130,9 +142,9 @@ public Notification()
/// <param name="properties">
/// An initialization value for the <see cref="P:Properties" /> property.
/// </param>
public Notification(string id, string ruleId, PhysicalLocation physicalLocation, Message message, NotificationLevel level, int threadId, DateTime timeUtc, ExceptionData exception, IDictionary<string, SerializedPropertyInfo> properties)
public Notification(string id, string ruleId, int ruleIndex, PhysicalLocation physicalLocation, Message message, NotificationLevel level, int threadId, DateTime timeUtc, ExceptionData exception, IDictionary<string, SerializedPropertyInfo> properties)
{
Init(id, ruleId, physicalLocation, message, level, threadId, timeUtc, exception, properties);
Init(id, ruleId, ruleIndex, physicalLocation, message, level, threadId, timeUtc, exception, properties);
}

/// <summary>
Expand All @@ -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()
Expand All @@ -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<string, SerializedPropertyInfo> properties)
private void Init(string id, string ruleId, int ruleIndex, PhysicalLocation physicalLocation, Message message, NotificationLevel level, int threadId, DateTime timeUtc, ExceptionData exception, IDictionary<string, SerializedPropertyInfo> properties)
{
Id = id;
RuleId = ruleId;
RuleIndex = ruleIndex;
if (physicalLocation != null)
{
PhysicalLocation = new PhysicalLocation(physicalLocation);
Expand Down
6 changes: 6 additions & 0 deletions src/Sarif/Autogenerated/NotificationEqualityComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions src/Sarif/Visitors/InsertOptionalDataVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public InsertOptionalDataVisitor(OptionallyEmittedData dataToInsert, IDictionary
{
_dataToInsert = dataToInsert;
_originalUriBaseIds = originalUriBaseIds;
_ruleIndex = -1;
}

public override Run VisitRun(Run node)
Expand Down