Implement v2->v1 conversion for rules dictionary #1228

ghost · 2019-01-21T19:16:07Z

No description provided.

PreRelCompatTrans loses the rules dictionary key, so we shouldn't expect it in the output.

ghost · 2019-01-21T19:17:21Z

src/Sarif/Schemata/sarif-schema.json

@@ -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.",


( [](start = 68, length = 1)

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

ghost · 2019-01-21T19:17:53Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

@@ -23,8 +23,11 @@ public class SarifCurrentToVersionOneVisitor : SarifRewritingVisitor
        private RunVersionOne _currentRun = null;

        // To understand the purpose of these fields, see the comment on CreateFileKeyIndexMappings.
-        private IDictionary<string, int> _v1FileKeyToV2IndexDictionary;
-        private IDictionary<int, string> _v2FileIndexToV1KeyDictionary;
+        private IDictionary<string, int> _v1FileKeyToV2IndexMap;


For consistency with v1->v2 transformer. You preferred Map to Dictionary. #ByDesign

src/Sarif/Writers/PrereleaseCompatibilityTransformer.cs

ghost · 2019-01-21T19:28:05Z

...rif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/ResultLocations.sarif

-          "parentKey": "collections::list",
-          "kind": "function"
+        "collections": {
+          "kind": "namespace"


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

ghost · 2019-01-21T19:30:26Z

...rif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/ResultLocations.sarif

@@ -101,14 +101,14 @@
        }
      ],
      "rules": {
-        "WEB1079.AttributeValueIsNotQuoted": {
+        "WEB1079": {


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.

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

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

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)

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)

michaelcfanning · 2019-01-21T20:35:26Z

...UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/TwoResultsWithFixes.sarif

          "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": {


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

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

See my response to your other comment on this issue.

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

michaelcfanning · 2019-01-21T20:37:14Z

src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/Inputs/OneRunWithRules.sarif

@@ -9,25 +9,29 @@
      },
      "results": [
        {
-          "ruleId": "C2009",
+          "ruleId": "C2001",
+          "ruleIndex":  0,


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

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)

michaelcfanning · 2019-01-21T20:37:28Z

src/Sarif.UnitTests/Visitors/SarifCurrentToVersionOneVisitorTests.cs

@@ -44,23 +44,21 @@ protected override string ConstructTestOutputFromInputResource(string inputResou
        [Fact]
        public void SarifTransformerTests_ToVersionOne_OneRunWithFiles() => RunTest("OneRunWithFiles.sarif");

-#if TRANSFORM_CODE_AUTHORED


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

yay! #Closed

michaelcfanning · 2019-01-21T20:42:05Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

+                            : 1;
+
+                        v2RuleIndexToV1KeyMap[i] = ruleIdToCountMap[rule.Id] == 1
+                            ? null


why store null in some cases? instead of just persisting the rule id? then look-up just works exclusively against this look-up. or perhaps you use the null case to indicate something special later? #Resolved

You're right. I do not use null for anything special later, and with your suggested change, I can eliminate the null check in ConvertRulesArrayToDictionary. We can tell this is a good change because that null check required a comment to remind us what we did here. Now we don't need the comment either.

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

Hah! Unit tests FTW. The reason I set rule key to null is to prevent it being persisted. Otherwise you end up with:

"ruleId": "TST0001", "ruleKey": "TST0001"

I will add a comment.

In reply to: 249615764 [](ancestors = 249615764,249573985)

BUT! I found what I think is a clearer way to express this logic. Now, I do set the map entry to ruleId rather than null, and I don't need the null check (and its comment) in ConvertRulesArrayToDictionary. Instead, I do the null check right at the point where we care about its effect: at the point where I assign result.RuleKey.

See the last commit.

In reply to: 249616588 [](ancestors = 249616588,249615764,249573985)

michaelcfanning · 2019-01-21T20:43:25Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

+                for (int i = 0; i < rules.Count; ++i)
+                {
+                    Rule rule = rules[i];
+                    if (rule.Id != null)


if (rule.Id != null) [](start = 20, length = 20)

ok, so in old SARIF, if you had no rule.Id, you had no look-up key, so there won't be any coupling between results and rule metadata for these checks. #ByDesign

michaelcfanning · 2019-01-21T20:45:50Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

+            IDictionary<int, string> v2RuleIndexToV1KeyMap)
+        {
+            string ruleKey = null;
+            if (0 <= ruleIndex && ruleIndex < v2RuleIndexToV1KeyMap?.Count)


0 <= ruleIndex [](start = 16, length = 14)

can you just make this ruleIndex > -1 perhaps? your comparison here is confusing. if you retain it, please comment the code. #ByDesign

I've got a principled objection to treating a sentinel value numerically just because it happens to be ordered properly. ruleIndex != -1 ("it's not the sentinel!") is tempting, but it lets -2 slip through. I like ruleIndex >= 0 because it states the validity condition independent of what we happened to choose as the sentinel.

Or were you objecting to the idiom min <= x && x < limit (which I occasionally use to mimic the math notation min < x < limit)?

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

Marking this one "By Design" (because I really do like the comparison the way I wrote it), but marking the next one "Resolved" because you're right, we don't need the comparison at all.

In reply to: 249613504 [](ancestors = 249613504,249574859)

michaelcfanning · 2019-01-21T20:50:05Z

src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs

+            IDictionary<int, string> v2RuleIndexToV1KeyMap)
+        {
+            string ruleKey = null;
+            if (0 <= ruleIndex && ruleIndex < v2RuleIndexToV1KeyMap?.Count)


ruleIndex < v2RuleIndexToV1KeyMap [](start = 34, length = 33)

wait, are you sure about this? v2RuleIndexToV1 is a map, and we skip rule entries with no ids. So I don't think a comparison to the map count is right.

Consider a rules array with 49 checks with no ids. the 50th does have a rule idea and we have a single SARIF result pointing to it with ruleIndex == 49. your v2RuleIndexToV1KeyMap will only have a single entry right? and so we'll fail this test.

I don't understand all these hijinks in any case. why can't you just look for ruleIndex and try to find the key? it will be set to null in all failures (which is right). All your code assumes that the map may have values in it for things like ruleIndex <0 (which we want to avoid retrieving). why would that be? #Resolved

You're right that this is a bug. And you're right that there's no need for the condition at all. I don't know what I was thinking.

Then the question is whether to get rid of this helper altogether. It has the advantage of returning its value rather than using an out parameter. I think I'll keep it.

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

michaelcfanning

🕐

* Fix up tests * Conversion to files array. WIP. Core SARIF component build complete except for SarifLogger tail. * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * Reduced files array build (#1191) * Sarif and Sarif.Converters now building * Files array (#1188) * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * DRY out converters to isolate shared code. * Restore essential change in schema that converts files dictionary to an array. * Simplify ShouldSerialize logic * Remove unused namespaces * Respond to PR feedback. * Respond to PR feedback * End-to-end build works. Now we can author core transformation and fix tests. (#1192) * Fix up merge from 'develop' branch. * Update supporting test code for processing checked in files. (#1202) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * Files array basic transform (#1205) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * WIP. Furhter progress * Fix up samples build * Fix up merge from basic transform branch * Mime type validation (#1206) * Fix up merge from basic transform branch * Fix up mime test * Start work on v1 <-> v2 transformation (#1208) * Restore TransformCommand and first (unaffected) unit test * Restore "minimal prerelease v2 to current v2" test. * estore "minimal v1 to current v2" test. * Restore remaining TransformCommand unit tests. * Uncomment v2 => v1 tests to observe failures. * Uncomment 'transform' command. * Restore MakeUrisAbsoluteVisitor tests (#1210) This change updates the visitor that expands URIs in the presence of `originalUriBaseIds`. Turns out there was technical debt here, because our tests provided `originalUriBaseIds` equivalents in the property bag (because we had no official SARIF slot for them). I did not notice this gap when we made the schema change to add `originalUriBaseIds`. * Get v2 -> v1 transform working with files array (#1211) Test failure count is down to 32; will be 28 when you merge your fix. There is not -- and never was -- a test case for fileLocations that use uriBaseId (never was one). I know for a fact that there is no code to support that case. You’ll see a comment to that effect in the code. I will take care of that next. Then I will move on to v1 -> v2 transform. As part of this change, the `SarifCurrentToVersionOneVisitorTests` are now based on the `RunTest` helper method from the base class `FileDiffingTests`. * Convert debug assert to exception to make test execution more deterministic (#1214) * Update insert optional data tests and update indices visitor. (#1212) * Update insert optional data tests and update indices visitor. * Delete speculatively needed file * Remove erroneous override of base visit method. * Rework summary comment on DeletesOutputsDirectoryOnClassInitializationFixture. * Update clang analyzer name. Flow converter log verification through JToken comparison. (#1213) * The multiool, core sarif, and validation test binaries now all pass (#1215) * The multiool, core sarif, and validation test binaries now all pass completely. * Remove unwanted assert that may fire during unit testing. * Merge from files-array * PR feedback. * PR feedback tweaks * Accept PR feedback from previous change. (#1216) Use LINQ IEnuemrable.Except in the unit test, which improves readability without compromising efficiency (because Except uses a Set to do its work in O(N) time). * Fix Sarif.Driver and Sarif.Functional tests. (#1217) * Fix Sarif.Driver and Sarif.Functional tests. * Restore test file * Fix Semmle tests and Fortify converter: all tests now pass. (#1218) * Sarif converters fixups (#1219) * Fix semmle tests and fority. * Final test fixups * Invoke appveyor for files-array branch.: (#1220) * Update SarifVersionOneToCurrentVisitor for run.files array (#1221) * Uncomment v1 -> v2 tests; 3/14 fail. * Move test data to locations expected by FileDiffingTests. * Fix up some IDE C#7 code cleanups. * Use FileDiffingTests helper. * Fix bug in FileDiffingTests that caused a test failure. * Remove default-valued argument from a call to RunTest. * Create basic files array Does not yet have fileIndex, parentIndex, or response file handling. * Revert incorrect change in FileDiffingTests. * Fix one unit test with spot fix to "expected" file. * Fix up some C#7 IDE warnings * Force update in FileDiffing tests to avoid deserialization errors from out of date "expected" files. * Fix missing "modified" flag sets in PreRelCompatTransformer. * Populate fileIndex in run.files array. * Fix unit test by fixing fileLocation creation. * Restore response file handling. * Populate fileIndex on fileLocations as appropriate. * Fix last test failure by reworking response file handling. * Feedback: Introduce transformer helper PopulatePropertyIfAbsent. * Feedback: Tighten platform-independent string compare. Also: - Reformat unit test lines. * Feedbakc: Revert FileDiffingTest change; downgrade affected test files to provoke transform * Basic rules transformation (except for v1 <-> v2 conversion) (#1223) * Basic rules transformation (except for v1 <-> v2 conversion) * Respond to very excellent PR feedback. * PR feedback * Add files array tests for nested files and uriBaseIds (#1226) * Add failing v1 -> v2 nested files test * Fix existing mis-handling of analysisTarget and resultFile. * Get nested files test case working. * Add failing v1 => v2 uriBaseId test. * Populate v2 uriBaseId. * Fix up expected v2 fileLocation.properties: test passes. * Enhance uriBaseIds test case. * Implement v2->v1 conversion for rules dictionary (#1228) * Notification rule index (#1229) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Notification rule index (#1230) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Missed feedback from previous PR (#1231) * Implement v1->v2 conversion for rules dictionary (#1232) * Partial implementation * Get last test working. * Somebody missed checking in a generated file. * Schema changes from TC #30 (#1233) * Add source language, fix rank default. * Adjust rank minimum to accommoodate default. * Fix broken test. * Remove unnecessary None items from project file. * PR feedback * Files array results matching (#1234) * WIP preliminary results matching * Restore results matching for files array * Add back autogenerated (unused) namespace directive

@fishoak

…#1264) * Fix tests that are broken in appveyor (#1134) * Properly persist run level property bags (#1136) * Fix #1138: Add validation rule: contextRegion requires region (#1142) Also: - Enhance the "new-style" verification so that we no longer require the file "Invalid_Expected.sarif". Each file can now contain a property that specifies the expected locations of all the validation results. * Prep for 2018-11-28 schema update. Remove run.architecture. (#1145) * Add run.newlineSequences to schema (#1146) * Mark result.message as required in the schema (#1147) * Mark result.message as required in the schema * Update release history with result.message breaking change. * Fix typo in testoutput. * Rename tool.fileVersion to tool.dottedQuadFileVersion (#1148) * Upgrade more validator functional tests (#1149) We apply the new functional test pattern to four more rules: - `EndColumnMustNotBeLessThanStartColumn` - `EndLineMustNotBeLessThanStartLine` - `EndTimeMustBeAfterStartTime` (which is misnamed, and in a future PR we will rename it to `EndTimeMustNotBeBeforeStartTime`) - `MessageShouldEndWithPeriod` In addition, we remove the test data for a rule that no longer exists, `HashAlgorithmsMustBeUnique` (which no longer applies because `file.hashes` is now an object keyed by the hash algorithm). Because there are so many properties of type `message`, exhaustively testing the rule `MessageShouldEndWithPeriod` revealed many holes in the visitor class `SarifValidationSkimmerBase`, which I plugged. As we have discussed, we should generate this class from the schema. After this, there are only two more rules to convert: - `UriBaseIdRequiresRelativeUri` - `UseAbsolutePathsForNestedFileUriFragments` ... but this PR is already large enough. * Remove 'open' from list of valid rule configuration default values. (#1158) * Emit column kind default explicitly for Windows SDK SARIF emit. (#1160) * Emit column kind default explicitly for Windows SDK SARIF emit. * Update release notes * More column kind test fixes * Change behavior to always serialize column kind. * Always serialize column kind * Finish validator functional test upgrade (#1159) * Rename rule EndTimeMustBeAfterStartTime => ...MustNotBeBefore... * Upgrade UriBaseIdRequiresRelativeUri tests. * Remove obsolete rule UseAbsolutePathsForNestedFileUriFragments. * Remove support for old test design. * Remove 'package' as a documented logical location kind in the schema. Documentation only change. (#1162) * Fortify FPR converter improvements + unit tests (#1161) * Improvements and corrections Populate originalUriBaseIds from <SourceBasePath> Populate tFL.kind from <Action type="..."> Add default node to result.locations * Add location annotation for Action elements with no type attribute * Support node annotations + uribasepath + test updates * Update FortifyTest.fpr.sarif * Add converter tests & assets + opportunistic code cleanup * PR feedback * Logical locations dictionaries to arrays (#1170) The core change here is the transformation of `run.logicalLocations` from a dictionary (which is keyed, generally, by the fully qualified name of a logical location) to an array of logical locations. Result locations now reference logical locations by a logical location index. This changes removes the necessity of resolving key name collisions for logical locations that differ only by type (a namespace that collides with the fully qualified name of a type being the classic example). In addition to making the core change, we have also authored a transformation that converts existing pre-release SARIF v2 files to the new design. We accomplish this by creating dictionaries, with value type comparison for keys, that are keyed by logical locations. This processing requires that any parent keys already exist in the array (so that a logical location can populate its parent logical location index, if any). In addition to the core functionality and any transformation of individual log files, result matching presents special complications. In a result matching scenario, the logical location index of a result is not relevant to its identify: only the contents of the logical location this index points to are relevant. Furthermore, when merging a baseline file (which can contain results that are exclusive to a single log file within the matching domain), logical location indices are subject to change and must be updated. For this scenario and at least one other, we use a visitor pattern to update indices. The reason is that locations are pervasive in the format and the visitor provides a convenient mechanism to put common location processing logical. This visitor uses puts additional pressure on the transformation logic, as it entails additional deserialization of the JSON. With more time/effort, we could have exhaustively updated all locations using the JParser/JObject/etc. API domain. Oh well. Finally, we must update the logical that transforms v1 to v2 and vice versa. Whew. If that was not already sufficiently intrusive, this work revealed some minor flaws in various converters (the ones that handle logical locations): AndroidStudio, FxCop and PREfast. This change is complex but valuable. Logical locations are now expressed as coherent objects in their table. In the main, I have preferred to leave `result.fullyQualifiedName` populated (in addition to `result.logicalLocationIndex`, to support easily looking up matching logical locations). * Add result.rank and ruleConfiguration.defaultRank (#1167) As we discussed offline with @fishoak, the design is good as it stands. The only change is that the default should be -1. I filed oasis-tcs/sarif-spec#303 for that, and put it on the agenda for TC #30. * Logical locations notes (#1184) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Logical locations notes (#1185) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * Incorporate "provenance" schema changes and fix package licenses (#1193) * Add autogenerated RuleConfiguration.cs missed from earlier commit. * Upgrade to NuGet.exe 4.9.2 to handle new license tag. * Remove unused 'Owners' element from build.props. * Add package Title. * Use packageLicenseExpression to specify package license. * Suppress NU5105 (about SemVer 2.0.0 strings) for "dotnet pack" packages. NuGet.exe still warns for ".nuspec" packages. * Incorporate latest "provenance" schema changes. * Address PR feedback. * External property files (#1194) * Update spec for externalPropertiesFile object. * Add external property files to schema. * Finish merge of provenance changes. * Update release notes. * Remove vertical whitespace. * PR feedback. Fix 'properties' to refer to an external file not an actual properties bag. * Remove code gen hint that makes external property files a property bag holder. * Introduce missing brace. Fix up code emit for 'properties' property that isn't a property bag. * Incorporate schema changes for versionControlDetails.mappedTo and rule.deprecatedIds (#1198) * Incorporate "versionControlDetails.mappedTo" schema change. * Incorporate "rule.deprecatedIds" schema change. * Revert updates to comprehensive.sarif (to allow transformer to continue to use this as test content). * Array scrub part 1: everything but anyOf-or-null properties. (#1201) NOTE: For explicitness, I added schema attributes even when they matched the JSON schema defaults, namely: `"minItems": 0` and `"uniqueItems": false`. * Fix v1->v2 hash transformation (#1203) CreateHash must be called to handle algorithm names that aren't in our translation table. Also updated a unit test to cover this case. * Integrate jschema 0.61.0 into SDK (#1204) * Merging arrays transformations back into 'develop' branch (#1236) * Fix up tests * Conversion to files array. WIP. Core SARIF component build complete except for SarifLogger tail. * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * Reduced files array build (#1191) * Sarif and Sarif.Converters now building * Files array (#1188) * Add fileIndex property to file object (#1186) * Fix up tests * PR feedback to improve schema comment * Logical locations notes (#1185) (#1187) * Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline * DRY out converters to isolate shared code. * Restore essential change in schema that converts files dictionary to an array. * Simplify ShouldSerialize logic * Remove unused namespaces * Respond to PR feedback. * Respond to PR feedback * End-to-end build works. Now we can author core transformation and fix tests. (#1192) * Fix up merge from 'develop' branch. * Update supporting test code for processing checked in files. (#1202) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * Files array basic transform (#1205) * Update supporting test code for processing checked in files. * Update nested files test to contain single file. * WIP. Furhter progress * Fix up samples build * Fix up merge from basic transform branch * Mime type validation (#1206) * Fix up merge from basic transform branch * Fix up mime test * Start work on v1 <-> v2 transformation (#1208) * Restore TransformCommand and first (unaffected) unit test * Restore "minimal prerelease v2 to current v2" test. * estore "minimal v1 to current v2" test. * Restore remaining TransformCommand unit tests. * Uncomment v2 => v1 tests to observe failures. * Uncomment 'transform' command. * Restore MakeUrisAbsoluteVisitor tests (#1210) This change updates the visitor that expands URIs in the presence of `originalUriBaseIds`. Turns out there was technical debt here, because our tests provided `originalUriBaseIds` equivalents in the property bag (because we had no official SARIF slot for them). I did not notice this gap when we made the schema change to add `originalUriBaseIds`. * Get v2 -> v1 transform working with files array (#1211) Test failure count is down to 32; will be 28 when you merge your fix. There is not -- and never was -- a test case for fileLocations that use uriBaseId (never was one). I know for a fact that there is no code to support that case. You’ll see a comment to that effect in the code. I will take care of that next. Then I will move on to v1 -> v2 transform. As part of this change, the `SarifCurrentToVersionOneVisitorTests` are now based on the `RunTest` helper method from the base class `FileDiffingTests`. * Convert debug assert to exception to make test execution more deterministic (#1214) * Update insert optional data tests and update indices visitor. (#1212) * Update insert optional data tests and update indices visitor. * Delete speculatively needed file * Remove erroneous override of base visit method. * Rework summary comment on DeletesOutputsDirectoryOnClassInitializationFixture. * Update clang analyzer name. Flow converter log verification through JToken comparison. (#1213) * The multiool, core sarif, and validation test binaries now all pass (#1215) * The multiool, core sarif, and validation test binaries now all pass completely. * Remove unwanted assert that may fire during unit testing. * Merge from files-array * PR feedback. * PR feedback tweaks * Accept PR feedback from previous change. (#1216) Use LINQ IEnuemrable.Except in the unit test, which improves readability without compromising efficiency (because Except uses a Set to do its work in O(N) time). * Fix Sarif.Driver and Sarif.Functional tests. (#1217) * Fix Sarif.Driver and Sarif.Functional tests. * Restore test file * Fix Semmle tests and Fortify converter: all tests now pass. (#1218) * Sarif converters fixups (#1219) * Fix semmle tests and fority. * Final test fixups * Invoke appveyor for files-array branch.: (#1220) * Update SarifVersionOneToCurrentVisitor for run.files array (#1221) * Uncomment v1 -> v2 tests; 3/14 fail. * Move test data to locations expected by FileDiffingTests. * Fix up some IDE C#7 code cleanups. * Use FileDiffingTests helper. * Fix bug in FileDiffingTests that caused a test failure. * Remove default-valued argument from a call to RunTest. * Create basic files array Does not yet have fileIndex, parentIndex, or response file handling. * Revert incorrect change in FileDiffingTests. * Fix one unit test with spot fix to "expected" file. * Fix up some C#7 IDE warnings * Force update in FileDiffing tests to avoid deserialization errors from out of date "expected" files. * Fix missing "modified" flag sets in PreRelCompatTransformer. * Populate fileIndex in run.files array. * Fix unit test by fixing fileLocation creation. * Restore response file handling. * Populate fileIndex on fileLocations as appropriate. * Fix last test failure by reworking response file handling. * Feedback: Introduce transformer helper PopulatePropertyIfAbsent. * Feedback: Tighten platform-independent string compare. Also: - Reformat unit test lines. * Feedbakc: Revert FileDiffingTest change; downgrade affected test files to provoke transform * Basic rules transformation (except for v1 <-> v2 conversion) (#1223) * Basic rules transformation (except for v1 <-> v2 conversion) * Respond to very excellent PR feedback. * PR feedback * Add files array tests for nested files and uriBaseIds (#1226) * Add failing v1 -> v2 nested files test * Fix existing mis-handling of analysisTarget and resultFile. * Get nested files test case working. * Add failing v1 => v2 uriBaseId test. * Populate v2 uriBaseId. * Fix up expected v2 fileLocation.properties: test passes. * Enhance uriBaseIds test case. * Implement v2->v1 conversion for rules dictionary (#1228) * Notification rule index (#1229) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Notification rule index (#1230) * Add notification.ruleIndex and increase flatten messages testing * Notification message tests + add notification.ruleIndex to schema * Missed feedback from previous PR (#1231) * Implement v1->v2 conversion for rules dictionary (#1232) * Partial implementation * Get last test working. * Somebody missed checking in a generated file. * Schema changes from TC #30 (#1233) * Add source language, fix rank default. * Adjust rank minimum to accommoodate default. * Fix broken test. * Remove unnecessary None items from project file. * PR feedback * Files array results matching (#1234) * WIP preliminary results matching * Restore results matching for files array * Add back autogenerated (unused) namespace directive * Updated release notes for TC30 changes. (#1240) * Mention rules array change in release history. (#1243) * Baseline states (#1245) * Add 'updated' state to baselineState and rename 'existing' to 'unchanged' * Update prerelease transformer * Enable appveyor build + test. Correct version constant. * Update test. Respond to PR feedback. * Fix #1251 #1252 #1253 (#1254) * Fixes + test coverage + cleanup * Update SDK version * Update version in test assets * Fix multitool nuspec (#1256) * Revert unintentional change to BaselineState (#1262) The `develop` branch should match TC #30, but we inadvertently introduced a change from TC #31: replacing `BaselineState.Existing` with `.Unchanged` and `Updated`. I did not revert the entire change. Some things (like having AppVeyor build the `tc-31` branch instead of the defunct `files-array` branch, and some C# 7 updates to the `PrereleaseCompatibilityTransformer`) were good, and I kept them. Also: - Update the version to `2019-01-09` in preparation for merge to `master`. * Transfer Bogdan's point fix (analysisTarget handling) from master to develop (#1263) In preparation for merging `develop` to `master` for the publication of version 2019-01-09 (TC #30), we apply the recent changes in `master` to the `develop` branch. These changes fixed two bugs in the handling of `analysisTarget` in the v1-to-v2 converter (`SarifVersionOneToCurrentVisitor`). Now `develop` is completely up to date, and when we merge `develop` to `master`, we _should_ be able to simply take the "incoming" changes on all conflicting files. * Cherry-pick: v1 transformer analysis target region persistence fix. (#1238) * Mention NuGet publishing changes in release history. * Cherry pick: Don't emit v2 analysisTarget if there is no v1 resultFile. (#1247)

Larry Golding added 9 commits January 21, 2019 09:09

Schema: run.results not unique; add notification.ruleIndex.

76feeec

Schema: Fix version number in title.

0f346de

Set up OneRuleWithRules test to fail.

be72499

Populate v1 ruleId.

c82cd69

Fix tests: Don't adjust level "open" until rules is an array.

841b088

Synthesize keys on rule id

46c273d

Construct v1 rules dictionary; "OneRunWithRules" test passes.

8ab228c

Get TwoResultsWithFixes test working

21c4f9b

PreRelCompatTrans loses the rules dictionary key, so we shouldn't expect it in the output.

Rebaseline one test to avoid spurious file ordering difference.

e2ce115

ghost requested a review from michaelcfanning January 21, 2019 19:16

ghost commented Jan 21, 2019

View reviewed changes

src/Sarif/Writers/PrereleaseCompatibilityTransformer.cs Show resolved Hide resolved

ghost commented Jan 21, 2019

View reviewed changes

Add a comment.

713f359

michaelcfanning reviewed Jan 21, 2019

View reviewed changes

michaelcfanning requested changes Jan 21, 2019

View reviewed changes

Incorporate PR feedback.

f66d0b2

ghost merged commit 0f25460 into files-array Jan 22, 2019

ghost deleted the users/lgolding/files-array-rules-v2-to-v1 branch January 22, 2019 01:47

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement v2->v1 conversion for rules dictionary #1228

Implement v2->v1 conversion for rules dictionary #1228

ghost commented Jan 21, 2019

ghost Jan 21, 2019 •

edited by ghost

Loading

ghost Jan 21, 2019 •

edited by michaelcfanning

Loading

ghost Jan 21, 2019 •

edited by michaelcfanning

Loading

ghost Jan 21, 2019 •

edited by michaelcfanning

Loading

michaelcfanning Jan 21, 2019

michaelcfanning Jan 21, 2019

ghost Jan 22, 2019

michaelcfanning Jan 21, 2019

ghost Jan 22, 2019

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

ghost Jan 22, 2019

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

ghost Jan 22, 2019

ghost Jan 22, 2019

ghost Jan 22, 2019

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

ghost Jan 22, 2019

ghost Jan 22, 2019

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

ghost Jan 22, 2019

michaelcfanning left a comment

Implement v2->v1 conversion for rules dictionary #1228

Implement v2->v1 conversion for rules dictionary #1228

Conversation

ghost commented Jan 21, 2019

ghost Jan 21, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jan 21, 2019 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Jan 21, 2019 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

ghost Jan 21, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 21, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 21, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Jan 21, 2019 • 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

Choose a reason for hiding this comment

michaelcfanning Jan 21, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Jan 21, 2019 • 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 Jan 21, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

ghost Jan 21, 2019 •

edited by ghost

Loading

ghost Jan 21, 2019 •

edited by michaelcfanning

Loading

ghost Jan 21, 2019 •

edited by michaelcfanning

Loading

ghost Jan 21, 2019 •

edited by michaelcfanning

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading

michaelcfanning Jan 21, 2019 •

edited by ghost

Loading