Skip to content
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

chore: upgrade to spring 6 #18314

Merged
merged 138 commits into from
Oct 3, 2024
Merged

chore: upgrade to spring 6 #18314

merged 138 commits into from
Oct 3, 2024

Conversation

vietnguyen
Copy link
Contributor

@vietnguyen vietnguyen commented Aug 8, 2024

Goal

Notes

  • SocketUtils is removed from spring utils package. So I copied the class to our project as a temporary solution.
  • Support for Apache HttpClient has been removed in Spring Framework 6.0, immediately replaced by org.apache.httpcomponents.client5:httpclient5
  • Spring 6 Introduce HttpStatusCode interface . Response.getStatusCode() return the interface HttpStatusCode instead of enum HttpStatus like before. This breaks many existing codes.
  • In WebhookHandler the method httpClient.setTimeOut() has been removed. Need to replace by setting SocketConfig.setSoTimeout
  • Remove LocalVariableTableParameterNameDiscoverer spring-projects/spring-framework#29559 - This causes some of the bean overriding in test config stop working.
  • Introduce TaskDecorator variant for Context Propagation spring-projects/spring-framework#31130 - This change causes current code with @EventListener stop working.
  • MultiPart changes required
    • Spring 6 MultipartResolver changed from using CommonsMultipartResolver to StandardServletMultipartResolver (change already included by Viet)
    • Seems like 1 more change was required to get FileStoreTest#shouldStoreFileResourceInExternalStore passing
    • Before the change it was failing with a 400 with message Required part 'file' is not present.. The servlet could not recognise that there was a file submitted as part of the multipart request.
    • searched and found spring docs (1) (2) that recommend adding one more piece of config (setting multipart config on dispatch servlet)
    • tested locally and file upload stored in file system directory dhis2_home/files/icon
    • able to save & retrieve the file from system

Spring-security changes

@vietnguyen vietnguyen changed the title chore: upgrade spring 6 chore: upgrade spring 6 [WIP] Aug 8, 2024
@@ -0,0 +1 @@
ALTER TABLE programruleaction ALTER COLUMN notificationtemplateid TYPE BIGINT;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new jpa schema validator complain about this column has type integer in database but long in Java

</dependency>
<dependency>
<groupId>com.vladmihalcea</groupId>
<artifactId>hibernate-types-52</artifactId>
<version>${hibernate-types.version}</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

com.vladmihalcea is replaced by io.hypersistence

@@ -453,7 +452,7 @@ private void configureCspFilter(
}

private void configureCorsFilter(HttpSecurity http) {
http.addFilterBefore(CorsFilter.get(), BasicAuthenticationFilter.class);
http.addFilterBefore(DhisCorsFilter.get(), BasicAuthenticationFilter.class);
Copy link
Contributor Author

@vietnguyen vietnguyen Aug 15, 2024

Choose a reason for hiding this comment

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

Fix below error. Basically Spring 6 complains about our custom object has same name with the default object.

Bean named 'corsFilter' is expected to be of type 
'org.springframework.web.filter.CorsFilter' but was actually of type org.hisp.dhis.webapi.filter.CorsFilter

@@ -419,7 +419,6 @@ public <O extends BasicAuthenticationFilter> O postProcess(O filter) {
.and()
////////////////////
.sessionManagement()
.requireExplicitAuthenticationStrategy(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix below error

Caused by: java.lang.IllegalStateException: Invalid configuration that explicitly sets requireExplicitAuthenticationStrategy to true but implicitly requires it due to the following properties being set: [sessionCreationPolicy = ALWAYS, maximumSessions = 10]
	at org.spr

@vietnguyen vietnguyen requested a review from a team as a code owner August 15, 2024 09:59
Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

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

Things to look at after merge (team tasks):

  • Using default implementation of new methods in MessageSender seems off as their default implementation looks more like a stub
  • (as discussed) have a look at job execute now (likely simplification is possible now)

@@ -175,6 +175,8 @@ public interface JobConfigurationStore extends GenericDimensionalObjectStore<Job
*/
boolean tryRevertNow(@Nonnull String jobId);

boolean executeNow(@Nonnull String jobId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not exist as there is already a method with that exact task (we look at that later)

@@ -287,6 +287,23 @@ public boolean tryExecuteNow(@Nonnull String jobId) {
return nativeSynchronizedQuery(sql).setParameter("id", jobId).executeUpdate() > 0;
}

@Override
public boolean executeNow(@Nonnull String jobId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is a copy of tryExecuteNow without the @Transactional annotation which we should try to remove anyhow as we usually don't want them on store level.


default List<OutboundMessage> getAllMessages() {
return List.of();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering what these new default methods are for? are they used anywhere?
should a MessageSender just send messages, or get messages as well? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

i've seen it used in the FakeMessageSender while looking through the rest of the PR. This seems like a code smell if it's only used for tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is currently a workaround for fixing the issue of overriding a bean in test environment. As @Qualifer doesn't work for that purpose. I can give an explanation in next week meeting.
Basically in test we have FakeMessageSender SmsMessageSender and EmailMessageSender.
We want the FakeMessageSender to override both SmsMessageSender and EmailMessageSender.
But FakeMessageSender has specific methods getAllMessages(), which then can't be casted back to the others messageSenders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Spring 5 you can override a bean by declaring same bean name.
But in spring 6, it doesn't work anymore. Need to implement same interface.

Copy link
Contributor

@david-mackessy david-mackessy left a comment

Choose a reason for hiding this comment

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

great work

@david-mackessy david-mackessy changed the title chore: upgrade spring 6 [WIP] chore: upgrade spring Oct 3, 2024
@david-mackessy david-mackessy changed the title chore: upgrade spring chore: upgrade to spring 6 Oct 3, 2024
Copy link

sonarqubecloud bot commented Oct 3, 2024

@david-mackessy david-mackessy merged commit 2097f7e into master Oct 3, 2024
15 checks passed
@david-mackessy david-mackessy deleted the viet-spring-6 branch October 3, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-api-analytics-tests Enables analytics e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants