Святослав Размыслов

27 Ноя 2020

Теги:

#CSharp

Исследование качества кода Open XML SDK от Microsoft

27 Ноя 2020

Автор: Святослав Размыслов

Введение
Почему Word API, а не Open XML SDK
Два значения для string
Про компактность кода
Те самые Always true/false
Остальные предупреждения
Заключение

Моё знакомство с Open XML SDK началось с того, что мне понадобилась библиотека для создания документов Word с некоторой отчётностью. После работы с Word API более 7 лет, захотелось попробовать что-нибудь новое и более удобное. Так я узнал, что у Microsoft есть альтернативное решение. По традиции, используемые в компании программы и библиотеки мы предварительно проверяем с помощью анализатора PVS-Studio.

Введение

Office Open XML, также известный как OpenXML или OOXML, представляет собой формат на основе XML для офисных документов, включая текстовые документы, электронные таблицы, презентации, а также диаграммы, фигуры и другой графический материал. Спецификация была разработана Microsoft и принята ECMA International в 2006 году. В июне 2014 года Microsoft выпустила Open XML SDK в open source. Сейчас исходники доступны на GitHub под лицензий MIT.

Для поиска ошибок в исходном коде библиотеки использовался PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS.

Проект достаточно маленький и предупреждений нашлось немного. Но выбор титульной картинки основывался как раз на результатах. Уж очень много бесполезных условных операторов в коде нашлось. Мне кажется, если отрефакторить все такие места в коде, то объём заметно сократится. Вследствие чего ещё и читаемость кода повысится.

Почему Word API, а не Open XML SDK

Как Вы могли догадаться из заголовка, я продолжил использовать Word API. У этого способа достаточно много минусов:

Старый неудобный API;
Должен быть установлен Microsoft Office;
Необходимость распространять дистрибутив с библиотеками Office;
Зависимость работы Word API от настроек локали системы;
Низкая скорость работы.

С локалью вообще забавный случай произошёл. В Windows есть десяток региональных настроек. На одном из серверных компьютеров оказалась каша из локалей USA и UK. Из-за этого создавались документы Word, где вместо символа доллара был рубль, а фунты вообще не выводились. Доработка настроек операционной системы исправила проблему.

Перечисляя всё это, я снова задумался, почему я до сих пор этим пользуюсь....

Но нет, Word API мне пока нравится больше, и вот почему.

OOXML выглядит таким образом:

<?xml version="1.0" encoding="utf-8" standalone="yes"?>
<w:document ....>
  <w:body>
    <w:p w:rsidR="00E22EB6"
         w:rsidRDefault="00E22EB6">
      <w:r>
        <w:t>This is a paragraph.</w:t>
      </w:r>
    </w:p>
    <w:p w:rsidR="00E22EB6"
         w:rsidRDefault="00E22EB6">
      <w:r>
        <w:t>This is another paragraph.</w:t>
      </w:r>
    </w:p>
  </w:body>
</w:document>

Где <w:r> (Word Run) - не предложение, и даже не слово, а любой фрагмент текста, имеющий атрибуты, отличные от соседних фрагментов текста.

Программируется это примерно таким кодом:

Paragraph para = body.AppendChild(new Paragraph());
Run run = para.AppendChild(new Run());
run.AppendChild(new Text(txt));

У документа специфичная внутренняя структура, и в коде нужно создавать те же самые элементы. У Open XML SDK, я считаю, недостаточно абстрактный уровень доступа к данным. Создание документа с помощью Word API будет более понятым и коротким. Особенно, когда дело дойдёт до таблиц и других сложных структур данных.

В свою очередь, Open XML SDK решает большой ряд задач. С ним можно создавать документы не только для Word, но и для Excel и PowerPoint. Наверное, для некоторых задач эта библиотека больше подходит, но я решил пока остаться на Word API. Полностью от него отказаться в любом случае не получится, т.к. для внутренних нужд мы разрабатываем плагин для Word, а там возможно использование только Word API.

Два значения для string

V3008 The '_rawOuterXml' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 164, 161. OpenXmlElement.cs 164

