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

#2227 Move Transform functionality into Rewrite #2252

Merged
merged 24 commits into from
Jan 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
47e9796
Move some code from TransformCommand to DriverExtensionMethods and co…
jameswinkler Jan 20, 2021
ac7ea6e
Refatoring, add a bunch of functionality to RewriteCommand
jameswinkler Jan 21, 2021
ff37584
Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite
eddynaka Jan 21, 2021
5050c7f
Add some comments, new method "TransformationNecessary"
jameswinkler Jan 21, 2021
9bea969
Merge branch 'users/v-jwinkler/#2227_MergeTransformRewrite' of https:…
jameswinkler Jan 21, 2021
cc56480
Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite
michaelcfanning Jan 22, 2021
1763489
Checkin preliminary changes, 4 unit tests fail because Rewrite can't …
jameswinkler Jan 22, 2021
8067797
Merge branch 'users/v-jwinkler/#2227_MergeTransformRewrite' of https:…
jameswinkler Jan 22, 2021
e06c072
Merge main -> #2227_MergeTranformRewrite
jameswinkler Jan 25, 2021
17af1e5
Transform to version 2 first, rewrite, then if needed, transform to v…
jameswinkler Jan 25, 2021
8b700de
Address failing unit tests, refactor per discussions. Now transformi…
jameswinkler Jan 26, 2021
188db02
Delete transform command entirely
jameswinkler Jan 26, 2021
79ca0bc
One last comment
jameswinkler Jan 26, 2021
d537617
Restore constructor
jameswinkler Jan 26, 2021
405c18d
Default to current version, undo reordering of commands, and consolid…
jameswinkler Jan 26, 2021
74c7c74
Fix formatting
jameswinkler Jan 26, 2021
263e636
Create issues for TODOs
jameswinkler Jan 26, 2021
2d348c3
Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite
jameswinkler Jan 26, 2021
8dc8f9b
Add info to ReleaseHistory
jameswinkler Jan 26, 2021
4066d45
Add one more issue for TODO
jameswinkler Jan 26, 2021
f6efc3e
Remove redundant assignments
jameswinkler Jan 26, 2021
a06f8e4
correct release history
jameswinkler Jan 26, 2021
01d26b9
Update ReleaseHistory.md
eddynaka Jan 26, 2021
8bf66a5
Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite
eddynaka Jan 28, 2021
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
2 changes: 2 additions & 0 deletions src/ReleaseHistory.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# SARIF Package Release History (SDK, Driver, Converters, and Multitool)

