Niebezpieczny isEmpty() z utili Springa

Wyobraźmy sobie, że znajdujemy w projekcie taki kod w Javie (lub prawie identyczny — w Kotlinie):

public void assertNoViolations(Collection<ConstraintViolation> violations) {
    if (!isEmpty(violations)) {
        throw new IllegalStateException("violates constraints: " + violations);
    }
}

Na oko — kod jak kod, robi co trzeba, nuda. Co może pójść nie tak?

Metoda zgodnie z oczekiwaniami bezproblemowo akceptuje nulla. Rzuca wyjątkiem, gdy przekażemy jej listę zawierającą contraint violations. Jest jeden szkopuł:

assertNoViolations(emptyList());

java.lang.IllegalStateException: violates constraints: []

Jak to jest możliwe?

Statyczne importy to jeden z kontrowersyjnych elementów składni Javy, o ich stosowaniu lub nie toczą się debaty na blogach i StackOverflow. Zazwyczaj jako uzasadnione użycie przytacza się statyczny import assertEquals/assertThat w testach, żeby zyskać na czytelności. Od Javy 8 statyczne importy wchodzą dużo mocniej w główny kod projektu, a to ze względu na streamy. Możemy pisać „na piechotę”:

var datesByYear = dates.stream()
        .collect(Collectors.groupingBy(
                d -> d.get(ChronoField.YEAR),
                Collectors.mapping(
                        d -> d.format(DateTimeFormatter.ISO_DATE_TIME),
                        Collectors.toList()))
        );

ale użycie statycznych importów powoduje, że kod staje się zauważalnie bardziej przejrzysty i deklaratywny:

var datesByYear = dates.stream()
        .collect(groupingBy(
                d -> d.get(YEAR),
                mapping(
                        d -> d.format(ISO_DATE_TIME),
                        toList()))
        );

Statycznych importów w ten sposób używa nawet dokumentacja API streamów.

Podobny mechanizm na dostanie się bezpośrednio do metody istnieje też w Kotlinie, choć tam nie mówi się o statycznych importach — to po prostu importy. Poza pominięciem słowa „static” w używanym określeniu wszystko wygląda w zasadzie identycznie jak w Javie.

Skoro w powyższym kodzie używającym streamów statyczne importy przynoszą nam korzyść, czemu nie mamy wycisnąć z nich maksimum, importować tak wszędzie, gdzie zyskamy czytelność i kod bliższy zdaniom w języku naturalnym? Tu wracamy do fragmentu z początku artykułu :

    if (!isEmpty(violations)) {

(poniższa linijka będzie wyglądała identycznie w Kotlinie)

Czyta się to bardziej naturalnie niż CollectionUtils.isEmpty(violations). Problem tkwi w tym, czego nie widać: skąd pochodzi metoda isEmpty. Dużo klas na naszym classpathie deklaruje metody o takiej nazwie i jeśli mamy pecha importując metodę StringUtils.isEmpty z powszechnie używanej biblioteki spring-core dojdziemy do zwodniczego zachowania opisanego we wstępie.

Jeśli na spokojnie zastanowić się nad tym feralnym importem ze StringUtils, powinniśmy krzyknąć: ale jakim cudem się to kompiluje? Pchamy przecież kolekcję do utila od stringów, w języku z silnym typowaniem.

Niestety autorzy Springa zadeklarowali tę metodę jako

public static boolean isEmpty(Object str)

przez co mamy na naszym classpathie wyrwę w systemie kontroli typów czekającą, aż w nią wpadniemy. Weźmie bez skarg wszystko, co tylko przekażesz, ale zadziała poprawnie tylko dla stringów. Taka sygnatura jest zdaje się traktowana przez autorów jako feature niż bug — pewnie ze względu na SpEL i inne zabawy z parsowaniem i ładowaniem kontekstu aplikacji. Nie liczyłbym, że się zmieni w najbliższej przyszłości.

Co zrobić? Jak żyć?

Można przyjąć ryzyko na klatę i dalej trzymać się w naszym projekcie konwencji, że importujemy statycznie wszystko, co poprawi czytelność — ufamy, że nasze testy automatyczne i code review wyłapią błędy. Takich przypadków jak metoda isEmpty() nie powinno być w końcu wiele.

Jeśli uważamy, że faktycznie wystarczy uniknąć nielicznych pułapek, możemy zdecydować się na automatyczne ich wykrywanie. Niektórzy argumentują, że należy odciążać ludzi przy code review i sprawdzanie prostych reguł zrzucać na komputery. CheckStyle ma regułę AvoidStaticImport, ale działa odwrotnie, niż byśmy chcieli: przyjmuje listę miejsc, z których wolno importować, a nie czarną listę. Użytkownicy Kotlina mogą skorzystać z reguły ForbiddenImport narzędzia Detekt. Jest ona jednak traktowana jako problem ze „stylem”, nie przerywa sama w sobie buildu i nie da się tego przekonfigurować.

Inna opcja to przyjęcie, że nie da się określić uniwersalnej i prostej reguły, bo sprawa jest zbyt subiektywna. Skoro tak, to odpuszczamy sobie statyczne importy w sytuacji, gdy sama nazwa statycznej metody (bez klasy przed nią) nie daje jednoznacznej informacji, z czym mamy do czynienia. Czyli: kolektory z biblioteki standardowej tak (bo mają wystarczająco unikalne nazwy), ale już CośtamUtil.isEmpty() albo Optional.of() nie. Do takiej konwencji zdaje się stosować Oracle w dokumentacji: wracając do Collectors.toMap() mamy w przykładowym kodzie zaimportowaną statycznie metodę toMap(), ale już nie Function.identity().