Вместо «Посвящается ...»


Описанная ниже задача не была новаторской или чертовски полезной, компания в которой я работаю не получит за нее прибыль, а я премию.

Но эта задача была, а значит ее пришлось решить.

Intro


В статье вы часто будете встречать слово Lombok, прошу хейтеров не торопиться с выводами.
Я не собираюсь «топить» за Lombok или его отсутствие, я как Геральт Сапковского, стараюсь хранить нейтралитет, и могу спокойно и без дрожи в веке читать код как с Lombok, так и без оного.

Но на текущем проекте упомянутая библиотека присутствует, и что-то подсказывает мне, что наш проект такой не единственный.
Так вот.

Последнее время в java безусловно есть тренд к анноташкам. Во славу концепции fast fail часто параметры методов аннотируются аннотацией @NonNull (чтоб если что, как зашло — так и вышло пало).

Вариантов импорта для данной(или похожей по идеологии аннотации) довольно много, мы же, как наверняка уже стало понятно, остановимся на версии

import lombok.NonNull;

Если вы используете эту(или подобную) аннотацию, то имеете некоторый контракт, который необходимо проверить тестом и любой статический анализатор кода любезно это подскажет(Sonar точно подсказывает).

Протестировать эту аннотацию unit-тестом достаточно просто, проблема в том что такие тесты будут размножаться в вашем проекте со скоростью кроликов по весне, а кролики, как известно, нарушают принцип DRY.

В статье мы напишем небольшой тестовый фреймворк, для тестирования контракта аннотаций @NonNull(и для того чтоб Sonar не светил вам в глаз противным красным светом).

P.S На написания названия меня вдохновила песня группы PowerWolf, которая заиграла(ей богу) когда я писал название(в оригинале название звучит более позитивно)

Основная часть


Изначально мы тестировали аннотацию как-то так:

@Test
  void methodNameWithNullArgumentThrowException() {
    try {
      instance.getAnyType(null);
      fail("Exception not thrown");
    } catch (final NullPointerException e) {
      assertNotNull(e);
    }
  }

вызывали метод и подавали null в качестве параметра, аннотированного аннотацией @NonNull.
Получали NPE и оставались довольны(Sonar тоже радовался).

Потом стали делать то же самое, но с более модным assertThrow который работает через Supplier(мы же любим лямбды):

@TestUnitRepeatOnce
  void methodNameWithNullArgumentThrowException() {
    assertThrows(NullPointerException.class, () -> instance.getAnyType(null));
  }

Стильно. Модно. Молодежно.

Казалось бы можно и закончить, аннотации протестированы, чего же боле?

Проблема (не то чтобы проблема, но все же) данного способа тестирования «всплыла» когда в один прекрасный день я написал тест на метод, он благополучно отработал, а потом я заметил, что аннотации @NonNull на параметре нет.

Оно и понятно: вы вызываете тестовый метод, при этом не описываете поведение моковых классов, через when()/then(). Исполняющий поток благополучно заходит внутрь метода, где то внутри ловит NPE, на незамоканном (или замоканном, но без when()/then()) объекте, и падает, впрочем с NPE, как вы и предупреждали, а значит тест зеленый

Получается что тестируем мы в таком случае уже не аннотацию, а непонятно что. При правильной работе теста мы не должны были вообще зайти вглубь метода(свалившись на пороге).
У @NonNull аннотации Lombok есть одна особенность: если мы падаем с NPE на аннотации, в ошибку записывается имя параметра.

На это мы и завяжемся, после того как упадем с NPE, дополнительно будем проверять текст stacktrace, вот так:

exception.getCause().getMessage().equals(parameter.getName())

А если вдруг...
На случай если вдруг Lombok обновится и перестанет писать в stacktrace имя параметра получившего null, то пересмотрим лекцию Андрея Пангина по JVM TI и напишем плагинчик для JVM, в котором один фиг-таки передадим имя параметра.

Все бы вроде ничего, сейчас мы действительно проверяем то что надо, но проблема «кроликов» не решена.

Хотелось бы иметь некий инструмент, которому можно было бы сказать, например так:

@TestUnitRepeatOnce
  @SneakyThrows
  void nonNullAnnotationTest() {
    assertNonNullAnnotation(YourPerfectClass.class);
  }

а он бы сам пошел и просканировал все публичные методы указанного класса и проверил все их @NonNull параметры тестом.

