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.

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

Preview and Apply action links are disabled in Fixes #13

Closed
ghost opened this issue Aug 20, 2018 · 1 comment
Closed

Preview and Apply action links are disabled in Fixes #13

ghost opened this issue Aug 20, 2018 · 1 comment
Assignees
Labels
bug Something isn't working resolved-fixed

Comments

@ghost
Copy link

ghost commented Aug 20, 2018

Repro:

  1. Build and run the sarif-sdk sample application:
SarifSdkSample.exe create Test.sarif
  1. Open Test.sarif in the viewer.
  2. In the Error List Window, click on the issue CA1820.
  3. In the SARIF Explorer Window, click Fixes.
  4. Expand the tree nodes "Replace empty string with test for zero length", "Analysis Sample.cs"

Expected: The Preview and Apply links are enabled and work properly.

Actual: The links are disabled.

@ghost ghost added the bug Something isn't working label Aug 20, 2018
@ghost ghost assigned EasyRhinoMSFT Aug 20, 2018
ghost pushed a commit to microsoft/sarif-sdk that referenced this issue Aug 20, 2018
Now that I can build and install the VSIX viewer, I used it to validate the SARIF file produced by the SDK sample app's `create` verb. As a result:

1. I added result message strings, two of which are parameterized with arguments.
2. I corrected the regions for each result.

Also, I noticed and filed two bugs on the viewer's handling of fixes:

microsoft/sarif-visualstudio-extension#12: Fixes don't recognize text-based regions.
microsoft/sarif-visualstudio-extension#13: Preview and Apply action links are disabled in Fixes.
@EasyRhinoMSFT
Copy link
Collaborator

Enabled state is fixed by #14, but functionality is broken. I'll file a new issue for that one.