internal string RawOuterXml
{
    get => _rawOuterXml;

    set
    {
        if (string.IsNullOrEmpty(value))
        {
            _rawOuterXml = string.Empty;
        }

        _rawOuterXml = value;
    }
}

Тип string может иметь 2 типа значений: null и текстовое значение. Использовать текстовое значение определённо безопаснее, но оба подхода имеют права на существование. Вот в этом проекте значение null использовать неприемлемо и его перезаписывают на string.Empty... по крайней мере, так задумывалось. Но из-за ошибки в RawOuterXml всё же можно записать null, а потом обратиться к этому полю, получив NullReferenceException.

V3022 Expression 'namespaceUri != null' is always true. OpenXmlElement.cs 497

public OpenXmlAttribute GetAttribute(string localName, string namespaceUri)
{
    ....
    if (namespaceUri == null)
    {
        // treat null string as empty.
        namespaceUri = string.Empty;
    }
    ....
    if (HasAttributes)
    {
        if (namespaceUri != null)  // <=
        {
            ....
        }
        ....
    }
    ....
}

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

Про компактность кода

V3009 It's odd that this method always returns one and the same value of '".xml"'. CustomXmlPartTypeInfo.cs 31

internal static string GetTargetExtension(CustomXmlPartType partType)
{
    switch (partType)
    {
        case CustomXmlPartType.AdditionalCharacteristics:
            return ".xml";

        case CustomXmlPartType.Bibliography:
            return ".xml";

        case CustomXmlPartType.CustomXml:
            return ".xml";

        case CustomXmlPartType.InkContent:
            return ".xml";

        default:
            return ".xml";
    }
}

Не знаю, имеет ли место тут какая-нибудь опечатка или просто автор кода написал "красивый" код по его мнению. Я уверен, что нет смысла возвращать из функции столько однотипных значений и код можно сильно сократить.

Это не единственное такое место. Вот ещё парочка таких предупреждений:

V3009 It's odd that this method always returns one and the same value of '".xml"'. CustomPropertyPartTypeInfo.cs 25
V3009 It's odd that this method always returns one and the same value of '".bin"'. EmbeddedControlPersistenceBinaryDataPartTypeInfo.cs 22

Интересно было бы услышать, зачем так писать.

V3139 Two or more case-branches perform the same actions. OpenXmlPartReader.cs 560

private void InnerSkip()
{
    Debug.Assert(_xmlReader != null);

    switch (_elementState)
    {
        case ElementState.Null:
            ThrowIfNull();
            break;

        case ElementState.EOF:
            return;

        case ElementState.Start:
            _xmlReader.Skip();
            _elementStack.Pop();
            GetElementInformation();
            return;

        case ElementState.End:
        case ElementState.MiscNode:
            // cursor is end element, pop stack
            _xmlReader.Skip();
            _elementStack.Pop();
            GetElementInformation();
            return;
        ....
    }
    ....
}

К этому коду возникает меньше вопросов. Скорее всего, идентичные кейсы можно объединить и код станет короче и очевиднее.

Ещё несколько таких мест:

V3139 Two or more case-branches perform the same actions. OpenXmlMiscNode.cs 312
V3139 Two or more case-branches perform the same actions. CustomPropertyPartTypeInfo.cs 30
V3139 Two or more case-branches perform the same actions. CustomXmlPartTypeInfo.cs 15
V3139 Two or more case-branches perform the same actions. OpenXmlElement.cs 1803

Те самые Always true/false

Настало время привести примеры кода, которые определили мой выбор титульной картинки.

Предупреждение 1

V3022 Expression 'Complete()' is always false. ParticleCollection.cs 243

private bool IsComplete => Current is null ||
                           Current == _collection._element.FirstChild;

public bool MoveNext()
{
    ....
    if (IsComplete)
    {
        return Complete();
    }

    if (....)
    {
        return Complete();
    }

    return IsComplete ? Complete() : true;
}

Свойство IsComplete используется 2 раза, и по коду легко понять, что его значение не изменится. Таким образом, в конце функции можно просто возвращать второе значение тернарного оператора – true.

Предупреждение 2

V3022 Expression '_elementStack.Count > 0' is always true. OpenXmlDomReader.cs 501

private readonly Stack<OpenXmlElement> _elementStack;

private bool MoveToNextSibling()
{
    ....
    if (_elementStack.Count == 0)
    {
        _elementState = ElementState.EOF;
        return false;
    }
    ....
    if (_elementStack.Count > 0) // <=
    {
        _elementState = ElementState.End;
    }
    else
    {
        // no more element, EOF
        _elementState = ElementState.EOF;
    }
    ....
}

Очевидно, что если в стеке_elementStack не 0 элементов, то их больше. Код можно сократить как минимум на 8 строк.

Предупреждение 3

V3022 Expression 'rootElement == null' is always false. OpenXmlPartReader.cs 746

private static OpenXmlElement CreateElement(string namespaceUri, string name)
{
    if (string.IsNullOrEmpty(name))
    {
        throw new ArgumentException(....);
    }

    if (NamespaceIdMap.TryGetNamespaceId(namespaceUri, out byte nsId)
        && ElementLookup.Parts.Create(nsId, name) is OpenXmlElement element)
    {
        return element;
    }

    return new OpenXmlUnknownElement();
}

private bool ReadRoot()
{
  ....
  var rootElement = CreateElement(....);

  if (rootElement == null) // <=
  {
      throw new InvalidDataException(....);
  }
  ....
}

Функция CreateElement не может вернуть null. Если в компании было принято правило писать методы для создания xml-нод, которые либо возвращают валидный объект, либо кидают исключение, то пользователям таких функций можно не злоупотреблять дополнительными проверками.

Предупреждение 4

V3022 Expression 'nameProvider' is always not null. The operator '?.' is excessive. OpenXmlSimpleTypeExtensions.cs 50

public static XmlQualifiedName GetSimpleTypeQualifiedName(....)
{
    foreach (var validator in validators)
    {
        if (validator is INameProvider nameProvider &&
            nameProvider?.QName is XmlQualifiedName qname) // <=
        {
            return qname;
        }
    }

    return type.GetSimpleTypeQualifiedName();
}

Оператор is имеет такой паттерн:

expr is type varname

Если результат выражения is будет true, то в varname будет записана ненулевая ссылка, так что дополнительная её проверка на null является лишней.

Предупреждение 5

V3022 Expression 'extension == ".xlsx" || extension == ".xlsm"' is always false. PresentationDocument.cs 246

