Skip to content

Доработка поддержки генерации изображений#99

Open
linarkou wants to merge 5 commits intomainfrom
GH-76
Open

Доработка поддержки генерации изображений#99
linarkou wants to merge 5 commits intomainfrom
GH-76

Conversation

@linarkou
Copy link
Collaborator

@linarkou linarkou commented Feb 28, 2026

  • Add URL response format support for image generation(b64_json/url)
  • Add getFileUrl() method to GigaChatApi for constructing image URLs
  • Update GigaChatImageModel to handle both base64 and URL response formats
  • Remove default system prompt for ImageModel
  • Update README with image generation examples and links
  • Remove unnecessary spring-web dependency from pom.xml

Resolves #76

- Add getFileUrl() method to GigaChatApi for constructing image URLs
- Update GigaChatImageModel to handle both base64 and URL response formats
- Remove default system prompt for ImageModel
- Update README files with image generation examples and links
- Remove unnecessary spring-web dependency from pom.xml

Signed-off-by: Linar Abzaltdinov <abzaltdinov@gmail.com>
@linarkou linarkou requested a review from a-simeshin February 28, 2026 17:52
@linarkou linarkou self-assigned this Feb 28, 2026
@linarkou linarkou added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 28, 2026
@github-actions
Copy link
Contributor

Замечания по PR

Найдено несколько проблем, требующих внимания.

Критично: style = null -> null-контент в системном сообщении

После удаления SYSTEM_PROMPT как значения по умолчанию поле style теперь равно null, если явно не задано. При этом buildCompletionRequest безусловно добавляет системное сообщение с sys.setContent(prompt.getOptions().getStyle()). Если и в defaultOptions, и в prompt style не задан — в API уйдёт сообщение с null-контентом, что вызовет ошибку. Нужна проверка на null в buildCompletionRequest (или возврат минимального дефолтного промпта).

@Setter на уровне класса в GigaChatImageOptions

Lombok-аннотация @Setter генерирует сеттеры для всех полей, включая responseFormat. Ранее были только явные сеттеры для style и model. Это нарушает инкапсуляцию builder-паттерна.

Wildcard-импорт в GigaChatImageModel

import org.springframework.ai.image.* — нарушение Java best practices, предыдущий код использовал явные импорты.

normalizePrompt — антипаттерн тестирования

Метод стал package-private ради unit-тестов. Лучше тестировать через публичный call(), не открывая детали реализации.

Неточность в документации (docs/image-model.md, строка 69)

«Необходимо получить fileId...» — fileId доступен через метаданные в обоих форматах ответа, не только в URL.

        - Replaced import org.springframework.ai.image.*; with explicit imports

    2. Removed `@Setter` annotation (GigaChatImageOptions.java)
        - Removed class-level @Setter that broke builder pattern encapsulation
        - Added @Builder.Default for the style field with null value

    3. Made `normalizePrompt` method private (GigaChatImageModel.java)
        - Changed from package-private back to private
        - Removed tests that directly called this method

    4. Fixed documentation about `fileId` (docs/image-model.md)
        - Removed misleading statement that fileId is only available for URL format
        - Added clarifying comments that fileId is available in metadata for both formats
        - Updated "When to use URL" section with accurate use cases

    5. Fixed potential null style issue (GigaChatImageModel.java)
        - Added null check in buildCompletionRequest() method
        - System message is now only added when style != null

    6. Updated tests (GigaChatImageModelTest.java)
        - Removed 4 tests that directly tested the private normalizePrompt method
        - Kept tests that verify behavior through the public call() method

Signed-off-by: Linar Abzaltdinov <abzaltdinov@gmail.com>
@linarkou
Copy link
Collaborator Author

linarkou commented Mar 3, 2026

@claude review pr once again

@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @linarkou's task in 2m 6s —— View job


Повторный обзор PR

  • Gather context
  • Read changed files
  • Analyze issues from previous review
  • Post comprehensive review

Статус прежних замечаний

Все замечания из предыдущего review устранены:

Замечание Статус
Wildcard-импорт в GigaChatImageModel.java ✅ Исправлено
null-контент в системном сообщении ✅ Исправлено — добавлена проверка if (style != null)
normalizePrompt — package-private видимость ✅ Исправлено — теперь private
@Setter на уровне класса в GigaChatImageOptions ✅ Отсутствует
Неточность в документации про fileId ✅ Исправлено

Новые / оставшиеся замечания

1. Wildcard-импорт в ImageController.java (Minor)

spring-ai-gigachat-example/src/main/java/chat/giga/springai/example/ImageController.java:9

import org.springframework.ai.image.*;   // нарушение Java best practices

Та же проблема, что была исправлена в GigaChatImageModel.java. Стоит заменить на явные импорты: ImageModel, ImageGeneration, ImageMessage, ImageOptions, ImagePrompt, ImageResponse.

Fix this →


2. Недостижимый код в processRequest (Minor)

spring-ai-gigachat/src/main/java/chat/giga/springai/image/GigaChatImageModel.java:86–90

String fileId = extractFileId(completion);  // throws IllegalStateException if no match
if (fileId == null) {                        // <- UNREACHABLE
    log.warn("Unable to extract file_id ...");
    return new ImageResponse(List.of());
}

Метод extractFileId никогда не возвращает null — он либо возвращает строку, либо бросает IllegalStateException. Проверка if (fileId == null) никогда не выполняется и вводит в заблуждение. Следует либо:

  • убрать null-проверку (полагаясь на IllegalStateException из extractFileId), или
  • переделать extractFileId чтобы возвращал Optional<String> и обрабатывать отсутствие через orElse/orElseThrow.

Fix this →


3. URL-формат требует аутентификации — не задокументировано (Minor)

spring-ai-gigachat/src/main/java/chat/giga/springai/api/chat/GigaChatApi.java:259–262

public String getFileUrl(String fileId) {
    String base = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl;
    return base + "/files/" + fileId + "/content";
}

URL /files/{fileId}/content требует Bearer-токен (как и все остальные вызовы GigaChat API). Пользователь, получив url из ImageResponse, не сможет обратиться к нему напрямую без передачи заголовка Authorization. Это потенциально неожиданное поведение — стоит добавить примечание в документацию (docs/image-model.md).


4. @Builder.Default с явным null — избыточно (Trivial)

spring-ai-gigachat/src/main/java/chat/giga/springai/image/GigaChatImageOptions.java:15–16

@Builder.Default
private String style = null;   // null — и без того дефолт для String

Аннотация @Builder.Default не нужна, если значение по умолчанию — null, поскольку null — это стандартное значение для объектных полей. Достаточно просто private String style;.


Общая оценка

Код значительно улучшен относительно первоначальной версии. Критические баги (null-контент, видимость методов, import) исправлены. Оставшиеся замечания — незначительные (minor/trivial). PR готов к мёржу после устранения пункта 2 (недостижимый код) и при желании — пункта 1 (wildcard import).

@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @linarkou's task in 2m 15s —— View job


Обзор PR


✅ Исправленные замечания

Все критичные замечания из предыдущего ревью устранены:

Замечание Статус
Wildcard-импорт в GigaChatImageModel ✅ Исправлено — явные импорты
@Setter на уровне класса в GigaChatImageOptions ✅ Убран
normalizePrompt — package-private ради тестов ✅ Исправлено — метод снова private, тесты через call()
style = null → null-контент в системном сообщении ✅ Исправлено — добавлена проверка if (style != null)
Документация: fileId только в URL-формате ✅ Исправлено — теперь правильно описаны оба формата

⚠️ Оставшиеся замечания

1. Мёртвый код в processRequestif (fileId == null)

GigaChatImageModel.java, строки 87–90:

String fileId = extractFileId(completion);
if (fileId == null) {                                   // ← никогда не выполнится
    log.warn("Unable to extract file_id ...");
    return new ImageResponse(List.of());
}

Метод extractFileId никогда не возвращает null — при отсутствии тега <img> он бросает IllegalStateException. Проверка на null недостижима. Нужно либо убрать мёртвый null-check, либо изменить extractFileId так, чтобы возвращал null вместо исключения, в зависимости от желаемой семантики.

Fix this →


2. Wildcard-импорт в примере ImageController.java

spring-ai-gigachat-example/src/main/java/chat/giga/springai/example/ImageController.java, строка 9:

import org.springframework.ai.image.*;  // ← wildcard

