Skip to content

Remove deprecated authentication properties and update related code#87

Merged
linarkou merged 5 commits intomainfrom
remove-deprecations
Jan 30, 2026
Merged

Remove deprecated authentication properties and update related code#87
linarkou merged 5 commits intomainfrom
remove-deprecations

Conversation

@linarkou
Copy link
Collaborator

  • Removed deprecated fields and methods in GigaChatApiProperties related to authentication
  • Updated GigaChatAutoConfiguration to use new authentication properties
  • Updated GigaChatEmbeddingModel to extend AbstractEmbeddingModel and use known embedding dimensions
  • Updated GigaChatApi to use new authentication properties and constants
  • Updated GigaChatAuthProperties with helper methods and documentation
  • Updated GigaChatOAuthClient to use new authentication properties
  • Updated EmbeddingsModel to implement EmbeddingModelDescription and add dimensions
  • Updated tests to reflect changes in authentication and model usage
  • Removed deprecated ChatModel values and updated tests to use new models

@linarkou linarkou requested a review from a-simeshin January 29, 2026 21:40
@linarkou linarkou self-assigned this Jan 29, 2026
@a-simeshin
Copy link
Collaborator

@claude проведи ревью

@claude
Copy link

claude bot commented Jan 29, 2026

Claude encountered an error —— View job


I'll analyze this and get back to you.

auth.getCerts().getCertificate().getInputStream(),
auth.getCerts().getPrivateKey().getInputStream()));
}
} else if (gigaChatApiProperties.getClientKey() != null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наконец-то)

public enum EmbeddingsModel implements EmbeddingModelDescription {
EMBEDDINGS("Embeddings", 1024),
EMBEDDINGS_2("Embeddings-2", 1024),
EMBEDDINGS_GIGA_R("EmbeddingsGigaR", 2560);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Недавно спрашивали, когда появится

- Removed deprecated fields and methods in GigaChatApiProperties related to authentication
- Updated GigaChatAutoConfiguration to use new authentication properties
- Updated GigaChatEmbeddingModel to extend AbstractEmbeddingModel and use known embedding dimensions
- Updated GigaChatApi to use new authentication properties and constants
- Updated GigaChatAuthProperties with helper methods and documentation
- Updated GigaChatOAuthClient to use new authentication properties
- Updated EmbeddingsModel to implement EmbeddingModelDescription and add dimensions
- Updated tests to reflect changes in authentication and model usage
- Removed deprecated ChatModel values and updated tests to use new models

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@linarkou linarkou force-pushed the remove-deprecations branch from ffff9e7 to 86e012b Compare January 30, 2026 06:45
@github-actions
Copy link
Contributor

Issues Found:

  1. GigaChatAuthProperties.java:318 - Missing JavaDoc return description in buildApiKey() method. The @return tag is empty.

  2. GigaChatApi.java:442 - Commented code in Russian should be removed: // мб нужно добавить новые модели?

  3. GigaChatEmbeddingModel.java:97 - Potential NullPointerException if defaultOptions.getModel() returns null. Consider adding a null check before using it as a map key.

- Add description to @return in buildApiKey() method in GigaChatAuthProperties.java
- Remove commented code in Russian in GigaChatApi.java
- Add null check before using defaultOptions.getModel() in GigaChatEmbeddingModel.java

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@github-actions
Copy link
Contributor

Результаты ревью

Нашёл одну потенциальную проблему в методе buildApiKey(), где возможен NPE если clientId или clientSecret равны null.

Остальные изменения выглядят корректно:

  • ✅ Правильно удалены deprecated поля и методы
  • ✅ Рефакторинг аутентификации логичен и согласован
  • ✅ Переход на AbstractEmbeddingModel с использованием известных размерностей - хорошее улучшение производительности
  • ✅ Обновление констант и документации
  • ✅ Тесты обновлены корректно

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

buildApiKey() return null apiKey if neither apiKey nor clientId/clientSecret are present

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Comment on lines +94 to +97
public String buildApiKey() {
if (StringUtils.hasText(this.apiKey)) {
return this.apiKey;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Дублирование кода: проверка if (StringUtils.hasText(this.apiKey)) выполняется дважды. Нужно удалить один из блоков.

Suggested change
public String buildApiKey() {
if (StringUtils.hasText(this.apiKey)) {
return this.apiKey;
}
public String buildApiKey() {
if (StringUtils.hasText(this.apiKey)) {
return this.apiKey;
}
if (StringUtils.hasText(this.clientId) && StringUtils.hasText(this.clientSecret)) {
return HttpHeaders.encodeBasicAuth(this.clientId, this.clientSecret, StandardCharsets.UTF_8);
}
return null;
}

@github-actions
Copy link
Contributor

Обзор PR

Найдена критическая проблема:

Ошибка компиляции

  • В методе GigaChatAuthProperties.Bearer.buildApiKey() есть дублирование кода - проверка if (StringUtils.hasText(this.apiKey)) выполняется дважды, что приведет к ошибке компиляции.

Пожалуйста, исправьте эту проблему перед мержем.

Общие замечания

В остальном изменения выглядят корректно:

  • Удаление deprecated полей и методов выполнено последовательно
  • Обновление тестов для использования новых моделей (GigaChat-2)
  • Правильная миграция на AbstractEmbeddingModel с использованием известных размерностей embeddings
  • Корректная рефакторинг аутентификации

Signed-off-by: Linar Abzaltdinov <abzaltdinov@gmail.com>
@github-actions
Copy link
Contributor

Найдена критическая ошибка

В методе GigaChatAuthProperties.Bearer.buildApiKey() присутствует дублирование проверки apiKey (строки 293-298), что делает невозможным использование clientId/clientSecret для аутентификации.

После исправления этой проблемы, PR можно будет одобрить.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Найдена критическая ошибка

В методе GigaChatAuthProperties.Bearer.buildApiKey() (строки 293-298 добавленного кода) обнаружено дублирование проверки:

if (StringUtils.hasText(this.apiKey)) {
    return this.apiKey;
}
if (StringUtils.hasText(this.apiKey)) {  // ← Дубликат!
    return this.apiKey;
}

Второй блок проверки apiKey (строки 296-298) является мертвым кодом и никогда не выполнится, так как если apiKey имеет значение, метод уже вернется на строке 294.

Нужно удалить строки 296-298 или изменить вторую проверку на что-то другое (если это была опечатка).


Остальные изменения выглядят хорошо:

  • ✅ Корректное удаление deprecated полей
  • ✅ Правильная миграция на новые GigaChatAuthProperties
  • ✅ Обновление тестов
  • ✅ Добавление размерностей в EmbeddingsModel
  • ✅ Использование AbstractEmbeddingModel для оптимизации

После исправления дубликата в buildApiKey() PR можно будет одобрить.

Signed-off-by: Linar Abzaltdinov <abzaltdinov@gmail.com>
@github-actions
Copy link
Contributor

Обнаружена одна проблема с потенциальным NPE в GigaChatEmbeddingModel. Пожалуйста, посмотрите inline комментарий.

Copy link
Collaborator Author

@linarkou linarkou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все недочеты исправлены

@linarkou linarkou merged commit 5c3f950 into main Jan 30, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants