This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7796-87eaf5c109d85d18a7f8e2729e23a9c059c18d8a in repository https://gitbox.apache.org/repos/asf/texera.git
commit fd25ce83c397021c42bd926d587c5d492a617475 Author: Xinyuan Lin <[email protected]> AuthorDate: Mon Oct 12 03:25:27 2026 +0000 refactor(services): use the shared request-logging filter and SQL-state constant (#7796) ### What changes were proposed in this PR? Two places where a shared helper already existed but a local copy or literal was used instead. **9 insertions, 27 deletions.** **1. `WorkflowCompilingService` now calls the shared `RequestLoggingFilter`.** It was the only Dropwizard-4 service still inlining its own anonymous request-logging filter; the other five (access-control, computing-unit-managing, config, file, notebook-migration) all call `RequestLoggingFilter.register(environment.getApplicationContext)` at exactly this point. The two implementations were compared line by line before swapping, not assumed equivalent. Normalising fully-qualified prefixes and the logger's local name, every observable matches: logger name `"org.eclipse.jetty.server.RequestLog"`, the `isInfoEnabled` guard, the same access-log format string, url pattern `"/*"`, and `EnumSet.allOf(classOf[DispatcherType])`. The inline copy also sat immediately after `RoleAnnotationEnforcer.enforce`, which is exactly where the siblings place the `register` call — so this is a body-only replacement. **2. Four sites now use `SqlStates.UNIQUE_VIOLATION` instead of the literal `"23505"`.** | Site | Was | |---|---| | `PveResource.scala:110`, `:148` | `e.sqlState() == "23505"` | | `NotebookMigrationResource.scala:404` | `e.sqlState == "23505"` | | `JupyterProvisioner.scala:168` | `e.sqlState == "23505"` | Both modules can already see `common/dao`: `NotebookMigrationService` has a direct DAO dependency, and amber reaches it transitively (`Auth → DAO`, and `WorkflowCompiler → WorkflowOperator → WorkflowCore → DAO`). No build change was needed. The constant was already in use by `ExternalAuthProvisioner`, `LocalAuthProvisioner` and `ResourceNaming`. The remaining `23505` occurrences are CSV test fixtures, a `yarn.lock` checksum, `SqlStates.scala` itself, and three comments that explain what the code means — those are left alone. > **Updated after merging `main`.** The `DatasetResource.scala` site originally in this PR is gone: `main` (#7760) moved that check into `ResourceNaming.failOnDuplicateName`, which already uses `SqlStates.UNIQUE_VIOLATION`, so `DatasetResource.scala` is now identical to `main`. `main` also added a new literal in `JupyterProvisioner.register` (#8032), which this PR now converts as well. ### amber's two inline copies are deliberately untouched Worth recording so they are not "fixed" later: amber's remaining inline filters **cannot** use the shared one. They are `javax.servlet`, because amber pins Jetty 9.4.20, while `RequestLoggingFilter` is `jakarta.servlet`. Their existing TODO — replace once Dropwizard is upgraded — is accurate, and this PR does not touch them. ### Verification Re-run on the branch after merging `main`: - `WorkflowCompilingService/test`: **5 suites, 55 tests, 0 failures.** This matters because `WorkflowCompilingServiceRunSpec` has cases asserting the filter's registration, its dispatch set, and that it forwards the request while logging one access line. They read the filter back out of the captured `FilterHolder` rather than by class, so they survived the swap unchanged — and they are what confirms the shared filter behaves identically in place. - `NotebookMigrationService/test`: **6 suites, 123 tests, 0 failures.** `NotebookMigrationResourceSpec` drives a real unique-constraint violation against embedded Postgres, and its "keep the winning row when two requests provision at once" case exercises the `JupyterProvisioner` race path. - `PveResourceSpec` fails the same 6 tests **both with and without this change** (45 succeeded / 6 failed / 1 canceled either way, verified by swapping in `main`'s `PveResource.scala` and re-running; identical failing set). All 6 are `PveManager` venv/pip cases that are pre-existing on this machine; none touch the `23505` → 409 paths. - `scalafmtCheckAll` and `scalafixAll --check` pass for `WorkflowCompilingService`, `NotebookMigrationService`, `FileService` and `WorkflowExecutionService`; no imports were left orphaned (`FilterHolder` removed, `RequestLoggingFilter` added). ### Any related issues, documentation, discussions? Closes #7792 ### How was this PR tested? ``` sbt "WorkflowCompilingService/test" "NotebookMigrationService/test" "WorkflowExecutionService/testOnly org.apache.texera.web.resource.pythonvirtualenvironment.PveResourceSpec" ``` ``` [info] Tests: succeeded 55, failed 0, canceled 0, ignored 0, pending 0 [info] Tests: succeeded 123, failed 0, canceled 0, ignored 0, pending 0 [info] Tests: succeeded 45, failed 6, canceled 1, ignored 0, pending 0 (identical on main) ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../pythonvirtualenvironment/PveResource.scala | 5 +++-- .../resource/NotebookMigrationResource.scala | 3 ++- .../texera/service/util/JupyterProvisioner.scala | 3 ++- .../texera/service/WorkflowCompilingService.scala | 25 ++-------------------- 4 files changed, 9 insertions(+), 27 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala index aeac02b167..96ffe5d691 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala @@ -25,6 +25,7 @@ import com.fasterxml.jackson.module.scala.DefaultScalaModule import com.typesafe.scalalogging.LazyLogging import io.dropwizard.auth.Auth import org.apache.texera.auth.SessionUser +import org.apache.texera.dao.SqlStates import org.jooq.exception.DataAccessException import javax.ws.rs._ @@ -107,7 +108,7 @@ class PveResource extends LazyLogging { if (updated) Response.ok(Map("veid" -> veid).asJava).build() else Response.status(Response.Status.NOT_FOUND).build() } catch { - case e: DataAccessException if e.sqlState() == "23505" => + case e: DataAccessException if e.sqlState() == SqlStates.UNIQUE_VIOLATION => Response .status(Response.Status.CONFLICT) .entity(s"""An environment named "$name" already exists.""") @@ -145,7 +146,7 @@ class PveResource extends LazyLogging { val veid = PveManager.savePve(sessionUser.getUid.intValue(), name, packagesJson) Response.status(Response.Status.CREATED).entity(Map("veid" -> veid).asJava).build() } catch { - case e: DataAccessException if e.sqlState() == "23505" => + case e: DataAccessException if e.sqlState() == SqlStates.UNIQUE_VIOLATION => Response .status(Response.Status.CONFLICT) .entity(s"""An environment named "$name" already exists.""") diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala index 63f6e23888..f7ee3aba4f 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala @@ -26,6 +26,7 @@ import jakarta.ws.rs._ import jakarta.ws.rs.core._ import org.apache.texera.auth.SessionUser import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.SqlStates import org.jooq.JSONB import org.jooq.exception.DataAccessException import org.jooq.impl.DSL @@ -401,7 +402,7 @@ object NotebookMigrationResource extends LazyLogging { // Backstop for the pre-check TOCTOU race: two writers on a shared workflow can both // pass the existence check, then one INSERT trips the UNIQUE(wid) constraint. Translate // that (Postgres SQLState 23505) to a 409 rather than a generic 500. - case e: DataAccessException if e.sqlState == "23505" => + case e: DataAccessException if e.sqlState == SqlStates.UNIQUE_VIOLATION => Response .status(Response.Status.CONFLICT) .entity(errorJson("A notebook is already stored for this workflow")) diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/util/JupyterProvisioner.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/util/JupyterProvisioner.scala index f0b607e5b5..ac44309bc4 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/util/JupyterProvisioner.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/util/JupyterProvisioner.scala @@ -21,6 +21,7 @@ import com.typesafe.scalalogging.LazyLogging import io.fabric8.kubernetes.client.KubernetesClientException import org.apache.texera.common.config.{KubernetesConfig, StorageConfig} import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.SqlStates import org.apache.texera.dao.jooq.generated.tables.daos.UserJupyterDao import org.apache.texera.dao.jooq.generated.tables.pojos.UserJupyter import org.jooq.exception.DataAccessException @@ -165,7 +166,7 @@ class JupyterProvisioner( // Two concurrent first requests can both provision. uid is the primary key, so the // loser trips 23505; the winner's row holds the same uid-derived addresses, so leaving // it in place is correct. - case e: DataAccessException if e.sqlState == "23505" => + case e: DataAccessException if e.sqlState == SqlStates.UNIQUE_VIOLATION => logger.info(s"Jupyter for user $uid was registered concurrently; keeping that row") } } diff --git a/workflow-compiling-service/src/main/scala/org/apache/texera/service/WorkflowCompilingService.scala b/workflow-compiling-service/src/main/scala/org/apache/texera/service/WorkflowCompilingService.scala index 71aff7800d..01faed615c 100644 --- a/workflow-compiling-service/src/main/scala/org/apache/texera/service/WorkflowCompilingService.scala +++ b/workflow-compiling-service/src/main/scala/org/apache/texera/service/WorkflowCompilingService.scala @@ -24,14 +24,13 @@ import io.dropwizard.core.Application import io.dropwizard.core.setup.{Bootstrap, Environment} import org.apache.texera.common.config.StorageConfig import org.apache.texera.amber.util.{JSONUtils, ObjectMapperUtils} -import org.apache.texera.auth.{AuthFeatures, RoleAnnotationEnforcer} +import org.apache.texera.auth.{AuthFeatures, RequestLoggingFilter, RoleAnnotationEnforcer} import org.apache.texera.dao.SqlServer import org.apache.texera.service.resource.{ HealthCheckResource, WorkflowCompilationResource, WorkflowToPythonResource } -import org.eclipse.jetty.servlet.FilterHolder import java.nio.file.Path @@ -78,27 +77,7 @@ class WorkflowCompilingService extends Application[WorkflowCompilingServiceConfi ) // Route request logs through SLF4J, controlled by TEXERA_SERVICE_LOG_LEVEL - val requestLogger = org.slf4j.LoggerFactory.getLogger("org.eclipse.jetty.server.RequestLog") - environment.getApplicationContext.addFilter( - new FilterHolder(new jakarta.servlet.Filter { - override def doFilter( - request: jakarta.servlet.ServletRequest, - response: jakarta.servlet.ServletResponse, - chain: jakarta.servlet.FilterChain - ): Unit = { - chain.doFilter(request, response) - if (requestLogger.isInfoEnabled) { - val req = request.asInstanceOf[jakarta.servlet.http.HttpServletRequest] - val resp = response.asInstanceOf[jakarta.servlet.http.HttpServletResponse] - requestLogger.info( - s"""${req.getRemoteAddr} - "${req.getMethod} ${req.getRequestURI} ${req.getProtocol}" ${resp.getStatus}""" - ) - } - } - }), - "/*", - java.util.EnumSet.allOf(classOf[jakarta.servlet.DispatcherType]) - ) + RequestLoggingFilter.register(environment.getApplicationContext) } }
