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

Spring Boot 3 #1230

Merged

Conversation

Stuckya
Copy link

@Stuckya Stuckya commented Sep 13, 2022

Pull request checklist

  • Please read our contributor guide
  • Consider creating a discussion on the discussion forum first
  • Make sure the PR doesn't introduce backward compatibility issues
  • Make sure to have sufficient test cases

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Issue #948

Spring Boot 3 has been released! This PR has prepared dgs-framework for Spring Boot 3. Many project maintainers are currently validating their projects against Sprint Boot 3 to prepare for the upgrade. I have tested this branch extensively against my own Spring Boot 3 project.

Version upgrades:

  • Spring Boot 3.0.0
  • Spring Framework 6.0.3
  • Spring Security 6.0.1
  • Spring Cloud 2022.0.0
  • JDK target 17

Alternatives considered

Describe alternative implementation you have considered

N/A

import org.springframework.boot.context.properties.NestedConfigurationProperty
import org.springframework.boot.context.properties.bind.DefaultValue

@ConfigurationProperties(prefix = DgsAPQSupportProperties.PREFIX)
@ConstructorBinding
Copy link
Author

Choose a reason for hiding this comment

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

Spring Boot 3 @ConstructorBinding update

... This means that the binder will find a constructor with the parameters that you wish to have bound. If your class has multiple constructors, the @ConstructorBinding annotation can be used to specify which constructor to use for constructor binding.

See: https://docs.spring.io/spring-boot/docs/3.0.0-SNAPSHOT/reference/htmlsingle/#features.external-config.typesafe-configuration-properties.constructor-binding

build.gradle.kts Outdated Show resolved Hide resolved
@srinivasankavitha
Copy link
Contributor

Thanks for the doing the pre-work and investigating this. Just to provide some context, we have some constraints with switching the framework to Spring Boot 3 due to internal usage and that will affect how we make this available. We don't have concrete timelines yet, but will be planning for it. I will post updates on this thread as and when we have more information from our side.

@arjunyel
Copy link

arjunyel commented Oct 6, 2022

Is there a way to test the WIP version?

@srinivasankavitha
Copy link
Contributor

You'll have to check out the branch locally, publish to your local maven repo using ./gradlew publishToMavenLocal and pin to the local snapshot build in order to test it out. Unfortunately, we don't have a great way to publish candidates off of branches, so this is the mechanism you would need to test it out locally.

@@ -33,7 +34,7 @@ import org.springframework.context.annotation.Configuration
havingValue = "true",
matchIfMissing = true
)
@Configuration(proxyBeanMethods = false)
@AutoConfiguration
Copy link
Author

@Stuckya Stuckya Oct 20, 2022

Choose a reason for hiding this comment

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

Another notable change in functionality as of 3.0.0-M5.

With this release, support for registering auto-configurations in spring.factories has been removed in favor of the imports file.

A new @AutoConfiguration annotation has been introduced. It should be used to annotate top-level auto-configuration classes that are listed in the new META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports file, replacing @Configuration.

https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0.0-M5-Release-Notes#auto-configuration-registration

Copy link
Author

Choose a reason for hiding this comment

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

@Stuckya
Copy link
Author

Stuckya commented Oct 20, 2022

@srinivasankavitha & @berngp, thank you both for your continued support and eyes on this PR.

I wanted to provide a quick update on the Spring Boot 3 release timeline. The release candidate process has begun. Last week the Spring team began publishing release candidates for Spring Framework & Spring Security, which this PR is now using.

Spring announced today that they will be releasing the Spring Boot 3.0.0-RC1 later this evening. https://spring.io/blog/2022/10/20/spring-framework-6-0-0-rc2-available-now

I will update this PR with the latest release candidates as they become available.

It appears that the Spring team is on track for a GA release in mid-late November.

Cheers!

@srinivasankavitha
Copy link
Contributor

Thanks for all the work on the PR so far!

@NestedConfigurationProperty
var autotime: AutoTimeProperties = AutoTimeProperties(),
var autotime: PropertiesAutoTimer = PropertiesAutoTimer(autotimeProperties),
Copy link
Author

@Stuckya Stuckya Oct 21, 2022

Choose a reason for hiding this comment

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

This one probably needs some input from @srinivasankavitha & @berngp.

This change wasn't well documented and I had to do some digging to figure out why this change was made. Spring Boot 3.0.0-RC1 attempts to decouple Micrometer from MetricsProperties and MetricsProperties references AutoTimeProperties. Seems reasonable...

See spring boot change here: spring-projects/spring-boot@180d0ed

