The GitHub Actions job "Required Checks" on texera.git/gh-readonly-queue/main/pr-7796-87eaf5c109d85d18a7f8e2729e23a9c059c18d8a has failed. Run started by GitHub user aglinxinyuan (triggered by aglinxinyuan).
Head commit for run: fd25ce83c397021c42bd926d587c5d492a617475 / Xinyuan Lin <[email protected]> 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) Report URL: https://github.com/apache/texera/actions/runs/38201377066 With regards, GitHub Actions via GitBox