michaelcfanning added a commit to microsoft/sarif-sdk that referenced this issue Aug 29, 2018
* Adding develop to appveyor (#988)

* Remove unused Sarif.Driver.nuspec. (#990)

* Weak types (#989)

* Correct weakly typed things by providing codegen hints

* Fix up weakly typed items (due to missing codegen hints)

* Fortify converter enhancements (#991)

* Fix converter bug + Viewer menu items

* Populate run.description with "executive summary" text node

* Tweaks

* Use trace frame node text for tFL snippet

* Set file.encoding

* Add (limited) embedded link support

* Final features + tests + PR feedback

* Remove BOM added by VS

* Viewer bug in invocation model population

* Tweaks

* Process text tokens + test updates + PR feedback

* Remove unsupported XML content tags + convert HTML to Markdown

* Unit test for content XML & HTML parsing

* Refactor to get correct region offsets for snippets

* Changing TSLint converter to set result region CharOffset and CharLength (#993)

* Changing TSLint converter to set result region CharOffset and CharLength instead of ByteOffset and ByteLength.

* Updating unit test for TSLint converter to reflect CharOffset and CharLength change.

* Updating TSLintConverter test data to reflect CharOffset and CharLength change.

* Pr notes + flatten notifications (#992)

* PR notes + enable notifications/other message flattening

* Eliminate unnecessary results processing.

* PR feedback.

* Changing TryReconstructAbsoluteUri to respect if the uri is already absolute instead of returning a null uri.  Fixing TSLintConverter length calculation to not add 1. (#994)

* Update all packages to meet nuget ship requirements (#995)

* Fix UpdateBaselines + issue #934

Also persist file length property

* PR feedback

* Add ignored autogen files (#1000)

* Addressing lgolding feedback on a PR completed earlier.

* spelling fix

* Adressing a nit.

* Add option to RebaseUri verb to allow rebase of all relative uris to provided path (#1001)

* Adding optional RebaseUri multitool option to rebase existing relative uris as well as absolute uris with the provided root.

* Rebasing the Files list when rebasing result location uris to relative a base uri.

* Fixing rebaseuri stage test to be in line with new logic.

* Fixing rebaseuri unittest to account for new logic.

* Removing dead code in RebaseUri visitor based on PR feedback.

* Do not serialize empty message string dictionaries (#1005)

* CSD.1 package changes (#1007)

* Restore three more validation rules and remove one (#1009)

This change restores the following validation rules:

- `SARIF1006`: Hash algorithms must be unique.
- `SARIF1007`: End time must be after start time.
- `SARIF1008`: Messages should end with period.

It also removes the following rule:

- `SARIF1011`: Importance most appear only on code flow locations.
    This rule was necessary because `annotatedCodeLocations` appeared in many places, but some of their properties, including the `importance` property, were only applicable to code flows. Now, with the [`location` object redesign ](oasis-tcs/sarif-spec#130), the `annotatedCodeLocation` object is gone, all but one of the places where it was previously used now use the improved general-purpose `location` object, and `importance` only appears on the single-purpose `threadFlowLocation` object.

* Restore the functionality of DelistCurrentPackages.cmd (#1010)

This change implements a PowerShell script `DelistCurrentPackages.ps1` that does just what `DelistCurrentPackages.cmd` used to do. `DelistCurrentPackages.cmd` still exists, but it just invokes `DelistCurrentPackages.ps1`.

Implementing the script in PowerShell allows us to take advantage of the PowerShell-based machinery I implemented for BuildAndTest, including:

- Extracting version information from the same location (`build.props`) that the build uses.
- Taking advantage of utilities such as `Exit-WithFailureMessage` and `Write-CommandLine` to ensure a uniform user experience with the scripts.

**NOTE** I have not actually delisted the v1.7.5 packages. I tested this change by temporarily commenting out the line that issues the "`nuget delete`" command, and observing (by way of `Write-CommandLine`) that the correct commands would be executed.

* Add v2 BinSkim files (#1011)

* Remove VSIX code and build from tree (#1012)

* Remove VSIX code and build from tree

* Remove env from appveyor config

* Add import for nuget utilities

* Remove use of [Theory] which greatly reduces test execution time (#1013)

* Fixes for issues #1016 and #1017

* PR feedback & cleanup

* Removing incorrect baseuri being set by AndroidStudio converter.  Upd… (#1021)

* Removing incorrect baseuri being set by AndroidStudio converter.  Updating corresponding test data.  Adding new AndroidStudio test data and readme on how to generate. Added PyLint and TSLing test data and readme on how to generate.

* Rermove errant commas from calls to Get-ProjectBinDirectory

* Restore three more validation rules and remove one (#1018)

This change restores the following validation rules:

- `SARIF1003`: URI must be valid.
- `SARIF1009`: Step values must form one-based sequence.
- `SARIF1014`: `UriBaseId` requires relative URI.

It also removes the following rule:

- `SARIF1010`: Step must appear only on code flow locations.
    This rule was necessary because `annotatedCodeLocations` appeared in many places, but some of their properties, including the `step` property, were only applicable to code flows. Now, with the [`location` object redesign ](oasis-tcs/sarif-spec#130), the `annotatedCodeLocation` object is gone, all but one of the places where it was previously used now use the improved general-purpose `location` object, and `step` only appears on the single-purpose `threadFlowLocation` object.

This description might look very similar to a previous PR that also restored three rules and remove one. Don't worry, this is a different set of rules.

After this, there is only one rule left to restore. I could have done it in this PR but it's already too big and it's the end of the day.

* Restore last rule: UseAbsolutePathsForNestedFileUriFragments (#1023)

As of this change, the v2 Multitool `validate` command is at parity with the old v1 validator tool.

Just as was the case in the previous PR, where we restored the rule `UrisMustBeValid`, we require little test coverage for this rule. We need only verify that it is checked for URIs that appear as the value of the `fileLocation.uri` property, and for URIs that appear as property names in the `run.files` dictionary.

The exhaustive test coverage for the rule `UriBaseIdRequiresRelativeUri` ensures that we actually visit every `fileLocation` object in the schema. So for any other rule that validates any aspect of a `fileLocation` object, we only require one test case -- any SARIF file that contains a `fileLocation` object anywhere will do.

* Setting version and schema in merge command to 2.0 (#1026)

* Setting version and schema in merge command to 2.0.

* Setting version and schema in merge command to 2.0.

* Restore the SDK sample to working order. (#1024)

Add a step to the build so that the sample build doesn't regress again.

* Make CreateNotifications strongly typed to prevent silent use-facing string construction woes. (#1025)

* Build sample solution in appveyor (#1027)

* Build sample solution in appveyor

For an explanation of the technique, see https://help.appveyor.com/discussions/questions/833-target-multiple-solution-files.

* Restore packages for sample solution before build.

* Build with BuildAndTest

* Fix bug on BuildAndTest.ps1 command line.

* Restore NuGet packages for samples in BeforeBuild.

* Respect NuGet verbosity on both nuget.exe and dotnet restore.

* Fix build breaks related to SDK sample project. (#1029)

* Rename ContextCodeSnippet to ContextRegionSnippet (to match associate… (#1028)

* Rename ContextCodeSnippet to ContextRegionSnippet (to match associated SARIF property name)

* Copy test files

* Adding CppCheck test data with regen readme. (#1032)

* Add result message strings and correct the regions. (#1035)

Now that I can build and install the VSIX viewer, I used it to validate the SARIF file produced by the SDK sample app's `create` verb. As a result:

1. I added result message strings, two of which are parameterized with arguments.
2. I corrected the regions for each result.

Also, I noticed and filed two bugs on the viewer's handling of fixes:

microsoft/sarif-visualstudio-extension#12: Fixes don't recognize text-based regions.
microsoft/sarif-visualstudio-extension#13: Preview and Apply action links are disabled in Fixes.

* Build the SDK sample, but only on the desktop (#1036)

On AppVeyor, the invocation of `NuGet restore` results in a null reference exception for reasons we don't yet understand. So in AppVeyor, disable the building of the sample.

*grin* to MikeFan, who asked me of what use the "-NoBuildSample" switch might be ;-)

Also:
- Add a bunch of build-related files to the solution so we can edit them more easily in VS.
- Remove now-unused native.props, and merge build.common.props into build.props, because we no longer have any "old-style" projects in our solution (they moved into the sarif-visualstudio-extension repo).

* Remove on up-directory

* Enhance comment

* Comment adjustment

* Remove another updir

* Add new validation rule UriMustBeAbsolute (#1040)

Also:
- Fix appveyor email notification address.

* Fix script bug to allow SDK sample to build in AppVeyor (#1041)

I had neglected to specify the package output directory on the "NuGet.exe restore" command line .

* Remove identical duplicate NuGet.config. (#1042)

* Fix build break in VS due to collision on VersionConstants.cs (#1043)

In VS, it seems, the two platform versions of the Sarif project (net461 and netstandard20) are built simultaneously. Sometimes that results in a collision in the attempt to create `VersionConstants.cs`.

Instead, create it in `BeforeBuild.ps1`. This makes life a little more complicated, because now we have to read a property value from `build.props` instead of letting MSBuild do it for us. Note that `Get-VersionConstants` was already doing that, so we already had code that demonstrated the pattern.

We perform a little refactoring to DRY out a couple of Powershell variables that are now used in multiple places.

If we only built for one platform -- netstandard2.0, as @Evmaus-MS has suggested -- we wouldn't have hit this problem. Still, it's not a bad idea to move this into `BeforeBuild.ps1` in case we ever do in future build multiple platforms for flavors simultaneously (such as a Debug and a Release build).

* FxCop converter improvements (#1044)

* csd1.0.1 RTM (#1030)

* Adding develop to appveyor (#988)

* Remove unused Sarif.Driver.nuspec. (#990)

* Weak types (#989)

* Correct weakly typed things by providing codegen hints

* Fix up weakly typed items (due to missing codegen hints)

* Fortify converter enhancements (#991)

* Fix converter bug + Viewer menu items

* Populate run.description with "executive summary" text node

* Tweaks

* Use trace frame node text for tFL snippet

* Set file.encoding

* Add (limited) embedded link support

* Final features + tests + PR feedback

* Remove BOM added by VS

* Viewer bug in invocation model population

* Tweaks

* Process text tokens + test updates + PR feedback

* Remove unsupported XML content tags + convert HTML to Markdown

* Unit test for content XML & HTML parsing

* Refactor to get correct region offsets for snippets

* Changing TSLint converter to set result region CharOffset and CharLength (#993)

* Changing TSLint converter to set result region CharOffset and CharLength instead of ByteOffset and ByteLength.

* Updating unit test for TSLint converter to reflect CharOffset and CharLength change.

* Updating TSLintConverter test data to reflect CharOffset and CharLength change.

* Pr notes + flatten notifications (#992)

* PR notes + enable notifications/other message flattening

* Eliminate unnecessary results processing.

* PR feedback.

* Changing TryReconstructAbsoluteUri to respect if the uri is already absolute instead of returning a null uri.  Fixing TSLintConverter length calculation to not add 1. (#994)

* Update all packages to meet nuget ship requirements (#995)

* Fix UpdateBaselines + issue #934

Also persist file length property

* PR feedback

* Add ignored autogen files (#1000)

* Addressing lgolding feedback on a PR completed earlier.

* spelling fix

* Adressing a nit.

* Add option to RebaseUri verb to allow rebase of all relative uris to provided path (#1001)

* Adding optional RebaseUri multitool option to rebase existing relative uris as well as absolute uris with the provided root.

* Rebasing the Files list when rebasing result location uris to relative a base uri.

* Fixing rebaseuri stage test to be in line with new logic.

* Fixing rebaseuri unittest to account for new logic.

* Removing dead code in RebaseUri visitor based on PR feedback.

* Do not serialize empty message string dictionaries (#1005)

* CSD.1 package changes (#1007)

* Restore three more validation rules and remove one (#1009)

This change restores the following validation rules:

- `SARIF1006`: Hash algorithms must be unique.
- `SARIF1007`: End time must be after start time.
- `SARIF1008`: Messages should end with period.

It also removes the following rule:

- `SARIF1011`: Importance most appear only on code flow locations.
    This rule was necessary because `annotatedCodeLocations` appeared in many places, but some of their properties, including the `importance` property, were only applicable to code flows. Now, with the [`location` object redesign ](oasis-tcs/sarif-spec#130), the `annotatedCodeLocation` object is gone, all but one of the places where it was previously used now use the improved general-purpose `location` object, and `importance` only appears on the single-purpose `threadFlowLocation` object.

* Restore the functionality of DelistCurrentPackages.cmd (#1010)

This change implements a PowerShell script `DelistCurrentPackages.ps1` that does just what `DelistCurrentPackages.cmd` used to do. `DelistCurrentPackages.cmd` still exists, but it just invokes `DelistCurrentPackages.ps1`.

Implementing the script in PowerShell allows us to take advantage of the PowerShell-based machinery I implemented for BuildAndTest, including:

- Extracting version information from the same location (`build.props`) that the build uses.
- Taking advantage of utilities such as `Exit-WithFailureMessage` and `Write-CommandLine` to ensure a uniform user experience with the scripts.

**NOTE** I have not actually delisted the v1.7.5 packages. I tested this change by temporarily commenting out the line that issues the "`nuget delete`" command, and observing (by way of `Write-CommandLine`) that the correct commands would be executed.

* Add v2 BinSkim files (#1011)

* Remove VSIX code and build from tree (#1012)

* Remove VSIX code and build from tree

* Remove env from appveyor config

* Add import for nuget utilities

* Remove use of [Theory] which greatly reduces test execution time (#1013)

* Fixes for issues #1016 and #1017

* PR feedback & cleanup

* Removing incorrect baseuri being set by AndroidStudio converter.  Upd… (#1021)

* Removing incorrect baseuri being set by AndroidStudio converter.  Updating corresponding test data.  Adding new AndroidStudio test data and readme on how to generate. Added PyLint and TSLing test data and readme on how to generate.

* Rermove errant commas from calls to Get-ProjectBinDirectory

* Restore three more validation rules and remove one (#1018)

This change restores the following validation rules:

- `SARIF1003`: URI must be valid.
- `SARIF1009`: Step values must form one-based sequence.
- `SARIF1014`: `UriBaseId` requires relative URI.

It also removes the following rule:

- `SARIF1010`: Step must appear only on code flow locations.
    This rule was necessary because `annotatedCodeLocations` appeared in many places, but some of their properties, including the `step` property, were only applicable to code flows. Now, with the [`location` object redesign ](oasis-tcs/sarif-spec#130), the `annotatedCodeLocation` object is gone, all but one of the places where it was previously used now use the improved general-purpose `location` object, and `step` only appears on the single-purpose `threadFlowLocation` object.

This description might look very similar to a previous PR that also restored three rules and remove one. Don't worry, this is a different set of rules.

After this, there is only one rule left to restore. I could have done it in this PR but it's already too big and it's the end of the day.

* Restore last rule: UseAbsolutePathsForNestedFileUriFragments (#1023)

As of this change, the v2 Multitool `validate` command is at parity with the old v1 validator tool.

Just as was the case in the previous PR, where we restored the rule `UrisMustBeValid`, we require little test coverage for this rule. We need only verify that it is checked for URIs that appear as the value of the `fileLocation.uri` property, and for URIs that appear as property names in the `run.files` dictionary.

The exhaustive test coverage for the rule `UriBaseIdRequiresRelativeUri` ensures that we actually visit every `fileLocation` object in the schema. So for any other rule that validates any aspect of a `fileLocation` object, we only require one test case -- any SARIF file that contains a `fileLocation` object anywhere will do.

* Setting version and schema in merge command to 2.0 (#1026)

* Setting version and schema in merge command to 2.0.

* Setting version and schema in merge command to 2.0.

* Restore the SDK sample to working order. (#1024)

Add a step to the build so that the sample build doesn't regress again.

* Make CreateNotifications strongly typed to prevent silent use-facing string construction woes. (#1025)

* Build sample solution in appveyor (#1027)

* Build sample solution in appveyor

For an explanation of the technique, see https://help.appveyor.com/discussions/questions/833-target-multiple-solution-files.

* Restore packages for sample solution before build.

* Build with BuildAndTest

* Fix bug on BuildAndTest.ps1 command line.

* Restore NuGet packages for samples in BeforeBuild.

* Respect NuGet verbosity on both nuget.exe and dotnet restore.

* Fix build breaks related to SDK sample project. (#1029)

* Rename ContextCodeSnippet to ContextRegionSnippet (to match associate… (#1028)

* Rename ContextCodeSnippet to ContextRegionSnippet (to match associated SARIF property name)

* Copy test files

* FxCop converter improvements

* Update tests. Retrieve exception details for appveyor failure.

* Improve appveyor test output

* Output change

* test outpu

* Alter dictionary insertion behavior

* Remove exception handling code

* Update FxCop test generation readme

* Fix #1047: correct message string creation for rule & resource strings (#1048)

* Fix + add unit test coverage

* Update Driver unit tests

* Add support for resource strings

* Add plain message.text test case

* Remove ruleMessageId stuff for pending OASIS issue #216

* PR feedback

* PR feedback

* Fix issue #1049 GetFirstSentence (#1050)

* Fix GetFirstSentence

* Change sentence pattern to match upcase first letter

* PR feedback

* Typo

* Pick up JSchema references from NuGet. Eliminate result.ruleMessageId (#1052)

* Pick up JSchema references from NuGet. Eliminate result.ruleMessageId

* Remove unnecessary copy step

* Fix sample build break

* Fix #1046: Comparing two identical log file failed (#1056)

The reason was a bad codegen hint that caused `Rule` objects to be compared by reference rather than by using the `RuleEqualityComparer`.

* First Result Matching Implementation Pass (#1034)

* Starting on an initial extensible SARIF driven result matching algorithm

* Some more sarif v2 result matching work on tests.

* Fixed an issue with the identical result matcher--we still want to match if the fields used in baselines are set.

Wrote/fixed some tests.  A number of heuristic result matchers & tests remain.

* Initial results matching algorithm is now working with an identical results matcher.

* Making sure all tests will work in this branch.

* Spacing fix.

* Fixed up the full fingerprint matcher.

* Bugfixes to the result matcher for empty + null arguments and the result output.  Removed a currently broken matcher from the default result matcher.

* Adding some basic result matching properties to the output results.

* Propagate existing result matching properties if we aren't setting them in the result matcher ourselves.

* Whitespace fix, comment.

* Changing where we pull our metadata from if not specified, and adds special handling around propagating a "Found Date" forward.

* Cleanup post sarif v2 merge

* Fixing an issue with the ResultMatchingProperties.

* Further small fixes.

* Addressed the stuff I caught in CR.

* Addressing round 1 of feedback

* Small fix, merge from develop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved-fixed
Projects
None yet
Development

No branches or pull requests

1 participant