Как стать автором
Обновить

Комментарии 15

В общем случае @Entity не должна быть @Data классом, т.к. дата-классы это Value-objects, и, например, проверяют идентичность объектов по всем полям. Для entity же идентичность обычно значит совпадение ID или NaturalId, это важно для многих внутренних механизмов JPA.


Здесь Влад рассказывает как раз как правильно реализовать equals / hashCode.

спасибо за уточнение и ресурсы.
Если Вы собираетесь в дальнейшем использовать этот код, Вам рано или поздно придётся распечатать Data и заменить на отдельные аннотации — Setter, ToString и проч. Иначе, у Вас будут существенные ограничения на кастомизацию полей.

Логика в контроллерах? Ай-ай-ай. Хотя, о том, что CMainViewReport является контроллером, нужно ещё догадаться.

Исключения ради исключений — тоже не очень хорошо.
Исключения ради исключений — тоже не очень хорошо.

Сказал человек, генерирующий исключения на ровном месте и пишущий интерсепторы для их последующей обработки :)
CMainViewReport здесь скорее мне ай-ай-ай за пихание всего в master ветку. А на самом деле я логику отрабатываю по месту, после этого рождается сервис, если он нужен.
А исключения, да здесь, обеими руками за и головой также. Jaskson удалось отловить благодаря именованному исключению.
Первый момент кто вызвал, а дальше уже разбор.
а по поводу Data alek_sys уже навел, что лучше бы его не юзать в entity. Спасибо, за доп. подтверждение.
Здравствуйте, я посмотрел ваш код на гитхабе и написал несколько замечаний с рекомендациями, вообщем небольшое такое код-ревью провёл, раз уж вы выложили код на хабр:)

1. для чего в CProductService аннотации @Transactional над самим классом и методами create и update? Они явно лишние на мой взгляд, что касается метода delete, я не помню его поведение в случае попытки удалить несуществующую сущность, по-моему падает эксепшн — что лучше, 2 раза сходить в БД или ловить эксепшны, вопрос спорный, если у кого есть какие мысли — пишите, мне тоже интересно
2.Хьюстон, тут у нас явно проблемы…
final CProduct [] arr=new CProduct[1];
        arr[0]=null;
        findAll().stream().forEach(x -> {if(name.compareTo(x.getName()) == 0){
            arr[0]=x;
        }});

        if(arr[0] != null){
            return arr[0];
        }else{
            throw new CProductNotFoundException(name);
        }

a. Почему findAll()? вытаскивать все продукты из БД и бегать по ним стримом на стороне сервера в будущем будет очень жирно, тем более что уже есть метод productRepository.findByName
b.
name.compareTo(x.getName()) == 0
— весьма странное решение, если вы не собираетесь ничего сортировать, достаточно equals или equalsIgnoreCase
с. создавать массив с 1 элементом чтобы его поменять… примерно тоже самое будет выглядеть как-нибудь вот так:
productRepository.findByName(name) 
                         .stream()
                         .filter(it -> it.getName().equalsIgnoreCase(name))
                         .findAny()
                         .orElseThrow(() -> new CProductNotFoundException(name))

Но вообще я не понял смысла этого метода т.к. уже есть findByName, и по сигнатуре они похожи:
public List<CProduct> findByName(String name)
public CProduct findOne(String name) throws CProductNotFoundException 

… можно было сделать что-то вроде:
        List<CProduct> products = productRepository.findByName(name);
        if (products.isEmpty()){
            throw new CProductNotFoundException(name);
        }
        return products.get(0);

