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 13 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
20 changes: 16 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
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>
8 changes: 3 additions & 5 deletions src/Sarif.Driver/Sdk/CommandBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,9 @@ public abstract class CommandBase

protected IFileSystem FileSystem { get; set; }

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.
protected static bool ValidateNonNegativeCommandLineOption<T>(long optionValue, string optionName)
{
bool valid = true;
Expand Down
98 changes: 89 additions & 9 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,29 @@ public int Run(RewriteOptions options)
Stopwatch w = Stopwatch.StartNew();

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

SarifLog actualLog = ReadSarifFile<SarifLog>(_fileSystem, options.InputFilePath);
if (!valid)
{
return FAILURE;
}

string actualOutputPath = CommandUtilities.GetTransformedOutputFileName(options);

SarifLog actualLog = null;

string inputVersion = SniffVersion(options.InputFilePath);
if (!inputVersion.Equals(SarifUtilities.StableSarifVersion))
{
// Transform file and write to output path. Then continue rewriting as before.
// TODO: Consolidate this so only a single write is necessary.
TransformFileToVersionTwo(options, inputVersion, actualOutputPath);
// We wrote to the output path, so read from it again.
actualLog = ReadSarifFile<SarifLog>(_fileSystem, actualOutputPath);
}
else
{
// Read from the original input path.
actualLog = ReadSarifFile<SarifLog>(_fileSystem, options.InputFilePath);
}

OptionallyEmittedData dataToInsert = options.DataToInsert.ToFlags();
OptionallyEmittedData dataToRemove = options.DataToRemove.ToFlags();
Expand All @@ -38,9 +64,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 +90,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.
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
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 void TransformFileToVersionTwo(SingleFileOptionsBase options, string inputVersion, string outputFilePath)
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.

void [](start = 16, length = 4)

can you return SarifLog instead of void?
in the if, you could return visitor.SarifLog

remove the else and return JsonConvert.... to SarifLog.
With this, we won't need to save in disk #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

i would change from options to the sarif string to facilitate, since we won't need to use the options


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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


In reply to: 564742595 [](ancestors = 564742595,564740510)

{
if (inputVersion == "1.0.0")
{
// Converting version 1 to version 2
SarifLogVersionOne actualLog = ReadSarifFile<SarifLogVersionOne>(_fileSystem, options.InputFilePath, SarifContractResolverVersionOne.Instance);
var visitor = new SarifVersionOneToCurrentVisitor();
visitor.VisitSarifLogVersionOne(actualLog);
WriteSarifFile(_fileSystem, visitor.SarifLog, outputFilePath, options.Minify);
}
else
{
// Converting prerelease version 2 to version 2
PrereleaseCompatibilityTransformer.UpdateToCurrentVersion(
_fileSystem.FileReadAllText(options.InputFilePath),
formatting: options.Formatting,
out string sarifText);

_fileSystem.FileWriteAllText(outputFilePath, sarifText);
}
}
}
}
152 changes: 0 additions & 152 deletions src/Sarif.Multitool.Library/TransformCommand.cs

This file was deleted.

3 changes: 1 addition & 2 deletions src/Sarif.Multitool/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,12 @@ public static int Main(string[] args)
(ExportValidationConfigurationOptions options) => new ExportValidationConfigurationCommand().Run(options),
(ExportValidationRulesMetadataOptions options) => new ExportValidationRulesMetadataCommand().Run(options),
(FileWorkItemsOptions fileWorkItemsOptions) => new FileWorkItemsCommand().Run(fileWorkItemsOptions),
(ResultMatchingOptions baselineOptions) => new ResultMatchingCommand().Run(baselineOptions),
(MergeOptions mergeOptions) => new MergeCommand().Run(mergeOptions),
(PageOptions pageOptions) => new PageCommand().Run(pageOptions),
(QueryOptions queryOptions) => new QueryCommand().Run(queryOptions),
(RebaseUriOptions rebaseOptions) => new RebaseUriCommand().Run(rebaseOptions),
(ResultMatchingOptions baselineOptions) => new ResultMatchingCommand().Run(baselineOptions),
Copy link
Collaborator

@eddynaka eddynaka Jan 21, 2021

Choose a reason for hiding this comment

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

ResultMatchingCommand [](start = 63, length = 21)

I didn't set in the alphabetical order because the resultmatching is equal match-results-foward command. #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.

Would it be worthwhile to rename the file/class to something like "MatchResultOptions" for consistency?


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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do it in another pr, separating what we are doing right now :)


In reply to: 561480791 [](ancestors = 561480791,561433654)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


In reply to: 564739449 [](ancestors = 564739449,561480791,561433654)

(RewriteOptions rewriteOptions) => new RewriteCommand().Run(rewriteOptions),
(TransformOptions transformOptions) => new TransformCommand().Run(transformOptions),
(ValidateOptions validateOptions) => new ValidateCommand().Run(validateOptions),
_ => HandleParseError(args));
}
Expand Down
1 change: 1 addition & 0 deletions src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ 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" ?
return null;
}

Expand Down
2 changes: 2 additions & 0 deletions src/Sarif/Writers/ConsoleLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ namespace Microsoft.CodeAnalysis.Sarif.Writers
{
public class ConsoleLogger : BaseLogger, IAnalysisLogger
{
// TODO: We directly instantiate this logger in two classes, creating
// unamanged dependencies. Fix this pattern with dependency injection or a factory.
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.

I don't think we have to worry in this. Since the lifetime of the use is a command, the unmanaged resource will go away when it finishes. #ByDesign

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, created an issue for this.


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

public ConsoleLogger(bool quietConsole, string toolName, IEnumerable<FailureLevel> levels = null, IEnumerable<ResultKind> kinds = null) : base(levels, kinds)
{
_quietConsole = quietConsole;
Expand Down
Loading