Azure PowerShell: «в основном безвреден»

Привет всем. Сегодня у нас на тесте очередной проект Microsoft. По названию статьи можно догадаться, что в этот раз разработчики не смогли порадовать нас большим количеством ошибок. Надеемся, авторов проекта не обидит название. Ведь малое количество ошибок — это отлично, не правда ли? Тем не менее, кое-что интересное в коде Azure PowerShell всё же удалось найти. Предлагаем ознакомиться с особенностями этого проекта и взглянуть на ошибки, найденные с помощью C#-анализатора PVS-Studio.

О проекте

Azure PowerShell — это набор командлетов (cmdlet), которые позволяют управлять ресурсами Azure непосредственно из командной строки PowerShell. Основное назначение этого набора — упрощение изучения и быстрый старт разработки для Azure. Azure PowerShell предоставляет администраторам и разработчикам мощные функции для создания, развертывания и управления приложениями Microsoft Azure.

Azure PowerShell разработан в среде .NET Standard, поддерживается PowerShell 5.1 для Windows и PowerShell 6.x и выше для всех платформ. Исходный код Azure PowerShell доступен на GitHub.

В последнее время мне на проверку часто попадают проекты Microsoft. По моему мнению, качество этих проектов обычно на высоте. Хотя, конечно, не обходится без исключений, как было в статье "WinForms: ошибки, Холмс". Однако на этот раз всё в норме. Проект большой: 6845 файлов исходного кода .cs содержат примерно 700 тысяч строк, без учета пустых (тесты, как и предупреждения третьего уровня достоверности, я не учитывал). Ошибок для такого объёма кода нашлось совсем немного: не более ста. Встречается довольно много однотипных ситуаций, поэтому для статьи я отобрал самые интересные срабатывания. Ошибки, как я это обычно делаю, отсортированы по номерам диагностик PVS-Studio.

Также мне встретились фрагменты кода, которые выглядят как ошибки, но я не смог сделать однозначного вывода о наличии реальной проблемы, так как недостаточно знаком с особенностями разработки для PowerShell. Надеюсь, среди читателей найдутся специалисты в данном вопросе и помогут мне. Об этом ниже.

Перед началом «разбора полетов» отмечу особенность проекта с точки зрения его структуры. Исходный код Azure PowerShell состоит из более чем семидесяти решений Visual Studio. Некоторые решения включают проекты из других решений. Такая структура немного замедлила работу по анализу (не сильно). В остальном проверка не вызвала затруднений. Для удобства, в строке сообщения об ошибке (в скобках) я буду указывать название решения, в котором эта ошибка была обнаружена.

Результаты проверки

V3001 There are identical sub-expressions 'strTimespan.Contains(«M»)' to the left and to the right of the '||' operator. AzureServiceBusCmdletBase.cs 187 (EventGrid)

public static TimeSpan ParseTimespan(string strTimespan)
{
  ....
  if (strTimespan.Contains("P") 
    || strTimespan.Contains("D") 
    || strTimespan.Contains("T") 
    || strTimespan.Contains("H") 
    || strTimespan.Contains("M") 
    || strTimespan.Contains("M"))
  ....
}

Пример довольно очевидной ошибки, исправить которую в состоянии лишь разработчик. В данном случае совершенно непонятно, имеем ли мы дело с дублированием кода, которое ни на что не влияет, или вместо «M» в одной из двух последних проверок должно фигурировать что-то другое.

V3001 There are identical sub-expressions 'this.AggregationType != null' to the left and to the right of the '&&' operator. GetAzureRmMetricCommand.cs 156 (Monitor)

public AggregationType? AggregationType { get; set; }
....
protected override void ProcessRecordInternal()
{
  ....
  string aggregation = (this.AggregationType != null &&
    this.AggregationType.HasValue) ?
    this.AggregationType.Value.ToString() : null;
  ....
}