но ещё короче было бы сделать всё в репозитории с помощью аннотации Query(nativeQuery = true, ...)
3. пункт 1 и 2 актуальны для всех сервисов как оказалось, т.к. там везде примерно одинаково
4. вообще в моей практике пакет с @RestController'ами всегда так и называли — controller, реже — rest… но это уже на ваше предусмотрение, просто resources я ещё не встречал
5. пакеты названы то в единственном числе, то во множественном — нужно единообразие
6. возвращать из контроллеров ResponseEntity.ok нет смысла, они итак будут по-умолчанию 200, если нужны разные статусы, тогда ок, но можно было бы это делать через @ControllerAdvice, т.к. как правило логика примерно одинакова
7. CHomeResource возвращает статику, можно было бы возвращать чистый html из ресурсов
8. в CMViewRepResource
@GetMapping
    public ResponseEntity<byte[]> report(String moduleName){
        byte[] bytes = null;
        try {
            bytes = jreportService.getMainReportPdfAsBytes(main_template_path);
            return ResponseEntity.ok()
                    .header("Content-Type", "application/pdf; charset=UTF-8")
                    .header("Content-Disposition", "inline; filename=\"" + moduleName + ".pdf\"")
                    .body(bytes);
        } catch (Exception e) {
            e.printStackTrace();
        }
        return null;
    }

очень странно что вы возвращаете null, когда у вас есть ResponseEntity, можно возвращать код с ошибкой
9. совсем нет логирования
10. JasperReportService не бин, он не будет инжектиться в CMViewRepResource, будет NPE
11. лучше пользоваться интерфейсами, когда пишите сервисы
Не ожидал код ревью, огромная благодарность.
Я думаю под Lombock Вы подразумевали Project Lombok?
ок, подправил

А тут можно задавать вопросы?

Борюсь со Spring и Firebird (сервер v 2.5). Вопросы:

  1. Firebird и Intellij IDEA. Использую встроенного в Ultimate клиента для работы с БД. Прописываю параметры подключения, жму Test Connection и получаю такое:

    DBMS: Red Database 2.5 (ver. WI-V2.5.0.10968) Case sensitivity: plain=upper, delimited=exact Driver: Jaybird JCA/JDBC driver (ver. 4.0.4, JDBC4.3) Effective version: Firebird (ver. 2.5) Ping: 50 ms (keep-alive query results in error)

    [42000][335544851] Dynamic SQL Error; SQL error code = -104; Unexpected end of command - line 1, column 8 [SQLState:42000, ISC error code:335544851]

    [00000][335544436] SQL error code = -104 [42000][335544851] Unexpected end of command - line 1, column 8.

    При этом всё работает, таблички показывает, запросы в консоли работают и пр. Но ошибка напрягает. Нашёл что сервер БД (?) имеет нестандартный терминатор (символ конца строки), как это победить в запросах вроде есть рецепты, но как сделать чтобы тест в IDEA проходил нормально?

  2. Теперь со Спрингом. При запросах из кода получаю такие предупреждения:

    WARN 11008 --- [ntContainer#0-1] o.h.engine.jdbc.spi.SqlExceptionHelper : SQL Warning Code: 0, SQLState: null
    WARN 11008 --- [ntContainer#0-1] o.h.engine.jdbc.spi.SqlExceptionHelper : Connection.isValid does not support non-zero timeouts, timeout value 5 has been ignored

    Опять же, запрос проходит, данные получены. Но где-то что-то недонастроено. Что и где подкрутить?

можно уточнить, используется jpa или напрямую sql пишутся?

у меня хороший перерыв с java вышел, но все равно интересно.

Ну Спринг же, значит JPA в реализации Spring Data :)

Со вторым вопросом уже разобрался: надо было понизить версию драйвера JDBC до 2.2, видимо, старому серверу СУБД - старый драйвер :)

С первым вопросом этот трюк не прошёл, пока непонятно что делать.

А имена колонок, табличек не совпадают с зарезервированными в firebird?

Да нет, самые обычные имена, база полностью рабочая. Родная утилита IB Expert работает с ней без проблем, только вот IDEA ругается, хотя потом тоже работает.

Зарегистрируйтесь на Хабре, чтобы оставить комментарий

Публикации

Истории