-
Notifications
You must be signed in to change notification settings - Fork 2k
GH-3379: Spring Framework 7.x compatibility #4771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
GH-3379: Spring Framework 7.x compatibility #4771
Conversation
- Use only Spring Framework APIs available both in 6.x and 7.x branches - Add constructors without logic to ``*Api` classes in `models` modules to simplify extensibility; effective final fields are marked as final - Kotlin 2.x support; use kotlin compiler version 2.2.21 - Update MCP SDK to 0.15.0 - Update MCP Annotations to 0.6.0 Signed-off-by: Dmitry Bedrin <dmitry.bedrin@gmail.com>
- Updated Spring Boot dependency to 4.0.0.RC1 - Spring Framework 7 API compatibility (see below) - Migration to Spring Boot 4.x as per https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-4.0-Migration-Guide : * Adopt new modular design of starters * Use new Jackson 3.x API * Support Property Mapper API changes around null handling - Updated Swagger codegen templates used by huggingface model to align with latest Spring Framework APIs - Update elasticsearch test container to 9.2.0 - Added `FailureDetectingExternalResource` from old versions of `testcontainers` library to support `gemfire-testcontainers` - Other related dependencies updates: * Spring Retry to 2.0.12 * TestContainers to 2.0.1 * GemFire testcontainers to 2.3.3 * opensearch-testcontainers to 4.0.0 * Rest Assured to 5.5.6 * swagger-codegen-maven-plugin to 3.0.75 Fixes spring-projectsGH-3379 (spring-projects#3379) Built on-top of spring-projects#4771 - Use only Spring Framework APIs available both in 6.x and 7.x branches - Add constructors without logic to `*Api` classes in `models` modules to simplify extensibility; effective final fields are marked as final - Kotlin 2.x support; use kotlin compiler version 2.2.21 - Update MCP SDK to 0.15.0 - Update MCP Annotations to 0.6.0 Future tasks: - Raise issue to migrate from Spring Retry to Spring Framework 7 built-in retry functionality - Raise issue with `swagger-codegen-maven-plugin` to support Spring Framework 7 - Raise issue with GemFire to support testcontainers 2.x Signed-off-by: Dmitry Bedrin <dmitry.bedrin@gmail.com>
- Updated Spring Boot dependency to 4.0.0.RC1 - Spring Framework 7 API compatibility (see below) - Migration to Spring Boot 4.x as per https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-4.0-Migration-Guide : * Adopt new modular design of starters * Use new Jackson 3.x API * Support Property Mapper API changes around null handling * Migrated from Spring Retry to Spring Framework Retry functionality - Updated Swagger codegen templates used by huggingface model to align with latest Spring Framework APIs - Update elasticsearch test container to 9.2.0 - Added `FailureDetectingExternalResource` from old versions of `testcontainers` library to support `gemfire-testcontainers` - Other related dependencies updates: * TestContainers to 2.0.1 * GemFire testcontainers to 2.3.3 * opensearch-testcontainers to 4.0.0 * Rest Assured to 5.5.6 * swagger-codegen-maven-plugin to 3.0.75 Fixes spring-projectsGH-3379 (spring-projects#3379) Includes changes by @paulbakker from spring-projects#4681 Built on-top of spring-projects#4771 - Use only Spring Framework APIs available both in 6.x and 7.x branches - Add constructors without logic to `*Api` classes in `models` modules to simplify extensibility; effective final fields are marked as final - Kotlin 2.x support; use kotlin compiler version 2.2.21 - Update MCP SDK to 0.15.0 - Update MCP Annotations to 0.6.0 Future tasks: - [x] Raise issue to migrate from Spring Retry to Spring Framework 7 built-in retry functionality: spring-projects#4681 - [ ] Raise issue with `swagger-codegen-maven-plugin` to support Spring Framework 7 - [x] Raise issue with GemFire to support testcontainers 2.x: gemfire/gemfire-testcontainers#7 Signed-off-by: Dmitry Bedrin <dmitry.bedrin@gmail.com>
82590ca to
6457787
Compare
|
Merged |
filiphr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ericbottard for pointing me to this PR. It is indeed a bit more complete that mine at #4817. However, I believe that some of the things I did in my PR are a bit more performant. e.g. the use of HttpHeaders.readOnlyHttpHeaders is less optimal than the one I am using
additionalHttpHeader.forEach(headers::addAll);I've left some comments inline.
Additionally, I believe that this PR will still not make the entire codebase compile against Spring 7 and that's because of the HuggingFace generated ApiClient using removed methods in Spring 7. If the ApiClient.mustache from my PR is added then this would be complete.
Additionally, I also added a GitHub action (https://github.com/spring-projects/spring-ai/pull/4817/files#diff-e76e5134b85a9eb3e0d9ce8e671ce97c8c577ff3c24162c45d71691078201347) which compiles the project against Spring 7, so if you want to make sure that Spring AI 1.x stays compatible with Spring 7 I would advise adding that as well.
| .uri(this.completionsPath) | ||
| .headers(headers -> { | ||
| headers.addAll(additionalHttpHeader); | ||
| headers.addAll(HttpHeaders.readOnlyHttpHeaders(additionalHttpHeader)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works however it is less performant as it keeps creating new objects even when additionalHttpHeader is empty. I would advise something more like:
additionalHttpHeader.forEach(headers::addAll);| private static Duration getHeaderAsDuration(ResponseEntity<?> response, String headerName) { | ||
| var headers = response.getHeaders(); | ||
| if (headers.containsKey(headerName)) { | ||
| if (null != headers.getFirst(headerName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be optimized a bit since at the moment the headers are fetched twice. The getFirst is not needed. The entire section could be
var values = headers.get(headerName);
if (!CollectionUtils.isEmpty(values)) {
return DurationFormatter.TIME_UNIT.parse(values.get(0));
}or
var value = headers.getFirst(headerName);
if (value != null) {
return DurationFormatter.TIME_UNIT.parse(value);
}| private static Long getHeaderAsLong(ResponseEntity<?> response, String headerName) { | ||
| var headers = response.getHeaders(); | ||
| if (headers.containsKey(headerName)) { | ||
| if (null != headers.getFirst(headerName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for getHeaderAsDuration
| <maven.compiler.source>${java.version}</maven.compiler.source> | ||
| <maven.compiler.target>${java.version}</maven.compiler.target> | ||
| <kotlin.compiler.jvmTarget>${java.version}</kotlin.compiler.jvmTarget> | ||
| <kotlin.compiler.version>2.2.21</kotlin.compiler.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a Kotlin expert, but what does it mean if you use Kotlin compiler 2.2.21? When I tried to compile the project using Kotlin version 1.9.25 it was complaining because Spring Framework 7 had classes that were compiled with 2.2. It could mean that users would have the same problem if they want to use Spring AI 1.x with Spring 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
It's already done in PR for Spring Boot 4 support: This PR for Spring Framework 7 support might have some tradeoffs to keep everything compatible with both Spring Framework 6 and 7 but I don't think it makes any noticeable difference.
I did it in separate PR for Spring Boot 4 support but I agree it should be part of Spring 7 support:
Need input from maintainers here if it makes sense to have it or keep it simple before 2.0 |
This PR adds compatibility with Spring Framework 7 to enable early adoption
*Apiclasses inmodelsmodules to simplify extensibility; effective final fields are marked as finalNOTE: this PR doesn't add support of Spring Boot 4 - it requires a separate and much bigger change and will be carried over in separate PR. This PR doesn't break existing Spring Boot 3.5 / Spring Framework 6 compatibility and can be merged into Spring AI 1.1.x release