Здесь, вероятно, ошибки нет. Это пример избыточного кода. Иногда такой код может указывать на недостаточный уровень знаний разработчика. Дело в том, что проверки this.AggregationType != null и this.AggregationType.HasValue идентичны. Достаточно использовать одну из них (любую). Лично мне больше нравится вариант с HasValue:

string aggregation = this.AggregationType.HasValue ?
  this.AggregationType.Value.ToString() : 
  null;

V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 152, 163. GetAzureRmRecoveryServicesBackupProtectionPolicy.cs 152 (RecoveryServices)

public override void ExecuteCmdlet()
{
  ....
  if( WorkloadType == Models.WorkloadType.AzureVM )
  {
    ....
  }
  ....
  else if( WorkloadType == Models.WorkloadType.AzureFiles )
  {
    if( BackupManagementType != Models.BackupManagementType.AzureStorage )
    {
      throw new ArgumentException(
        Resources.AzureFileUnsupportedBackupManagementTypeException );
    }
    serviceClientProviderType = ServiceClientHelpers.
      GetServiceClientProviderType( Models.WorkloadType.AzureFiles );
  }
  else if( WorkloadType == Models.WorkloadType.AzureFiles )
  {
    if( BackupManagementType != Models.BackupManagementType.AzureStorage )
    {
      throw new ArgumentException(
        Resources.AzureFileUnsupportedBackupManagementTypeException );
    }
    serviceClientProviderType = ServiceClientHelpers.
      GetServiceClientProviderType( Models.WorkloadType.AzureFiles );
  }
  ....
}

Два блока else if абсолютно идентичны, включая как условие, так и тело блока. Такие ошибки обычно допускают при использовании метода copy-paste в работе. Вопрос снова в критичности данной ошибки. Если это не простое дублирование кода, то речь может идти об отсутствии нужной проверки и соответствующего набора действий. Необходима правка кода автором.

V3005 The 'this.VM.OSProfile.WindowsConfiguration.ProvisionVMAgent' variable is assigned to itself. SetAzureVMOperatingSystemCommand.cs 298 (Compute)

public override void ExecuteCmdlet()
{
  ....
  // OS Profile
  this.VM.OSProfile.WindowsConfiguration.ProvisionVMAgent =
    this.VM.OSProfile.WindowsConfiguration.ProvisionVMAgent;
  ....
}

Значение свойства присваивается само себе. Взглянем на его объявление:

[JsonProperty(PropertyName = "provisionVMAgent")]
public bool? ProvisionVMAgent { get; set; }

В описании к JsonProperty указано «Instructs the Newtonsoft.Json.JsonSerializer to always serialize the member with the specified name». Кажется, тут все бесхитростно и допущена очевидная ошибка. Также смущает явное использование this для доступа к свойству. Возможно, по ошибке вместо this не указали какую-то другую переменную. Но пока не будем делать выводов. Дело в том, что в коде Azure PowerShell мне встретилось довольно много таких присваиваний (свойство присваивается само себе). Вот еще пример присваивания, очень похожего на ошибку:

V3005 The 'this.LastHeartbeat' variable is assigned to itself. PSFabricDetails.cs 804 (RecoveryServices)

public ASRInMageAzureV2SpecificRPIDetails(
  InMageAzureV2ReplicationDetails details)
{
  this.LastHeartbeat = this.LastHeartbeat;  // <=
  this.RecoveryAvailabilitySetId = details.RecoveryAvailabilitySetId;
  this.AgentVersion = details.AgentVersion;
  this.DiscoveryType = details.DiscoveryType;
  ....
}

Обратите внимание на второе и последующие присваивания. В правой части выражения там фигурирует вовсе не this, а details. Взглянем на объявление свойства this.LastHeartbeat:

public DateTime? LastHeartbeat { get; set; }

Наконец, поищем свойство с таким же именем в классе InMageAzureV2ReplicationDetails. Такое свойство там объявлено:

