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

[WIP] update to »SpringBoot 3« (w/ Spring Framework 6) #1375

Closed
wants to merge 3 commits into from
Closed

[WIP] update to »SpringBoot 3« (w/ Spring Framework 6) #1375

wants to merge 3 commits into from

Conversation

agebhar1
Copy link
Contributor

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Closes gh-1344.

@agebhar1
Copy link
Contributor Author

Let this PR start the discussion to update Logbook for SpringBoot 3 (requires JakartaEE 10).

@marcindabrowski
Copy link

Maybe introducing new modules dedicated to Jakarta and Spring 6/Spring Boot 3 will be a better approach? The old modules will still work, and we will have Spring 6 support.

@agebhar1
Copy link
Contributor Author

agebhar1 commented Nov 17, 2022

@marcindabrowski that's fine. So these new modules I can think of:

  • logbook-servlet-jakarta (?) - which is mainly a copy of logbook-servlet` but with the new packages of JakartaEE
  • logbook-spring-boot3-autoconfigure
  • logbook-spring-boot3-starter

@marcindabrowski
Copy link

@marcindabrowski that's fine. So these new modules I can think of:

  • logbook-servlet-jakarta (?) - which is mainly a copy of logbook-servlet` but with the new packages of JakartaEE
  • logbook-spring-boot3-autoconfigure
  • logbook-spring-boot3-starter

It looks great.
One another thing - because Spring 6 / Spring Boot is targeted to Java 17, those two new modules also should be.
Jakarta Servlet requires Java 8 so it is compatible with the logbook target.

@agebhar1
Copy link
Contributor Author

Jakarta Servlet 6 requires at least Java 11 which should not be a problem.

@agebhar1
Copy link
Contributor Author

@marcindabrowski, I have created the new three modules and updated the GitHub workflow.

The logbook-servlet-jakarta module is just a copy of logbook-servlet which feels a bit dirty. It might be possible to copy the source and update the imports on-the-fly in the Maven goals 'generate-sources'/'generate-test-sources'. The same applies for logbook-spring-boot3-autoconfigure.

The auto configuration for SpringBoot v3 handles @ConditionalOnBean differently ...

Unfortunately, the Dependency Check complains some CVE affected dependencies. To execute the tests, the Dependency Check is temporarily disabled.

@@ -0,0 +1 @@
org.springframework.boot.autoconfigure.EnableAutoConfiguration = org.zalando.logbook.autoconfigure.LogbookAutoConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this won't work in SpringBoot 3. We need META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports. See https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0.0-M5-Release-Notes#auto-configuration-registration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

import static org.zalando.logbook.autoconfigure.LogbookAutoConfiguration.ServletFilterConfiguration.newFilter;

