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

Implement v1->v2 conversion for rules dictionary #1232

Merged
merged 3 commits into from
Jan 23, 2019
Merged

Implement v1->v2 conversion for rules dictionary #1232

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
d34490c
Partial implementation
Jan 23, 2019
5704e1f
Get last test working.
Jan 23, 2019
1fc73f0
Somebody missed checking in a generated file.
Jan 23, 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
32 changes: 17 additions & 15 deletions ....UnitTests/TestData/SarifVersionOneToCurrentVisitor/ExpectedOutputs/OneRunWithRules.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"$schema": "http://json.schemastore.org/sarif-2.0.0-csd.2.beta.2018-10-10",
"version": "2.0.0-csd.2.beta.2018-10-10",
"$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",
Copy link
Author

Choose a reason for hiding this comment

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

28 [](start = 39, length = 2)

The prerelease compatibility transformer, although it was performing correctly, masked some of the behavior this test needed to demonstrate. We can talk about the details offline (assuming I haven't forgotten them by then).

"runs": [
{
"tool": {
Expand All @@ -10,30 +10,32 @@
"results": [
{
"ruleId": "C2001",
"ruleIndex": 0,
"message": {
"arguments": [ "someVariable" ],
"messageId": "default"
}
},
{
"ruleId": "C2001",
"ruleIndex": 0,
"message": {
"arguments": [ "anotherVariable" ],
"messageId": "default"
}
},
{
"ruleId": "C2002-1",
"ruleId": "C2002",
Copy link
Author

Choose a reason for hiding this comment

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

C2002 [](start = 21, length = 5)

See the comment in the visitor code. It was never right to assign a ruleKey to a ruleId.

Copy link
Author

Choose a reason for hiding this comment

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

In this case, in the input, the ruleKey did not point to any entry in the rules dictionary, so this result has no ruleIndex property (it defaults to -1).

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

"message": { "text": "Some testing occurred." }
},
{
"ruleId": "C2003",
"ruleIndex": 2,
Copy link
Author

Choose a reason for hiding this comment

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

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

In the input file, this result has no ruleId, and again it was never right to assign a ruleKey to a ruleId -- so the v2 result has no ruleId property. On the other hand, the v1 ruleKey did point to a real rule object, so this v2 result does have a ruleIndex property.

"message": { "text": "Some testing occurred." }
}
],
"resources": {
"rules": {
"C2001": {
"rules": [
{
"id": "C2001",
"shortDescription": {
"text": "A variable was used without being initialized."
Expand All @@ -45,18 +47,19 @@
"some_key": "FoxForceFive"
}
},
"C2002": {
{
"id": "C2002",
"fullDescription": {
"text": "Catfish season continuous hen lamb include dose copy grant."
},
"configuration": {
"enabled": true,
"defaultLevel": "error"
"defaultLevel": "error",
"defaultRank": 0.0
Copy link
Author

Choose a reason for hiding this comment

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

defaultRank [](start = 15, length = 11)

I do not know why the serializer is emitting defaultRank with its default value, but I wouldn't hold this PR to look at it. Considering this occurs in the v1->v2 transform, I don't think we should ever take the time to look at it. If something similar shows up in direct production, that will be the time to look at it.

},
"helpUri": "http://www.domain.com/rules/c2002.html"
},
"C2003": {
{
"id": "C2003",
"name": {
"text": "Rule C2003"
Expand All @@ -68,14 +71,13 @@
"text": "Rent internal rebellion competence biography photograph."
},
"configuration": {
"defaultLevel": "note"
"defaultLevel": "note",
"defaultRank": 0.0
}
},
"C2002-1": {
Copy link
Author

Choose a reason for hiding this comment

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

"C2002-1" [](start = 10, length = 9)

The v1 file has a result with a ruleKey that does not exist in the rules dictionary. The visitor code was constructing an empty rule object for that key to refer to. I don't see the point of that. The new code just sets the v2 ruleIndex to -1 in this case. See how on the right-hand side of this diff, the result starting on L28 has no ruleIndex. The old code would have given it a synthetic one.

Copy link
Member

Choose a reason for hiding this comment

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

ok. side issue: what does v2 say about no rule id and ruleIndex == -1. Seems like an invalid condition. A rule (especially for a converted log file) might not have the SARIF notion of a rule id but in this case, should have a name, so the rule object should exist.

IOW, suggest we say in the spec that either rule id or ruleIndex (with a corresponding rule object) SHOULD be present.

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

Copy link
Author

@ghost ghost Jan 28, 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.

We discussed this in person, but for the record: This condition is valid. If a tool does not provide opaque, stable rule ids as required by the spec, ruleId must be absent. If the log file does not provide rule metadata (or at least, does not provide metadata for the rule implicated by this result), then ruleIndex must be absent/-1. Both of those might be true.

In reply to: 250389698 [](ancestors = 250389698,250286632)

"id": "C2002"
}
}
}
]
},
"columnKind": "utf16CodeUnits"
}
]
}
4 changes: 2 additions & 2 deletions ...tTests/TestData/SarifVersionOneToCurrentVisitor/ExpectedOutputs/TwoResultsWithFixes.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
"version": "2.0.0",
Copy link
Author

Choose a reason for hiding this comment

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

"2.0.0" [](start = 13, length = 7)

Force pre-release compatibility transformation.

"runs": [
{
"tool": {
Expand Down
8 changes: 4 additions & 4 deletions src/Sarif.UnitTests/Visitors/SarifVersionOneToCurrentVisitorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ public void SarifTransformerTests_ToCurrent_OneRunWithLogicalLocations()
[Fact]
public void SarifTransformerTests_ToCurrent_OneRunWithFiles()
=> RunTest("OneRunWithFiles.sarif");
#if TRANSFORM_CODE_AUTHORED

[Fact]
public void SarifTransformerTests_ToCurrent_OneRunWitRules()
=> RunTest("OneRunWithRules.sarif");
#endif

[Fact]
public void SarifTransformerTests_ToCurrent_OneRunWithBasicInvocation()
=> RunTest("OneRunWithBasicInvocation.sarif");
Expand All @@ -71,15 +71,15 @@ public void SarifTransformerTests_ToCurrent_OneRunWithNotificationsButNoInvocati
[Fact]
public void SarifTransformerTests_ToCurrent_NotificationExceptionWithStack()
=> RunTest("NotificationExceptionWithStack.sarif");
#if TRANSFORM_CODE_AUTHORED

[Fact]
public void SarifTransformerTests_ToCurrent_BasicResult()
=> RunTest("BasicResult.sarif");

[Fact]
public void SarifTransformerTests_ToCurrent_TwoResultsWithFixes()
=> RunTest("TwoResultsWithFixes.sarif");
#endif

[Fact]
public void SarifTransformerTests_ToCurrent_UriBaseId()
=> RunTest("UriBaseId.sarif");
Expand Down
4 changes: 2 additions & 2 deletions src/Sarif/Autogenerated/Notification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ public SarifNodeKind SarifNodeKind
public string Id { get; set; }

/// <summary>
/// The stable, unique identifier of the rule (if any) to which this notification is relevant. This member can be used to retrieve rule metadata from the rules dictionary, if it exists.
/// The stable, unique identifier of the rule, if any, to which this notification is relevant.
Copy link
Author

Choose a reason for hiding this comment

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

if any [](start = 55, length = 6)

This was lost in the schema merge.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Would materialize on next BuildAndTest gesture. It's important to perform this step before finalizing a push to get relevant changes in autogen code, sorry for omitting this step.

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

/// </summary>
[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.
/// The index within the run resources array of the rule object, if any, associated with this notification.
/// </summary>
[DataMember(Name = "ruleIndex", IsRequired = false, EmitDefaultValue = false)]
[DefaultValue(-1)]
Expand Down
81 changes: 33 additions & 48 deletions src/Sarif/Visitors/SarifVersionOneToCurrentVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using Microsoft.CodeAnalysis.Sarif.VersionOne;
Expand All @@ -22,6 +21,7 @@ public class SarifVersionOneToCurrentVisitor : SarifRewritingVisitorVersionOne
private RunVersionOne _currentV1Run;
private int _threadFlowLocationNestingLevel;
private IDictionary<string, int> _v1FileKeytoV2IndexMap;
private IDictionary<string, int> _v1RuleKeyToV2IndexMap;

private IDictionary<string, string> _v1KeyToFullyQualifiedNameMap;
private IDictionary<LogicalLocation, int> _v2LogicalLocationToIndexMap;
Expand Down Expand Up @@ -732,51 +732,11 @@ internal Result CreateResult(ResultVersionOne v1Result)
result.AnalysisTarget = CreateFileLocation(v1Result.Locations[0].AnalysisTarget);
}

if (v1Result.RuleKey == null)
{
result.RuleId = v1Result.RuleId;
}
else
{
if (v1Result.RuleId == null)
{
result.RuleId = v1Result.RuleKey;
Copy link
Author

@ghost ghost Jan 23, 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.

result [](start = 24, length = 6)

This was never right. It was never valid to assign a ruleKey to a ruleId. The situation "ruleId == null; ruleKey != null" meant:

"This rule has no id (which is perfectly valid, both in v1 and v2). If you want to know more about this rule (like its description), you can find the rule metadata here."

In v2, "ruleId == null; ruleIndex != -1" means the same thing.

}
else
{
if (v1Result.RuleId == v1Result.RuleKey)
{
result.RuleId = v1Result.RuleId;
}
else
{
result.RuleId = v1Result.RuleKey;

if (_currentRun.Resources == null)
{
_currentRun.Resources = new Resources();
}

#if TRANSFORM_CODE_AUTHORED
if (_currentRun.Resources.Rules == null)
{
_currentRun.Resources.Rules = new List<string, Rule>();
}

IDictionary<string, Rule> rules = _currentRun.Resources.Rules;
result.RuleId = v1Result.RuleId;

if (!rules.ContainsKey(v1Result.RuleKey))
{
Rule rule = new Rule() { Id = v1Result.RuleId };
rules.Add(v1Result.RuleKey, rule);
}
string ruleKey = v1Result.RuleKey ?? v1Result.RuleId;
result.RuleIndex = GetRuleIndexForRuleKey(ruleKey, _v1RuleKeyToV2IndexMap);

Debug.Assert(rules[v1Result.RuleKey].Id == v1Result.RuleId);
#endif
}
}
}

if (v1Result.FormattedRuleMessage != null)
{
if (result.Message == null)
Expand Down Expand Up @@ -875,6 +835,7 @@ internal Run CreateRun(RunVersionOne v1Run)
_currentV1Run = v1Run;

_v1FileKeytoV2IndexMap = CreateFileKeyToIndexMapping(v1Run.Files);
_v1RuleKeyToV2IndexMap = CreateRuleKeyToIndexMapping(v1Run.Rules);

RunAutomationDetails id = null;
RunAutomationDetails[] aggregateIds = null;
Expand Down Expand Up @@ -905,20 +866,18 @@ internal Run CreateRun(RunVersionOne v1Run)

_currentRun = run;

#if TRANSFORM_CODE_AUTHORED
if (v1Run.Rules != null)
{
run.Resources = new Resources
{
Rules = new Dictionary<string, Rule>()
Rules = new List<Rule>()
};

foreach (var pair in v1Run.Rules)
{
run.Resources.Rules.Add(pair.Key, CreateRule(pair.Value));
run.Resources.Rules.Add(CreateRule(pair.Value));
}
}
#endif

if (v1Run.Files != null)
{
Expand Down Expand Up @@ -1019,6 +978,32 @@ private static IDictionary<string, int> CreateFileKeyToIndexMapping(IDictionary<
return v1FileKeyToV2IndexMap;
}

private static IDictionary<string, int> CreateRuleKeyToIndexMapping(IDictionary<string, RuleVersionOne> v1Rules)
{
var v1RuleKeyToV2IndexMap = new Dictionary<string, int>();

if (v1Rules != null)
{
int index = 0;
foreach (KeyValuePair<string, RuleVersionOne> entry in v1Rules)
{
v1RuleKeyToV2IndexMap[entry.Key] = index++;
}
}

return v1RuleKeyToV2IndexMap;
}

private int GetRuleIndexForRuleKey(string ruleKey, IDictionary<string, int> v1RuleKeyToV2IndexMap)
{
if (ruleKey == null || !v1RuleKeyToV2IndexMap.TryGetValue(ruleKey, out int index))
{
index = -1;
}

return index;
}

private void PopulateLogicalLocation(
Run v2Run,
IDictionary<string, LogicalLocationVersionOne> v1LogicalLocations,
Expand Down
2 changes: 1 addition & 1 deletion src/Sarif/Visitors/UpdateIndicesVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public override Result VisitResult(Result node)
{
if (_ruleKeyToIndexMap != null)
{
if (_ruleKeyToIndexMap.TryGetValue(node.RuleId, out int ruleIndex))
if (node.RuleId != null &&_ruleKeyToIndexMap.TryGetValue(node.RuleId, out int ruleIndex))
Copy link
Author

Choose a reason for hiding this comment

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

node.RuleId [](start = 20, length = 11)

One of the test cases has a result with a ruleKey but no ruleId. That is valid SARIF v1, and we need to protect against it.

{
node.RuleIndex = ruleIndex;

Expand Down