Вы скажете, доставай рефлексию, и проверяй, есть ли на методе @NonNull и если есть пуляй в него null.

Все бы ничего, да RetentionPolicy не тот.

У всех аннотаций есть параметр RetentionPolicy, который может быть 3 типов: SOURCE, CLASS и RUNTIME, так вот у Lombok, по умолчанию RetentionPolicy.SOURCE, а это значит что в Runtime этой аннотации не видно и через reflection вы ее не найдете.

В нашем проекте аннотируются все параметры публичных методов(не считая примитивов), если подразумевается что параметр не может быть null, если подразумевается обратное — то параметр аннотируется спринговой @Nullable. На это можно завязаться, мы будем искать все публичные методы, и все параметр в них, не помеченные @Nullable и не являющиеся примитивами.
Подразумеваем, что для всех остальных случаев, на параметрах должна стоять аннотация @NonNull.

Для удобства по возможности будем расскидывать логику по приватным методам, для начала получим все публичные методы:

private List<Method> getPublicMethods(final Class clazz) {
    return Arrays.stream(clazz.getDeclaredMethods())
        .filter(METHOD_FILTER)
        .collect(toList());
  }

где METHOD_FILTER обычный предикат, в котором мы говорим что:

  • Метод должен быть public
  • Не должен быть syntetic(а такое случается когда у вас есть метод с raw параметром)
  • Не должен быть абстрактный(про абстрактные классы отдельно и ниже)
  • Имя метода не должно быть equals(на случай если какой то злой человек решит запулить на вход нашего фреймворка POJO класс с переопределенным equals())

После того как мы получили все нужные нам методы начинаем перебирать их в цикле,
если у метода вообще нет параметров, то это не наш кандидат:

if (method.getParameterCount() == 0) {
        continue;
      }

