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

fix: Spring Boot 2.6 compatibility #944

Merged
merged 2 commits into from
Nov 29, 2021
Merged

fix: Spring Boot 2.6 compatibility #944

merged 2 commits into from
Nov 29, 2021

Conversation

Artur-
Copy link
Member

@Artur- Artur- commented Nov 24, 2021

Fixes #941

@ZheSun88
Copy link
Contributor

do we need to update https://github.com/vaadin/spring/blob/master/pom.xml#L47 to spring-boot 2.6.0?

@Artur-
Copy link
Member Author

Artur- commented Nov 25, 2021

This PR should be compatible with Spring Boot 2.4+. Maybe we should still update the version at the same time

@Artur-
Copy link
Member Author

Artur- commented Nov 25, 2021

This fix could go into V22, we probably shouldn't go to Spring Boot 2.6 for that at this point

@Artur-
Copy link
Member Author

Artur- commented Nov 25, 2021

Let's do the version update in #940

@knoobie
Copy link

knoobie commented Nov 25, 2021

The FQN defined in isSpringBootConfigured is probably faulty as well

@lmorocz
Copy link

lmorocz commented Nov 25, 2021

And there is an extra org.springframework.boot.autoconfigure.web.ResourceProperties.class in the isSpringBootConfigured method as well.

@Artur-
Copy link
Member Author

Artur- commented Nov 25, 2021

Simplified

@@ -22,6 +22,8 @@
import java.util.Optional;
import java.util.stream.Collectors;

import org.springframework.beans.factory.NoSuchBeanDefinitionException;
import org.springframework.boot.autoconfigure.web.WebProperties.Resources;
Copy link

Choose a reason for hiding this comment

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

Is this class to be expected to work with plain spring or just spring boot?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is supposed to work with Spring. Seems like there are no proper tests for plain Spring projects in this repository

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep the untested code then and hope that it might work with plain Spring..

Copy link

Choose a reason for hiding this comment

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

I would like to have a more spring boot oriented implementation as well.. but I've seen multiple PRs in this repo in the past where explicitly a more complex route was taken to reduce the coupling with spring boot.. that's why I wanted to mention it

platosha
platosha previously approved these changes Nov 26, 2021
@haijian-vaadin
Copy link

could you @Artur- help to fix the sonar issue, let's get this merged soon

@mstahv
Copy link
Member

mstahv commented Nov 29, 2021

I wouldn't care about Sonar issues at all, but Spring Boot 2.6.0 support for 22 would be cool ;-)

@knoobie
Copy link

knoobie commented Nov 29, 2021

^ for 14.6 as well 😀

@mstahv
Copy link
Member

mstahv commented Nov 29, 2021

For 14 series as well yeah 👍

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 2 issues

  1. MAJOR SpringVaadinServletService.java#L144: Either log or rethrow this exception. rule
  2. MAJOR SpringVaadinServletService.java#L175: Either log or rethrow this exception. rule

@Artur- Artur- enabled auto-merge (squash) November 29, 2021 09:15
@Artur-
Copy link
Member Author

Artur- commented Nov 29, 2021

Seems to work now, just waiting for a review

@Artur- Artur- merged commit 7589e30 into master Nov 29, 2021
@Artur- Artur- deleted the springboot26-compat branch November 29, 2021 09:28
@vaadin-bot
Copy link
Collaborator

Hi @Artur- , this commit cannot be picked to 19.0 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick 7589e30
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

git commit --allow-empty

Otherwise, please use 'git cherry-pick --skip'

@Artur-
Copy link
Member Author

Artur- commented Nov 29, 2021

Seems like this is already in the 19.0 branch

@joheriks
Copy link

joheriks commented Nov 29, 2021

Seems like this is already in the 19.0 branch

There was no 19.0 branch at that point, created it after this commit.

joheriks pushed a commit that referenced this pull request Nov 29, 2021
joheriks pushed a commit that referenced this pull request Nov 30, 2021
Co-authored-by: Artur <artur@vaadin.com>
joheriks pushed a commit that referenced this pull request Dec 2, 2021
caalador pushed a commit that referenced this pull request Dec 2, 2021
Co-authored-by: Artur <artur@vaadin.com>
manolo pushed a commit to vaadin/flow that referenced this pull request Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not work with Spring Boot 2.6
10 participants