Implement smart validation in SarifCli tool #501
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
assigned
| @@ -0,0 +1,25 @@ | |||
| { | |||
There was a problem hiding this comment.
This test file does not violate the rule. #Closed
added 18 commits
August 24, 2016 07:16
The build broke on AppVeyor. I don't know why it built on my machine.
SarifCli had a P2P dependency on Sarif.csproj, which is currently
building version 1.5.28 of Sarif.dll. But SarifCli also has a binary
dependency on a foreign checked in binary (FCIB),
Microsoft.Json.Schema.dll, which in turn took a NuGet dependency on Sarif
version 1.5.25. This broke the build because (for example), the code
tried to assign an object of type "Result from Sarif v1.5.25" to a
variable of type "Result from Sarif v1.5.28".
To fix this, we did the following:
1. Update the JSchema project to take a NuGet dependency on Sarif 1.5.27
(the latest published version).
2. Bump the JSchema version number from 0.44.0 to 0.45.0.
3. Rebuild JSchema, and publish v0.45.0 of its NuGet packages.
4. Update the FCIBs with the v0.45.0 of the JSchema assemblies.
5. Replace the P2P dependencies on Sarif.Sdk and Sarif.Driver with
NuGet dependencies on v1.5.27 of the SARIF packages.
This should fix the problem. Again, this builds on my machine, but we
won't know for sure until the AppVeyor build passes.
added 5 commits
August 29, 2016 10:18
| @@ -31,6 +31,7 @@ test: | |||
| - '**\Sarif.FunctionalTests.dll' | |||
| - '**\Sarif.ValidationTests.dll' | |||
| - '**\Sarif.Viewer.VisualStudio.UnitTests.dll' | |||
| - '**\Microsoft.CodeAnalysis.SarifCli.FunctionalTests.dll' | |||
There was a problem hiding this comment.
Microsoft.CodeAnalysis [](start = 10, length = 22)
Why is this naming convention different? Should be SarifCli.FunctionalTests.dll. #Closed
There was a problem hiding this comment.
added 6 commits
August 29, 2016 16:14
This will let us use IAnalyzeOptions.Quiet in the SarifCli.FunctionalTests.
| { | ||
| ReportInvalidSchemaErrors(ex, schemaFilePath, logger); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Exception ex) [](start = 19, length = 13)
do you need this? doesn't the framework catch unhandled exceptions? one consideration here is the return value of the tool. An unhandled exception should certainly return a non-zero (i.e., non-success code). is that happening? #Closed
There was a problem hiding this comment.
|
|
|
Per our discussion, I'll merge this and address the feedback in a separate PR> #Closed |
ghost
merged commit
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We implement the framework for the validator and its functional tests, and we implement the following rules:
AnnotatedCodeLocationEssentialIsObsoleteAnnotatedCodeLocationIdIsObsoleteDoNotUseFriendlyNameAsRuleIdEndColumnMustNotBeLessThanStartColumnEndLineMustNotBeLessThanStartLineEndTimeMustBeAfterStartTimeHashAlgorithmsMustBeUniqueImportanceMustAppearOnlyInCodeFlowLocationsMessagesShouldEndWithPeriodStepMustAppearOnlyInCodeFlowLocationsStepValuesMustFormOneBasedSequenceUrisMustBeValidUseAbsolutePathsForNestedUriFragmentsSubsequent PRs will implement more rules.
The validator is built on the
Sarif.Driverframework. The validator performs schema validation in addition to the "smart validation" rules.The smart validation rules get their location information as follows:
"/runs/0/rules/SV0001") to represent the logical location of the result.JTokenthat represents the entire document, producing aJTokenthat represents the element specified by the pointer.JToken.We use the JSON Pointer implementation from
Microsoft.Json.Pointer, which is obtained from NuGet along withMicrosoft.Json.Schema. This is the first use of the JSON Pointer implementation.Although a skeleton project for the validator already existed, you should read this as completely new code.
NOTE 1: I ran into the circular dependency thing we've seen before, where a project in sarif-sdk (in this case, the new
SarifCliandSarifCli.FunctionalTestprojects) depends on JSchema, which in turn depends on a different version of sarif-sdk. The comments on commit bf4b015 explain how I fixed it.NOTE 2: I also took the opportunity to fix our dependency on the foreign checked in binary Microsoft.Json.Schema.dll in the
src\Referencesdirectory. Commits 046c98f, d6de920, and c1e2261 removed the FCIB dependencies, replaced them with NuGet dependencies on Microsoft.Json.Schema and Microsoft.Json.Pointer, and removed thesrc\Referencesdirectory.NOTE 3: I took the opportunity to upgrade our dependency on Microsoft.Json.Schema.ToDotNet from version 0.42.0 to the latest 0.45.0 (commit 00a6e30). As a result, all the files in the
Autogenerateddirectory were modified (the version specified in theirGeneratedCodeattributes changed from0.42.0to0.45.0.