public class InMageAzureV2ReplicationDetails :
  ReplicationProviderSpecificSettings
{
  ....
  [JsonProperty(PropertyName = "lastHeartbeat")]
  public DateTime? LastHeartbeat { get; set; }
  ....  
}

Что ж, в данном случае я готов признать это срабатывание настоящей ошибкой. Но вот что делать со следующими срабатываниями? В них, в отличие от двух предыдущих фрагментов кода, присутствует множественное присваивание свойств самим себе. Это уже менее похоже на ошибку:

• V3005 The 'this.ResourceGroupName' variable is assigned to itself. RemoveAzureRmExpressRouteConnectionCommand.cs 84 (CognitiveServices)
• V3005 The 'this.ExpressRouteGatewayName' variable is assigned to itself. RemoveAzureRmExpressRouteConnectionCommand.cs 85 (CognitiveServices)
• V3005 The 'this.Name' variable is assigned to itself. RemoveAzureRmExpressRouteConnectionCommand.cs 86 (CognitiveServices)

[Cmdlet(VerbsCommon.Remove,
  ResourceManager.Common.AzureRMConstants.AzureRMPrefix +
    "ExpressRouteConnection",
  DefaultParameterSetName =
    CortexParameterSetNames.ByExpressRouteConnectionName,
  SupportsShouldProcess = true),
  OutputType(typeof(bool))]
public class RemoveExpressRouteConnectionCommand :
  ExpressRouteConnectionBaseCmdlet
{
  [Parameter(
    Mandatory = true,
    ParameterSetName = CortexParameterSetNames.ByExpressRouteConnectionName,
    HelpMessage = "The resource group name.")]
  [ResourceGroupCompleter]
  [ValidateNotNullOrEmpty]
  public string ResourceGroupName { get; set; }
  ....
  public override void Execute()
  {
    if (....)
    {
      this.ResourceGroupName = this.ResourceGroupName;
      this.ExpressRouteGatewayName = this.ExpressRouteGatewayName;
      this.Name = this.Name;
    }
    ....
  }    
  ....
}

Метод Execute содержит сразу три подряд идущих присваивания свойств самим себе. На всякий случай, я привел полное объявление класса RemoveExpressRouteConnectionCommand с указанием всех его атрибутов, а также объявление свойства ResourceGroupName (другие два свойства объявлены аналогичным образом). Именно такие срабатывания заставили меня задуматься над вопросом: «А ошибка ли это?» Подозреваю, что здесь может твориться какая-то внутренняя магия PowerShell-разработки. Надеюсь, что среди читателей найдутся осведомлённые в данном вопросе специалисты. Я не готов сделать какой-то вывод в данном случае.

V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ArgumentException(FOO). StartAzureRmRecoveryServicesAsrTestFailoverJob.cs 259 (RecoveryServices)

private void StartRPITestFailover()
{
  ....
  if (....)
  {
    ....
  }
  else
  {
    new ArgumentException(
      Resources
        .UnsupportedDirectionForTFO); // Throw Unsupported Direction
                                      // Exception
  }
  ....
}

Пропущено ключевое слово throw. Причем в комментарии написано, что исключение как раз должно быть выброшено. В решении RecoveryServices мне встретилось ещё несколько подобных ошибок:

• V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ArgumentException(FOO). StartAzureRmRecoveryServicesAsrTestFailoverJob.cs 305 (RecoveryServices)
• V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ArgumentException(FOO). StartAzureRmRecoveryServicesAsrUnPlannedFailover.cs 278 (RecoveryServices)
• V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ArgumentException(FOO). StartAzureRmRecoveryServicesAsrUnPlannedFailover.cs 322 (RecoveryServices)
• V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ArgumentException(FOO). UpdateAzureRmRecoveryServicesAsrProtectionDirection.cs 421 (RecoveryServices)
• V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ArgumentException(FOO). UpdateAzureRmRecoveryServicesAsrProtectionDirection.cs 452 (RecoveryServices)

V3022 Expression 'apiType.HasValue' is always false. ApiManagementClient.cs 1134 (ApiManagement)

