Yicong-Huang commented on code in PR #5983: URL: https://github.com/apache/texera/pull/5983#discussion_r3741170206
########## common/auth/src/main/scala/org/apache/texera/auth/ServiceBootstrap.scala: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.auth + +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider} +import io.dropwizard.core.{Application, Configuration} +import io.dropwizard.core.setup.Bootstrap +import org.apache.texera.common.config.StorageConfig +import org.apache.texera.dao.SqlServer + +import java.nio.file.Path + +/** Shared Dropwizard service bootstrap steps, identical across every Texera + * service. Kept here so the services don't drift apart. + */ +object ServiceBootstrap { Review Comment: This is the third round on placement, so let me add evidence rather than repeat the question. Nothing in this object is authentication: the imports are `DefaultScalaModule` (JSON binding), `StorageConfig`/`SqlServer` (persistence), and `Application`/`Bootstrap` (Dropwizard lifecycle). Supporting it also added `jackson-module-scala` to `common/auth/build.sbt:66`, so the module every service links for its security stack now carries a JSON dependency no auth path uses. I'd move it to `common/resource`: all six services already `.dependsOn(Resource)`, and it already holds the shared `HealthCheckResource`. It needs `DAO`/`Config` added there, which is a smaller cost than widening what `common/auth` means. ########## build.sbt: ########## @@ -141,7 +147,11 @@ lazy val AccessControlService = (project in file("access-control-service")) dependencyOverrides ++= Seq( // override it as io.dropwizard 4 require 2.16.1 or higher "com.fasterxml.jackson.module" %% "jackson-module-scala" % jacksonVersion - ) + ), + // AccessControlServiceRunSpec's initialize test opens the JVM-wide SqlServer + // singleton that the DB-backed AccessControlResourceSpec also relies on; run + // the suites serially so they can't clobber each other's connection. + Test / parallelExecution := false Review Comment: Deleting the tautological `ServiceBootstrapSpec` case removes the `Auth` hunk but not this one or the notebook-migration one — those exist so `*RunSpec."initialize"` can open the real pool. Each buys a single `verify(bootstrap).setConfigurationSourceProvider(...)`, which `ServiceBootstrapSpec:43-54` already asserts for the shared helper. Is asserting delegation worth permanently serializing these suites? If it is, the comment should say so, since the constraint outlives the test that caused it. ########## common/auth/build.sbt: ########## @@ -63,6 +63,7 @@ libraryDependencies ++= Seq( "org.glassfish.jersey.core" % "jersey-server" % "3.0.12" % "provided", // for RoleAnnotationEnforcer's ResourceConfig overload and AuthFeatures' RolesAllowedDynamicFeature "io.dropwizard" % "dropwizard-core" % "4.0.7" % "provided", // for AuthFeatures' Environment "io.dropwizard" % "dropwizard-auth" % "4.0.7" % "provided", // for AuthFeatures' AuthDynamicFeature/AuthValueFactoryProvider + "com.fasterxml.jackson.module" %% "jackson-module-scala" % "2.18.6" % "provided", // for ServiceBootstrap's DefaultScalaModule Review Comment: The build already manages this artifact as `jacksonVersion = "2.18.8"` (build.sbt:83), and the five service build files pin the same 2.18.8 literal. Nothing breaks today — it is `provided`, so each service supplies its own at runtime — but this is a seventh copy at a different value, and the next jackson bump will move the six and miss this one. ```suggestion "com.fasterxml.jackson.module" %% "jackson-module-scala" % "2.18.8" % "provided", // for ServiceBootstrap's DefaultScalaModule ``` ########## common/auth/src/test/scala/org/apache/texera/auth/ServiceBootstrapSpec.scala: ########## @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.auth + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import io.dropwizard.configuration.ConfigurationSourceProvider +import io.dropwizard.core.{Application, Configuration} +import io.dropwizard.core.setup.Bootstrap +import org.apache.texera.dao.SqlServer +import org.mockito.ArgumentCaptor +import org.mockito.ArgumentMatchers.{any, isA} +import org.mockito.Mockito.{mock, verify, when} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.io.ByteArrayInputStream +import java.nio.charset.StandardCharsets +import java.nio.file.Paths +import scala.util.control.NonFatal + +class ServiceBootstrapSpec extends AnyFlatSpec with Matchers { + + // Every service shares this bootstrap helper, so its behavior is verified once here + // rather than per service. + "ServiceBootstrap.configure" should "wrap the config source provider and register the Scala module" in { + val bootstrap = mock(classOf[Bootstrap[Configuration]]) + val objectMapper = mock(classOf[ObjectMapper]) + val sourceProvider = mock(classOf[ConfigurationSourceProvider]) + when(bootstrap.getObjectMapper).thenReturn(objectMapper) + when(bootstrap.getConfigurationSourceProvider).thenReturn(sourceProvider) + + ServiceBootstrap.configure(bootstrap) + + verify(bootstrap).setConfigurationSourceProvider(any(classOf[ConfigurationSourceProvider])) + verify(objectMapper).registerModule(isA(classOf[DefaultScalaModule])) + } + + it should "install a source provider that substitutes environment variables in the config" in { + assume(sys.env.contains("HOME")) + val bootstrap = mock(classOf[Bootstrap[Configuration]]) + val delegate = mock(classOf[ConfigurationSourceProvider]) + when(bootstrap.getObjectMapper).thenReturn(mock(classOf[ObjectMapper])) + when(bootstrap.getConfigurationSourceProvider).thenReturn(delegate) + when(delegate.open("config.yaml")).thenReturn( + new ByteArrayInputStream( + "home: ${HOME}\nliteral: ${TEXERA_UNSET_TEST_VAR}".getBytes(StandardCharsets.UTF_8) + ) + ) + + ServiceBootstrap.configure(bootstrap) + + val captor = ArgumentCaptor.forClass(classOf[ConfigurationSourceProvider]) + verify(bootstrap).setConfigurationSourceProvider(captor.capture()) + val substituted = + new String(captor.getValue.open("config.yaml").readAllBytes(), StandardCharsets.UTF_8) + + substituted should include(sys.env("HOME")) + // strict = false: a placeholder with no matching env var must pass through unchanged + // rather than fail service startup. + substituted should include("${TEXERA_UNSET_TEST_VAR}") + } + + "ServiceBootstrap.configFilePath" should "resolve the conventional resources path under the service dir" in { + val result = ServiceBootstrap.configFilePath("file-service", "file-service-web-config.yaml") + + val expectedSuffix = Paths + .get("file-service", "src", "main", "resources", "file-service-web-config.yaml") + .toString + result should endWith(expectedSuffix) + Paths.get(result).isAbsolute shouldBe true + } + + "ServiceBootstrap.start" should "launch the Dropwizard server command with the conventional config path" in { + val app = mock(classOf[Application[Configuration]]) + + ServiceBootstrap.start(app, "config-service", "config-service-web-config.yaml") + + verify(app).run( + "server", + ServiceBootstrap.configFilePath("config-service", "config-service-web-config.yaml") + ) + } + + "ServiceBootstrap.initDatabase" should "run the shared SQL connection-pool setup from storage config" in { Review Comment: This case cannot fail. The failure branch is `case NonFatal(_) => succeed`, and the success branch asserts `SqlServer.getInstance()` is not null — that is `instance.get`, non-null by construction. Delete the body of `ServiceBootstrap.initDatabase` and the suite still goes green. It is not free either: `initConnection` closes whichever pool a sibling suite opened and replaces the singleton, which is the sole reason `build.sbt:126-129` serializes all 11 `common/auth` suites. The spec this one is modelled on, `AuthFeaturesSpec`, is pure-mock with four `verify` calls that each fail on a dropped registration. I would delete this case and the build.sbt hunk with it. ########## file-service/src/test/scala/org/apache/texera/service/FileServiceRunSpec.scala: ########## @@ -40,4 +52,39 @@ class FileServiceRunSpec extends AnyFlatSpec with Matchers { ) ) shouldBe empty } + + "FileService.initialize" should "run the shared bootstrap and register the dataset serializer module" in { + val bootstrap = mock(classOf[Bootstrap[FileServiceConfiguration]]) + val objectMapper = mock(classOf[ObjectMapper]) + when(bootstrap.getObjectMapper).thenReturn(objectMapper) + when(bootstrap.getConfigurationSourceProvider) + .thenReturn(mock(classOf[ConfigurationSourceProvider])) + + new FileService().initialize(bootstrap) + + verify(bootstrap).setConfigurationSourceProvider(any(classOf[ConfigurationSourceProvider])) + // Scala module (via ServiceBootstrap.configure) + the DatasetFileNode serializer module. + verify(objectMapper, org.mockito.Mockito.atLeastOnce()).registerModule(any()) Review Comment: The comment above claims two registrations, but `atLeastOnce()` passes with one — this stays green if `ServiceBootstrap.configure` stops registering the Scala module. ```suggestion verify(objectMapper, org.mockito.Mockito.times(2)).registerModule(any()) ``` ########## config-service/src/main/scala/org/apache/texera/service/ConfigService.scala: ########## @@ -19,37 +19,25 @@ package org.apache.texera.service -import com.fasterxml.jackson.module.scala.DefaultScalaModule import com.typesafe.scalalogging.LazyLogging -import io.dropwizard.configuration.{EnvironmentVariableSubstitutor, SubstitutingSourceProvider} import io.dropwizard.core.Application import io.dropwizard.core.setup.{Bootstrap, Environment} -import org.apache.texera.auth.{AuthFeatures, RequestLoggingFilter, RoleAnnotationEnforcer} -import org.apache.texera.common.config.{DefaultsConfig, StorageConfig} +import org.apache.texera.auth.{ + AuthFeatures, + RequestLoggingFilter, + RoleAnnotationEnforcer, + ServiceBootstrap +} +import org.apache.texera.common.config.DefaultsConfig import org.apache.texera.dao.SqlServer import org.apache.texera.service.resource.{ConfigResource, HealthCheckResource} import org.eclipse.jetty.server.session.SessionHandler import org.jooq.impl.DSL -import java.nio.file.Path - class ConfigService extends Application[ConfigServiceConfiguration] with LazyLogging { override def initialize(bootstrap: Bootstrap[ConfigServiceConfiguration]): Unit = { - // enable environment variable substitution in YAML config - bootstrap.setConfigurationSourceProvider( - new SubstitutingSourceProvider( - bootstrap.getConfigurationSourceProvider, - new EnvironmentVariableSubstitutor(false) - ) - ) - // Register Scala module to Dropwizard default object mapper - bootstrap.getObjectMapper.registerModule(DefaultScalaModule) - - SqlServer.initConnection( - StorageConfig.jdbcUrl, - StorageConfig.jdbcUsername, - StorageConfig.jdbcPassword - ) + ServiceBootstrap.configure(bootstrap) + ServiceBootstrap.initDatabase() Review Comment: Since the point of this PR is to stop the drift: `initDatabase()` still lands in two different Dropwizard phases — `initialize()` here and in access-control and notebook-migration, but `run()` in file, workflow-compiling and computing-unit-managing. Both work, since `StorageConfig` is HOCON-sourced rather than read from the YAML, so neither phase is too early. Is the split deliberate? A one-line note either way would stop the next service from copying whichever one it happens to open first. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
