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


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

0777_OpenXML_SDK_ru/image1.png

Введение

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)  // <=
        {
            ....
        }
        ....
    }
    ....
}

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

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

0777_OpenXML_SDK_ru/image2.png

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 в своих проектах, но тех, которые не используют, ещё больше! И наш опыт работы с открытыми проектами показывает, что им явно не хватает хорошего инструмента для поиска ошибок ;).

GetFreeTrialImage

Ещё из новостей, кому интересен анализ кода на C++, C# и Java: мы недавно добавили поддержку стандарта OWASP и активно увеличиваем его покрытие.


Вы можете обсудить эту статью с другими читателями на сайте habr.com


Найдите ошибки в своем C, C++, C# и Java коде

Предлагаем попробовать проверить код вашего проекта с помощью анализатора кода PVS-Studio. Одна найденная в нём ошибка скажет вам о пользе методологии статического анализа кода больше, чем десяток статей.

goto PVS-Studio;



Найденные ошибки

Проверено проектов
427
Собрано ошибок
14 526

А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++, C#
и Java

goto PVS-Studio;
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять