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

Fixed #89: allow user to customize rules xml #90

Merged
merged 5 commits into from
Aug 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 30 additions & 0 deletions PluginGenerator/DataModel/DebtRemediationFunctionType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* SonarQube Roslyn SDK
* Copyright (C) 2015-2018 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarQube.Plugins.Roslyn
{
public enum DebtRemediationFunctionType

Choose a reason for hiding this comment

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

SonarC# and SonarVB currently support only CONSTANT_ISSUE so I am not sure if we will be able to provide the other values.

Copy link
Author

Choose a reason for hiding this comment

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

I'll add a comment saying they are unsupported by the C# plugin currently.

{
Unspecified,
LINEAR, // Currently not supported by the C# plugin (v7.3)
LINEAR_OFFSET, // Currently not supported by the C# plugin (v7.3)
CONSTANT_ISSUE
}
}
29 changes: 29 additions & 0 deletions PluginGenerator/DataModel/IssueType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* SonarQube Roslyn SDK
* Copyright (C) 2015-2017 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarQube.Plugins.Roslyn
{
public enum IssueType

Choose a reason for hiding this comment

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

Shall we allow external users to provide the new type (hotspot)?

Copy link
Author

Choose a reason for hiding this comment

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

The plugin in LTS-compatible so I don't think hotspots are available.

{
CODE_SMELL,
BUG,
VULNERABILITY
}
}
33 changes: 33 additions & 0 deletions PluginGenerator/DataModel/Rule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,42 @@ public XmlCDataSection DescriptionAsCDATA
[XmlElement(ElementName = "status")]
public string Status { get; set; }

[XmlElement(ElementName = "type")]
public IssueType Type { get; set; }

[XmlElement(ElementName = "tag")]
public string[] Tags { get; set; }

[XmlIgnore]
public DebtRemediationFunctionType DebtRemediationFunction { get; set; }

/// <summary>
/// Do not read/write this property directly - use <see cref="DebtRemediationFunction"/> instead.
/// </summary>
/// <remarks>This property is a hack that allows us to use an enum to specify
/// debt remediation function, but to have nothing written to the XML if the
/// the value is not specified. There are other ways to do this, but they required
/// much more code.</remarks>
[XmlElement(ElementName = "debtRemediationFunction")]
public string DebtRemediationFunction_DoNotUse_ForSerializationOnly
{
get
{
if (DebtRemediationFunction == DebtRemediationFunctionType.Unspecified)
{
return null;
}
return DebtRemediationFunction.ToString();
}
set
{
this.DebtRemediationFunction = (DebtRemediationFunctionType)Enum.Parse(typeof(DebtRemediationFunctionType), value);

Choose a reason for hiding this comment

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

Isn't it better to do a TryParse and assign Unspecified in case of wrong value?

Copy link
Author

Choose a reason for hiding this comment

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

I think this is ok; we'll fail fast if the user supplies an invalid value.

}
}

[XmlElement(ElementName = "debtRemediationFunctionOffset")]
public string DebtRemediationFunctionOffset { get; set; }

/// <summary>
/// Specified the culture and case when comparing rule keys
/// </summary>
Expand Down
2 changes: 2 additions & 0 deletions PluginGenerator/SonarQube.Plugins.PluginGenerator.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
<Reference Include="System.Xml" />
</ItemGroup>
<ItemGroup>
<Compile Include="DataModel\DebtRemediationFunctionType.cs" />
<Compile Include="DataModel\IssueType.cs" />
<Compile Include="DataModel\Rule.cs" />
<Compile Include="DataModel\Rules.cs" />
<Compile Include="JarManifestBuilder.cs" />
Expand Down
76 changes: 69 additions & 7 deletions RoslynPluginGenerator/AnalyzerPluginGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ public class AnalyzerPluginGenerator
/// </summary>
private static readonly string[] excludedFileExtensions = { ".nupkg", ".nuspec" };

