Одних тестов не достаточно, нужна хорошая архитектура
Мы все понимаем, что такое автоматические тесты. Мы разрабатываем софт, и хотим, чтобы он решал какие-то проблемы пользователей. Написав тест, мы убеждаемся, что конкретная проблема решается конкретным участком кода. Потом требования изменяются, мы меняем тесты и меняем код соответствующим новым требованиям образом. Но это не всегда спасает. Кроме высокого тестового покрытия наш код должен быть спроектирован таким образом, чтобы защищать разработчика от ошибок ещё при его написании.
В статье я постарался описать одну из проблем, которую может решить хорошая архитектура: связанные участки кода могут разъезжаться между собой, это может приводить к багам, и тесты тут не спасут. А грамотный дизайн может помочь.
Жизненный пример
Мы разрабатываем маркетинг CRM-систему для агрегации данных покупателей интернет-магазинов. В эту платформу можно с помощью API загружать заказы, а потом анализировать, какие потребители что покупают, что можно рекомендовать и тому подобное. Также нужно иметь возможность видеть в интерфейсе историю изменения по каждому заказу.
Для начала, представим нашу доменную модель.
У нас есть покупатели, заказы и позиции заказов. Покупатель может иметь много заказов, заказ может иметь много линий. Каждая линия содержит базовую цену за единицу товара, цену за линию, количество, идентификатор товара, ссылку на заказ, в котором линия содержится, а также статус линии (например, «оформлена» или «отменена»). Заказ содержит идентификатор заказа у клиента, дату и время совершения заказа, стоимость заказа с учётом всех скидок, стоимость доставки, ссылку на магазин покупки, а так же ссылку на потребителя.
Так же у нас есть требование просмотра истории изменения заказа и запрет на удаления данных о заказах, которые к нам когда-то загружались. В итоге, получается следующая структура классов:
class Customer
{
public int Id { get; set; }
public string Name { get; set; }
public string Email { get; set; }
}class Order
{
public int Id {get;set;}
public string ExternalId {get;set;}
public ICollection History {get;}
} class OrderHistoryItem
{
public ICollection OrderLines {get;}
public Order Order { get; set; }
public DateTime OrderChangeDateTime { get; set; }
public decimal TotalAmountAfterDiscounts { get; set; }
public Customer Customer { get; set; }
public decimal DeliveryCost { get; set; }
public string ShopExternalId { get; set; }
} class OrderHistoryLine
{
public decimal BasePricePerItem { get; set; }
public OrderHistoryItem OrderHistoryItem { get; set; }
public decimal PriceOfLine { get; set; }
public decimal Quantity { get; set; }
public string Status { get; set; }
public string ProductExternalId { get; set; }
}
Заказ (Order) — агрегат, в который может входить множество исторических состояний заказа (OrderHistoryItem), каждое из которых содержит линии (OrderHistoryLine). Таким образом, при изменении заказа мы сохраним его состояния до и после изменения. При этом актуальное состояние заказа всегда можно получить, упорядочив исторические состояния по времени и взяв первое.
Теперь опишем формат запроса API, которым могли бы пользоваться наши клиенты для передачи заказов:
{
id: "123”,
customer: { id: 1 },
dateTimeUtc: "21.08.2017 11:11:11”,
totalAmountAfterDiscounts: 2000,
shop: "shop-moscow”,
deliveryCost: 0,
items: [{
basePricePerItem: 100,
productId: "456”,
status: "Paid”,
quantity: 20,
priceOfLineAfterDiscounts: 2000
}]
}
Представим, что мы реализовали этот API и написали на него тесты. Наши клиенты начали им пользоваться, и мы осознали проблему.
Дело в том, что наш API подразумевает достаточно простую интеграцию со стороны клиента (например, интернет магазина). Предполагается, что сервис передачи данных о заказе должен вызываться клиентом на каждое изменение заказа на своей стороне. Таким образом снижается сложность интеграции, так как никакой логики, кроме трансляции, на стороне клиента нет.
Однако, бывает такое, что нашей системе безразличны некоторые изменения заказов, которые происходят в клиентской системе, так как для маркетинга и аналитики они не представляют никакого интереса. Такие изменения заказа на нашей стороне просто не будут ничем отличаться.
Кроме того, возможны ситуации, когда из-за каких-то сетевых ошибок клиент использует наш сервис по-нескольку раз для одного и того же изменения заказа, и мы сохраняем их все. Для победы над ошибками сетевого плана, когда одна и та же транзакция передаётся несколько раз, можно использовать ключ идемпотентности, но он не поможет для решения проблемы незначащих изменений. Мы реально по бизнесу не хотим хранить несколько изменений заказа, если они ничем не отличаются.
Сказано — сделано. Решение в лоб: давайте напишем код, который при поступлении нового изменения заказа будет среди уже существующих изменений того же заказа искать точно такое же, и если оно есть, то просто не будет сохранять ещё одно. Код будет выглядеть примерно так (если не думать об оптимизациях с вычислением хешей):
public void SaveOrder(OrderHistoryItem historyItem) {
if (historyItem.Order.IsNew)
{
SaveOrderCore(historyItem.Order);
SaveOrderHistoryCore(historyItem);
return;
}
var doesSameHistoryItemExist = historyItem.Order.History
.Where(h => h != historyItem)
.Where(h => h.IsEquivalent(historyItem))
.Any();
if (doesSameHistoryItemExist)
return;
SaveOrderHistoryCore(historyItem);
}
В этом коде мы проходим по всем историческим записям заказа и ищем такой же с помощью метода IsEquivalent. При этом IsEquivalent должен сравнивать каждое поле заказа, а потом искать соответствующие линии заказа и сравнивать каждое поле этих линий. Наивная реализация могла бы выглядеть вот так:
public bool IsEquivalent(OrderHistoryItem otherHistoryItem)
{
return IsTotalPriceEquivalent(otherHistoryItem) &&
IsDeliveryCostEquivalent(otherHistoryItem) &&
IsShopEquivalent(otherHistoryItem) &&
AreLinesEquivalent(otherHistoryItem);
}Тесты не спасают
OrderHistoryItem.IsEquivalent является довольно простой функцией, её довольно легко протестировать. Написав на неё тесты, мы убедимся, что выполняется бизнес-требование, запрещающее создавать несколько одинаковых состояний одного заказа.
Однако, тут есть проблема, которая заключается в том, что в enterprise-системах, подобно той, о которой мы сейчас поговорили, довольно много кода. В нашей компании работает 5 команд по 5 разработчиков, мы все развиваем один и тот же продукт, и уже при таких масштабах довольно сложно организовать работу так, чтобы каждый человек в каждой команде хорошо понимал предметную область каждой фичи продукта. В принципе, это и не очень нужно, так как команды делятся по фичам, но иногда фичи пересекаются, и тогда возникают сложности. Проблема, о которой я говорю, заключается в сложности большой системы, где есть много компонентов, которые должны быть связаны между собой.
Представим, что в заказ захотели добавить ещё одно поле: тип оплаты.
Исправим класс OrderHistoryItem:
class OrderHistoryItem
{
public Order Order { get; set; }
public DateTime OrderChangeDateTime { get; set; }
public decimal TotalAmountAfterDiscounts { get; set; }
public Customer Customer { get; set; }
public decimal DeliveryCost { get; set; }
public string ShopExternalId { get; set; }
public string PaymentType {get;set;}
}
Кроме того, мы должны добавить возможность передачи типа оплаты в наш сервис.
Ничего ли мы не забыли? Ах, ну да, нам нужно исправить IsEquivalent. И это очень печально, так как фича «не создавать дубли одинаковых изменений заказа» довольно незаметна, о ней сложно помнить. Напишет ли владелец продукта о том, как мы должны учитывать тип оплаты при сравнении заказов? Вспомнит ли об этом архитектор этой фичи? Подумает ли об этом программист, который будет реализовывать тип оплаты и добавлять его в сервис? Ведь если никто не вспомнит об этом, то никакой тест не упадёт, и в ситуации, когда к нам придёт изменение заказа сначала с одним типом оплаты, а потом с другим, второе изменение сохранено не будет.
Одним из важнейших критериев качества выбранной архитектуры приложения является простота модификации приложения под изменившиеся требования бизнеса. Также одним из столпов разработки нетривиального софта является уменьшение сложности. Проблема, которую я описал выше, как раз связана с высокой сложностью и с возможностью изменений. Короче говоря, архитектура должна быть такой, в которой было бы невозможно забыть о том, что нужно как-то сравнивать каждое из свойств истории изменения заказа.
Декларативные стратегии как выход
Мы написали императивный код проверки свойств заказа, который легко протестировать, но сложно проанализировать. Попробуем применить преимущества декларативного подхода. Сейчас метод IsEquivalent внутри вызывает методы IsTotalPriceEquivalent, IsDeliveryCostEquivalent и т.п. Введем интерфейс стратегии сравнения свойства заказа IOrderHistoryItemPropertyEqualityStrategy:
interface IOrderHistoryItemPropertyEqualityStrategy
{
bool ArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem);
}
Сделаем по реализации вместо каждого метода сравнения в IsEquivalent:
class TotalPriceOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy
{
public bool ArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem){
return firstOrderHistoryItem.TotalAmountAfterDiscounts == secondOrderHistoryItem.TotalAmountAfterDiscounts;
}
}class DeliveryCostOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy
{
public bool ArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem)
{
return firstOrderHistoryItem.DeliveryCost == secondOrderHistoryItem.DeliveryCost;
}
}
и сделаем сравнение более декларативным:
public bool IsEquivalent(OrderHistoryItem otherHistoryItem)
{
return new TotalPriceOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem) &&
new DeliveryCostOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem) &&
new ShopOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem) &&
new LinesOrderHistoryItemPropertyEqualityStrategy().ArePropertiesEquivalent(this, otherHistoryItem);
}
Немного отрефакторим:
private static IOrderHistoryItemPropertyEqualityStrategy[] equalityStrategies = new[] {
new TotalPriceOrderHistoryItemPropertyEqualityStrategy(),
new DeliveryCostOrderHistoryItemPropertyEqualityStrategy(),
new ShopOrderHistoryItemPropertyEqualityStrategy(),
new LinesOrderHistoryItemPropertyEqualityStrategy()
}
public bool IsEquivalent(OrderHistoryItem otherHistoryItem)
{
return equalityStrategies.All(strategy => strategy.ArePropertiesEquivalent(this, otherHistoryItem))
}
Теперь при добавлении нового свойства нужно создавать новую стратегию сравнения свойств и добавлять её в массив. Пока ничего не поменялось, мы всё так же не защищены от ошибки. Но за счет того, что код стал декларативным, мы можем что-нибудь проверить. Было бы здорово иметь возможность понять, за какое свойство какая стратегия отвечает. Тогда мы могли бы получить все свойства у заказа и проверить, что они все покрыты стратегиями. И это довольно несложно доработать. Добавим в интерфейс стратегии свойство PropertyName и реализуем его в каждой из стратегий:
public interface IOrderHistoryItemPropertyEqualityStrategy
{
string PropertyName {get;}
bool ArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem);
}
public class TotalPriceOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy
{
public string PropertyName => nameof(OrderHistoryItem.TotalAmountAfterDiscounts);
public bool ArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem){
return firstOrderHistoryItem.TotalAmountAfterDiscounts == secondOrderHistoryItem.TotalAmountAfterDiscounts;
}
}
Теперь напишем проверку:
public static void CheckOrderHistoryEqualityStrategies() {
var historyItemPropertyNames = typeof(OrderHistoryItem)
.GetProperties()
.Select(p => p.Name);
var equalityStrategiesPropertyNames = equalityStrategies
.Select(es => es.PropertyName);
var propertiesWithoutCorrespondingStrategy = historyItemPropertyNames
.Except(equalityStrategiesPropertyNames)
.ToArray();
if (!propertiesWithoutCorrespondingStrategy.Any())
return;
var missingPropertiesString = string.Join(", ", propertiesWithoutCorrespondingStrategy);
throw new InvalidOperationException($"Для свойств {missingPropertiesString} не зарегистрировано стратегии сравнения свойства");
}
Теперь достаточно написать на эту проверку тест, и можно быть спокойным за то, что мы что-нибудь забудем. Отдельно отмечу, что показанная реализация не полная. Во-первых, не учитываются свойства, которые не нужно сравнивать. Для них можно использовать вот такую реализацию стратегии:
public class NoCheckOrderHistoryItemPropertyEqualityStrategy : IOrderHistoryItemPropertyEqualityStrategy
{
private string propertyName;
public NoCheckOrderHistoryItemPropertyEqualityStrategy(string propertyName) {
this.propertyName = propertyName;
}
public string PropertyName => propertyName;
public bool ArePropertiesEquivalent(OrderHistoryItem firstOrderHistoryItem, OrderHistoryItem secondOrderHistoryItem)
{
return true;
}
}
Во-вторых, не показано, как сделать такую стратегию для сложных свойств, таких, как линии. Так как это делается совершенно аналогично, можно додумать это самостоятельно.
Кроме того, приверженность защитному программированию может подсказать, что стоит проверять, что не зарегистрировано более одной стратегии на каждое свойство, а также что не зарегистрировано стратегий на неизвестные свойства. Но это уже детали.
Другие примеры
Удаление потребителей
Ещё одним примером подобной проблемы у нас был функционал «удаление потребителей». Проблема с удалением была в том, что с потребителем у нас в системе связано довольно много всего, и при удалении нужно подумать обо всех сущностях, с которыми потребители связаны, возможно опосредовано через другие сущности, а ещё этот функционал должен был быть полиморфным, потому что у конечных заказчиков может быть больше сущностей, чем в основном продукте.
Как можно заметить, проблема в том же самом: если при добавлении новой сущности или даже просто какой-то ссылки забыть о том, что существует удаление, можно получить баг в продакшне о нарушении FK при попытке удаления.
Решение предлагается аналогичное: путем сравнительно сложного рефлексивного кода вполне возможно обойти всю модель данных, в том числе и в конечных реализациях для конкретного клиента, при старте приложения. В этой модели найти, какие сущности каким образом связаны с потребителями. После этого поискать, зарегистрированы ли для всех из этих сущностей стратегии удаления, и если чего-то нет — просто упасть.
Провайдеры времени пересчёта условий фильтрации
Ещё один пример той же проблемы: регистрация условий фильтрации. У нас есть довольно гибкий конструктор фильтров в UI: 
Эти фильтры используются как для фильтрации на страницах списков практически любых сущностей, а также для настройки всяких автоматически срабатывающих механик — триггеров.
Триггер работает так: каждые 15 минут он выбирает всех потребителей, над которыми он должен сработать, как раз используя сложное условие фильтрации, собранное в конструкторе, и по очереди их обрабатывает, отправляя им письмо, начисляя баллы или выдавая какой-нибудь приз или скидку.
Со временем стало понятно, что этот подход нужно оптимизировать, потому что триггеров много, потребителей много, и каждые 15 минут выбирать их всех тяжело. Оптимизация, которая напрашивалась сама собой — давайте не будем проверять потребителей, которые не изменялись, будем каждый раз смотреть только на изменившихся потребителей. Это был бы вполне валидный подход, если бы не одно но: некоторые условия фильтрации становятся верными для потребителя не из-за того, что с ним что-то произошло, а из-за того, что просто пришло время. Такими условиями являются всяческие временные условия, типа «до дня рождения осталось менее 5 дней».
Мы выделили все условия фильтрации, истинность которых может измениться от времени, и решили, что они должны сообщать нам время, за которое их имеет смысл пересчитывать. Грубо говоря, если мы считаем в днях, то имеет смысл пересчитывать каждые 12 часов, если в часах — то каждый час.
Проблема заключается в том, что модуль, в котором определены наши условия фильтрации — один из самых базовых, и нам бы не хотелось, чтобы в нём мы думали о том, каким должно быть время пересчёта. Поэтому интерфейс получения времени пересчета для условия фильтрации отделен от самих условий фильтрации, и мы снова попадаем в ситуацию, когда при создании нового условия фильтрации можно легко забыть о том, что оно требует указания времени пересчёта.
Решение такое же, как и раньше. У нас есть этап инициализации приложения, когда добавляются условия фильтрации. После этого есть этап, когда добавляются провайдеры времени пересчета для условий фильтрации в другом модуле. После этого критически важно проверить, что для всех условий фильтрации такие провайдеры зарегистрированы. Ни в коем случае нельзя полагаться на поведение по-умолчанию: если провайдер получения времени не зарегистрирован, считать, что фильтр не требует пересчёта. Эта возможность очень искушает, потому что рефакторинг в такой ситуации выглядит сильно проще: нужно зарегистрировать провайдеры только для тех условий фильтрации, которые были отобраны. Если выбрать такой подход, можно сэкономить время сейчас, но потом кто-то напишет новое условие фильтрации по времени, забудет о том, что нужно написать провайдер времени пересчёта для него, и триггер с таким условием фильтрации фактически не будет работать.
В заключение
Вот и всё. Постарался объяснить в статье своё видение проблемы «ненаписанного кода». К сожалению, мы не можем проверить таким образом отсутствие любого кода, нам нужно от чего-то отталкиваться, искать какие-то несоответствия в конфигурации большой системы, и падать, падать, падать, до тех пор, пока они есть.
Старайтесь делать архитектуру непробиваемой, чтобы при неправильном использовании она всегда ругалась, таким образом можно защититься от неприятных ошибок на бою. Старайтесь не идти на компромиссы с надежностью: если нужно отрефакторить много кода, чтобы снизить риски впоследствие, то стоит всё-таки потратить время.
