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 v2->v1 conversion for rules dictionary #1228

Merged
11 commits merged into from
Jan 22, 2019
Merged

Implement v2->v1 conversion for rules dictionary #1228

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
76feeec
Schema: run.results not unique; add notification.ruleIndex.
Jan 21, 2019
0f346de
Schema: Fix version number in title.
Jan 21, 2019
be72499
Set up OneRuleWithRules test to fail.
Jan 21, 2019
c82cd69
Populate v1 ruleId.
Jan 21, 2019
841b088
Fix tests: Don't adjust level "open" until rules is an array.
Jan 21, 2019
46c273d
Synthesize keys on rule id
Jan 21, 2019
8ab228c
Construct v1 rules dictionary; "OneRunWithRules" test passes.
Jan 21, 2019
21c4f9b
Get TwoResultsWithFixes test working
Jan 21, 2019
e2ce115
Rebaseline one test to avoid spurious file ordering difference.
Jan 21, 2019
713f359
Add a comment.
Jan 21, 2019
f66d0b2
Incorporate PR feedback.
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
12 changes: 9 additions & 3 deletions ....UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/OneRunWithRules.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
},
"results": [
{
"ruleId": "C2009",
"ruleId": "C2001",
"message": "Some testing occurred."
},
{
Expand Down Expand Up @@ -45,10 +45,16 @@
"defaultLevel": "error",
"helpUri": "http://www.domain.com/rules/c2002.html"
},
"C2003": {
"id": "C2003",
"name": "Rule C2003 for C#",
"shortDescription": "C# rules were meant to be broken.",
"fullDescription": "Rent internal rebellion competence biography photograph."
},
"C2003-1": {
"id": "C2003",
"name": "Rule C2003",
"shortDescription": "Rules were meant to be broken.",
"name": "Rule C2003 for VB",
"shortDescription": "VB rules were meant to be broken.",
"fullDescription": "Rent internal rebellion competence biography photograph."
}
}
Expand Down
16 changes: 8 additions & 8 deletions ....UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/ResultLocations.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@
"language": "en-US"
},
"logicalLocations": {
"collections::list::add": {
"name": "add",
"parentKey": "collections::list",
"kind": "function"
"collections": {
"kind": "namespace"
Copy link
Author

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

namespace [](start = 19, length = 9)

I reordered the properties in this object to match the actual output. The JToken comparison doesn't care; the test passes either way. But when the test fails for some other reason -- as it did for reasons I'll explain below -- then this additional, non-significant difference was just confusing. #ByDesign

},
"collections::list": {
"name": "list",
"parentKey": "collections",
"kind": "type"
},
"collections": {
"kind": "namespace"
"collections::list::add": {
"name": "add",
"parentKey": "collections::list",
"kind": "function"
}
},
"results": [
Expand Down Expand Up @@ -101,14 +101,14 @@
}
],
"rules": {
"WEB1079.AttributeValueIsNotQuoted": {
"WEB1079": {
Copy link
Author

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

WEB1079 [](start = 9, length = 7)

When we were converting a dictionary of v2 rules to a dictionary of v1 rules, we could preserve the v2 keys in the v1 dictionary. Now that the PreReleaseCompatibilityTransformer is converting the dictionary to an array, the keys are lost before the v2-to-v1 transformer is invoked.

In fact, the v2 format has no place for this "friendly compound id". If you want a place for it, we could add it to v2 rule.

Copy link
Member

Choose a reason for hiding this comment

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

This compound id is a very useful construct and highly utilized internally. I'll open a TC issue.

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. These terms should be persisted to rule.name, can you go ahead and do that? still an open question whether we document using rule.id and rule.name in this way to compose a compound id (with an opaque stable and instable readable component in well known locations).

In reply to: 249572234 [](ancestors = 249572234,249559914)

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea and makes sense for this file. The problem is that the dictionary key is not always a friendly name (consider TST0001-2) and there's no way to tell whether it was intended as a friendly name. Thoughts?

In reply to: 249572657 [](ancestors = 249572657,249572234,249559914)

"id": "WEB1079",
"shortDescription": "The attribute value is not quoted.",
"messageFormats": {
"default": "The value of the '{0}' attribute is not quoted. Wrap the attribute value in single or double quotes."
}
},
"WEB1066.TagNameIsNotLowercase": {
"WEB1066": {
"id": "WEB1066",
"shortDescription": "The tag name is not lowercase.",
"messageFormats": {
Expand Down
4 changes: 2 additions & 2 deletions ...tTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/TwoResultsWithFixes.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,14 @@
}
],
"rules": {
"WEB1079.AttributeValueIsNotQuoted": {
"WEB1079": {
"id": "WEB1079",
"shortDescription": "The attribute value is not quoted.",
"messageFormats": {
"default": "The value of the '{0}' attribute is not quoted. Wrap the attribute value in single or double quotes."
}
},
"WEB1066.TagNameIsNotLowercase": {
Copy link
Member

Choose a reason for hiding this comment

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

TagNameIsNotLowercase [](start = 17, length = 21)

same note, move the readable names to rule.name: NIT

Copy link
Author

Choose a reason for hiding this comment

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

See my response to your other comment on this issue.

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

"WEB1066": {
"id": "WEB1066",
"shortDescription": "The tag name is not lowercase.",
"messageFormats": {
Expand Down
34 changes: 25 additions & 9 deletions src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/Inputs/OneRunWithRules.sarif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,29 @@
},
"results": [
{
"ruleId": "C2009",
"ruleId": "C2001",
"ruleIndex": 0,
Copy link
Member

@michaelcfanning michaelcfanning Jan 21, 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.

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

why couldn't the prerelease transformer fix things up for you? i.e., was there a reason you had to introduce the ruleIndex object? generally, i think we should avoid piecemeal upgrade of these files to current format. #ByDesign

Copy link
Author

Choose a reason for hiding this comment

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

FileDiffingTests runs the prerelease transformer on the expected output (except in the case of tests that, like this one, produce SARIF v1), not on the input.

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

"message": { "text": "Some testing occurred." }
},
{
"ruleId": "C2002",
"ruleIndex": 1,
"message": { "text": "Some testing occurred." }
},
{
"ruleId": "C2003",
"ruleIndex": 2,
"message": { "text": "Some testing occurred." }
},
{
"ruleId": "C2003-1",
"ruleId": "C2003",
"ruleIndex": 3,
"message": { "text": "Some testing occurred." }
}
],
"resources": {
"rules": {
"C2001": {
"rules": [
{
"id": "C2001",
"shortDescription": {
"text": "A variable was used without being initialized."
Expand All @@ -39,7 +43,7 @@
"some_key": "FoxForceFive"
}
},
"C2002": {
{
"id": "C2002",
"fullDescription": {
"text": "Catfish season continuous hen lamb include dose copy grant."
Expand All @@ -50,19 +54,31 @@
},
"helpUri": "http://www.domain.com/rules/c2002.html"
},
"C2003-1": {
{
"id": "C2003",
"name": {
"text": "Rule C2003"
"text": "Rule C2003 for C#"
},
"shortDescription": {
"text": "Rules were meant to be broken."
"text": "C# rules were meant to be broken."
},
"fullDescription": {
"text": "Rent internal rebellion competence biography photograph."
}
},
{
"id": "C2003",
"name": {
"text": "Rule C2003 for VB"
},
"shortDescription": {
"text": "VB rules were meant to be broken."
},
"fullDescription": {
"text": "Rent internal rebellion competence biography photograph."
}
}
}
]
}
}
]
Expand Down
6 changes: 2 additions & 4 deletions src/Sarif.UnitTests/Visitors/SarifCurrentToVersionOneVisitorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,21 @@ protected override string ConstructTestOutputFromInputResource(string inputResou
[Fact]
public void SarifTransformerTests_ToVersionOne_OneRunWithFiles() => RunTest("OneRunWithFiles.sarif");

#if TRANSFORM_CODE_AUTHORED
Copy link
Member

@michaelcfanning michaelcfanning Jan 21, 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.

TRANSFORM_CODE_AUTHORED [](start = 4, length = 23)

yay! #Closed

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

[Fact]
public void SarifTransformerTests_ToVersionOne_OneRunWithBasicInvocation() => RunTest("OneRunWithBasicInvocation.sarif");

[Fact]
public void SarifTransformerTests_ToVersionOne_NotificationExceptionWithStack() => RunTest("NotificationExceptionWithStack.sarif");

#if TRANSFORM_CODE_AUTHORED
[Fact]
public void SarifTransformerTests_ToVersionOne_ResultLocations() => RunTest("ResultLocations.sarif");

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

[Fact]
public void SarifTransformerTests_ToVersionOne_Regions() => RunTest("Regions.sarif");
}
Expand Down
15 changes: 11 additions & 4 deletions src/Sarif/Schemata/sarif-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"$schema": "http://json-schema.org/draft-04/schema#",
"title": "Static Analysis Results Format (SARIF) Version 2.0.0-csd.2.beta.2018-10-10 JSON Schema",
"title": "Static Analysis Results Format (SARIF) Version 2.0.0-csd.2.beta.2018-11-28 JSON Schema",
"description": "Static Analysis Results Format (SARIF) Version 2.0.0-csd.2.beta-2018-11-28 JSON Schema: a standard format for the output of static analysis tools.",
"additionalProperties": false,
"type": "object",
Expand Down Expand Up @@ -347,7 +347,7 @@
"description": "An array of external property files containing run.results arrays to be merged with the root log file.",
"type": "array",
"minItems": 1,
"uniqueItems": true,
"uniqueItems": false,
"items": {
"$ref": "#/definitions/externalPropertyFile"
}
Expand Down Expand Up @@ -1002,10 +1002,17 @@
},

"ruleId": {
"description": "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.",
"description": "The stable, unique identifier of the rule, if any, to which this notification is relevant.",
"type": "string"
},

"ruleIndex": {
"description": "The index within the run resources array of the rule object, if any, associated with this notification.",
"type": "integer",
"default": -1,
"minimum": -1
},

"physicalLocation": {
"description": "The file and region relevant to this notification.",
"$ref": "#/definitions/physicalLocation"
Expand Down Expand Up @@ -1274,7 +1281,7 @@
"properties": {

"ruleId": {
"description": "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.",
Copy link
Author

@ghost ghost Jan 21, 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 = 68, length = 1)

This is the only place we used parens instead of commas on "if any". #ByDesign

"description": "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.",
"type": "string"
},

Expand Down
Loading