# Исследование качества кода Open XML SDK от Microsoft
Моё знакомство с 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](https://github.com/OfficeDev/Open-XML-SDK) под лицензий MIT\.
Для поиска ошибок в исходном коде библиотеки использовался [PVS\-Studio](https://pvs-studio.ru/ru/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 выглядит таким образом:
```cpp
This is a paragraph.
This is another paragraph.
```
Где _ \(Word Run\)_ \- не предложение, и даже не слово, а любой фрагмент текста, имеющий атрибуты, отличные от соседних фрагментов текста\.
Программируется это примерно таким кодом:
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3008/) The '\_rawOuterXml' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 164, 161\. OpenXmlElement\.cs 164
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'namespaceUri \!\= null' is always true\. OpenXmlElement\.cs 497
```cpp
public OpenXmlAttribute GetAttribute(string localName, string namespaceUri)
{
....
if (namespaceUri == null)
{
// treat null string as empty.
namespaceUri = string.Empty;
}
....
if (HasAttributes)
{
if (namespaceUri != null) // <=
{
....
}
....
}
....
}
```
В этом фрагменте используется тот же самой подход, автор кода не сделал какую\-то серьёзную ошибку, но всё ещё чувствуется запах неудачного рефакторинга\. Скорее всего, одну проверку тут можно удалить, что уменьшит ширину кода, а, следовательно, повысит его читаемость\.
## Про компактность кода
[V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) It's odd that this method always returns one and the same value of '"\.xml"'\. CustomXmlPartTypeInfo\.cs 31
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. OpenXmlPartReader\.cs 560
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'Complete\(\)' is always false\. ParticleCollection\.cs 243
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression '\_elementStack\.Count \> 0' is always true\. OpenXmlDomReader\.cs 501
```cpp
private readonly Stack _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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'rootElement \=\= null' is always false\. OpenXmlPartReader\.cs 746
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'nameProvider' is always not null\. The operator '?\.' is excessive\. OpenXmlSimpleTypeExtensions\.cs 50
```cpp
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 имеет такой паттерн:
```cpp
expr is type varname
```
Если результат выражения _is_ будет _true_, то в _varname_ будет записана ненулевая ссылка, так что дополнительная её проверка на _null_ является лишней\.
**Предупреждение 5**
[V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'extension \=\= "\.xlsx" \|\| extension \=\= "\.xlsm"' is always false\. PresentationDocument\.cs 246
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'OpenSettings\.MarkupCompatibilityProcessSettings \=\= null' is always false\. OpenXmlPackage\.cs 661
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'previousSibling'\. OpenXmlCompositeElement\.cs 380
```cpp
public OpenXmlElement PreviousSibling()
{
if (!(Parent is OpenXmlCompositeElement parent))
{
return null;
}
....
}
public override T InsertBefore(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](https://pvs-studio.ru/ru/docs/warnings/v3093/) The '&' operator evaluates both operands\. Perhaps a short\-circuit '&&' operator should be used instead\. UniqueAttributeValueConstraint\.cs 60
```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3097/) Possible exception: type marked by \[Serializable\] contains non\-serializable members not marked by \[NonSerialized\]\. OpenXmlPackageValidationEventArgs\.cs 15
```cpp
[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 проекты, [проверенные](https://pvs-studio.ru/ru/blog/inspections/) с помощью PVS\-Studio, мы даже выделили для Microsoft отдельный раздел\. Там уже накопился 21 проект, про которые написано 26 статей\. Это 27\-я\.
Уверен, Вас интересует, есть ли среди наших клиентов Microsoft\. Ответ – да\! Но не будем забывать, что это огромная корпорация, ведущая разработку по всему миру\. Определённо есть подразделения, которые уже используют PVS\-Studio в своих проектах, но тех, которые не используют, ещё больше\! И наш опыт работы с открытыми проектами показывает, что им явно не хватает хорошего инструмента для поиска ошибок ;\)\.
[](https://pvs-studio.ru/ru/pvs-studio-download/)
Ещё из новостей, кому интересен анализ кода на C\+\+, C\# и Java: мы недавно добавили поддержку стандарта [OWASP](https://pvs-studio.ru/ru/pvs-studio/sast/owasp/) и активно увеличиваем его покрытие\.