private string GetApiTypeForImport(...., PsApiManagementApiType? apiType)
{
  ....
  if (apiType.HasValue)
  {
    switch(apiType.Value)
    {
      case PsApiManagementApiType.Http: return SoapApiType.SoapToRest;
      case PsApiManagementApiType.Soap: return SoapApiType.SoapPassThrough;
      default: return SoapApiType.SoapPassThrough;
    }
  }

  return apiType.HasValue ?        // <=
    apiType.Value.ToString("g") : 
    PsApiManagementApiType.Http.ToString("g");
}

Нарушена логика работы. Если apiType содержит значение, то до выражения return в конце метода управление не дойдет (все ветви switch содержат return). В противном случае метод всегда вернёт PsApiManagementApiType.Http.ToString(«g»), а значение apiType.Value.ToString(«g»), соответственно, не будет возвращено никогда.

V3022 Expression 'automationJob != null && automationJob == null' is always false. NodeConfigurationDeployment.cs 199 (Automation)

public NodeConfigurationDeployment(
  ....,
  Management.Automation.Models.Job automationJob = null, 
  ....)
{
  ....
  if (automationJob != null && automationJob == null) return;
  ....
}

Парадоксальный код. Две проверки, противоречащие друг другу. Вероятно, вторая проверка на равенство null содержит не ту переменную.

V3022 Expression is always false. DataFactoryClient.Encrypt.cs 37 (DataFactory)

public virtual string OnPremisesEncryptString(....)
{
  ....
  if ( linkedServiceType == LinkedServiceType.OnPremisesSqlLinkedService 
    && linkedServiceType == LinkedServiceType.OnPremisesOracleLinkedService
    && linkedServiceType == LinkedServiceType.OnPremisesFileSystemLinkedService
    && (value == null || value.Length == 0))
  {
    throw new ArgumentNullException("value");
  }
  ....
}

Проверка бессмысленна, а исключение не будет выброшено никогда. Условие требует одновременного равенства переменной linkedServiceType трём различным значениям. Вероятно, перепутаны операторы && и ||. Исправленный код:

if (( linkedServiceType == LinkedServiceType.OnPremisesSqlLinkedService 
  || linkedServiceType == LinkedServiceType.OnPremisesOracleLinkedService
  || linkedServiceType == LinkedServiceType.OnPremisesFileSystemLinkedService)
  && (value == null || value.Length == 0))
....

V3022 Expression 'Ekus == null' is always false. PSKeyVaultCertificatePolicy.cs 129 (KeyVault)

internal CertificatePolicy ToCertificatePolicy()
{
  ....
  if (Ekus != null)
  {
    x509CertificateProperties.Ekus = Ekus == null ? 
      null : new List<string>(Ekus);
  }                
  ....
}

Лишняя проверка переменной Ekus на равенство null. Вероятно, ничего страшного, но код выглядит некрасиво.

V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. PolicyRetentionObjects.cs 207 (RecoveryServices)

public virtual void Validate()
{
  if (RetentionTimes == null 
    || RetentionTimes.Count == 0 
    || RetentionTimes.Count != 1)
  {
    throw new ArgumentException(
      Resources.InvalidRetentionTimesInPolicyException);
  }
}

Избыточная проверка, либо ошибочное условие. Проверка RetentionTimes.Count != 1 лишена смысла, так как перед этим проверяют RetentionTimes.Count == 0.

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: this.ResourceGroupName. NewScheduledQueryRuleCommand.cs 117 (Monitor)

protected override void ProcessRecordInternal()
{
  ....
  if (this.ShouldProcess(this.Name,
    string.Format("Creating Log Alert Rule '{0}' in resource group {0}",
      this.Name, this.ResourceGroupName)))
  {
    ....
  }
  ....
}

Ошибка в строке форматирования. Дважды используют спецификатор {0}, при этом методу Format передано два аргумента. Правильный вариант:

