![image1.png](https://habrastorage.org/getpro/habr/post_images/8f5/246/ef7/8f5246ef75a5de2b1a78d40b204296bb.png)
Моё знакомство с 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) // <=
{
....
}
....
}
....
}
В этом фрагменте используется тот же самой подход, автор кода не сделал какую-то серьёзную ошибку, но всё ещё чувствуется запах неудачного рефакторинга. Скорее всего, одну проверку тут можно удалить, что уменьшит ширину кода, а, следовательно, повысит его читаемость.
Про компактность кода
![image2.png](https://habrastorage.org/getpro/habr/post_images/815/8c7/11e/8158c711e4f4638d29aedd2b00684163.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 – определённо жертва рефакторинга.
Предупреждение 7
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 в своих проектах, но тех, которые не используют, ещё больше! И наш опыт работы с открытыми проектами показывает, что им явно не хватает хорошего инструмента для поиска ошибок ;).
![](https://habrastorage.org/getpro/habr/post_images/04b/0d5/022/04b0d5022b3a6cfadc4732dce7842da7.png)
Ещё из новостей, кому интересен анализ кода на C++, C# и Java: мы недавно добавили поддержку стандарта OWASP и активно увеличиваем его покрытие.
![](https://habrastorage.org/webt/d5/1k/2e/d51k2edrgxyrg5voswox_46pg3o.png)
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing the Code Quality of Microsoft's Open XML SDK.