/// <summary>
/// Specifies the format for the name of the template rules xml file
/// </summary>
public const string RulesTemplateFileNameFormat = "{0}.{1}.rules.template.xml";

/// <summary>
/// Specifies the format for the name of the placeholder SQALE file
/// </summary>
Expand Down Expand Up @@ -134,7 +139,7 @@ public bool Generate(ProcessedArgs args)
// Initial run with the user-targeted package and arguments
if (analyzersByPackage.ContainsKey(targetPackage))
{
string generatedJarPath = GeneratePluginForPackage(args.OutputDirectory, args.Language, args.SqaleFilePath, targetPackage, analyzersByPackage[targetPackage]);
string generatedJarPath = GeneratePluginForPackage(args.OutputDirectory, args.Language, args.SqaleFilePath, args.RuleFilePath, targetPackage, analyzersByPackage[targetPackage]);
if (generatedJarPath == null)
{
return false;
Expand All @@ -147,12 +152,12 @@ public bool Generate(ProcessedArgs args)
// Dependent package generation changes the arguments
if (args.RecurseDependencies)
{
logger.LogWarning(UIResources.APG_RecurseEnabled_SQALENotEnabled);
logger.LogWarning(UIResources.APG_RecurseEnabled_SQALEandRuleCustomisationNotEnabled);

foreach (IPackage currentPackage in analyzersByPackage.Keys)
{
// No way to specify the SQALE file for any but the user-targeted package at this time
string generatedJarPath = GeneratePluginForPackage(args.OutputDirectory, args.Language, null, currentPackage, analyzersByPackage[currentPackage]);
// No way to specify the SQALE or rules xml files for any but the user-targeted package at this time
string generatedJarPath = GeneratePluginForPackage(args.OutputDirectory, args.Language, null, null, currentPackage, analyzersByPackage[currentPackage]);
if (generatedJarPath == null)
{
return false;
Expand All @@ -172,7 +177,7 @@ public bool Generate(ProcessedArgs args)
return true;
}

private string GeneratePluginForPackage(string outputDir, string language, string sqaleFilePath, IPackage package, IEnumerable<DiagnosticAnalyzer> analyzers)
private string GeneratePluginForPackage(string outputDir, string language, string sqaleFilePath, string rulesFilePath, IPackage package, IEnumerable<DiagnosticAnalyzer> analyzers)
{
Debug.Assert(analyzers.Any(), "The method must be called with a populated list of DiagnosticAnalyzers.");

Expand All @@ -187,6 +192,7 @@ private string GeneratePluginForPackage(string outputDir, string language, strin
{
Language = language,
SqaleFilePath = sqaleFilePath,
RulesFilePath = rulesFilePath,
PackageId = package.Id,
PackageVersion = package.Version.ToString(),
Manifest = CreatePluginManifest(package)
Expand All @@ -197,10 +203,23 @@ private string GeneratePluginForPackage(string outputDir, string language, strin
definition.SourceZipFilePath = CreateAnalyzerStaticPayloadFile(packageDir, baseDirectory);
definition.StaticResourceName = Path.GetFileName(definition.SourceZipFilePath);

definition.RulesFilePath = GenerateRulesFile(analyzers, baseDirectory);
bool generate = true;

string generatedRulesTemplateFile = null;
if (definition.RulesFilePath == null)
{
definition.RulesFilePath = GenerateRulesFile(analyzers, baseDirectory);
generatedRulesTemplateFile = CalculateRulesTemplateFileName(package, outputDir);
File.Copy(definition.RulesFilePath, generatedRulesTemplateFile, overwrite: true);
}
else
{
this.logger.LogInfo(UIResources.APG_UsingSuppliedRulesFile, definition.RulesFilePath);
generate = IsValidRulesFile(definition.RulesFilePath);
}

string generatedSqaleFile = null;
bool generate = true;

if (definition.SqaleFilePath == null)
{
generatedSqaleFile = CalculateSqaleFileName(package, outputDir);
Expand All @@ -209,6 +228,7 @@ private string GeneratePluginForPackage(string outputDir, string language, strin
}
else
{
this.logger.LogInfo(UIResources.APG_UsingSuppliedSqaleFile, definition.SqaleFilePath);
generate = IsValidSqaleFile(definition.SqaleFilePath);
}

Expand All @@ -217,11 +237,21 @@ private string GeneratePluginForPackage(string outputDir, string language, strin
createdJarFilePath = BuildPlugin(definition, outputDir);
}

LogMessageForGeneratedRules(generatedRulesTemplateFile);
LogMessageForGeneratedSqale(generatedSqaleFile);

return createdJarFilePath;
}

private void LogMessageForGeneratedRules(string generatedFile)
{
if (generatedFile != null)

Choose a reason for hiding this comment

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

Isn't it better to also log (another message) when the file is null?

Copy link
Author

Choose a reason for hiding this comment

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

In that case the user will have supplied the file name as a parameter, and a message will have been logged in line 218 above.

{
// Log a message about the generated rules xml file for every plugin generated
this.logger.LogInfo(UIResources.APG_TemplateRuleFileGenerated, generatedFile);
}
}

private void LogMessageForGeneratedSqale(string generatedSqaleFile)
{
if (generatedSqaleFile != null)
Expand Down Expand Up @@ -347,6 +377,15 @@ private string GenerateRulesFile(IEnumerable<DiagnosticAnalyzer> analyzers, stri
return rulesFilePath;
}

private static string CalculateRulesTemplateFileName(IPackage package, string directory)
{
string filePath = string.Format(System.Globalization.CultureInfo.CurrentCulture,
RulesTemplateFileNameFormat, package.Id, package.Version);

filePath = Path.Combine(directory, filePath);
return filePath;
}

private static string CalculateSqaleFileName(IPackage package, string directory)
{
string filePath = string.Format(System.Globalization.CultureInfo.CurrentCulture,
Expand All @@ -371,6 +410,29 @@ private void GenerateFixedSqaleFile(IEnumerable<DiagnosticAnalyzer> analyzers, s
logger.LogDebug(UIResources.APG_SqaleGeneratedToFile, outputFilePath);
}

/// <summary>
/// Checks that the supplied rule file has valid content
/// </summary>
private bool IsValidRulesFile(string filePath)
{
Debug.Assert(!string.IsNullOrWhiteSpace(filePath));

Choose a reason for hiding this comment

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

IMO those 2 assertions needs to be if/throw.

Copy link
Author

Choose a reason for hiding this comment

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

They're assertions because they are checking the validity of our code, not the user input. We don't expect our code to reach this point if the file is null or missing.
If the user has supplied a valid file then the code will already have thrown (see comment in line 419 below).

Choose a reason for hiding this comment

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

Ok.

// Existence is checked when parsing the arguments
Debug.Assert(File.Exists(filePath), "Expecting the rule file to exist: " + filePath);

try
{
// TODO: consider adding further checks
Copy link

Choose a reason for hiding this comment

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

Code Smell Code Smell: Complete the task associated to this 'TODO' comment. (csharpsquid:S1135)

See it in SonarCloud

Serializer.LoadModel<Rules>(filePath);

}
catch (InvalidOperationException) // will be thrown for invalid xml
{
this.logger.LogError(UIResources.APG_InvalidRulesFile, filePath);
return false;
}
return true;
}

/// <summary>
/// Checks that the supplied sqale file has valid content
/// </summary>
Expand Down
58 changes: 45 additions & 13 deletions RoslynPluginGenerator/CommandLine/ArgumentProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ private static class KeywordIds
{
public const string AnalyzerRef = "analyzer.ref";
public const string SqaleXmlFile = "sqale.xml";

Choose a reason for hiding this comment

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

If SQALE is no longer supported, why do we still have this sqale.xml field?

public const string RuleXmlFile = "rules.xml";
public const string AcceptLicenses = "accept.licenses";
public const string RecurseDependencies = "recurse.dependencies";
}
Expand All @@ -55,6 +56,8 @@ static ArgumentProcessor()
id: KeywordIds.AnalyzerRef, prefixes: new string[] { "/analyzer:", "/a:" }, required: true, allowMultiple: false, description: CmdLineResources.ArgDescription_AnalzyerRef),
new ArgumentDescriptor(
id: KeywordIds.SqaleXmlFile, prefixes: new string[] { "/sqale:" }, required: false, allowMultiple: false, description: CmdLineResources.ArgDescription_SqaleXmlFile),
new ArgumentDescriptor(
id: KeywordIds.RuleXmlFile, prefixes: new string[] { "/rules:" }, required: false, allowMultiple: false, description: CmdLineResources.ArgDescription_RuleXmlFile),
new ArgumentDescriptor(
id: KeywordIds.AcceptLicenses, prefixes: new string[] { "/acceptLicenses" }, required: false, allowMultiple: false, description: CmdLineResources.ArgDescription_AcceptLicenses, isVerb: true),
new ArgumentDescriptor(
Expand Down Expand Up @@ -113,6 +116,8 @@ public ProcessedArgs Process(string[] commandLineArgs)

parsedOk &= TryParseSqaleFile(arguments, out string sqaleFilePath);

parsedOk &= TryParseRuleFile(arguments, out string ruleFilePath);

bool acceptLicense = GetLicenseAcceptance(arguments);
bool recurseDependencies = GetRecursion(arguments);

Expand All @@ -124,6 +129,7 @@ public ProcessedArgs Process(string[] commandLineArgs)
analyzerRef.Version,
SupportedLanguages.CSharp, /* TODO: support multiple languages */
sqaleFilePath,
ruleFilePath,
acceptLicense,
recurseDependencies,
System.IO.Directory.GetCurrentDirectory());
Expand Down Expand Up @@ -182,24 +188,50 @@ private NuGetReference TryParseNuGetReference(string argumentValue)

private bool TryParseSqaleFile(IEnumerable<ArgumentInstance> arguments, out string sqaleFilePath)
{
bool sucess = true;
sqaleFilePath = null;
ArgumentInstance arg = arguments.SingleOrDefault(a => ArgumentDescriptor.IdComparer.Equals(KeywordIds.SqaleXmlFile, a.Descriptor.Id));

if (arg != null)
if (arg == null)
{
if (File.Exists(arg.Value))
{
sqaleFilePath = arg.Value;
logger.LogDebug(CmdLineResources.DEBUG_UsingSqaleFile, sqaleFilePath);
}
else
{
sucess = false;
logger.LogError(CmdLineResources.ERROR_SqaleFileNotFound, arg.Value);
}
return true;
}

bool success = true;
if (File.Exists(arg.Value))
{
sqaleFilePath = arg.Value;
logger.LogDebug(CmdLineResources.DEBUG_UsingSqaleFile, sqaleFilePath);
}
else
{
success = false;
logger.LogError(CmdLineResources.ERROR_SqaleFileNotFound, arg.Value);
}
return success;
}

private bool TryParseRuleFile(IEnumerable<ArgumentInstance> arguments, out string ruleFilePath)
{
ruleFilePath = null;
ArgumentInstance arg = arguments.SingleOrDefault(a => ArgumentDescriptor.IdComparer.Equals(KeywordIds.RuleXmlFile, a.Descriptor.Id));

if (arg == null)
{
return true;
}

bool success = true;
if (File.Exists(arg.Value))
{
ruleFilePath = arg.Value;
this.logger.LogDebug(CmdLineResources.DEBUG_UsingRuleFile, ruleFilePath);
}
else
{
success = false;
this.logger.LogError(CmdLineResources.ERROR_RuleFileNotFound, arg.Value);
}
return sucess;
return success;
}

private static bool GetLicenseAcceptance(IEnumerable<ArgumentInstance> arguments)
Expand Down
Loading