if (this.ShouldProcess(this.Name,
  string.Format("Creating Log Alert Rule '{0}' in resource group {1}",
    this.Name, this.ResourceGroupName)))
....

Еще одна подобная ошибка:

• V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: this.ResourceGroupName. RemoveScheduledQueryRuleCommand.cs 88 (Monitor)

V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'imageAndOsType' object VirtualMachineScaleSetStrategy.cs 81 (Compute)

internal static ResourceConfig<VirtualMachineScaleSet> 
CreateVirtualMachineScaleSetConfig(...., ImageAndOsType imageAndOsType, ....)
{
  ....
  VirtualMachineProfile = new VirtualMachineScaleSetVMProfile
  {
    OsProfile = new VirtualMachineScaleSetOSProfile
    {
        ....,
        WindowsConfiguration = 
          imageAndOsType.CreateWindowsConfiguration(),  // <=
        ....,
    },
    StorageProfile = new VirtualMachineScaleSetStorageProfile
    {
        ImageReference = imageAndOsType?.Image,  // <=
        DataDisks = DataDiskStrategy.CreateVmssDataDisks(
          imageAndOsType?.DataDiskLuns, dataDisks)  // <=
    },  
  },
  ....
}

При создании объекта VirtualMachineScaleSetVMProfile переменную imageAndOsType используют без проверки на равенство null. Однако далее, при создании VirtualMachineScaleSetStorageProfile, эту переменную уже проверяют при помощи оператора условного доступа, причем дважды. Код выглядит небезопасно.

V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'existingContacts' object RemoveAzureKeyVaultCertificateContact.cs 123 (KeyVault)

public override void ExecuteCmdlet()
{
  ....
  List<PSKeyVaultCertificateContact> existingContacts;
  
  try
  {
    existingContacts = this.DataServiceClient.
                       GetCertificateContacts(VaultName)?.ToList();
  }
  catch (KeyVaultErrorException exception)
  {
    ....
    existingContacts = null;
  }
  
  foreach (var email in EmailAddress)
  {
    existingContacts.RemoveAll(....);  // <=
  }
  ....
}

Как в случае нормального выполнения, так и в результате обработки исключения, переменная existingContacts может получить значение null, после чего работа продолжится. Далее в коде эта переменная используется безо всякой проверки.

V3066 Possible incorrect order of arguments passed to 'PersistSyncServerRegistration' method: 'storageSyncServiceUid' and 'discoveryUri'. EcsManagementInteropClient.cs 364 (StorageSync)

public class EcsManagementInteropClient : IEcsManagement
{
  ....
  public int PersistSyncServerRegistration(....)
  {
    return m_managementObject.PersistSyncServerRegistration(
      serviceUri,
      subscriptionId,
      storageSyncServiceName,
      resourceGroupName,
      clusterId,
      clusterName,
      storageSyncServiceUid,  // <=
      discoveryUri,           // <=
      serviceLocation,
      resourceLocation);
  }
  ....
}

Анализатор заподозрил, что перепутан порядок передачи аргументов в метод PersistSyncServerRegistration. Объявление метода:

public interface IEcsManagement : IDisposable
{
  ....
  int PersistSyncServerRegistration(
    [In, MarshalAs(UnmanagedType.BStr)]
    string serviceUri,
    [In, MarshalAs(UnmanagedType.BStr)]
    string subscriptionId,
    [In, MarshalAs(UnmanagedType.BStr)]
    string storageSyncServiceName,
    [In, MarshalAs(UnmanagedType.BStr)]
    string resourceGroupName,
    [In, MarshalAs(UnmanagedType.BStr)]
    string clusterId,
    [In, MarshalAs(UnmanagedType.BStr)]
    string clusterName,
    [In, MarshalAs(UnmanagedType.BStr)]
    string discoveryUri,                              // <=
    [In, MarshalAs(UnmanagedType.BStr)]
    string storageSyncServiceUid,                     // <=
    [In, MarshalAs(UnmanagedType.BStr)]
    string serviceLocation,
    [In, MarshalAs(UnmanagedType.BStr)]
    string resourceLocation);
  ....
}