@API(status = STABLE)
@Configuration(proxyBeanMethods = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This and @AutoConfigureAfter should be replaced with @AutoConfiguration(after = { JacksonAutoConfiguration.class, WebMvcAutoConfiguration.class } )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@agebhar1
Copy link
Contributor Author

Also had to skip Spring Boot 1 tests, because ErrorMvcAutoConfiguration is not present in Spring Boot 1.

@phejl
Copy link
Contributor

phejl commented Nov 23, 2022

Thank you @agebhar1. I'll try to look into that soon, though I'm no maintainer here. Honestly it feels like the project is kind of abandoned. My judgment is based on my feature PR being opened without comment for almost a year now. At worst case we would need a fork to be able to use Spring Boot 3 soon. 🤷‍♂️

@whiskeysierra
Copy link
Collaborator

Sorry for the late response.

I used to be the maintainer of this project, but I've left Zalando 2.5+ years ago.
Up until this year I did retain my push rights, which meant I was able to merge and release new versions.
Unfortunately, due to a policy change at Zalando, I am no longer allowed to continue having admin privileges to Github repositories or push privileges to maven central.

Maybe @otrosien can chime in here.
As per our last discussion, my understanding was that there might be the possibility for someone within Zalando to perform the actual releases.
Personally for me, that's too much bureaucracy.

@otrosien Maybe there is a chance to move the whole project out of Zalando's Github org and Maven's groupId into its own separate space?

@whiskeysierra
Copy link
Collaborator

If someone would be willing to create a (non-compatible) fork, I'd definitely be willing to participate there.

@agebhar1
Copy link
Contributor Author

@whiskeysierra, @phejl I'll be willing to be one of the maintainer if there will be a fork.

@whiskeysierra
Copy link
Collaborator

@agebhar1 I opened a discussion: #1379

@agebhar1
Copy link
Contributor Author

Thanks @whiskeysierra, I appreciate it.

@phejl
Copy link
Contributor

phejl commented Nov 23, 2022

Thank you @agebhar1 @whiskeysierra! How can I help at the moment?

@phejl
Copy link
Contributor

phejl commented Nov 25, 2022

@agebhar1 Can you navigate me to "Also had to skip Spring Boot 1 tests"? I would like to check that, but I'm blind :)

@agebhar1
Copy link
Contributor Author

@phejl the tests for Spring 4 (it's a profile in the Project pom) are skipped. Spring Boot's 1 dependency was Spring Framework 4. The commercial support for Spring Boot 1 ended over two years ago.

@phejl
Copy link
Contributor

phejl commented Nov 30, 2022

@agebhar1 I would suggest these changes (attached). It is mostly about reducing dependenices. Otherwise lot of baggage would be pulled to the user of the library. This branch works great in my project.
There is one attempt to fix failing test when localhost's canonical hostname was different from localhost. Feel free to ignore that part.
lb.patch.txt

@agebhar1
Copy link
Contributor Author

agebhar1 commented Dec 2, 2022

@phejl I'v applied your patch

@joshlong
Copy link

joshlong commented Dec 5, 2022

HI

i was reviewing the code, superficially on github, and wanted to encourage you to not create a dedicated spring boot 3 starter, preferring instead to try to make it so that one configuration class could handle both frameworks.

spring boot 2 autoconfiguration can be written in such a way that they're compatible with spring boot 3. the major thing is you'll need to add that AutoConfiguration.imports text file which would just live alongside your code and be ignored in earlier versions.

you're going to experience binary incompatibilities with jakarta ee vs java ee, but this isn't a dimension of your spring support so much as the core libraries. could you link to both, as optional dependencies, and guard your bean registrations with @ConditionalOnClass("jakarta...") or @ConditionalOnClass("javax....")?

@Configuration
class MyConfiguration {
	
	@Bean 
	@ConditionalOnClass("jakarta.servlet.HttpServletRequest")
	MyJakartaEeServletFilter jakartaFilter(){ ... }

	@Bean
	@ConditionalOnClass("javax.servlet.HttpServletRequest")
	MyLegacyEeServletFilter javaxFilter () { ... }

}

this way, to the extent possible, things just work for new users and old.

where you might want to have a new module, otoh, is in supporting the Spring AOT engine.

Spring's AOT engine provides a powerful way to furnish the metadata required for GraalVM native image compilation. There is no equivalent in Spring Boot 2; you need Spring Boot 3, which in turn requires Java 17. So perhaps you could create a logbook-spring-boot-aot-starter module which links to the regular starter, but also provides the required support (if any) for compiling your app into a graalvm native image.

@whiskeysierra
Copy link
Collaborator

@joshlong Thanks for the details!

This project will either be transferred or forked (tbd) which will not just change the github org, but also the maven coordinates and the top level package name.
I'd say we take that opportunity and jump directly to Spring 6 / Spring Boot 3 exclusive support.
The old versions of Logbook are still available via Maven central, for any users of Spring 5 / Spring Boot 2 who may want to use them for the time being.

@joshlong
Copy link

joshlong commented Dec 6, 2022

Sounds good to me! Let me know if I may of service post restructuring.

@cismael
Copy link

cismael commented Dec 15, 2022

up ?

@nhmarujo
Copy link
Contributor

Hi. Is there any progress on this? 🙂

@agebhar1
Copy link
Contributor Author

See the discussion #1379 about 'The future of Logbook'.

@afrancis-caregility
Copy link

Hi All, please prioritize merging this PR. This is the only thing that is not working after I upgraded to Spring Boot 3. Request and response logging is a big deal for me and so please get this resolved as soon as you all can. Thanks @agebhar1 for doing this.

@grassehh
Copy link

grassehh commented Feb 7, 2023

Hello, is there any update about this ?
We want to migrate to Spring Boot 3 but there are no longer Logbook bean that is created: "bean of type 'org.zalando.logbook.Logbook' that could not be found".
Is there at least a workaround maybe ?
Thanks :)

@hemju
Copy link

hemju commented Feb 7, 2023

@grassehh more information here: #1379 (reply in thread) As @leviferreira said, there should be updates incoming this week.

@hemju
Copy link

hemju commented Feb 10, 2023

There is again activity in this project, thank you!

@leviferreira or @kasmarian can any of you share the plans supporting Spring Boot 3? As the comments show, this is currently a blocker for many. Will Logbook support it? If yes, when do you think we can expect it?

@kasmarian
Copy link
Member

Hi everyone, hi @hemju, we definitely aim to support Spring Boot 3, bear with us, and apologies for all these delays.
We're also discussing the approach to move forward with the support of Spring Boot 2. We're also looking at the suggestion to have a signle spring boot starter that would support both Spring Boot 2 and 3. Or, to have a major update that would only support Spring Boot 3, and Spring Boot 2 will be kept in another branch for security patches only.

If you have thoughts on this please share.

@hemju
Copy link

hemju commented Feb 10, 2023

@kasmarian thanks for the quick update!

We're also discussing the approach to move forward with the support of Spring Boot 2. We're also looking at the suggestion to have a signle spring boot starter that would support both Spring Boot 2 and 3. Or, to have a major update that would only support Spring Boot 3, and Spring Boot 2 will be kept in another branch for security patches only.

My position on this is if the downward compatibility doesn't block new development or creates much more maintenance costs, then it is ok, otherwise, I would create a new breaking version. So it also depends on what the plans are for logbook. Do you plan many new features/additions in the future? That may cause a lot of additional work if the new features have to support two different Spring versions. Especially with new Spring versions popping up at a faster pace (https://www.youtube.com/watch?v=mitWK_DwKGs).

Personally, I never understood why everything needs to be 100 % downward compatible in Java. I don't see the downside in an old version that supports the old Spring Boot 2 version and is in maintenance mode. And a new breaking version that supports the new Spring Boot. Obviously, that is a subjective view, but I also see a change in the Java ecosystem toward breaking versions and faster updates in recent years (JDK releases, Springs new release schedule).

@whiskeysierra
Copy link
Collaborator

I don't see the downside in an old version that supports the old Spring Boot 2 version and is in maintenance mode.

I tend to agree, depending on what we collectively understand as maintenance mode.
If it's a separate branch, e.g. backport/2.x which receives the occasional dependency update in case of CVEs without any new features, then I'd fine with that.

Maintaining both Spring Boot 1.x and 2.x compatibility has been a pain in the recent years of Logbook.
Any chance to reduce the pain is highly appreciated on my end.

@marcindabrowski
Copy link

I also agree that you should release a new version that will support only Spring 6.

But at the same time, it would be great if you can upgrade your dependent libraries to newer versions which also can have breaking changes.

I'm aware that it will take more time, but I can't imagine upgrading only Spring and leaving other libs unchanged.

But for most people Spring dependency is the most important so maybe you should consider releasing it as first as the Release Candidate version?

@marcindabrowski
Copy link

And it would be nice to make it compliant with at least Java 11 instead of Java 8, but probably Java 17 would be the best option.

@whiskeysierra
Copy link
Collaborator

whiskeysierra commented Feb 10, 2023 via email

@sandra-markerud
Copy link

Any estimate on when this will be solved?

@hemju
Copy link

hemju commented Mar 9, 2023

Any estimate on when this will be solved?

I also wanted to ask that. I see that there are nearly daily commits (thank you for that), but many of the community are waiting on Spring Boot 3 compatibility. Can you please give us an estimate or let us know if there are some blockers? We would gladly help (e.g. test it).

@dswiecki
Copy link

I've built a snapshot of the latest commit and tested it with my Boot 3 app, works pretty decent.

implementation 'org.zalando:logbook-spring-boot3-starter:2.17.0-SNAPSHOT'

@ChristianLohmann
Copy link
Member

done with #1467

@agebhar1 agebhar1 deleted the feature/gh1344-SpringBoot@v3 branch March 27, 2023 17:08
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.

Spring Framework 6 / Spring Boot 3 Support?