I composed the properties and timer in this data class to provide easy access to AutoTimeProperties, but very open to different approaches / ideas.

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach looks good to me!

@Stuckya
Copy link
Author

Stuckya commented Nov 12, 2022

Another quick update! The release candidates seem to be stabilizing from the Spring team. I'm seeing less refactoring or feature addition in these latest updates. I've updated this branch again to use the latest release candidates.

We also now have a targeted release date, the Spring team announced:

Our final release is scheduled for November 24, 2022

@Stuckya
Copy link
Author

Stuckya commented Nov 29, 2022

@Stuckya - the current plan for internal consumption is to maintain both 2.x and 3.x branches for a temporary period of time when we are ready to start consuming 3.x. Likely to happen in the next couple months.

Sounds great, thanks for the update @srinivasankavitha!

Should I continue to target master with this PR?

FWIW in my experience, master usually becomes the most recent release and another branch is maintained for backwards compatibility. Preserves trunk based development for the new work, but provides the flexibility of a short-lived release branch for these weird times when internal limitations complicate ideal workflows.

3.x support would == master, corresponding with a DGS 6.x release.
2.x support would == new temporary branch, maintaining the DGS 5.x release line.

Please let me know if I can help in anyway!

@DGSMutation and @DGSQuery field need to be annotated with @AliasFor due to:

spring-projects/spring-framework#28760

Thanks for bringing this back to my attention. I would be happy to address this update in my PR.

import java.lang.annotation.*;

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@DgsData(parentType = "Mutation")
@Inherited
public @interface DgsMutation {
@AliasFor(annotation = DgsData.class)
Copy link
Author

@Stuckya Stuckya Nov 30, 2022

Choose a reason for hiding this comment

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

Spring Framework 6.0 was logging a semi-noisy warning about an upcoming deprecation. It was triggered by all of @DgsData's children. Example output:

o.s.c.annotation.AnnotationTypeMapping : Support for convention-based annotation attribute overrides is deprecated and will be removed in Spring Framework 6.1. Please annotate the following attributes in @com.netflix.graphql.dgs.DgsMutation with appropriate @AliasFor declarations: [field]

The change is discussed here: spring-projects/spring-framework#28760

Since explicit overrides are favorable to implicit overrides, and since the support for convention-based overrides increases the complexity of Spring's annotation search algorithms, we will deprecate convention-based overrides in 6.0 and remove the support in 6.1.

For this PR, I kept @Inherited in place on all annotations. I did some digging into why @Inherited was being used in DGS and found: #1110.

In a separate PR, I'd be happy to implement Spring's AnnotationUtils to allow us to remove @Inherited. Just LMK!

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Yes, we can tackle that as part of a separate PR.

Copy link
Contributor

@srinivasankavitha srinivasankavitha left a comment

Choose a reason for hiding this comment

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

Finally managed to go through all the whole process independently, and all the changes listed here look great.

@NestedConfigurationProperty
var autotime: AutoTimeProperties = AutoTimeProperties(),
var autotime: PropertiesAutoTimer = PropertiesAutoTimer(autotimeProperties),
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach looks good to me!

import java.lang.annotation.*;

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@DgsData(parentType = "Mutation")
@Inherited
public @interface DgsMutation {
@AliasFor(annotation = DgsData.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Yes, we can tackle that as part of a separate PR.

@srinivasankavitha
Copy link
Contributor

srinivasankavitha commented Dec 16, 2022

Thanks so much for all the great work in this PR! 👏

I still need to do some more testing with all these changes, so will work on that over the next few days.
Next steps for me:

  1. Test with more apps to validate the branch
  2. Create a new branch for 2.7 maintenance and do a test release
  3. Clone and update dgs-examples that are used for the pipeline to Spring Boot 3, Java 17 - this should get the CI build passing. We will need separate examples that use SB 2.7 and 3.x

Our current plan is to aim for early Jan for a target 3.x compatible DGS release once the above is ready. We'll backport only essential changes and fixes to 2.7 branch, but new feature development will be enabled on master going forward.

@GrantGochnauer
Copy link

GrantGochnauer commented Dec 16, 2022

Hello -

Thanks for all your work on this PR - We are moving to Spring Boot 3 across our projects and so anxiously awaiting this update. I'm trying to get a ahead and give this update a test. I built the branch out to my local maven repo and pulled it in successfully implementation(platform("com.netflix.graphql.dgs:graphql-dgs-platform-dependencies:5.6.0-SNAPSHOT"))

However when I start my app, I am not seeing the DGS framework initialize and no /graphql endpoint is available. Do you know if there are other updates to ensure Spring Boot is autowiring all the needed DGS components?

Also looks like Spring Cloud final is supposed to be released today: https://calendar.spring.io/

Thank you!

@Stuckya Stuckya changed the title WIP: Spring Boot 3 Spring Boot 3 Dec 20, 2022
@srinivasankavitha
Copy link
Contributor

@Stuckya - I am thinking of merging your PR to a spring-boot-3 branch to further test and add any fixes on top of. Once we are ready, I can merge that branch to main (likely beginning of Jan). This will allow for us to make further fixes, if needed based on our testing. Does that sound ok?

@Stuckya
Copy link
Author

Stuckya commented Dec 21, 2022

I updated this branch today to make use of the spring-cloud-dependencies 2022 GA release, which means we are no longer relying on the milestone spring repository.

@Stuckya - I am thinking of merging your PR to a spring-boot-3 branch to further test and add any fixes on top of. Once we are ready, I can merge that branch to main (likely beginning of Jan). This will allow for us to make further fixes, if needed based on our testing. Does that sound ok?

@srinivasankavitha - Of course, that sounds like it will make things easier for you all to continue making updates util early Jan. Please let me know if I can help in any way.

@srinivasankavitha
Copy link
Contributor

Great, I'll create a new branch and merge your PR there and continue with my testing. It's looking really good so far! Thanks for the change you just made to update the spring-cloud-dependencies to GA. I am updating the example projects so we can get the build passing here and will take it from there.

Thanks again for the contributions!

@srinivasankavitha
Copy link
Contributor

srinivasankavitha commented Dec 21, 2022

Hello -

Thanks for all your work on this PR - We are moving to Spring Boot 3 across our projects and so anxiously awaiting this update. I'm trying to get a ahead and give this update a test. I built the branch out to my local maven repo and pulled it in successfully implementation(platform("com.netflix.graphql.dgs:graphql-dgs-platform-dependencies:5.6.0-SNAPSHOT"))

However when I start my app, I am not seeing the DGS framework initialize and no /graphql endpoint is available. Do you know if there are other updates to ensure Spring Boot is autowiring all the needed DGS components?

Also looks like Spring Cloud final is supposed to be released today: https://calendar.spring.io/

Thank you!

@GrantGochnauer Have you checked to make sure you are on the same branch as this PR? My guess is you don't have the changes to make it work with spring boot 3. I am able to use a local snapshot built from this branch in my test projects successfully.

@srinivasankavitha
Copy link
Contributor

srinivasankavitha commented Dec 21, 2022

@Stuckya - could you please make the following changes as well as part of the PR. I updated example projects to Spring Boot 3 and this branch needs to use the updated examples for the build to pass. I cannot merge to a branch until then anyway.

  1. Please update this file to use dgs-examples-kotlin and dgs-examples-java repositories: https://github.com/Netflix/dgs-framework/blob/master/scripts/config.yml#L21
  2. Same in this file: https://github.com/Netflix/dgs-framework/blob/master/.github/workflows/ci.yml#L62

@Stuckya
Copy link
Author

Stuckya commented Dec 21, 2022

@Stuckya - could you please make the following changes as well as part of the PR. I updated example projects to Spring Boot 3 and this branch needs to use the updated examples for the build to pass. I cannot merge to a branch until then anyway.

  1. Please update this file to use dgs-examples-kotlin and dgs-examples-java repositories: https://github.com/Netflix/dgs-framework/blob/master/scripts/config.yml#L21
  2. Same in this file: https://github.com/Netflix/dgs-framework/blob/master/.github/workflows/ci.yml#L62

You got it, update coming shortly.

@Stuckya
Copy link
Author

Stuckya commented Dec 21, 2022

@srinivasankavitha, should be good to go!

@srinivasankavitha
Copy link
Contributor

Thanks @Stuckya - build seems to have passed ci checks as well with your latest commit. 🥳 I'll merge it to a spring-boot-3-candidate branch and continue testing it. As mentioned earlier, I'll merge the branch to master once we are ready and have created a separate branch for 2.7. So early Jan, 2023.

@srinivasankavitha srinivasankavitha changed the base branch from master to spring-boot-3-candidate December 21, 2022 19:01
@srinivasankavitha srinivasankavitha merged commit ed2955b into Netflix:spring-boot-3-candidate Dec 21, 2022
@srinivasankavitha srinivasankavitha mentioned this pull request Jan 16, 2023
4 tasks
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.

None yet

7 participants