* BREAKING: Move `transform` functionality into `rewrite` and delete redundant `transform` command. [#2252](https://github.com/microsoft/sarif-sdk/pull/2252)

## **v2.4.0** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.0) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.0) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.0) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.0) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.0)
* BREAKING: Entirely remove `verbose` whose fuctionality has been replaced by `--level` and `--kind`. [#2241](https://github.com/microsoft/sarif-sdk/pull/2241)
* BREAKING: Rename `LoggingOptions` to `LogFilePersistenceOptions`. [#2241](https://github.com/microsoft/sarif-sdk/pull/2241)
Expand Down
21 changes: 17 additions & 4 deletions src/Sarif.Driver/DriverExtensionMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,24 @@ public static LogFilePersistenceOptions ConvertToLogFilePersistenceOptions(this
/// </returns>
public static bool Validate(this SingleFileOptionsBase options)
{
bool valid = true;
if (options.SarifOutputVersion == SarifVersion.Unknown)
{
// Parsing the output version failed and the the enum evaluated to 0.
Console.WriteLine(DriverResources.ErrorInvalidTransformTargetVersion);
return false;
}

valid &= options.ValidateOutputLocationOptions();
valid &= options.ValidateOutputFormatOptions();
if (!options.ValidateOutputLocationOptions())
{
return false;
}

return valid;
if (!options.ValidateOutputFormatOptions())
{
return false;
}

return true;
}

private static bool ValidateOutputLocationOptions(this SingleFileOptionsBase options)
Expand Down Expand Up @@ -139,6 +151,7 @@ public static bool ValidateOutputOptions(this MultipleFilesOptionsBase options)
bool valid = true;

// TODO: Is it correct to modify options in a "validate" method?
// #2267 https://github.com/microsoft/sarif-sdk/issues/2267
if (options.Inline)
{
options.Force = true;
Expand Down
9 changes: 9 additions & 0 deletions src/Sarif.Driver/DriverResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Sarif.Driver/DriverResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -184,4 +184,7 @@
<data name="OptionValueMustBeNonNegative" xml:space="preserve">
<value>The value of the '{0}' option must be greater than or equal to zero.</value>
</data>
<data name="ErrorInvalidTransformTargetVersion" xml:space="preserve">
<value>Target version must be 1 or 2</value>
</data>
</root>
7 changes: 7 additions & 0 deletions src/Sarif.Driver/Sdk/CommandBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,18 @@ public abstract class CommandBase

protected IFileSystem FileSystem { get; set; }

// TODO: Removing this constructor broke the one of AbsoluteUriCommand entirely but all unit tests were passing
// Add unit tests that will break when this constructor is deleted or malfunctioning.
// #2268 https://github.com/microsoft/sarif-sdk/issues/2268
public CommandBase(IFileSystem fileSystem = null)
Copy link
Collaborator

@eddynaka eddynaka Jan 26, 2021

Choose a reason for hiding this comment

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

public CommandBase(IFileSystem fileSystem = null) [](start = 8, length = 49)

don't remove this. For now, this is the way we initialize the FileSytem. If you check, that is used in MultifileCommandBase, calling the FileSystem directly. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


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

{
this.FileSystem = fileSystem ?? Sarif.FileSystem.Instance;
}

// TODO: What's the point of having a bunch of static methods in an abstract class?
// We even have a static class, "CommandUtilities" which seems like the more appropriate
// place for these to go.
// #2269 https://github.com/microsoft/sarif-sdk/issues/2269
protected static bool ValidateNonNegativeCommandLineOption<T>(long optionValue, string optionName)
{
bool valid = true;
Expand Down
2 changes: 1 addition & 1 deletion src/Sarif.Driver/Sdk/CommonOptionsBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public class CommonOptionsBase
HelpText =
"The SARIF version of the output log file. Valid values are OneZeroZero and Current",
Default = SarifVersion.Current)]
public SarifVersion SarifOutputVersion { get; set; }
public SarifVersion SarifOutputVersion { get; set; } = SarifVersion.Current;

[Option(
"threads",
Expand Down
91 changes: 83 additions & 8 deletions src/Sarif.Multitool.Library/RewriteCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,15 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;

using Microsoft.CodeAnalysis.Sarif.Driver;
using Microsoft.CodeAnalysis.Sarif.Readers;
using Microsoft.CodeAnalysis.Sarif.VersionOne;
using Microsoft.CodeAnalysis.Sarif.Visitors;
using Microsoft.CodeAnalysis.Sarif.Writers;

using Newtonsoft.Json;

namespace Microsoft.CodeAnalysis.Sarif.Multitool
{
Expand All @@ -27,9 +33,24 @@ public int Run(RewriteOptions options)
Stopwatch w = Stopwatch.StartNew();

bool valid = ValidateOptions(options);
if (!valid) { return FAILURE; }
if (!valid)
{
return FAILURE;
}

string actualOutputPath = CommandUtilities.GetTransformedOutputFileName(options);

SarifLog actualLog = null;

SarifLog actualLog = ReadSarifFile<SarifLog>(_fileSystem, options.InputFilePath);
string inputVersion = SniffVersion(options.InputFilePath);
if (!inputVersion.Equals(SarifUtilities.StableSarifVersion))
{
actualLog = TransformFileToVersionTwo(options.InputFilePath, inputVersion);
}
else
{
actualLog = ReadSarifFile<SarifLog>(_fileSystem, options.InputFilePath);
}

OptionallyEmittedData dataToInsert = options.DataToInsert.ToFlags();
OptionallyEmittedData dataToRemove = options.DataToRemove.ToFlags();
Expand All @@ -38,9 +59,17 @@ public int Run(RewriteOptions options)
SarifLog reformattedLog = new RemoveOptionalDataVisitor(dataToRemove).VisitSarifLog(actualLog);
reformattedLog = new InsertOptionalDataVisitor(dataToInsert, originalUriBaseIds).VisitSarifLog(reformattedLog);

string fileName = CommandUtilities.GetTransformedOutputFileName(options);
if (options.SarifOutputVersion == SarifVersion.OneZeroZero)
{
var visitor = new SarifCurrentToVersionOneVisitor();
visitor.VisitSarifLog(reformattedLog);

WriteSarifFile(_fileSystem, reformattedLog, fileName, options.Minify);
WriteSarifFile(_fileSystem, visitor.SarifLogVersionOne, actualOutputPath, options.Minify, SarifContractResolverVersionOne.Instance);
}
else
{
WriteSarifFile(_fileSystem, reformattedLog, actualOutputPath, options.Minify);
}

w.Stop();
Console.WriteLine($"Rewrite completed in {w.Elapsed}.");
Expand All @@ -56,13 +85,59 @@ public int Run(RewriteOptions options)

private bool ValidateOptions(RewriteOptions rewriteOptions)
{
bool valid = true;
if (!rewriteOptions.Validate())
{
return false;
}

valid &= rewriteOptions.Validate();
// While this is returning true for inline cases, I think it's doing so for the wrong reasons.
// TODO: validate whether "actualOutputPath" can be created.
// #2270 https://github.com/microsoft/sarif-sdk/issues/2270
if (!DriverUtilities.ReportWhetherOutputFileCanBeCreated(rewriteOptions.OutputFilePath, rewriteOptions.Force, _fileSystem))
{
return false;
}

valid &= DriverUtilities.ReportWhetherOutputFileCanBeCreated(rewriteOptions.OutputFilePath, rewriteOptions.Force, _fileSystem);
return true;
}

return valid;
// TODO Move this into a separate class for better unit testing
// #2271 https://github.com/microsoft/sarif-sdk/issues/2271
private string SniffVersion(string sarifPath)
{
using (JsonTextReader reader = new JsonTextReader(new StreamReader(_fileSystem.FileOpenRead(sarifPath))))
{
while (reader.Read())
{
if (reader.TokenType == JsonToken.PropertyName && ((string)reader.Value).Equals("version"))
{
reader.Read();
return (string)reader.Value;
}
}
}

return null;
}

private SarifLog TransformFileToVersionTwo(string inputFilePath, string inputVersion)
{
if (inputVersion == "1.0.0")
{
// Converting version 1 to version 2
SarifLogVersionOne actualLog = ReadSarifFile<SarifLogVersionOne>(_fileSystem, inputFilePath, SarifContractResolverVersionOne.Instance);
var visitor = new SarifVersionOneToCurrentVisitor();
visitor.VisitSarifLogVersionOne(actualLog);
return visitor.SarifLog;
}
else
{
// Converting prerelease version 2 to version 2
return PrereleaseCompatibilityTransformer.UpdateToCurrentVersion(
_fileSystem.FileReadAllText(inputFilePath),
formatting: Formatting.None,
out string _);
}
}
}
}
152 changes: 0 additions & 152 deletions src/Sarif.Multitool.Library/TransformCommand.cs

This file was deleted.

1 change: 0 additions & 1 deletion src/Sarif.Multitool/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ public static int Main(string[] args)
(QueryOptions queryOptions) => new QueryCommand().Run(queryOptions),
(RebaseUriOptions rebaseOptions) => new RebaseUriCommand().Run(rebaseOptions),
(RewriteOptions rewriteOptions) => new RewriteCommand().Run(rewriteOptions),
(TransformOptions transformOptions) => new TransformCommand().Run(transformOptions),
(ValidateOptions validateOptions) => new ValidateCommand().Run(validateOptions),
_ => HandleParseError(args));
}
Expand Down
2 changes: 2 additions & 0 deletions src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public override SarifLog VisitSarifLog(SarifLog v2SarifLog)
SarifLogVersionOne.Runs.Add(CreateRunVersionOne(v2Run));
}

// TODO: We always return null here. Is there a pattern that allows us to change the method signature to "void" ?
// #2266 https://github.com/microsoft/sarif-sdk/issues/2266
return null;
}

Expand Down
Loading