Действительно, что-то тут напутано с аргументами номер семь и восемь. Необходима проверка кода автором.

V3077 The setter of 'GetGuid' property does not utilize its 'value' parameter. RecoveryServicesBackupCmdletBase.cs 54 (RecoveryServices)

public abstract class RecoveryServicesBackupCmdletBase : AzureRMCmdlet
{
  ....
  static string _guid;
  
  protected static string GetGuid
  {
    get { return _guid; }
    set { _guid = Guid.NewGuid().ToString(); }
  }
  ....
}

В сеттере не используют передаваемый параметр. Вместо этого получают новый GUID и присваивают его полю _guid. Думаю, большинство читателей согласятся, что такой код выглядит как минимум некрасиво. Использовать такую конструкцию не вполне удобно: для (пере)инициализации свойства GetGuid необходимо выполнить присвоение ему какого-то фейкового значения, что совсем неочевидно. Но больше всего меня позабавил момент, как сами авторы используют эту конструкцию. В коде есть единственное место, где работают с GetGuid. Взгляните:

public override void ExecuteCmdlet()
{
  ....
  var itemResponse = ServiceClientAdapter.CreateOrUpdateProtectionIntent(
    GetGuid ?? Guid.NewGuid().ToString(),
    ....);  
  ....
}

Великолепно!

V3091 Empirical analysis. It is possible that a typo is present inside the string literal: «Management Group Id». The 'Id' word is suspicious. Constants.cs 36 (Resources)

public class HelpMessages
{
  public const string SubscriptionId = "Subscription Id of the subscription
                                        associated with the management";
  public const string GroupId = "Management Group Id";  // <=
  public const string Recurse = "Recursively list the children of the
                                 management group";
  public const string ParentId = "Parent Id of the management group";
  public const string GroupName = "Management Group Id";  // <=
  public const string DisplayName = "Display Name of the management group";
  public const string Expand = "Expand the output to list the children of the
                                management group";
  public const string Force = "Force the action and skip confirmations";
  public const string InputObject = "Input Object from the Get call";
  public const string ParentObject = "Parent Object";
}

Анализатор указал на возможную ошибку в присваиваемой строке для константы GroupName. Вывод делается на базе эмпирического анализа других присваиваний с учётом имен переменных. Я думаю, что в данном случае анализатор прав, а значение константы GroupName должно быть вида «Management Group name». Ошибка, вероятно, вызвана тем, что было скопировано значение для константы GroupId, но изменить его забыли.

Еще одна подобная ошибка:

• V3091 Empirical analysis. It is possible that a typo is present inside the string literal. The 'Name' word is suspicious. ParamHelpMsgs.cs 153 (RecoveryServices)

V3093 The '|' operator evaluates both operands. Perhaps a short-circuit '||' operator should be used instead. PSKeyVaultCertificatePolicy.cs 114 (KeyVault)

internal CertificatePolicy ToCertificatePolicy()
{
  ....
  if (!string.IsNullOrWhiteSpace(SubjectName) ||
    DnsNames != null ||
    Ekus != null ||
    KeyUsage != null |        // <=
    ValidityInMonths.HasValue)
  {
    ....
  }
  ....
}

Есть подозрение, что допущена ошибка, и между последними условиями в блоке if должен быть также использован оператор ||. Точный ответ, как это часто бывает, может дать лишь разработчик.

V3095 The 'certificate' object was used before it was verified against null. Check lines: 41, 43. CertificateInfo.cs 41 (Automation)

public CertificateInfo(
  ...., 
  Azure.Management.Automation.Models.Certificate certificate)
{
  ....
  this.Name = certificate.Name;
  
  if (certificate == null) return;
  ....
}

Классика. Сначала объект используется, и только затем ссылка проверяется на null. Мы встречаем подобные ошибки очень часто. Рассмотрим ещё одну подобную ошибку.