Если параметры есть, нам надо понять, аннотированы ли они @NonNull(точнее должны ли быть, согласно

логике
  • public method
  • не @Nullable
  • не примитив


Для этого сделаем мапку и положим в нее наши параметры по очередности следования в методе, а напротив них положим флаг, который будет говорить должна быть над параметром аннотация @NonNull или нет:

int nonNullAnnotationCount = 0;
      int index = 0;
      val parameterCurrentMethodArray = method.getParameters();
      val notNullAnnotationParameterMap = new HashMap<Integer, Boolean>();
      for (val parameter : parameterCurrentMethodArray) {
        if (isNull(parameter.getAnnotation(Nullable.class)) && isFalse(parameter.getType().isPrimitive())) {
          notNullAnnotationParameterMap.put(index++, true);
          nonNullAnnotationCount++;
        } else {
          notNullAnnotationParameterMap.put(index++, false);
        }
      }
      if (nonNullAnnotationCount == 0) {
        continue;
      }

эта мапка пригодится нам чтобы потом вызывать метод и передавать ему null во все параметры с аннотацией @NonNull поочередно, а не только в первый попавшийся.

Параметр nonNullAnnotationCount считает сколько параметров в методе должно быть аннотировано @NonNull, по нему будет определено число интераций вызова каждого метода.

Кстати если аннотаций @NonNull нет(параметры есть, но все примитивные либо @Nullable), то и говорить не о чем:


if (nonNullAnnotationCount == 0) {
        continue;
      }

Имеем на руках карту параметров. Знаем, сколько раз вызывать метод и в какие позиции пулять null, дело за малым (как я наивно полагал не разобравшись), нужно создавать instance класса и вызывать у них методы.

Проблемы начинаются когда понимаешь, насколько разные бывают instance: это может быть приватный класс, это может быть класс с одним дефолтным конструктором, с одним конструктором с параметрами, с таким и таким конструктором, абстрактный класс, интерфейс(со своими default методами, которые тоже public и которые тоже надо тестировать).

А когда мы соорудили-таки правдами и неправдами instance, нужно передать в метод invoke параметры и тут тоже раздолье: как создать instance финального класса? а Enum? а примитива? а массива примитивов(который тоже объект и тоже может быть аннотирован).

Ну давайте по порядку.

Первый случай это класс с одним приватным конструктором:

if (ONLY_ONE_PRIVATE_CONSTRUCTOR_FILTER.test(clazz)) {
            notNullAnnotationParameterMap.put(currentNullableIndex, false);
            method.invoke(clazz, invokeMethodParameterArray);
            makeErrorMessage(method);
          }

тут все просто вызываем у нашего метода invoke, передаем ему clazz который пришел из вне в тест и массив параметров, в котором уже заряжен null на первую позицию с флагом на аннотацию @NonNull(помните, выше мы создали карту @NonNull-ов)мы начинаем бежать в цикле и создавать массив параметров, поочередно меняя позицию null параметра, и обнуляя флаг перед вызовом метода, чтобы в следующей интерации другой параметр стал null.

В коде это выглядит так:

val invokeMethodParameterArray = new Object[parameterCurrentMethodArray.length];
        boolean hasNullParameter = false;
        int currentNullableIndex = 0;
        for (int i = 0; i < invokeMethodParameterArray.length; i++) {
          if (notNullAnnotationParameterMap.get(i) && isFalse(hasNullParameter)) {
            currentNullableIndex = i;
            invokeMethodParameterArray[i] = null;
            hasNullParameter = true;
          } else {
            mappingParameter(parameterCurrentMethodArray[i], invokeMethodParameterArray, i);
          }
        }

С первым вариантом инстанцирования разобрались.

Дальше интерфейсы, нельзя взять и создать instance интерфейса(у него даже конструктора нет).

Поэтому с интерфейсом это будет так:

if (INTERFACE_FILTER.test(clazz)) {
            notNullAnnotationParameterMap.put(currentNullableIndex, false);
            method.invoke(createInstanceByDynamicProxy(clazz, invokeMethodParameterArray), invokeMethodParameterArray);
            makeErrorMessage(method);
          }

createInstanceByDynamicProxy позволяет нам создать instance на класс, если он реализует хотя бы один интерфейс, либо сам является интерфейсом

Нюанс
имейте ввиду, что тут принципиально какие именно интерфейсы реализует класс, важен типовой интерфейс(а не какой-нибудь Comparable), в котором есть методы, которые вы реализуете в целевом классе иначе instance удивит вас своим типом

а внутри он какой-то такой:

private Object createInstanceByDynamicProxy(final Class clazz, final Object[] invokeMethodParameterArray) {
    return newProxyInstance(
        currentThread().getContextClassLoader(),
        new Class[]{clazz},
        (proxy, method1, args) -> {
          Constructor<Lookup> constructor = Lookup.class
              .getDeclaredConstructor(Class.class);
          constructor.setAccessible(true);
          constructor.newInstance(clazz)
              .in(clazz)
              .unreflectSpecial(method1, clazz)
              .bindTo(proxy)
              .invokeWithArguments(invokeMethodParameterArray);
          return null;
        }
    );
  }

Грабли
Кстати тут тоже были какие то грабли, уже не вспомню какие именно, их было много, но создавать проксю надо именно через Lookup.class

Следующий instance(мой любимый) это абстрактный класс. И тут Dynamic proxy нам уже не поможет, так как если абстрактный класс и реализует какой то интерфейс, то это явно не тот тип какой нам бы хотелось. И просто так взять и создать newInstance() у абстрактного класса мы не можем. Тут нам на помощь придет CGLIB, спринговая либа, которая создает прокси на основе наследования, но вот беда, целевой класс должен иметь default (без параметров) конструктор

Сплетня
Хотя судя по сплетням в интернете начиная со Spring 4 CGLIB умеет работать и без оного, так вот: Не работает!
Вариант для инстанцирования абстрактного класса будет такой:
if (isAbstract(clazz.getModifiers())) {
            createInstanceByCGLIB(clazz, method, invokeMethodParameterArray);
            makeErrorMessage();
          }

makeErrorMessage() который встречался уже в примерах кода, роняет тест, если мы вызывали метод с аннотированным @NonNull параметром передав null и он не упал, значит тест не отработал, надо падать.

Для маппинга параметров у нас один общий метод, который умеет мэппировать и мокировать как параметры конструктора, так и метода, выглядит он так:

private void mappingParameter(final Parameter parameter, final Object[] methodParam, final int index)
      throws InstantiationException, IllegalAccessException {
    if (isFinal(parameter.getType().getModifiers())) {
      if (parameter.getType().isEnum()) {
        methodParam[index] = Enum.valueOf(
            (Class<Enum>) (parameter.getType()),
            parameter.getType().getEnumConstants()[0].toString()
        );
      } else if (parameter.getType().isPrimitive()) {
        mappingPrimitiveName(parameter, methodParam, index);
      } else if (parameter.getType().getTypeName().equals("byte[]")) {
        methodParam[index] = new byte[0];
      } else {
        methodParam[index] = parameter.getType().newInstance();
      }
    } else {
      methodParam[index] = mock(parameter.getType());
    }
  }

Обратите внимание на создание Enum(вишенка на торте), вообщем нельзя просто так взять и создать Enum.

Здесь для финальных параметров свой маппинг, для нефинальных свой, а далее просто по тексту (кода).

Ну и после того как мы создали параметры для конструктора и для метода формируем наш instance:

val firstFindConstructor = clazz.getConstructors()[0];
          val constructorParameterArray = new Object[firstFindConstructor.getParameters().length];
          for (int i = 0; i < constructorParameterArray.length; i++) {
            mappingParameter(firstFindConstructor.getParameters()[i], constructorParameterArray, i);
          }
          notNullAnnotationParameterMap.put(currentNullableIndex, false);
          createAndInvoke(clazz, method, invokeMethodParameterArray, firstFindConstructor, constructorParameterArray);
          makeErrorMessage(method);

Мы уже точно знаем, что раз мы дошли до этого этапа кода, значит у нас есть минимум один конструктор, чтобы создать instance мы можем взять любой, поэтому берем первый попавшийся, смотрим, есть ли у него параметры в конструкторе и если нет то вызываем вот так:

method.invoke(spy(clazz.getConstructors()[0].newInstance()), invokeMethodParameterArray);


ну а если есть то так:
method.invoke(spy(clazz.getConstructors()[0].newInstance()), invokeMethodParameterArray);

Это та логика которая происходит в методе createAndInvoke() который вы видели чуть выше.
Полная версия тестового класса под спойлером, заливать на git я не стал, так как писал на рабочем проекте, но по сути это всего лишь один класс, который можно отнаследовать в ваших тестах и использовать.

Исходный код

public class TestUtil {

  private static final Predicate<Method> METHOD_FILTER = method ->
      isPublic(method.getModifiers())
          && isFalse(method.isSynthetic())
          && isFalse(isAbstract(method.getModifiers()))
          && isFalse(method.getName().equals("equals"));

  private static final Predicate<Class> ONLY_ONE_PRIVATE_CONSTRUCTOR_FILTER = clazz ->
      clazz.getConstructors().length == 0 && isFalse(clazz.isInterface());

  private static final Predicate<Class> INTERFACE_FILTER = clazz ->
      clazz.getConstructors().length == 0;

  private static final BiPredicate<Exception, Parameter> LOMBOK_ERROR_FILTER =
      (exception, parameter) -> isNull(exception.getCause().getMessage())
          || isFalse(exception.getCause().getMessage().equals(parameter.getName()));

protected void assertNonNullAnnotation(final Class clazz) throws Throwable {
    for (val method : getPublicMethods(clazz)) {
      if (method.getParameterCount() == 0) {
        continue;
      }
      int nonNullAnnotationCount = 0;
      int index = 0;
      val parameterCurrentMethodArray = method.getParameters();
      val notNullAnnotationParameterMap = new HashMap<Integer, Boolean>();
      for (val parameter : parameterCurrentMethodArray) {
        if (isNull(parameter.getAnnotation(Nullable.class)) && isFalse(parameter.getType().isPrimitive())) {
          notNullAnnotationParameterMap.put(index++, true);
          nonNullAnnotationCount++;
        } else {
          notNullAnnotationParameterMap.put(index++, false);
        }
      }
      if (nonNullAnnotationCount == 0) {
        continue;
      }
      for (int j = 0; j < nonNullAnnotationCount; j++) {
        val invokeMethodParameterArray = new Object[parameterCurrentMethodArray.length];
        boolean hasNullParameter = false;
        int currentNullableIndex = 0;
        for (int i = 0; i < invokeMethodParameterArray.length; i++) {
          if (notNullAnnotationParameterMap.get(i) && isFalse(hasNullParameter)) {
            currentNullableIndex = i;
            invokeMethodParameterArray[i] = null;
            hasNullParameter = true;
          } else {
            mappingParameter(parameterCurrentMethodArray[i], invokeMethodParameterArray, i);
          }
        }
        try {
          if (ONLY_ONE_PRIVATE_CONSTRUCTOR_FILTER.test(clazz)) {
            notNullAnnotationParameterMap.put(currentNullableIndex, false);
            method.invoke(clazz, invokeMethodParameterArray);
            makeErrorMessage(method);
          }
          if (INTERFACE_FILTER.test(clazz)) {
            notNullAnnotationParameterMap.put(currentNullableIndex, false);
            method.invoke(createInstanceByDynamicProxy(clazz, invokeMethodParameterArray), invokeMethodParameterArray);
            makeErrorMessage(method);
          }
          if (isAbstract(clazz.getModifiers())) {
            createInstanceByCGLIB(clazz, method, invokeMethodParameterArray);
            makeErrorMessage();
          }
          val firstFindConstructor = clazz.getConstructors()[0];
          val constructorParameterArray = new Object[firstFindConstructor.getParameters().length];
          for (int i = 0; i < constructorParameterArray.length; i++) {
            mappingParameter(firstFindConstructor.getParameters()[i], constructorParameterArray, i);
          }
          notNullAnnotationParameterMap.put(currentNullableIndex, false);
          createAndInvoke(clazz, method, invokeMethodParameterArray, firstFindConstructor, constructorParameterArray);
          makeErrorMessage(method);
        } catch (final Exception e) {
          if (LOMBOK_ERROR_FILTER.test(e, parameterCurrentMethodArray[currentNullableIndex])) {
            makeErrorMessage(method);
          }
        }
      }
    }
  }

  @SneakyThrows
  private void createAndInvoke(
      final Class clazz,
      final Method method,
      final Object[] invokeMethodParameterArray,
      final Constructor firstFindConstructor,
      final Object[] constructorParameterArray
  ) {
    if (firstFindConstructor.getParameters().length == 0) {
      method.invoke(spy(clazz.getConstructors()[0].newInstance()), invokeMethodParameterArray);
    } else {
      method.invoke(spy(clazz.getConstructors()[0].newInstance(constructorParameterArray)), invokeMethodParameterArray);
    }
  }

@SneakyThrows
  private void createInstanceByCGLIB(final Class clazz, final Method method, final Object[] invokeMethodParameterArray) {
    MethodInterceptor handler =
        (obj, method1, args, proxy) -> proxy.invoke(clazz, args);
    if (clazz.getConstructors().length > 0) {
      val firstFindConstructor = clazz.getConstructors()[0];
      val constructorParam = new Object[firstFindConstructor.getParameters().length];
      for (int i = 0; i < constructorParam.length; i++) {
        mappingParameter(firstFindConstructor.getParameters()[i], constructorParam, i);
      }
      for (val constructor : clazz.getConstructors()) {
        if (constructor.getParameters().length == 0) {
          val proxy = Enhancer.create(clazz, handler);
          method.invoke(proxy.getClass().newInstance(), invokeMethodParameterArray);
        }
      }
    }
  }

  private Object createInstanceByDynamicProxy(final Class clazz, final Object[] invokeMethodParameterArray) {
    return newProxyInstance(
        currentThread().getContextClassLoader(),
        new Class[]{clazz},
        (proxy, method1, args) -> {
          Constructor<Lookup> constructor = Lookup.class
              .getDeclaredConstructor(Class.class);
          constructor.setAccessible(true);
          constructor.newInstance(clazz)
              .in(clazz)
              .unreflectSpecial(method1, clazz)
              .bindTo(proxy)
              .invokeWithArguments(invokeMethodParameterArray);
          return null;
        }
    );
  }

  private void makeErrorMessage() {
    fail("Тестирование аннотации @NonNull в Абстрактных классах без DefaultConstructor не поддерживается");
  }

  private void makeErrorMessage(final Method method) {
    fail("Параметр в публичном методе " + method.getName() + " не аннотирован @NonNull");
  }

  private List<Method> getPublicMethods(final Class clazz) {
    return Arrays.stream(clazz.getDeclaredMethods())
        .filter(METHOD_FILTER)
        .collect(toList());
  }

  private void mappingParameter(final Parameter parameter, final Object[] methodParam, final int index)
      throws InstantiationException, IllegalAccessException {
    if (isFinal(parameter.getType().getModifiers())) {
      if (parameter.getType().isEnum()) {
        methodParam[index] = Enum.valueOf(
            (Class<Enum>) (parameter.getType()),
            parameter.getType().getEnumConstants()[0].toString()
        );
      } else if (parameter.getType().isPrimitive()) {
        mappingPrimitiveName(parameter, methodParam, index);
      } else if (parameter.getType().getTypeName().equals("byte[]")) {
        methodParam[index] = new byte[0];
      } else {
        methodParam[index] = parameter.getType().newInstance();
      }
    } else {
      methodParam[index] = mock(parameter.getType());
    }
  }

  private void mappingPrimitiveName(final Parameter parameter, final Object[] methodParam, final int index) {
    val name = parameter.getType().getName();
    if ("long".equals(name)) {
      methodParam[index] = 0L;
    } else if ("int".equals(name)) {
      methodParam[index] = 0;
    } else if ("byte".equals(name)) {
      methodParam[index] = (byte) 0;
    } else if ("short".equals(name)) {
      methodParam[index] = (short) 0;
    } else if ("double".equals(name)) {
      methodParam[index] = 0.0d;
    } else if ("float".equals(name)) {
      methodParam[index] = 0.0f;
    } else if ("boolean".equals(name)) {
      methodParam[index] = false;
    } else if ("char".equals(name)) {
      methodParam[index] = 'A';
    }
  }
}


Заключение


Данный код работает и тестирует аннотации в реальном проекте, на данный момент возможен только один вариант, когда все сказанное можно заколлапсить.

Объявить в классе ломбоковский setter (если найдется специалист, который ставит сеттер не в Pojo-классе, хотя чего только не бывает) и при этом поле на котором объявят сеттер будет не финальное.

Тогда фреймворк любезно скажет что мол есть публичный метод, а у него есть параметр на котором нет аннотации @NonNull, решение простое: объявить setter явно и аннотировать его параметр, исходя из контекста логики @NonNull/@Nullable.

Учтите, что если вы хотите как я, завязаться на имя параметра метода в своих тестах (или чем то еще), в Runtime по умолчанию недоступны имена переменных в методах, вы найдете там arg[0] и arg[1] и т.д.
Для включения отображения имен методов в Runtime используйте плагин Maven-а:


<plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-compiler-plugin</artifactId>
          <version>${maven.compiler.plugin.version}</version>
          <configuration>
            <source>${compile.target.source}</source/>
            <target>${compile.target.source}</target>
            <encoding>${project.build.sourceEncoding}</encoding>
            <compilerArgs><arg>-parameters</arg></compilerArgs>
          </configuration>
        </plugin>

и в частности этот ключ:

<compilerArgs><arg>-parameters</arg></compilerArgs>

Надеюсь вам было интересно.

Комментарии (18)


  1. BugM
    25.08.2019 18:34
    +1

    А зачем так сложно? Проверить исходники по критерию
    Должен быть NotNull у всех параметров которые:
    public method
    не @Nullable
    не примитив

    Можно гораздо проще на этапе прекоммит хуков или в любом другом месте вашей CI. И не надо никаких ничего не тестирующих тестов.


    1. mypanacea87 Автор
      26.08.2019 04:04

      а как Ваша хука внтутри работает не думали? а самому написать не хотелось? CI это хорошо, но тот же SOnar требует явного тестирования аннотаций, а не наличия аннотаций. Если бы вы писали JVM мне страшно подумать как бы осуществлялась верификация в байткод, например


      1. BugM
        26.08.2019 12:39

        Я в курсе как хуки внутри работают.


        Тестировать библиотечные функции своими тестами это чистый антипаттерн. Библиотеки должны тестироваться тестами библиотек.


        Когда пишешь jit тестируешь тоже jit. Когда пишешь бизнес логику тестируешь тоже бизнес логику. Все нижележащие уровни принимаются правильно работающими и не тестируются.


        1. poxvuibr
          26.08.2019 13:53

          Тестировать библиотечные функции своими тестами это чистый антипаттерн

          Ну надо же как-то проверять, что библиотечные функции применены правильно, то есть достигается именно тот эффект, которого ошидал разработчик.


  1. PastorGL
    25.08.2019 19:09

    Спасибо за красивый пример того, как ни в коем случае нельзя делать. Использую в обучающих материалах как есть (нарочно мне бы не удалось такого ужаса придумать).

    Когда кто-нибудь из моих младших коллег выкатывает нечто подобное, я обычно пишу в ревью что-то типа «если решение задачи получается настолько неожиданно громоздким и хрупким, возможно, стоило задуматься над правильностью её формулировки, прежде чем начинать писать код».


    1. sshikov
      25.08.2019 21:10
      +1

      Не, это вы зря. Я не про решение — возможно оно реально слишком громоздкое. Я про формулировку задачи — к сожалению, мы имеем то, что имеем. Если мы хотим в Java повесить на методы какую-то стороннюю логику, то аннотации — один из реально применимых способов. А вот что там дальше происходит в коде в момент выполнения — зачастую понять не так просто.

      Представьте, что у вас вместо @Nullable стоит @Transactional? Как вы предлагаете тестировать поведение метода с этой аннотацией? Как вы разделите поведение самого метода, и той логики, которую на него навесил условный ломбок (или условный спринг)? Задача вполне реальная, и я думаю надо автору сказать спасибо за то, что на ней было акцентировано внимание.


      1. mypanacea87 Автор
        26.08.2019 03:55

        то то и оно, статья не руководство к действию, а лишь повод и средство более детально понять как работают аннотации, как рефлексия и прокси на объекты.
        Автору выше, который с юными коллегами: а Вы Spring тоже брезгуете юзать? или предпочитчаете юзать не думая, что внутри него есть рефлексия или Вам приятно думать что аннотация работает сама по себе?
        Это небольшой тестовый фреймворк, даже если конкретно эта задача Вам кажется фейковой, все то что в ней использовано — используется в джавовых фреймворках.
        Это «громозлкое решение» — это 180 страниц кода и один класс, странное у вас представление о громоздкости. Попробуйте сломать, это «хрупкое решение». И это тест, утилька для того чтобы убрать повторение кода по вашим тестам. Да, мне не кажется что это самые полезные тесты, на аннотацию @NonNull, но мой тим лид требует тестить, дальше что? Странно, что при всем Вашем опыте, Вы не знаете, что проекты разные бывают и требования на них те же, хотя судя по пафосу, предположу что вы сидите 10 лет на одном проекте и брызжете пафосом на более молодых колег.
        P.S наверняка со своей принципиальностью в общем потоке просмотрели как не ок, так и годные идеи.
        P.S.S Я бы вас с удовольствием отревъювил. Со всеми выкладками на доки и мировые best practices.


        1. PastorGL
          26.08.2019 10:28
          +1

          Я мог бы ответить конструктивно, но вы уже перешли на личности.


          Я знаю, как работает рефлексия, писал и на спринге, и на EE (от разных вендоров) и даже собирал собственный контейнер из запчастей. Дважды, в разных окружениях. Это не говоря о плагинах для мавена и грэдла для нужд автоматизации разработки моих проектов. Таки 20 лет на джаве пишу с периодическими перерывами на другие платформы, потому меня и берут в команду как эксперта или играющего тренера.


          Давайте лучше не будем меряться толщиной mojo.


          Касательно «пафоса» — я искренне благодарен вам за красивый антипример антипаттерна. Спасибо.


      1. PastorGL
        26.08.2019 11:12

        C @Transactional проблем нет, коли он рантаймовый. Клин клином, аспект поверх аспекта, или можно в контейнер вклиниться, благо, большинство позволяют.

        Автор же зачем-то тестирует наличие аннотации, у которой Retention=SOURCE. На этапе выполнения. Если уж на самом деле такое настолько нужно, то это зона ответственности компилятора. Следовательно, выбран не тот инструмент на неправильной фазе жизненного цикла.


        1. sshikov
          26.08.2019 21:40
          +1

          >это зона ответственности компилятора.
          Ну, мне кажется — не обязательно. Например какого-то AST трансформера.

          Опять же — я сразу допустил, что инструмент не тот. Что вы можете предложить лучше для тестирования аннотаций? Ну так чтобы практически проверить?

          >аспект поверх аспекта, или можно в контейнер вклиниться, благо, большинство позволяют.
          Ну и чем это будет отличаться от описанного? :) Тоже ведь будет «магия» вполне возможно.


  1. mypanacea87 Автор
    26.08.2019 03:58

    А ко всем остальным минусерам, без аргументов, поражает ваше стадное чувство.
    Как только я опубликовал статью, в ту же секунду кинул ее в один из своих чатов в телеграм, в ту же секунду появился первый просмотр и первый минус, то есть даже прочитать название до конца было нереально. Ну а дальше стадное чувство сработало. Спасибо sshikov, что остановили это бред.


  1. AstarothAst
    26.08.2019 11:16
    +1

    Я может быть чего-то не понял, но. Получается вы взяли ломбок, взяли его аннотацию, и написали свой минифреймворк, который проверяет, что ломбок корректно обрабатывает собственную аннотацию во всех случаях, когда вы ее используете. Это как-то… избыточно. Логика подсказывает, что либо ломбок работает корректно — везде! — либо не корректно, но опять же — везде. То есть достаточно проверить только один раз, при чем можно специальный тестовый кейс. Или я чего-то не понял?


    1. igormich88
      26.08.2019 17:04

      У меня ощущение что эти тесты, сами нуждаются в тестировании. Ну в смысле по моему тест должен выглядеть проще чем тестируемый код.


  1. reforms
    26.08.2019 11:16

    Обратите внимание на создание Enum(вишенка на торте), вообщем нельзя просто так взять и создать Enum.


    Не совсем понятен код:
        if (parameter.getType().isEnum()) {
            methodParam[index] = Enum.valueOf(
                (Class<Enum>) (parameter.getType()),
                parameter.getType().getEnumConstants()[0].toString()
            );
          }
    

    В чем вишенка и почему нельзя упростить:
        if (parameter.getType().isEnum()) {
            methodParam[index] = parameter.getType().getEnumConstants()[0];
        }
    


    С enum действительно все не просто, такой код заработает в TestUtil:

        public enum ExtEnum {
        
        A("1") {}, // фигурные скобки заданы умышлено
        B("2") {}; // фигурные скобки заданы умышлено
        
        private final String text;
        
        
        ExtEnum(String text) {
            this.text = text;
        }
        
        String getTextWith(String title) {
            return title + ":" + text;
        }    
    }
    


  1. nehaev
    26.08.2019 14:34

    Статья навеяла несколько вопросов о работе с null:


    1. Можно ли прикрутить к @NonNull какие-нибудь статические проверки, что null туда реально не попадает? Что делать (и как вообще это узнать), когда аннотация поставлена неправильно?


    2. Стоит ли NPE в рантайме, которую генерирует Lombok, таких усилий по разметке аннотациями всех полей, тестированию этого всего? Будет ли сильно хуже, если все эти аннотации просто выкинуть?


    3. Почему нельзя использовать Optional? Оно хотя бы гарантирует корректную обработку nullable-значений на уровне компилятора, их уже не перепутаешь с non-null (хотя в обратную сторону — легко).



    1. igormich88
      26.08.2019 17:49

      Моё мнение:

      1. Насколько я понимаю IDEA например может делать что-то такое. Но в чистой Java довольно сложно понять по переменной или по полю что там точно-точно не может быть null, без явных проверок в коде.
      2. Разметка аннотациями еще и для тех кто будет читать и использовать этот код, во многих методах непонятно можно ли передавать параметр по умолчанию null без чтения документации. Зачем тестировать аннотацию и кодогенерацию я не знаю.
      3. С Optional код часто выглядит хуже (субъективное мнение), ну и не спасёт от тех кто передаст null вместо Optional.

      PS: Если проблема с null для вас актуальна, то я бы посоветовал попробовать Kotlin.


      1. nehaev
        26.08.2019 18:53

        Попробую возразить по поводу Optional.


        Возможность передачи null вместо Optional это конечно беда, от этого придется защищаться статическими проверками (вроде как Findbugs что-то умеет http://findbugs.sourceforge.net/bugDescriptions.html#NP_OPTIONAL_RETURN_NULL). Плюс видимо придется защищаться от Optional.get().


        Код, конечно может выглядеть более громоздко, чем аннотация, но если учесть, что он реально защищает от NPE, а аннотации — нет, может оно того и стоит.


    1. nehaev
      26.08.2019 18:25

      Отвечая на свой же первый вопрос, нагуглилась вот такая штука: https://checkerframework.org/manual/#nullness-checker. Есть ли у кого-нибудь опыт использования на практике? По идее, статическая проверка должна быть лучше, чем Lombok...


      UPD: есть еще набор проверок в Findbugs: http://findbugs.sourceforge.net/bugDescriptions.html#NP_ALWAYS_NULL