feat: [Guice 6] Migrate to use `jakarta.inject.*` for injection annotations #2890

Nava2 · 2023-07-31T19:56:54Z

Part of #2863.

This replaces usages to enable migration to Guice 7+. This is the first (only?) step to allow projects using Guice 6.0.0 or Guice 7.0.0 to use misk-inject.

By using jakarta.inject instead of javax.inject, injections work in both 6+ and 7+ versions. The new project, :misk-inject-guice7-test sets up a test project with the classpath forced to Guice 7+. This acts as a guard project for basic usages. As needed, we can add more content.

The exception is using com.google.inject.Provider as Guice 6 does not provide bindings that work with jakarta.inject.Provider in all cases, making this a frustrating requirement.

Part of cashapp#2863. This replaces usages to enable migration to [Guice 7+](https://github.com/google/guice/wiki/Guice700). This is the first (only?) step to allow projects using Guice 6.0.0 or Guice 7.0.0 to use misk-inject. By using `jakarta.inject` instead of `javax.inject`, injections work in both 6+ and 7+ versions. The new project, `:misk-inject-guice7-test` sets up a test project with the classpath forced to Guice 7+. This acts as a guard project for basic usages. As needed, we can add more content.

Nava2 · 2023-07-31T19:57:36Z

detektive/src/main/kotlin/cash/detektive/javacompat/AnnotatePublicApisWithJvmOverloads.kt