V3095 The 'clusterCred' object was used before it was verified against null. Check lines: 115, 118. InvokeHiveCommand.cs 115 (HDInsight)

public override void ExecuteCmdlet()
{
  ....
  _credential = new BasicAuthenticationCloudCredentials
  {
    Username = clusterCred.UserName,
    Password = clusterCred.Password.ConvertToString()
  };
  
  if (clusterConnection == null || clusterCred == null)
  ....
}

И ещё пара похожих ошибок:

• V3095 The '_profile' object was used before it was verified against null. Check lines: 47, 49. RMProfileClient.cs 47 (Accounts)
• V3095 The 'this.LoadBalancer.BackendAddressPools' object was used before it was verified against null. Check lines: 56, 63. AddAzureRmLoadBalancerBackendAddressPoolConfigCommand.cs 56 (CognitiveServices)
• Вообще, в коде Azure PowerShell мне встретилось гораздо больше ошибок V3095. Но все они довольно однотипны, поэтому не буду останавливаться на этом более подробно.

V3125 The 'startTime' object was used after it was verified against null. Check lines: 1752, 1738. AutomationPSClientDSC.cs 1752 (Automation)

private string GetNodeReportListFilterString(
  ....,
  DateTimeOffset? startTime,
  ....,
  DateTimeOffset? lastModifiedTime)
{
  ....
  if (startTime.HasValue)
  {
    odataFilter.Add("properties/startTime ge " +
      this.FormatDateTime(startTime.Value));      // <=
  }
  ....
  if (lastModifiedTime.HasValue)
  {
    odataFilter.Add("properties/lastModifiedTime ge " +
      this.FormatDateTime(startTime.Value));      // <=
  }
  ....
}

Также довольно частый тип ошибки. Переменную startTime проверили на наличие значения при первом использовании. При следующем использовании этого не делают. Но всё может быть еще хуже. Взгляните на второй блок if. Я думаю, что здесь вообще не должно быть переменной startTime. На это указывает как отсутствие её проверки на наличие значения перед использованием, так и строка, формируемая для передачи методу Add. В первой части этой строки упоминается другая переменная (lastModifiedTime).

V3125 The 'firstPage' object was used after it was verified against null. Check lines: 113, 108. IntegrationAccountAgreementOperations.cs 113 (LogicApp)

public IList<IntegrationAccountAgreement> 
ListIntegrationAccountAgreements(....)
{
  var compositeList = new List<IntegrationAccountAgreement>();
  var firstPage = this.LogicManagementClient.
                  IntegrationAccountAgreements.List(....);

  if (firstPage != null)
  {
    compositeList.AddRange(firstPage);
  }

  if (!string.IsNullOrEmpty(firstPage.NextPageLink))  // <=
  {
    ....
  }
  ....
}

Ещё одна очевидная ошибка. Переменную firstPage используют небезопасно, несмотря на то, что ранее в коде есть использование этой переменной с предварительной проверкой на равенство null.

Срабатываний V3125 в коде Azure PowerShell мне встретилось даже больше, чем описанных ранее V3095. Все они также однотипны. Думаю, можно ограничиться теми двумя, что мы рассмотрели.

V3137 The 'apiVersionSetId' variable is assigned but is not used by the end of the function. GetAzureApiManagementApiVersionSet.cs 69 (ApiManagement)

public String ApiVersionSetId { get; set; }
....
public override void ExecuteApiManagementCmdlet()
{
  ....
  string apiVersionSetId;            

  if (ParameterSetName.Equals(ContextParameterSet))
  {
    ....
    apiVersionSetId = ApiVersionSetId;
  }
  else
  {
    apiVersionSetId = ....;
  }

  if (string.IsNullOrEmpty(ApiVersionSetId))  // <=
  {
    WriteObject(....);
  }
  else
  {
    WriteObject(Client.GetApiVersionSet(...., ApiVersionSetId))  // <=
  }
}