Аналогичное нарушение исправили в GigaChatImageModel, но в ImageController оно осталось. Даже в примере стоит придерживаться явных импортов для последовательности.

Fix this →


3. Избыточная аннотация @Builder.Default для style = null

GigaChatImageOptions.java, строка 15:

@Builder.Default
private String style = null;  // null — это Java-умолчание, @Builder.Default здесь ничего не даёт

@Builder.Default имеет смысл только при ненулевых значениях по умолчанию (как у model и responseFormat). На null она ничего не делает — это косметика, но может ввести читателя в заблуждение. Аннотацию можно убрать.


Общая оценка

PR в хорошем состоянии — все критичные замечания предыдущего ревью устранены. Реализация корректна: null-guard для style, явные импорты в основном модуле, нормализация опций через call(). Оставшиеся замечания минорные.

linarkou added 2 commits March 4, 2026 01:03
Signed-off-by: Linar Abzaltdinov <abzaltdinov@gmail.com>
Signed-off-by: Linar Abzaltdinov <abzaltdinov@gmail.com>
@linarkou
Copy link
Collaborator Author

linarkou commented Mar 3, 2026

@claude я исправил минорные замечания

@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @linarkou's task in 48s —— View job


Проверка исправлений

  • Читаю изменённые файлы
  • Проверяю исправление: wildcard-импорт в ImageController.java
  • Проверяю исправление: мёртвый код if (fileId == null) в GigaChatImageModel.java
  • Проверяю исправление: @Builder.Default для style = null в GigaChatImageOptions.java
  • Проверяю исправление: документация по URL-формату и аутентификации
  • Публикую итоговый обзор

Итог проверки

✅ Все минорные замечания исправлены

Замечание Статус
Wildcard-импорт в ImageController.java ✅ Исправлено — явные импорты ImageGeneration, ImageModel, ImagePrompt, ImageResponse
Мёртвый код if (fileId == null) в GigaChatImageModel.java ✅ Исправлено — extractFileId теперь возвращает null вместо IllegalStateException, проверка достижима
Документация: URL-формат требует аутентификации ✅ Исправлено — в docs/image-model.md добавлена заметка о необходимости ключа доступа

⚠️ Одно тривиальное замечание осталось

@Builder.Default с явным null в GigaChatImageOptions.java (строка 15–16):

@Builder.Default
private String style = null;   // @Builder.Default не нужен для null

Это сугубо косметический вопрос — никакого влияния на поведение нет. Можно оставить как есть.


Общая оценка

PR полностью готов к мёржу. Все критические и минорные замечания устранены. Единственный оставшийся пункт — тривиальный косметический, на усмотрение автора.

@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @linarkou's task in 1m 17s —— View job


Результаты проверки исправлений

  • Собрать контекст
  • Проверить исправления по замечаниям
  • Опубликовать результаты

Статус замечаний

# Замечание Статус
1 Wildcard-импорт в ImageController.java ✅ Исправлено
2 Недостижимый код (if (fileId == null)) ✅ Исправлено
3 URL-формат требует аутентификации — не задокументировано ✅ Исправлено
4 @Builder.Default для style = null — избыточно ⚠️ Остаётся

Детали

1. Wildcard-импорт — Заменён на явные импорты: ImageGeneration, ImageModel, ImagePrompt, ImageResponse. ✅

2. Недостижимый кодextractFileId теперь возвращает null (вместо IllegalStateException), что делает проверку if (fileId == null) корректной и достижимой. ✅

3. Документация URL + аутентификация — В docs/image-model.md добавлен пункт в секцию «Когда использовать Base64»: «Нужно отдать изображение внешнему сервису, у которого нет ключа доступа к GigaChat API» — пользователь понимает, что URL требует доступа к API. ✅

4. @Builder.Default для nullGigaChatImageOptions.java:15 по-прежнему содержит @Builder.Default private String style = null;. Замечание тривиальное, оставляю на ваше усмотрение.


Итог

PR в отличном состоянии. Все критичные и минорные замечания устранены. Единственное оставшееся — тривиальная косметика с @Builder.Default. PR готов к мёржу.

Signed-off-by: Linar Abzaltdinov <abzaltdinov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Доработка поддержки ImageModel

1 participant