@@ -74,7 +74,7 @@ class AnnotatePublicApisWithJvmOverloads(config: Config) : Rule(config) {
    if (!element.valueParameters.any { it.hasDefaultValue() }) return false

    // Is not annotated with @Inject
-    if (element.hasAnyAnnotation("javax.inject.Inject", "com.google.inject.Inject")) return false
+    if (element.hasAnyAnnotation("javax.inject.Inject", "jakarta.inject.Inject", "com.google.inject.Inject")) return false


Actual feature change.

Added support to detekt rule to look at jakarta.

Nava2 · 2023-07-31T19:57:42Z

detektive/src/test/kotlin/cash/detektive/javacompat/AnnotatePublicApisWithJvmOverloadsTest.kt

@@ -129,15 +129,23 @@ internal class AnnotatePublicApisWithJvmOverloadsTest(private val env: KotlinCor
      NoErrorTestCase(
        description = "Public constructor annotated with javax Inject",
        code = """
-        import javax.inject.Inject
+        import jakarta.inject.Inject


added related test

Nava2 · 2023-07-31T19:58:57Z

misk-inject/misk-inject-guice7-test/README.md

@@ -0,0 +1,5 @@
+## misk-inject-guice7-test


these add the tests to verify that Guice 7 works with misk-inject

Nava2 · 2023-07-31T20:10:35Z

detekt.yaml

@@ -21,6 +21,22 @@ complexity:
  LongParameterList:
    active: false

+style:


Bans imports in future code

Nava2 · 2023-07-31T20:15:17Z

cc @adrw / @r3mariano

Nava2 · 2023-08-01T00:44:05Z

@r3mariano I'm unsure how to actually land this change. Could you advise on process or documentation?

Nava2 · 2023-08-01T02:09:30Z

@jdm-square looks like there's some dependency cleanups and general lint things. Is there an autoformat somewhere.. was hoping the detekt formatting rules were in place but seem to not be.

Also looks like I need to regenerate the API lockfiles, easy enough.

jdm-square · 2023-08-01T03:06:17Z

I don't see any linting issues that require formatting, only the missing api dump and dependency changes.

r3mariano · 2023-08-02T23:55:09Z

@r3mariano I'm unsure how to actually land this change. Could you advise on process or documentation?

@Nava2 Nothing particular for the OSS repo. We're already aware of the change and can handle the change in our internal services (i.e. upgrade everyone to 6). Releases already happen automatically, we switched to a date-based versioning scheme that publishes for every change in main.

There is an incoming change to add more (non-Guice) modules, hopefully that won't cause a conflict.

jdm-square · 2023-08-03T13:50:09Z

CronTest > leaseDenied() FAILED
    org.opentest4j.AssertionFailedError: 
    expected: 1
     but was: 0
        at java.base@17.0.7/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base@17.0.7/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at java.base@17.0.7/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base@17.0.7/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//misk.cron.CronTest.leaseDenied(CronTest.kt:78)

I'll run it again to see if it's an intermittent error.

…igrate-to-use-com.google.inject

Nava2 · 2023-08-03T20:00:28Z

@jdm-square / @r3mariano / @JGulbronson could one of you enable the tests to run again? :) This should be bueno now!

Nava2 · 2023-08-03T20:25:24Z

Thanks to whomever enabled the CI, I can't actually merge this

Nava2 force-pushed the kevin/guice-7/migrate-to-use-com.google.inject branch from 1d0bd2f to 4a60568 Compare July 31, 2023 20:00

Nava2 force-pushed the kevin/guice-7/migrate-to-use-com.google.inject branch from 4a60568 to 67c4984 Compare July 31, 2023 20:09

Nava2 commented Jul 31, 2023

View reviewed changes

remove BindingAnnotation, oops

c9b0254

Nava2 commented Jul 31, 2023

View reviewed changes

detekt.yaml

@@ -21,6 +21,22 @@ complexity:

LongParameterList:

active: false

style:

Copy link

Contributor Author

Nava2 Jul 31, 2023

Choose a reason for hiding this comment

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

Bans imports in future code

Nava2 marked this pull request as ready for review July 31, 2023 20:14

Nava2 changed the title ~~[Guice 6] Migrate to use jakarta.inject.* for injection annotations~~ feat: [Guice 6] Migrate to use jakarta.inject.* for injection annotations Jul 31, 2023

r3mariano approved these changes Jul 31, 2023

View reviewed changes

jdm-square enabled auto-merge (squash) August 1, 2023 02:00

Fix dependencies and apiDump

e0e85de

auto-merge was automatically disabled August 1, 2023 13:35
Head branch was pushed to by a user without write access

Nava2 added 3 commits August 3, 2023 15:04

fix bad refactors

25da791

Merge branch 'master' of github.com:cashapp/misk into kevin/guice-7/m…

82b0dc5

…igrate-to-use-com.google.inject

Merge branch 'master' of github.com:cashapp/misk into kevin/guice-7/m…

b7e8f77

…igrate-to-use-com.google.inject

jdm-square merged commit d7c6fe0 into cashapp:master Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [Guice 6] Migrate to use `jakarta.inject.*` for injection annotations #2890

feat: [Guice 6] Migrate to use `jakarta.inject.*` for injection annotations #2890

Nava2 commented Jul 31, 2023

Nava2 Jul 31, 2023

Nava2 Jul 31, 2023

Nava2 Jul 31, 2023

Nava2 Jul 31, 2023

Nava2 commented Jul 31, 2023

Nava2 commented Aug 1, 2023

Nava2 commented Aug 1, 2023

jdm-square commented Aug 1, 2023

r3mariano commented Aug 2, 2023

jdm-square commented Aug 3, 2023

Nava2 commented Aug 3, 2023 •

edited

Loading

Nava2 commented Aug 3, 2023

feat: [Guice 6] Migrate to use jakarta.inject.* for injection annotations #2890

feat: [Guice 6] Migrate to use jakarta.inject.* for injection annotations #2890

Conversation

Nava2 commented Jul 31, 2023

Nava2 Jul 31, 2023

Choose a reason for hiding this comment

Nava2 Jul 31, 2023

Choose a reason for hiding this comment

Nava2 Jul 31, 2023

Choose a reason for hiding this comment

Nava2 Jul 31, 2023

Choose a reason for hiding this comment

Nava2 commented Jul 31, 2023

Nava2 commented Aug 1, 2023

Nava2 commented Aug 1, 2023

jdm-square commented Aug 1, 2023

r3mariano commented Aug 2, 2023

jdm-square commented Aug 3, 2023

Nava2 commented Aug 3, 2023 • edited Loading

Nava2 commented Aug 3, 2023

feat: [Guice 6] Migrate to use `jakarta.inject.*` for injection annotations #2890

feat: [Guice 6] Migrate to use `jakarta.inject.*` for injection annotations #2890

Nava2 commented Aug 3, 2023 •

edited

Loading