Анализатор сообщает, что локальная переменная apiVersionSetId была проинициализирована, но никак не использована. Часто такой паттерн свидетельствует об ошибке. Думаю, в данном случае вероятность ошибки велика, учитывая то, что имя локальной переменной apiVersionSetId и имя свойства ApiVersionSetId отличаются лишь регистром первой буквы. Взгляните на код. После инициализации apiVersionSetId (тем или иным образом) далее в коде используют только свойство ApiVersionSetId. Очень, очень подозрительно.

V3137 The 'cacheId' variable is assigned but is not used by the end of the function. RemoveAzureApiManagementCache.cs 94 (ApiManagement)

public String CacheId { get; set; }
....
public override void ExecuteApiManagementCmdlet()
{
  ....
  string cacheId;

  if (....)
  {
    ....
    cacheId = InputObject.CacheId;
  }
  else if (....)
  {
    ....
    cacheId = cache.CacheId;
  }
  else
  {
    ....
    cacheId = CacheId;
  }
  var actionDescription = string.Format(...., CacheId);   // <=
  var actionWarning = string.Format(...., CacheId);       // <=
  ....
  Client.CacheRemove(resourceGroupName, serviceName, CacheId);  // <=
  ....  
}

Ситуация, почти в точности повторяющая ранее рассмотренную. Локальная переменная cacheId после инициализации никак не используется. А вместо неё используют свойство с очень похожим именем CacheId. Не знаю, возможно, это такой паттерн у разработчиков Azure PowerShell. Но выглядит как ошибка.

V3143 The 'value' parameter is rewritten inside a property setter, and is not used after that. NewAzureIntegrationAccountPartnerCommand.cs 67 (LogicApp)

[Parameter(Mandatory = false, 
  HelpMessage = "The integration account partner type.",
  ValueFromPipelineByPropertyName = false)]
[ValidateSet("B2B", IgnoreCase = false)]
[ValidateNotNullOrEmpty]
public string PartnerType
{
  get { return this.partnerType; }
  set { value = this.partnerType; }  // <=
}

Поле partnerType объявлено так:

/// <summary>
/// Default partner type.
/// </summary>
private string partnerType = "B2B";

Вопреки названию решения, в котором была обнаружена эта ошибка (LogicApp), я не вижу тут логики. Использование value в сеттере на запись — не такое уж редкое явление, но в данном случае речь идет о потере первоначального значения. Выглядит странно. В коде есть единственное обращение к этому свойству на чтение. Возможно, снова следует просить совета у экспертов. Может, я чего-то не понимаю. Дело в том, что мне встретилось еще несколько точно таких же паттернов:

• V3143 The 'value' parameter is rewritten inside a property setter, and is not used after that. NewAzureIntegrationAccountSchemaCommand.cs 79 (LogicApp)
• V3143 The 'value' parameter is rewritten inside a property setter, and is not used after that. NewAzureIntegrationAccountSchemaCommand.cs 87 (LogicApp)
• V3143 The 'value' parameter is rewritten inside a property setter, and is not used after that. UpdateAzureIntegrationAccountPartnerCommand.cs 67 (LogicApp)
• V3143 The 'value' parameter is rewritten inside a property setter, and is not used after that. UpdateAzureIntegrationAccountSchemaCommand.cs 80 (LogicApp)
• V3143 The 'value' parameter is rewritten inside a property setter, and is not used after that. UpdateAzureIntegrationAccountSchemaCommand.cs 88 (LogicApp)

Заключение

Вот и все интересные ошибки, которые удалось найти в коде Azure PowerShell. Энтузиастам и просто интересующимся я предлагаю провести самостоятельное изучение ошибок в коде этого (или любого другого) проекта. Вероятно, я мог пропустить ещё что-то интересное. Для этого достаточно скачать и установить PVS-Studio.

Спасибо, что дочитали. Безбажного всем кода!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Azure PowerShell: Mostly Harmless.

Автор: Сергей Хренов

Источник