diff --git a/detekt.yaml b/detekt.yaml index ef4b45dcc4f..a63c3b63737 100644 --- a/detekt.yaml +++ b/detekt.yaml @@ -40,6 +40,8 @@ style: # Until moved to Guice 7, jakarta.inject.Provider causes issues with type overloads in Guice 6 - value: 'jakarta.inject.Provider' reason: 'To be Guice 6 compliant, use `com.google.inject.Provider`' + excludes: + - '**/wisp/wisp-moshi/src/*/kotlin/wisp/moshi/ProviderJsonAdapter*.kt' detektive: active: true diff --git a/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt b/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt index 0fc9a2a517b..8255f439ab5 100644 --- a/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt +++ b/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt @@ -17,8 +17,9 @@ import misk.web.dashboard.AdminDashboardAccess import misk.web.mediatype.MediaTypes import misk.web.metadata.toFormattedJson import misk.web.v2.DashboardPageLayout +import wisp.moshi.ProviderJsonAdapterFactory import wisp.moshi.adapter -import wisp.moshi.defaultKotlinMoshi +import wisp.moshi.buildMoshi @Singleton class ServiceGraphTabIndexAction @Inject constructor( @@ -37,7 +38,7 @@ class ServiceGraphTabIndexAction @Inject constructor( } } .build { _, _, _ -> - val metadataArray = defaultKotlinMoshi + val metadataArray = buildMoshi(listOf(ProviderJsonAdapterFactory())) .adapter>() .toFormattedJson(serviceGraphMetadataProvider.get().graphVisual) diff --git a/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt b/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt index 7cacc662036..32c66e019ba 100644 --- a/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt +++ b/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt @@ -19,6 +19,8 @@ class AllMetadataActionTest { @Inject lateinit var action: AllMetadataAction + // TODO fix this test to rely on Misk injected moshi to generate response and assert against JSON instead of toString() + // so then can remove the manual fix in ServiceGraphBuilder.kt @Test fun `get all`() { val actual = action.getAll("all") diff --git a/misk-config/src/main/kotlin/misk/web/metadata/Metadata.kt b/misk-config/src/main/kotlin/misk/web/metadata/Metadata.kt index 48165cdbdf1..f7c47cba38b 100644 --- a/misk-config/src/main/kotlin/misk/web/metadata/Metadata.kt +++ b/misk-config/src/main/kotlin/misk/web/metadata/Metadata.kt @@ -2,7 +2,6 @@ package misk.web.metadata import com.squareup.moshi.JsonAdapter import kotlinx.html.TagConsumer -import kotlinx.html.div import kotlinx.html.h3 import misk.tailwind.components.AlertInfoHighlight import misk.tailwind.components.CodeBlock diff --git a/misk-service/src/main/kotlin/misk/CoordinatedService.kt b/misk-service/src/main/kotlin/misk/CoordinatedService.kt index 09a7bde4a9d..5429ca2c897 100644 --- a/misk-service/src/main/kotlin/misk/CoordinatedService.kt +++ b/misk-service/src/main/kotlin/misk/CoordinatedService.kt @@ -1,11 +1,15 @@ package misk import com.google.common.util.concurrent.AbstractService +import com.google.common.util.concurrent.Atomics import com.google.common.util.concurrent.MoreExecutors import com.google.common.util.concurrent.Service import com.google.common.util.concurrent.Service.Listener import com.google.common.util.concurrent.Service.State +import com.google.inject.Key import com.google.inject.Provider +import com.google.inject.name.Named +import misk.inject.toKey import java.util.concurrent.atomic.AtomicBoolean data class CoordinatedServiceMetadata( @@ -13,11 +17,52 @@ data class CoordinatedServiceMetadata( val directDependsOn: Set, ) +internal inline fun CoordinatedService(serviceProvider: Provider) = CoordinatedService( + key = T::class.toKey(), + serviceProvider = serviceProvider +) + internal class CoordinatedService( + private val key: Key<*>, private val serviceProvider: Provider ) : AbstractService(), DelegatingService { + + /** + * To avoid accessing the by lazy service property for toString, use key until the service + * is initiated by the service graph. This will be replaced with service.toString() during + * by lazy initialization. + */ + private val toStringLazy = Atomics.newReference<() -> String> { + buildString { + key.annotationType?.let { + if (key.annotation is Named) { + append((key.annotation as Named).value) + } else { + append("@" + key.annotationType.simpleName + " ") + append(key.typeLiteral.rawType.simpleName) + } + } ?: append(key.typeLiteral.rawType.simpleName) + + append(" [${this@CoordinatedService.state()}]") + if (this@CoordinatedService.state() == State.FAILED) { + append(" caused by: ${this@CoordinatedService.failureCause()}") + } + } + } + + /** + * Use care when accessing this value as it will instantiate the instance of the class at this + * time. We want the instantiation to follow the Coordinated Service Graph. + */ override val service: Service by lazy { - serviceProvider.get() + val realService = + runCatching { + serviceProvider.get() + } + .onFailure { + this@CoordinatedService.notifyFailed(it) + } + .getOrThrow() .also { created -> created.checkNew("$created must be NEW for it to be coordinated") created.addListener( @@ -42,6 +87,12 @@ internal class CoordinatedService( MoreExecutors.directExecutor() ) } + + toStringLazy.set { + realService.toString() + } + + realService } /** Services this starts before aka enhancements or services who depend on this. */ @@ -57,15 +108,16 @@ internal class CoordinatedService( private val innerServiceStarted = AtomicBoolean(false) /** - * Marks every [services] as a dependency and marks itself as directDependsOn on each service. + * Marks every [coordinatedServices] as a dependency and marks itself as + * directDependsOn on each service. * */ - fun addDependentServices(vararg services: CoordinatedService) { + fun addDependentServices(vararg coordinatedServices: CoordinatedService) { // Check that this service and all dependent services are new before modifying the graph. this.checkNew() - for (service in services) { - service.checkNew() - dependencies += service - service.directDependsOn += this + for (coordinatedService in coordinatedServices) { + coordinatedService.checkNew() + dependencies += coordinatedService + coordinatedService.directDependsOn += this } } @@ -111,7 +163,11 @@ internal class CoordinatedService( } } - override fun toString() = service.toString() + /** + * Get the service name from the current string source. This uses the key until the service + * is Injected to avoid early injecting the service out of band from the service graph. + */ + override fun toString() = toStringLazy.get().invoke() /** * Traverses the dependency graph to detect cycles. If a cycle is found, then a @@ -159,8 +215,10 @@ internal class CoordinatedService( } } - fun toMetadata() = CoordinatedServiceMetadata( - dependencies = dependencies.map { it.serviceProvider.get().javaClass.name }.toSet(), - directDependsOn = directDependsOn.map { it.serviceProvider.get().javaClass.name }.toSet(), - ) + fun toMetadataProvider() = Provider { + CoordinatedServiceMetadata( + dependencies = dependencies.map { it.serviceProvider.get().javaClass.name }.toSet(), + directDependsOn = directDependsOn.map { it.serviceProvider.get().javaClass.name }.toSet(), + ) + } } diff --git a/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt b/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt index d06b2d9f0a4..6ad44580f91 100644 --- a/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt +++ b/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt @@ -8,12 +8,16 @@ import com.google.inject.Provider import misk.CoordinatedService.Companion.CycleValidity data class ServiceGraphBuilderMetadata( - val serviceMap: Map, + val serviceMap: Map>, val serviceNames: Map, /** A map of downstream services -> their upstreams. */ val dependencyMap: Map, val asciiVisual: String, -) +) { + override fun toString(): String { + return "ServiceGraphBuilderMetadata(serviceMap=${serviceMap.mapValues { (_,v) -> v.get() }}, serviceNames=$serviceNames, dependencyMap=$dependencyMap, asciiVisual=$asciiVisual)" + } +} /** * Builds a graph of [CoordinatedService]s which defer start up and shut down until their dependent @@ -40,7 +44,7 @@ internal class ServiceGraphBuilder { check(serviceMap[key] == null) { "Service $key cannot be registered more than once" } - serviceMap[key] = CoordinatedService(serviceProvider) + serviceMap[key] = CoordinatedService(key, serviceProvider) serviceNames[key] = serviceName ?: "Anonymous Service(${key.typeLiteral.type.typeName})" } @@ -155,7 +159,7 @@ internal class ServiceGraphBuilder { } fun toMetadata() = ServiceGraphBuilderMetadata( - serviceMap = serviceMap.map { it.key.toString() to it.value.toMetadata() }.toMap(), + serviceMap = serviceMap.map { it.key.toString() to it.value.toMetadataProvider() }.toMap(), serviceNames = serviceNames.mapKeys { it.key.toString() }, dependencyMap = dependencyMap.asMap().map { (k,v) -> k.toString() to v.toString() }.toMap(), asciiVisual = toString() diff --git a/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt b/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt index 3800b9125e2..3c326f91f9c 100644 --- a/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt +++ b/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt @@ -6,19 +6,21 @@ import misk.ServiceGraphBuilderMetadata import misk.web.metadata.Metadata import misk.web.metadata.MetadataProvider import misk.web.metadata.toFormattedJson +import wisp.moshi.ProviderJsonAdapterFactory import wisp.moshi.adapter -import wisp.moshi.defaultKotlinMoshi +import wisp.moshi.buildMoshi data class ServiceGraphMetadata( val builderMetadata: ServiceGraphBuilderMetadata, ) : Metadata( metadata = builderMetadata, - prettyPrint = "Service Graph Ascii Visual\n\n${builderMetadata.asciiVisual}\n\nMetadata\n\n" + defaultKotlinMoshi + prettyPrint = "Service Graph Ascii Visual\n\n${builderMetadata.asciiVisual}\n\nMetadata\n\n" + buildMoshi(listOf(ProviderJsonAdapterFactory())) .adapter() .toFormattedJson(builderMetadata), descriptionString = "Guava service graph metadata, including a ASCII art visualization for easier debugging." ) { - val graphVisual = generateGraphVisual() + /** Only evaluate this lazily to avoid initiating service startup through the [CoordinatedService::toMetadataProvider()] method. */ + val graphVisual by lazy { generateGraphVisual() } data class GraphPairs( val source: String, @@ -42,7 +44,7 @@ data class ServiceGraphMetadata( val output = mutableListOf() builderMetadata.serviceMap.forEach { (key, value) -> - val dependencies = value.dependencies + val dependencies = value.get().dependencies dependencies.forEach { target -> output.add(GraphPairs(extractType(key), extractType(target))) diff --git a/misk-service/src/test/kotlin/misk/CoordinatedServiceTest.kt b/misk-service/src/test/kotlin/misk/CoordinatedServiceTest.kt index b66de14e432..e1fc2927134 100644 --- a/misk-service/src/test/kotlin/misk/CoordinatedServiceTest.kt +++ b/misk-service/src/test/kotlin/misk/CoordinatedServiceTest.kt @@ -6,6 +6,7 @@ import com.google.inject.Key import com.google.inject.Provider import com.google.inject.name.Names import misk.ServiceGraphBuilderTest.AppendingService +import misk.inject.toKey import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.RepeatedTest @@ -23,7 +24,7 @@ class CoordinatedServiceTest { services.forEach { builder.addService(it) } val randomKeys = services.shuffled().map { it.first }.toMutableList() val dependencies = mutableListOf>() - while(randomKeys.isNotEmpty()) { + while (randomKeys.isNotEmpty()) { val next = randomKeys.removeFirst() if (dependencies.isNotEmpty()) { builder.addDependency(dependent = dependencies.random(), dependsOn = next) @@ -51,8 +52,8 @@ class CoordinatedServiceTest { val canStartService = service("canStartService") val neverStartedService = service("neverStartedService") - val failOnStartService = - key("failOnStartService") to FailOnStartService().toCoordinated() + val failOnStartService = (key("failOnStartService") to FailOnStartService()) + .toCoordinated() val manager = ServiceGraphBuilder().also { builder -> builder.addService(canStartService) @@ -83,7 +84,7 @@ class CoordinatedServiceTest { val service = service("service") val failOnStopService = - key("failOnStopService") to FailOnStopService().toCoordinated() + (key("failOnStopService") to FailOnStopService()).toCoordinated() val manager = ServiceGraphBuilder().also { builder -> builder.addService(failOnStopService) @@ -103,16 +104,13 @@ class CoordinatedServiceTest { @Test fun cannotAddRunningServiceAsDependency() { val target = StringBuilder() - val runningService = CoordinatedService( - Provider { - AppendingService(target, "I will be running") - } - ) - val newService = CoordinatedService( - Provider { - AppendingService(target, "I will not run") - } - ) + val runningService = CoordinatedService { + AppendingService(target, "I will be running") + } + val newService = CoordinatedService { + AppendingService(target, "I will not run") + } + runningService.startAsync() @@ -132,8 +130,9 @@ class CoordinatedServiceTest { service.startAsync() val failure = assertFailsWith { - CoordinatedService(Provider { service }).startAsync().awaitRunning() + CoordinatedService{ service }.service.startAsync().awaitRunning() } + assertThat(failure).hasMessage("Running Service must be NEW for it to be coordinated") service.stopAsync() @@ -148,13 +147,17 @@ class CoordinatedServiceTest { } private fun service(id: String): Pair, CoordinatedService> = - key(id) to FakeService(id).toCoordinated() + (key(id) to FakeService(id)).toCoordinated() private fun ServiceGraphBuilder.addService(pair: Pair, Service>) { addService(pair.first, pair.second) } - private fun Service.toCoordinated() = CoordinatedService(Provider { this }) + private fun Pair, Service>.toCoordinated() = + this.first to CoordinatedService( + this.first, + Provider { this.second } + ) private fun key(name: String) = Key.get(Service::class.java, Names.named(name)) } diff --git a/misk-service/src/test/kotlin/misk/CoordinatedServiceToStringTest.kt b/misk-service/src/test/kotlin/misk/CoordinatedServiceToStringTest.kt new file mode 100644 index 00000000000..9f25b53a22b --- /dev/null +++ b/misk-service/src/test/kotlin/misk/CoordinatedServiceToStringTest.kt @@ -0,0 +1,103 @@ +package misk + +import com.google.common.util.concurrent.AbstractIdleService +import com.google.common.util.concurrent.Service +import com.google.common.util.concurrent.ServiceManager +import com.google.inject.Provides +import com.google.inject.util.Modules +import jakarta.inject.Inject +import jakarta.inject.Qualifier +import jakarta.inject.Singleton +import misk.inject.KAbstractModule +import misk.testing.MiskTest +import misk.testing.MiskTestModule +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.Test +import wisp.logging.getLogger + +@MiskTest +class CoordinatedServiceToStringTest { + @Retention(AnnotationRetention.RUNTIME) + @Qualifier + internal annotation class TestAnnotation + + @MiskTestModule + val module = Modules.combine( + MiskTestingServiceModule(), + ServiceModule(), + ServiceModule(TestAnnotation::class), + AnnotatedInstanceModule() + ) + + @Inject lateinit var serviceManager: ServiceManager + + @Test fun toStringDoesntInjectInstance() { + val coordinatedTestCoordinatedService = + serviceManager.servicesByState().get(Service.State.NEW).find { + it.toString() == "TestCoordinatedService [NEW]" + } + + val coordinatedTestCoordinatedServiceAnnotated = + serviceManager.servicesByState().get(Service.State.NEW).find { + it.toString() == "@TestAnnotation TestCoordinatedService [NEW]" + } + + assertThat(coordinatedTestCoordinatedService).isNotNull + assertThat(coordinatedTestCoordinatedService is CoordinatedService) + assertThat(coordinatedTestCoordinatedService.toString()).isEqualTo("TestCoordinatedService [NEW]") + + assertThat(coordinatedTestCoordinatedServiceAnnotated).isNotNull + assertThat(coordinatedTestCoordinatedServiceAnnotated is CoordinatedService) + assertThat(coordinatedTestCoordinatedServiceAnnotated.toString()).isEqualTo("@TestAnnotation TestCoordinatedService [NEW]") + + assertThat(TestCoordinatedService.instances).isEqualTo(0) + + serviceManager.startAsync() + serviceManager.awaitHealthy() + + assertThat(TestCoordinatedService.instances).isEqualTo(2) + assertThat(coordinatedTestCoordinatedService.toString()).isEqualTo("[1] TestCoordinatedService [RUNNING]") + assertThat(coordinatedTestCoordinatedServiceAnnotated.toString()).isEqualTo("[2] TestCoordinatedService [RUNNING]") + + serviceManager.stopAsync() + serviceManager.awaitStopped() + + assertThat(coordinatedTestCoordinatedService.toString()).isEqualTo("[1] TestCoordinatedService [TERMINATED]") + assertThat(coordinatedTestCoordinatedServiceAnnotated.toString()).isEqualTo("[2] TestCoordinatedService [TERMINATED]") + } + + @Singleton + internal class TestCoordinatedService @Inject constructor() : AbstractIdleService() { + private val id: Int + + init { + logger.info { "Created" } + id = ++instances + } + + override fun startUp() { + logger.info { "Started" } + } + + override fun shutDown() { + logger.info { "Shutdown" } + } + + override fun toString(): String = + "[$id] ${this::class.simpleName} [${state()}]" + + companion object { + private val logger = getLogger() + var instances = 0 + } + } + + internal class AnnotatedInstanceModule : KAbstractModule() { + @Provides + @TestAnnotation + @Singleton + fun providesAnnotatedInstance(): TestCoordinatedService { + return TestCoordinatedService() + } + } +} diff --git a/misk-service/src/test/kotlin/misk/ServiceGraphBuilderTest.kt b/misk-service/src/test/kotlin/misk/ServiceGraphBuilderTest.kt index 57b830c3d41..64858d67293 100644 --- a/misk-service/src/test/kotlin/misk/ServiceGraphBuilderTest.kt +++ b/misk-service/src/test/kotlin/misk/ServiceGraphBuilderTest.kt @@ -3,9 +3,9 @@ package misk import com.google.common.util.concurrent.AbstractService import com.google.common.util.concurrent.Service import com.google.inject.Key -import com.google.inject.Provider import com.google.inject.name.Named import com.google.inject.name.Names +import misk.inject.toKey import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.Test import kotlin.test.assertFailsWith @@ -119,7 +119,7 @@ class ServiceGraphBuilderTest { addDependency(dependsOn = unregistered, dependent = keyC) // Unregistered doesn't exist. } assertThat(failure).hasMessage( - "Service C requires $unregistered but no such service was " + + "Service C [NEW] requires $unregistered but no such service was " + "registered with the builder" ) } @@ -278,7 +278,7 @@ class ServiceGraphBuilderTest { } assertThat(failure) - .hasMessage("Detected cycle: Service A -> Service D -> Service C -> Service A") + .hasMessage("Detected cycle: Service A [NEW] -> Service D [NEW] -> Service C [NEW] -> Service A [NEW]") } @Test fun simpleDependencyCycle() { @@ -287,7 +287,7 @@ class ServiceGraphBuilderTest { addDependency(dependsOn = keyB, dependent = keyA) } - assertThat(failure).hasMessage("Detected cycle: Service A -> Service B -> Service A") + assertThat(failure).hasMessage("Detected cycle: Service A [NEW] -> Service B [NEW] -> Service A [NEW]") } @Test fun enhancementCycle() { @@ -295,14 +295,14 @@ class ServiceGraphBuilderTest { enhanceService(toBeEnhanced = keyA, enhancement = keyB) enhanceService(toBeEnhanced = keyB, enhancement = keyA) } - assertThat(failure).hasMessage("Detected cycle: Service A -> Service B -> Service A") + assertThat(failure).hasMessage("Detected cycle: Service A [NEW] -> Service B [NEW] -> Service A [NEW]") } @Test fun selfEnhancement() { val failure = buildAndExpectFailure(listOf(keyA)) { enhanceService(toBeEnhanced = keyA, enhancement = keyA) } - assertThat(failure).hasMessage("Detected cycle: ${keyA.name} -> ${keyA.name}") + assertThat(failure).hasMessage("Detected cycle: ${keyA.name} [NEW] -> ${keyA.name} [NEW]") } @Test fun enhancementDependencyCycle() { @@ -310,7 +310,7 @@ class ServiceGraphBuilderTest { enhanceService(toBeEnhanced = keyA, enhancement = keyB) addDependency(dependsOn = keyB, dependent = keyA) } - assertThat(failure).hasMessage("Detected cycle: Service A -> Service B -> Service A") + assertThat(failure).hasMessage("Detected cycle: Service A [NEW] -> Service B [NEW] -> Service A [NEW]") } @Test fun cannotChangeGraphOnceRunning() { diff --git a/misk/api/misk.api b/misk/api/misk.api index 6a3b9ae5aa6..34515adbd38 100644 --- a/misk/api/misk.api +++ b/misk/api/misk.api @@ -871,6 +871,7 @@ public final class misk/moshi/MoshiAdapterModule$Companion { } public final class misk/moshi/MoshiModule : misk/inject/KAbstractModule { + public static final field Companion Lmisk/moshi/MoshiModule$Companion; public fun ()V public fun (Z)V public fun (ZZ)V @@ -878,6 +879,10 @@ public final class misk/moshi/MoshiModule : misk/inject/KAbstractModule { public final fun provideMoshi (Ljava/util/List;Ljava/util/List;)Lcom/squareup/moshi/Moshi; } +public final class misk/moshi/MoshiModule$Companion { + public final fun getDefaultMoshiAdapters ()Ljava/util/List; +} + public final class misk/moshi/adapters/BigDecimalAdapter { public static final field INSTANCE Lmisk/moshi/adapters/BigDecimalAdapter; public final fun decode (Ljava/lang/String;)Ljava/math/BigDecimal; diff --git a/misk/src/main/kotlin/misk/moshi/MoshiModule.kt b/misk/src/main/kotlin/misk/moshi/MoshiModule.kt index 840f00469f7..b54dfcd999e 100644 --- a/misk/src/main/kotlin/misk/moshi/MoshiModule.kt +++ b/misk/src/main/kotlin/misk/moshi/MoshiModule.kt @@ -9,9 +9,10 @@ import misk.moshi.okio.ByteStringAdapter import misk.moshi.time.InstantAdapter import misk.moshi.time.LocalDateAdapter import wisp.moshi.buildMoshi -import java.util.Date import jakarta.inject.Singleton import misk.moshi.time.OffsetDateTimeAdapter +import wisp.moshi.ProviderJsonAdapterFactory +import java.util.Date import com.squareup.wire.WireJsonAdapterFactory as WireOnlyJsonAdapterFactory import misk.moshi.wire.WireMessageAdapter as MiskOnlyMessageAdapter @@ -39,12 +40,10 @@ class MoshiModule @JvmOverloads constructor( ) ) - install(MoshiAdapterModule(ByteStringAdapter)) install(MoshiAdapterModule(Rfc3339DateJsonAdapter())) - install(MoshiAdapterModule(InstantAdapter)) - install(MoshiAdapterModule(BigDecimalAdapter)) - install(MoshiAdapterModule(LocalDateAdapter)) - install(MoshiAdapterModule(OffsetDateTimeAdapter)) + defaultMoshiAdapters.forEach { + install(MoshiAdapterModule(it)) + } } @Provides @@ -55,4 +54,15 @@ class MoshiModule @JvmOverloads constructor( ): Moshi { return buildMoshi(jsonAdapters, jsonLastAdapters) } + + companion object { + val defaultMoshiAdapters = listOf( + ByteStringAdapter, + InstantAdapter, + BigDecimalAdapter, + LocalDateAdapter, + OffsetDateTimeAdapter, + ProviderJsonAdapterFactory(), + ) + } } diff --git a/misk/src/test/kotlin/misk/web/InvalidActionsTest.kt b/misk/src/test/kotlin/misk/web/InvalidActionsTest.kt index 076c2b61a75..e88fafa2547 100644 --- a/misk/src/test/kotlin/misk/web/InvalidActionsTest.kt +++ b/misk/src/test/kotlin/misk/web/InvalidActionsTest.kt @@ -20,7 +20,7 @@ import jakarta.inject.Singleton class InvalidActionsTest { @Test fun failIdenticalActions() { - val exception = assertThrows("Should throw an exception") { + val exception = assertThrows("Should throw an exception") { Guice.createInjector(IdenticalActionsModule()).getInstance(ServiceManager::class.java) .startAsync().awaitHealthy(Duration.ofSeconds(5)) } diff --git a/misk/src/test/kotlin/misk/web/actions/ReadinessCheckServiceTest.kt b/misk/src/test/kotlin/misk/web/actions/ReadinessCheckServiceTest.kt index ba136d1f2cf..dfc4e3c6a0f 100644 --- a/misk/src/test/kotlin/misk/web/actions/ReadinessCheckServiceTest.kt +++ b/misk/src/test/kotlin/misk/web/actions/ReadinessCheckServiceTest.kt @@ -1,23 +1,26 @@ package misk.web.actions +import com.google.common.util.concurrent.AbstractIdleService import com.google.common.util.concurrent.ServiceManager import com.google.inject.util.Modules +import jakarta.inject.Inject +import jakarta.inject.Singleton +import misk.ServiceModule import misk.healthchecks.FakeHealthCheckModule -import misk.services.FakeServiceModule import misk.testing.MiskTest import misk.testing.MiskTestModule import misk.time.FakeClock import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.Test -import jakarta.inject.Inject +import wisp.logging.getLogger @MiskTest class ReadinessCheckServiceTest { @MiskTestModule val module = Modules.combine( TestWebActionModule(), - FakeServiceModule(), FakeHealthCheckModule(), + ServiceModule() ) @Inject lateinit var clock: FakeClock @@ -25,16 +28,23 @@ class ReadinessCheckServiceTest { @Inject lateinit var serviceManager: ServiceManager @Test fun waitsForRunningServicesBeforeHealthChecks() { + assertThat(ReadinessTestService.instanceCreated).isFalse() assertThat(readinessCheckService.status).isNull() readinessCheckService.refreshStatuses() + // Ensure that refreshStatuses does not result in the service instance being injected. + // This should only happen as part of the ServiceManager startup. + assertThat(ReadinessTestService.instanceCreated).isFalse() + // still null assertThat(readinessCheckService.status).isNull() serviceManager.startAsync() serviceManager.awaitHealthy() + assertThat(ReadinessTestService.instanceCreated).isTrue() + // null until refreshed assertThat(readinessCheckService.status).isNull() @@ -47,4 +57,25 @@ class ReadinessCheckServiceTest { // status cleared assertThat(readinessCheckService.status).isNull() } + + @Singleton + internal class ReadinessTestService @Inject constructor() : AbstractIdleService() { + init { + logger.info { "Created" } + instanceCreated = true + } + + override fun startUp() { + logger.info { "Started" } + } + + override fun shutDown() { + logger.info { "Shutdown" } + } + + companion object { + private val logger = getLogger() + var instanceCreated = false + } + } } diff --git a/wisp/wisp-moshi/api/wisp-moshi.api b/wisp/wisp-moshi/api/wisp-moshi.api index 4574de78b47..b8a740837b8 100644 --- a/wisp/wisp-moshi/api/wisp-moshi.api +++ b/wisp/wisp-moshi/api/wisp-moshi.api @@ -1,3 +1,27 @@ +public final class wisp/moshi/GuiceProviderJsonAdapter : com/squareup/moshi/JsonAdapter { + public fun (Lcom/squareup/moshi/JsonAdapter;)V + public fun fromJson (Lcom/squareup/moshi/JsonReader;)Lcom/google/inject/Provider; + public synthetic fun fromJson (Lcom/squareup/moshi/JsonReader;)Ljava/lang/Object; + public fun toJson (Lcom/squareup/moshi/JsonWriter;Lcom/google/inject/Provider;)V + public synthetic fun toJson (Lcom/squareup/moshi/JsonWriter;Ljava/lang/Object;)V +} + +public final class wisp/moshi/JakartaProviderJsonAdapter : com/squareup/moshi/JsonAdapter { + public fun (Lcom/squareup/moshi/JsonAdapter;)V + public fun fromJson (Lcom/squareup/moshi/JsonReader;)Ljakarta/inject/Provider; + public synthetic fun fromJson (Lcom/squareup/moshi/JsonReader;)Ljava/lang/Object; + public fun toJson (Lcom/squareup/moshi/JsonWriter;Ljakarta/inject/Provider;)V + public synthetic fun toJson (Lcom/squareup/moshi/JsonWriter;Ljava/lang/Object;)V +} + +public final class wisp/moshi/JavaxProviderJsonAdapter : com/squareup/moshi/JsonAdapter { + public fun (Lcom/squareup/moshi/JsonAdapter;)V + public synthetic fun fromJson (Lcom/squareup/moshi/JsonReader;)Ljava/lang/Object; + public fun fromJson (Lcom/squareup/moshi/JsonReader;)Ljavax/inject/Provider; + public synthetic fun toJson (Lcom/squareup/moshi/JsonWriter;Ljava/lang/Object;)V + public fun toJson (Lcom/squareup/moshi/JsonWriter;Ljavax/inject/Provider;)V +} + public final class wisp/moshi/MoshiBuildKt { public static final fun buildMoshi (Ljava/util/List;)Lcom/squareup/moshi/Moshi; public static final fun buildMoshi (Ljava/util/List;Ljava/util/List;)Lcom/squareup/moshi/Moshi; @@ -5,3 +29,8 @@ public final class wisp/moshi/MoshiBuildKt { public static final fun getDefaultKotlinMoshi ()Lcom/squareup/moshi/Moshi; } +public final class wisp/moshi/ProviderJsonAdapterFactory : com/squareup/moshi/JsonAdapter$Factory { + public fun ()V + public fun create (Ljava/lang/reflect/Type;Ljava/util/Set;Lcom/squareup/moshi/Moshi;)Lcom/squareup/moshi/JsonAdapter; +} + diff --git a/wisp/wisp-moshi/build.gradle.kts b/wisp/wisp-moshi/build.gradle.kts index 50065241676..d0d7d6052ad 100644 --- a/wisp/wisp-moshi/build.gradle.kts +++ b/wisp/wisp-moshi/build.gradle.kts @@ -5,6 +5,13 @@ plugins { } dependencies { - api(libs.moshiCore) - implementation(libs.moshiKotlin) + api(libs.moshiCore) + api(libs.guice) + api(libs.jakartaInject) + api(libs.javaxInject) + implementation(libs.moshiKotlin) + + testRuntimeOnly(libs.junitEngine) + testImplementation(libs.junitApi) + testImplementation(libs.kotlinTest) } diff --git a/wisp/wisp-moshi/src/main/kotlin/wisp/moshi/MoshiBuild.kt b/wisp/wisp-moshi/src/main/kotlin/wisp/moshi/MoshiBuild.kt index fcec165b997..ca2e891d9c3 100644 --- a/wisp/wisp-moshi/src/main/kotlin/wisp/moshi/MoshiBuild.kt +++ b/wisp/wisp-moshi/src/main/kotlin/wisp/moshi/MoshiBuild.kt @@ -7,7 +7,7 @@ import com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory /** * Default build for Moshi using the Kotlin JSON adapter */ -val defaultKotlinMoshi = buildMoshi(emptyList()) +val defaultKotlinMoshi = buildMoshi(listOf(ProviderJsonAdapterFactory())) @JvmOverloads fun buildMoshi( diff --git a/wisp/wisp-moshi/src/main/kotlin/wisp/moshi/ProviderJsonAdapter.kt b/wisp/wisp-moshi/src/main/kotlin/wisp/moshi/ProviderJsonAdapter.kt new file mode 100644 index 00000000000..e7a02df2f45 --- /dev/null +++ b/wisp/wisp-moshi/src/main/kotlin/wisp/moshi/ProviderJsonAdapter.kt @@ -0,0 +1,94 @@ +package wisp.moshi + +import com.squareup.moshi.FromJson +import com.squareup.moshi.JsonAdapter +import com.squareup.moshi.JsonReader +import com.squareup.moshi.JsonWriter +import com.squareup.moshi.Moshi +import com.squareup.moshi.ToJson +import com.squareup.moshi.Types +import java.lang.reflect.ParameterizedType +import java.lang.reflect.Type +import java.lang.reflect.WildcardType +import com.google.inject.Provider as GuiceProvider +import javax.inject.Provider as JavaxProvider +import jakarta.inject.Provider as JakartaProvider + +class GuiceProviderJsonAdapter(private val delegate: JsonAdapter) : JsonAdapter>() { + @FromJson + override fun fromJson(reader: JsonReader): GuiceProvider { + val value = delegate.fromJson(reader) + return GuiceProvider { value } + } + + @ToJson + override fun toJson(writer: JsonWriter, value: GuiceProvider?) { + delegate.toJson(writer, value?.get()) + } +} + +class JavaxProviderJsonAdapter(private val delegate: JsonAdapter) : JsonAdapter>() { + @FromJson + override fun fromJson(reader: JsonReader): JavaxProvider { + val value = delegate.fromJson(reader) + return JavaxProvider { value } + } + + @ToJson + override fun toJson(writer: JsonWriter, value: JavaxProvider?) { + delegate.toJson(writer, value?.get()) + } +} + +class JakartaProviderJsonAdapter(private val delegate: JsonAdapter) : JsonAdapter>() { + @FromJson + override fun fromJson(reader: JsonReader): JakartaProvider { + val value = delegate.fromJson(reader) + return JakartaProvider { value } + } + + @ToJson + override fun toJson(writer: JsonWriter, value: JakartaProvider?) { + delegate.toJson(writer, value?.get()) + } +} + + +class ProviderJsonAdapterFactory : JsonAdapter.Factory { + override fun create(type: Type, annotations: Set, moshi: Moshi): JsonAdapter<*>? { + // Step 1: Check the raw type is what you want to match against + val rawType = Types.getRawType(type) + if (rawType != GuiceProvider::class.java && rawType != JavaxProvider::class.java && rawType != JakartaProvider::class.java) { + return null + } + + // Step 2: Check the type is actually parameterized + if (type !is ParameterizedType) { + return null + } + + // Step 3: Extract the parameterized type's upper bound + val providerType = getParameterUpperBound(0, type) + + // Step 4: Look up the adapter for the parameterized type + val delegate = moshi.adapter(providerType, annotations) + + // Step 5: Wrap its adapter in the respective provider adapter + return when (rawType) { + GuiceProvider::class.java -> GuiceProviderJsonAdapter(delegate) + JavaxProvider::class.java -> JavaxProviderJsonAdapter(delegate) + JakartaProvider::class.java -> JakartaProviderJsonAdapter(delegate) + else -> null + } + } + + private fun getParameterUpperBound(index: Int, type: ParameterizedType): Type { + val types = type.actualTypeArguments + require(!(index < 0 || index >= types.size)) { "Index " + index + " not in range [0," + types.size + ") for " + type } + val paramType = types[index] + if (paramType is WildcardType) { + return paramType.upperBounds[0] + } + return paramType + } +} diff --git a/wisp/wisp-moshi/src/test/kotlin/wisp/moshi/ProviderJsonAdapterTest.kt b/wisp/wisp-moshi/src/test/kotlin/wisp/moshi/ProviderJsonAdapterTest.kt new file mode 100644 index 00000000000..0ea1d2c3663 --- /dev/null +++ b/wisp/wisp-moshi/src/test/kotlin/wisp/moshi/ProviderJsonAdapterTest.kt @@ -0,0 +1,36 @@ +package wisp.moshi + +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import com.google.inject.Provider as GuiceProvider +import javax.inject.Provider as JavaxProvider +import jakarta.inject.Provider as JakartaProvider + +class ProviderJsonAdapterTest { + + @Test + fun `happy path`() { + val adapter = buildMoshi(listOf(ProviderJsonAdapterFactory())).adapter() + + val fromModel = Alpha( + guice = { "bingo" }, + javax = { "charlie" }, + jakarta = { "delta" }, + ) + val toJson = """{"guice":"bingo","javax":"charlie","jakarta":"delta"}""" + val actualJson = adapter.toJson(fromModel) + assertEquals(toJson, actualJson) + + val jsonModel = adapter.fromJson(actualJson) + assertEquals(fromModel.guice.get(), jsonModel?.guice?.get()) + assertEquals(fromModel.javax.get(), jsonModel?.javax?.get()) + assertEquals(fromModel.jakarta.get(), jsonModel?.jakarta?.get()) + } + + data class Alpha( + val guice: GuiceProvider, + val javax: JavaxProvider, + val jakarta: JakartaProvider, + ) + +}