Spring4 by shroffk · Pull Request #3907 · ControlSystemStudio/phoebus

shroffk · 2026-08-06T13:42:11Z

Checklist

Testing:
- The feature has automated tests
- Tests were run
- If not, explain how you tested your changes
Documentation:
- The feature is documented
- The documentation is up to date
- Release notes:
  - Added an entry if the change is breaking or significant
  - Added an entry when adding a new feature

…rest client

jacomago

First commits for spring boot are a nice commit structure, but not so much the elastic ones. I think better to pull out the elastic changes into a separate PR.

I don't really know why you are swapping to the low-level client? If this is really needed I think you should make a ElasticResponseObjectMapper and ElasticRequetObjectMapper to map between the objects with a strong set of unit tests.

Some comments in the code I think would be better as part of the commit messages as they explain why something has changed not why it is now.

jacomago · 2026-08-11T08:09:58Z

-            configurer.userSearchBase(ldap_user_search_base);
-        }
-        configurer.contextSource(contextSource);
+//        LdapAuthenticationProviderConfigurer configurer = new LdapAuthenticationProviderConfigurer();


Rather delete than comment out. Always have git history to fallback on.

jacomago · 2026-08-11T08:10:42Z

-@WebMvcTest(SearchController.class)
+/**
+ * Unit test for {@link SearchController} using standalone MockMvc setup.
+ * Migrated from @WebMvcTest (removed in Spring Boot 4.x) to


Don't need this comment.

jacomago · 2026-08-11T08:11:55Z

    @Bean
    public SnapshotUtil snapshotUtil() {
-        return new SnapshotUtil();
+        // Return a mock so tests can stub it with when(...); previously @MockBean was used


unneeded comment

jacomago · 2026-08-11T08:13:55Z

+
+    @BeforeEach
+    void setUp() {
+        mockMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext).apply(springSecurity()).build();


Feels weird this is repeated everywhere, feels like either we need a parent class for these tests or we're doing something wrong with how to use MockMvc.

jacomago · 2026-08-11T08:15:17Z

-            SearchResponse<JsonNode> strResponse = client.search(searchRequest, JsonNode.class);
-            return strResponse.hits().hits().stream().map(hit -> {
-                JsonNode jsonNode = hit.source();
+            // Build the search request body as JSON for low-level API


Why swap to the low level api?

#3907 (comment)

jacomago · 2026-08-11T08:15:50Z

+     * This works by using Jackson to serialize the SearchRequest object tree to JSON
+     * NOTE: We exclude 'index' from the JSON since indices are specified in the URL path
+     */
+    private static String buildSearchJson(SearchRequest searchRequest) throws IOException {


Please add a unit test for this method.

jacomago · 2026-08-11T08:16:26Z

                if (client != null) {
                    try {
-                        client.shutdown();
+                        // Do not call client.shutdown() with Rest5 transport because some


Don't need this comment

jacomago · 2026-08-11T08:17:21Z

-                esHttpHosts = Arrays.stream(esUrls.split(",")).map(HttpHost::create).toArray(HttpHost[]::new);
+                esHttpHosts = Arrays.stream(esUrls.split(","))
+                        .map(url -> {
+                            try {


This looks a bit ugly, not sure of a correct fix though, maybe just use a normal for loop instead.

jacomago · 2026-08-11T08:18:25Z

+
+            try {
+                int statusCode = restClient.performRequest(request).getStatusCode();
+                if (statusCode >= 200 && statusCode < 300) {


Does a response not come with a 'success' or failure method?

jacomago · 2026-08-11T08:19:52Z

Better to squash this with previous commit.

moving to jdk25 and javafx25

sonarqubecloud · 2026-08-11T13:37:50Z

Quality Gate failed

Failed conditions
12.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

shroffk · 2026-08-11T13:39:19Z

The low level elastic is need to ensure consistent functionality with the 9.x elastic client and 8.x version of elastic service

The low level client seems more resilient.

Using the old client conflicts with the upgrade to jackson3 and using the newer client fails with older service.... a lot of object mapping wrapping would be needed to continue using the high level client

That having been said... @georgweiss and I feel like for now we should move the main code base over to the newest elastic java client and use the low level rest client for now and address the elastic migration as a separate effort in the future

jacomago · 2026-08-11T13:42:52Z

The low level elastic is need to ensure consistent functionality with the 9.x elastic client and 8.x version of elastic service

That seems out of scope.

Using the old client conflicts with the upgrade to jackson3 and using the newer client fails with older service.... a lot of object mapping wrapping would be needed to continue using the high level client

That's annoying.

That having been said... @georgweiss and I feel like for now we should move the main code base over to the newest elastic java client and use the low level rest client for now and address the elastic migration as a separate effort in the future

Yeah, I agree.

shroffk · 2026-08-11T13:46:58Z

That's annoying.

Tell me about it...I had no desire to include this work but it is the fastest and cleanest way to get to jdk15,spring4, jackson3

I think I have one of my first codeathon project ideas (elastic upgrades + high level client) here and I think I also have a volunteer.

Copilot

Pull request overview

This PR modernizes the Phoebus services and tests for newer platform dependencies by migrating servlet APIs to jakarta.*, updating Spring Security/Spring test setup patterns, and switching several Elasticsearch interactions from the high-level Java client to low-level Rest5Client requests.

Changes:

Refactors save-and-restore controller tests to use @SpringBootTest + explicit MockMvc setup with springSecurity().
Reworks save-and-restore and alarm-logger Elasticsearch integration to use low-level Rest5Client requests (and adds supporting config/test beans).
Updates build/tooling pins (JDK 25, OpenJFX 25) and Springdoc dependency coordinates.

Reviewed changes

Copilot reviewed 61 out of 62 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/TakeSnapshotControllerTest.java	Migrates test to `@SpringBootTest` and manual `MockMvc` initialization.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/TagControllerTest.java	Same test harness migration for Tag endpoints.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/TagControllerPermitAllTest.java	Same test harness migration for permit-all scenario.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/StructureControllerTest.java	Same test harness migration for Structure endpoints.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/StructureControllerPermitAllTest.java	Same test harness migration for permit-all scenario.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/SnapshotRestorerControllerTest.java	Updates test wiring for `SnapshotUtil` under new Spring Boot testing approach.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/SnapshotControllerTest.java	Same test harness migration for Snapshot endpoints.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/SnapshotControllerPermitAllTest.java	Same test harness migration for permit-all scenario.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/SearchControllerTest.java	Migrates tests and switches request-param map type to Spring `MultiValueMap`.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/NodeControllerTest.java	Same test harness migration; renames `@BeforeAll` initializer.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/NodeControllerPermitAllTest.java	Same test harness migration; renames `@BeforeAll` initializer.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/HelpResourceTest.java	Same test harness migration for help endpoint.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/FilterControllerTest.java	Same test harness migration for Filter endpoints.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/FilterControllerPermitAllTest.java	Same test harness migration for permit-all scenario.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/ConfigurationControllerTest.java	Same test harness migration for Configuration endpoints.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/ConfigurationControllerPermitAllTest.java	Same test harness migration for permit-all scenario.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/CompositeSnapshotControllerTest.java	Same test harness migration for Composite Snapshot endpoints.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/CompositeSnapshotControllerPermitAllTest.java	Same test harness migration for permit-all scenario.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/ComparisonControllerTest.java	Uses `@SpringBootTest` and imports a test config to force a real `SnapshotUtil`.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/config/WebConfigTest.java	Updates Spring test annotations for web config checks.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/config/ControllersTestConfig.java	Expands test beans for Rest5 client + mapper; switches auth header encoding to `Base64`; provides mocked `SnapshotUtil`.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/config/ComparisonControllerRealSnapshotUtilTestConfig.java	Adds a test-only config to override `SnapshotUtil` with a real implementation.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/persistence/dao/impl/elasticsearch/ElasticTestConfig.java	Extends ES test config with Rest5 client + mapper and additional mocked repos/utilities.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/persistence/dao/impl/elasticsearch/ElasticsearchDAOTest.java	Simplifies Spring test wiring annotations.
services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/AbstractElasticsearchIT.java	Switches IT profile selection to `@ActiveProfiles("IT")`.
services/save-and-restore/src/main/resources/tree_node_mapping.json	Formatting-only change.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/TakeSnapshotController.java	Makes `@PathVariable` names explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/StructureController.java	Makes `@PathVariable` name explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/SnapshotController.java	Makes `@PathVariable` name explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/NodeController.java	Makes `@PathVariable` names explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/InfoController.java	Replaces ES high-level info call with low-level `Rest5Client` request + JSON parsing.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/HelpResource.java	Migrates servlet API usage to `jakarta.*` and makes `@PathVariable` name explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/ConfigurationController.java	Makes `@PathVariable` name explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/CompositeSnapshotController.java	Makes `@PathVariable` names explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/ComparisonController.java	Makes `@PathVariable` name explicit.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/BaseController.java	Migrates servlet API usage to `jakarta.*`.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/controllers/AuthenticationController.java	Simplifies unauthorized response creation.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/config/WebSecurityConfig.java	Migrates to newer Spring Security APIs (`EnableMethodSecurity`, new matcher DSL, provider managers).
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/config/WebConfiguration.java	Migrates servlet API usage to `jakarta.*`.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/web/config/AcceptHeaderResolver.java	Migrates servlet API usage to `jakarta.*`.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/persistence/dao/impl/elasticsearch/SnapshotDataRepository.java	Replaces high-level ES client usage with low-level `Rest5Client` CRUD calls.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/persistence/dao/impl/elasticsearch/FilterRepository.java	Same `Rest5Client` low-level migration for filters (incl. `_search`, `_mget`, delete-by-query).
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/persistence/dao/impl/elasticsearch/ElasticsearchTreeRepository.java	Same low-level migration plus custom serialization of `SearchRequest`.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/persistence/dao/impl/elasticsearch/ConfigurationDataRepository.java	Same low-level migration plus custom serialization of `SearchRequest`.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/persistence/dao/impl/elasticsearch/CompositeSnapshotDataRepository.java	Same low-level migration plus custom serialization of `SearchRequest`.
services/save-and-restore/src/main/java/org/phoebus/service/saveandrestore/persistence/config/ElasticConfig.java	Adds `Rest5Client` + `elasticObjectMapper` beans and reimplements bootstrap/index creation using low-level requests.
services/save-and-restore/pom.xml	Updates Springdoc dependency coordinates; adds Jakarta JSON dependency.
services/alarm-logger/src/test/java/org/phoebus/alarm/logging/rest/SearchControllerTest.java	Switches to standalone `MockMvc` setup (no Spring context).
services/alarm-logger/src/main/java/org/phoebus/alarm/logging/rest/SearchController.java	Replaces ES info + search calls to use `Rest5Client` and updated search utility signature.
services/alarm-logger/src/main/java/org/phoebus/alarm/logging/rest/AlarmLogSearchUtil.java	Reimplements ES searches via low-level `_search` POSTs and parses hits manually.
services/alarm-logger/src/main/java/org/phoebus/alarm/logging/purge/ElasticIndexPurger.java	Migrates purge logic to low-level ES APIs (`_cat/indices`, `_search`, index delete).
services/alarm-logger/src/main/java/org/phoebus/alarm/logging/ElasticClientHelper.java	Migrates to Rest5 transport, removes sniffer, and reworks bulk/template initialization for low-level APIs.
services/alarm-logger/src/main/java/org/phoebus/alarm/logging/AlarmMessageLogger.java	Small style refactors (method references, whitespace).
services/alarm-logger/pom.xml	Pins module JDK to 25; updates Springdoc coordinates; adds HttpClient5 deps; removes sniffer dep.
services/alarm-config-logger/pom.xml	Pins module JDK to 25 and minor comment/whitespace cleanup.
pom.xml	Pins compiler to Java 25, updates OpenJFX version, adds `-parameters`, introduces `springdoc.version`.
dependencies/phoebus-target/pom.xml	Updates Springdoc coordinates and minor whitespace cleanup.
app/save-and-restore/model/pom.xml	Adds Jakarta JSON alongside javax.json with clarified comments.
app/display/model/src/test/java/org/csstudio/display/builder/PathTest.java	Disables a test on Windows.
.github/CI_VERSIONS.md	Updates CI Java version tracking to 25.
.github/actions/setup-java/action.yml	Updates CI JDK to 25.

Suppressed comments (1)

services/save-and-restore/src/test/java/org/phoebus/service/saveandrestore/web/controllers/TakeSnapshotControllerTest.java:44

snapshotUtil is created as a local Mockito mock, but it is never injected into the Spring context. The controller under test will use the SnapshotUtil bean from ControllersTestConfig, so stubbing this local mock has no effect and the test can fail (e.g., response body becomes null). Autowire the context bean instead so when(snapshotUtil.takeSnapshot(...)) stubs the instance actually used by the controller.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

shroffk added 11 commits August 4, 2026 09:30

Phased upgrade of springboot

7954caa

update save and restore - spring 3, jakarta

5723ffc

spring 4.1

6188eab

now try to make save and restore to spring4.1

ad7a641

start migration to new spring security API

b03359b

updating save and restore tests for springboot 4.1

e0d2e1f

use named path variables as required by springboot4

d98fd57

fix windows test for the Path calculations

18eefbe

Add the need compiler flag "-parameter"

bc14788

switch some of the tests to use real SnapshotUtil

52a0484

moving to jdk25 and javafx25

7ceb150

shroffk mentioned this pull request Aug 6, 2026

moving to jdk25 and javafx25 #3906

Merged

6 tasks

shroffk requested review from georgweiss, jacomago, kasemir and tynanford August 6, 2026 13:51

shroffk added 6 commits August 6, 2026 14:09

upgrade springdoc version

16ad5da

updating the elastic client to 9.4.4

f75352f

consistent switch to low level elastic rest client

413818b

first pass of replacing elastic high level client with low level

cd3a07e

move sar to the low level elastic rest client

86d8ee9

switching all remaining elastic high level client usage to low level …

c82c486

…rest client

shroffk mentioned this pull request Aug 10, 2026

Jdk25 #3839

Closed

6 tasks

jacomago reviewed Aug 11, 2026

View reviewed changes

Merge pull request #3906 from ControlSystemStudio/jdk-25

4ea5933

moving to jdk25 and javafx25

shroffk requested a lite review from Copilot August 11, 2026 13:47

shroffk requested a review from lcaouen August 11, 2026 13:47

Copilot started reviewing on behalf of shroffk August 11, 2026 13:47 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Uh oh!

Conversation

shroffk commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jacomago left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 11, 2026

Quality Gate failed

Uh oh!

shroffk commented Aug 11, 2026

Uh oh!

jacomago commented Aug 11, 2026

Uh oh!

shroffk commented Aug 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shroffk commented Aug 6, 2026 •

edited

Loading