public static PresentationDocument CreateFromTemplate(string path)
{
    ....
    string extension = Path.GetExtension(path);
    if (extension != ".pptx" && extension != ".pptm" &&
        extension != ".potx" && extension != ".potm")
    {
        throw new ArgumentException("...." + path, nameof(path));
    }

    using (PresentationDocument template = PresentationDocument.Open(....)
    {
        PresentationDocument document = (PresentationDocument)template.Clone();

        if (extension == ".xlsx" || extension == ".xlsm")
        {
            return document;
        }
        ....
    }
    ....
}

Интересный код получился. Сначала автор отсеял все документы с расширениями не .pptx, .pptm, .potx и .potm, а потом решил для перестраховки проверить, что среди них нет .xlsx и .xlsm. Функция PresentationDocument – определённо жертва рефакторинга.

Предупреждение 6

V3022 Expression 'OpenSettings.MarkupCompatibilityProcessSettings == null' is always false. OpenXmlPackage.cs 661

public MarkupCompatibilityProcessSettings MarkupCompatibilityProcessSettings
{
    get
    {
        if (_mcSettings is null)
        {
            _mcSettings = new MarkupCompatibilityProcessSettings(....);
        }

        return _mcSettings;
    }

    set
    {
        _mcSettings = value;
    }
}

public MarkupCompatibilityProcessSettings MarkupCompatibilityProcessSettings
{
    get
    {
        if (OpenSettings.MarkupCompatibilityProcessSettings == null) // <=
        {
            return new MarkupCompatibilityProcessSettings(....);
        }
        else
        {
            return OpenSettings.MarkupCompatibilityProcessSettings;
        }
    }
}

Свойство MarkupCompatibilityProcessSettings никогда не возвращает null. Если в геттере выясняется, что поле класса имеет значение null, то объект перезаписывается на новый. Ещё обратите внимание, что это не рекурсивный вызов одного свойства, а это одноимённые свойства из разных классов. Возможно, некоторая путаница и привела к написанию лишних проверок.

Остальные предупреждения

Предупреждение 1

V3080 Possible null dereference. Consider inspecting 'previousSibling'. OpenXmlCompositeElement.cs 380

public OpenXmlElement PreviousSibling()
{
    if (!(Parent is OpenXmlCompositeElement parent))
    {
        return null;
    }
    ....
}

public override T InsertBefore<T>(T newChild, OpenXmlElement referenceChild)
{
    ....
    OpenXmlElement previousSibling = nextNode.PreviousSibling();
    prevNode.Next = nextNode;
    previousSibling.Next = prevNode;    // <=
    ....
}

А вот пример, где дополнительной проверки как раз не хватает. Метод PreviousSibling как раз может вернуть значение null, а результат этой функции сразу используется без проверки.

Ещё 2 опасных места:

V3080 Possible null dereference. Consider inspecting 'prevNode'. OpenXmlCompositeElement.cs 489
V3080 Possible null dereference. Consider inspecting 'prevNode'. OpenXmlCompositeElement.cs 497

Предупреждение 2

V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. UniqueAttributeValueConstraint.cs 60

public override ValidationErrorInfo ValidateCore(ValidationContext context)
{
    ....
    foreach (var e in root.Descendants(....))
    {
        if (e != element & e.GetType() == elementType) // <=
        {
            var eValue = e.ParsedState.Attributes[_attribute];

            if (eValue.HasValue && _comparer.Equals(....))
            {
                return true;
            }
        }
    }
    ....
}

Некоторые любят применять оператор '&' к логическим выражениям там, где не надо. В случае этого оператора сначала вычисляется второй операнд, независимо от результата первого. Здесь это не сильно серьёзная ошибка, но такой неаккуратный код после рефакторинга может приводить и к потенциальным исключениям NullReferenceException.

Предупреждение 3

V3097 Possible exception: type marked by [Serializable] contains non-serializable members not marked by [NonSerialized]. OpenXmlPackageValidationEventArgs.cs 15

[Serializable]
[Obsolete(ObsoleteAttributeMessages.ObsoleteV1ValidationFunctionality, false)]
[EditorBrowsable(EditorBrowsableState.Never)]
public sealed class OpenXmlPackageValidationEventArgs : EventArgs
{
    private string _message;

    [NonSerialized]
    private readonly object _sender;

    [NonSerialized]
    private OpenXmlPart _subPart;

    [NonSerialized]
    private OpenXmlPart _part;

    ....

    internal DataPartReferenceRelationship
        DataPartReferenceRelationship { get; set; } // <=
}

Сериализация класса OpenXmlPackageValidationEventArgs может стать неудачной из-за того, что одно из свойств забыли пометить несериализуемым. Либо необходимо доработать возвращаемый тип этого свойства до сериализуемого, иначе может возникать исключение в рантайме.

Заключение

Мы в компании любим проекты и технологии Microsoft. В разделе, где мы перечисляем Open Source проекты, проверенные с помощью PVS-Studio, мы даже выделили для Microsoft отдельный раздел. Там уже накопился 21 проект, про которые написано 26 статей. Это 27-я.

Уверен, Вас интересует, есть ли среди наших клиентов Microsoft. Ответ – да! Но не будем забывать, что это огромная корпорация, ведущая разработку по всему миру. Определённо есть подразделения, которые уже используют PVS-Studio в своих проектах, но тех, которые не используют, ещё больше! И наш опыт работы с открытыми проектами показывает, что им явно не хватает хорошего инструмента для поиска ошибок ;).