-
Notifications
You must be signed in to change notification settings - Fork 256
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
Migrate to Spring 6 / Spring Boot 3 #1417
Conversation
logbook-ktor-common/src/main/kotlin/org/zalando/logbook/common/ExperimentalLogbookKtorApi.kt
Show resolved
Hide resolved
@@ -105,7 +105,8 @@ private static final class Passing implements State { | |||
@Override | |||
public int getStatus() { | |||
try { | |||
return response.getRawStatusCode(); | |||
// This may throw NPE |
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.
Should we fallback to the same status on every implementation? I wouldn't say I like that in some modules it falls back to 200, but could a common practice. Or we could fallback to -1
as a way to say "no data" provided.
I can't say I'm a fan of the idea that a logging library can fail requests because one of the attributes (even the one that's probably always there) is missing.
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.
We should definitely discuss it. I also thought about changing the interface to return Integer
instead of int
, but didn't consider its implications.
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.
FYI: Tried my approach (Integer
instead of int
) and failed: One of the classes implements a Spring interface which has public int getStatus()
, and extends a class which would then have public Integer getStatus()
, and bam! clash.
I don't like default 200
either (because it can be misleading), and would like to see some fallback like -1
as you suggested.
logbook-spring-webflux/src/main/java/org/zalando/logbook/spring/webflux/ServerResponse.java
Show resolved
Hide resolved
Checked on a simple webflux app and works like a charm 🥳 |
Any ETA or update on when this PR might be merged in? I'm really looking forward to being able to use this library in SpringBoot 3 / Spring 6 |
Thanks a lot for the interest in Logbook! We are considering several approaches to add this support. While two working PRs exist, the decision in which way to go is yet to be made. We need some time to discuss this and make the best decision to support our users. Please have a look at the discussion I opened here: |
Thanks @msdousti , that makes sense and thanks for the explanation and update. |
@owebb98
|
@msdousti could you please give us an ETA when this will be fixed? I do understand that there needs to be decisions made, but the discussions started months ago and Spring Boot 3 was in November (with years of knowledge that javax will be removed). |
I'm closing this branch as the team decided against it: Logbook will keep support for older version of Spring, and add support for the new version as well. As for an ETA, I unfortuantely can't make an estimation. I know it's frustrating, but I ask for your understanding. |
Bumping version of Spring to 6, Spring Boot to 3, and using
jakarta.*
instead ofjavax.*
in most places. This is a breaking change, as it drops support for previous versions. SeeDescription
for further info.Description
Migrate Logbook to exclusively use Spring 6 / Spring Boot 3. That is, we drop support for Spring 5.x or Spring Boot 2.x.
It is yet to be decided whether we want to drop this support, so this PR is provided on a just-in-case basis.
Motivation and Context
Fixes #1344
Types of changes
Checklist: