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-7268-351ce201e91fa124987502c9235d16a8d07ab61c in repository https://gitbox.apache.org/repos/asf/texera.git
commit 6a5885c1bce0005a41525ba42f6995de4db14331 Author: Xinyuan Lin <[email protected]> AuthorDate: Mon Aug 3 13:22:14 2026 -0700 test(amber): cover WorkflowEmailNotifier and two GmailResource guards (#7268) ### What changes were proposed in this PR? `WorkflowEmailNotifier` was at **0%**. It looks untestable, but the blocker was never SMTP — it was the constructor: ``` new WorkflowEmailNotifier(wid, ...) └─ line 39: WorkflowResource.getWorkflowName(wid) └─ SqlServer.getInstance() // instance.get on an Option -> throws with no pool ``` So even the pure `shouldSendEmail` was unreachable. `WorkflowResource`'s DAOs are `private def` rather than vals, so the object loads fine and `MockTexeraDB` is enough; `getWorkflowName` reads only the `workflow` table, making **one seeded row** the entire fixture. **12 tests** on the notifier: `createDashboardUrl`'s port branches (−1 / 80 / 443 collapse, an explicit 8080 kept), the subject and content carrying the DB-resolved workflow name, `formatTimestamp` against a fixed `Instant`, `isValidEmail` both ways, `shouldSendEmail` across terminal and in-flight states, and the constructor propagating `NotFoundException` for an unseeded wid. **2 additions** to `GmailResourceSpec`, neither needing infrastructure: the 403 format guard on `notifyUnauthorizedUser` (first statement, ahead of any DAO touch) and the empty-receiver fallback to the session user's email, observable as a 502 rather than a 400. **Assertion strength was measured, not claimed** — 17 production mutations, every one killing at least one test, all reverted: | Mutation | Caught by | |---|---| | drop any one of the `-1` / `80` / `443` port branches | the matching port test (3 mutations) | | `path` loses the workflow id | all 4 URL tests + content | | keep the DB lookup but fake the resulting name | subject + content — proves the assertions read the DB *value*, not just that a call happened | | wrap the lookup in `Try(...).getOrElse` | the `NotFoundException` test | | `ZoneOffset.UTC` → `ZoneId.systemDefault()` | `formatTimestamp` | | drop the `'(UTC)'` suffix | `formatTimestamp` + content | | add/remove a terminal state | the matching `shouldSendEmail` test | | pin `isValidEmail` true / false | the opposite-direction test | | remove either Gmail guard | the corresponding new test | Deliberately **not** tested, with reasons in the file: `sendStatusEmail` (no injectable mail seam, so both branches are observationally identical to an empty body); `TERMINATED`/`UNKNOWN` in `shouldSendEmail` (adding `TERMINATED` is a plausible improvement, so pinning today's answer would cement a limitation); and `isValidEmail("")`, because Hibernate's validator returns **true** for empty input — an "empty is invalid" assertion would be asserting a falsehood, and note `GmailResource`'s own regex-based validator disagrees with it. The new Gmail test **fails fast** if `UserSystemConfig.gmail` is ever non-empty. The suite stays network-free only because it defaults to blank; `Transport.send` is configured with no connect or read timeout, so a misconfigured environment would hang unbounded rather than fail, and on a runner with egress would deliver a real email. **Bug found, reported not cemented:** `sendStatusEmail` wraps `GmailResource.sendEmail` in a `try/catch`, but `sendEmail` returns `Either` and already wraps everything in its own `Try` — so the catch can never fire, and the returned `Left` is discarded without even a log line. Every delivery failure is currently silent. ### Any related issues, documentation, discussions? Closes #7266 ### How was this PR tested? 14 new tests, run with the adjacent email specs to confirm no interference — 19 tests, Java 17: ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.WorkflowEmailNotifierSpec org.apache.texera.web.resource.GmailResourceSpec org.apache.texera.web.service.EmailNotificationServiceSpec" ``` ``` [info] Suites: completed 3, aborted 0 [info] Tests: succeeded 19, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` `Test/scalafmtCheck` and `Test/scalafix --check` both `[success]`. No production file is touched. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> --- .../texera/web/resource/GmailResourceSpec.scala | 63 +++++- .../web/service/WorkflowEmailNotifierSpec.scala | 219 +++++++++++++++++++++ 2 files changed, 281 insertions(+), 1 deletion(-) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala index 868b0e34cd..ec694f694f 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala @@ -20,11 +20,12 @@ package org.apache.texera.web.resource import org.apache.texera.auth.SessionUser +import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum import org.apache.texera.dao.jooq.generated.tables.pojos.User import org.scalatest.flatspec.AnyFlatSpec -import javax.ws.rs.{BadRequestException, WebApplicationException} +import javax.ws.rs.{BadRequestException, ForbiddenException, WebApplicationException} class GmailResourceSpec extends AnyFlatSpec { @@ -71,4 +72,64 @@ class GmailResourceSpec extends AnyFlatSpec { ) assert(ex.getResponse.getStatus == 502) } + + /** + * Every test here that reaches `sendEmail` relies on `UserSystemConfig.gmail` being empty, which + * is the shipped default: it makes `new InternetAddress(senderGmail)` throw inside sendEmail's own + * Try, so nothing is dialled. If it is ever overridden (USER_SYS_GOOGLE_SMTP_GMAIL) the code + * reaches `Transport.send` against smtp.gmail.com:465 with no connect/read timeout configured — + * an unbounded hang on a runner without egress, and a real email where there is one. Fail loudly + * rather than skip, so the misconfiguration is visible instead of silently deleting coverage. + */ + private def requireNoRealGmailSender(): Unit = + if (UserSystemConfig.gmail.nonEmpty) { + fail( + s"UserSystemConfig.gmail is set to '${UserSystemConfig.gmail}'; this suite would open a real " + + "SMTP connection. Unset USER_SYS_GOOGLE_SMTP_GMAIL and re-run." + ) + } + + requireNoRealGmailSender() + it should "fall back to the session user's email when the request carries an empty receiver" in { + requireNoRealGmailSender() + // An empty receiver reaching `sendEmail` unchanged fails the format regex and surfaces as a + // 400; a 502 (the AddressException path described above) means something valid-looking got + // through, i.e. the fallback fired. Strictly this pins "whatever reached sendEmail passed + // isValidEmail" rather than the substitution itself — widening that regex to accept "" would + // keep this green with the fallback deleted — but as production stands it is a genuine + // change-detector, and sendEmailRequest returns Unit so there is no seam for a direct assertion. + val resource = new GmailResource() + val msg = EmailMessage( + receiver = "", + subject = "subj", + content = "body" + ) + val ex = intercept[WebApplicationException] { + resource.sendEmailRequest(msg, newSessionUser()) + } + assert( + !ex.isInstanceOf[BadRequestException], + s"empty receiver was not replaced by the session user's email: ${ex.getMessage}" + ) + assert(ex.getResponse.getStatus == 502) + } + + "notifyUnauthorizedUser" should "reject a malformed receiver with HTTP 403 before any DB access" in { + // The format guard is the first statement, ahead of the admin lookup, so this needs no + // database: `new GmailResource()` touches none either, since the companion's context/userDao + // are defs and senderGmail is lazy. Remove the guard and `userDao.fetchByRole` throws something + // else — NoSuchElementException on a virgin JVM, or a jOOQ DataAccessException once another + // MockTexeraDB suite has initialised and closed the shared SqlServer — and either way + // `intercept[ForbiddenException]` rejects it. + val resource = new GmailResource() + val msg = EmailMessage( + receiver = "not-a-valid-email", + subject = "subj", + content = "body" + ) + val ex = intercept[ForbiddenException] { + resource.notifyUnauthorizedUser(msg) + } + assert(ex.getResponse.getStatus == 403) + } } diff --git a/amber/src/test/scala/org/apache/texera/web/service/WorkflowEmailNotifierSpec.scala b/amber/src/test/scala/org/apache/texera/web/service/WorkflowEmailNotifierSpec.scala new file mode 100644 index 0000000000..af21c7f687 --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/web/service/WorkflowEmailNotifierSpec.scala @@ -0,0 +1,219 @@ +/* + * 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.web.service + +import org.apache.texera.amber.engine.architecture.rpc.controlreturns.WorkflowAggregatedState +import org.apache.texera.amber.engine.architecture.rpc.controlreturns.WorkflowAggregatedState._ +import org.apache.texera.dao.MockTexeraDB +import org.apache.texera.dao.jooq.generated.Tables.WORKFLOW +import org.apache.texera.dao.jooq.generated.tables.daos.WorkflowDao +import org.apache.texera.dao.jooq.generated.tables.pojos.Workflow +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.{BeforeAndAfterAll, PrivateMethodTester} + +import java.net.URI +import java.sql.Timestamp +import java.time.Instant +import java.util.{Locale, TimeZone, UUID} +import javax.ws.rs.NotFoundException + +/** + * Covers the parts of [[WorkflowEmailNotifier]] that decide *whether* to notify and *what* the + * notification says. Concretely, these tests fail if: + * - the constructor stops resolving the workflow name from the `workflow` table (the name is + * seeded here with a random suffix, so a hard-coded or echoed value cannot satisfy the + * subject/content assertions), or stops surfacing a missing workflow as `NotFoundException`; + * - `createDashboardUrl` starts emitting `:80`/`:443`/no-port links, or drops the workflow id + * from the deep link, so the mail points somewhere the user cannot open; + * - `formatTimestamp` stops pinning UTC or changes the human-readable pattern (the JVM default + * zone is deliberately set to non-UTC around the assertion; the locale is pinned to US so the + * expected month/day text is stable across environments); + * - the terminal-state set stops covering a state that ends or suspends an execution, which + * would silently drop the only email a user gets for that run. + * + * SMTP is never exercised: only the pure message-building helpers and the state predicate are + * invoked, so no test here reaches `GmailResource.sendEmail`. + */ +class WorkflowEmailNotifierSpec + extends AnyFlatSpec + with BeforeAndAfterAll + with MockTexeraDB + with PrivateMethodTester { + + private val testWid: Int = 7000 + scala.util.Random.nextInt(1000) + private val unseededWid: Int = testWid + 1 + // Random suffix: the subject/content assertions can only pass if the notifier really read this + // value back out of the database. + private val testWorkflowName: String = "notifier_wf_" + UUID.randomUUID().toString.substring(0, 8) + private val userEmail = "[email protected]" + + private val createDashboardUrl = PrivateMethod[String](Symbol("createDashboardUrl")) + private val createEmailSubject = PrivateMethod[String](Symbol("createEmailSubject")) + private val createEmailContent = PrivateMethod[String](Symbol("createEmailContent")) + private val formatTimestamp = PrivateMethod[String](Symbol("formatTimestamp")) + private val isValidEmail = PrivateMethod[Boolean](Symbol("isValidEmail")) + + override protected def beforeAll(): Unit = { + initializeDBAndReplaceDSLContext() + deleteFixtureRows() + + val workflow = new Workflow + workflow.setWid(testWid) + workflow.setName(testWorkflowName) + workflow.setContent("{}") + workflow.setDescription("seeded by WorkflowEmailNotifierSpec") + workflow.setCreationTime(new Timestamp(System.currentTimeMillis())) + workflow.setLastModifiedTime(new Timestamp(System.currentTimeMillis())) + new WorkflowDao(getDSLContext.configuration()).insert(workflow) + } + + override protected def afterAll(): Unit = { + deleteFixtureRows() + shutdownDB() + } + + private def deleteFixtureRows(): Unit = + getDSLContext.deleteFrom(WORKFLOW).where(WORKFLOW.WID.in(testWid, unseededWid)).execute() + + /** Builds a notifier for the seeded workflow; the constructor hits the database every time. */ + private def notifierFor(sessionUri: String): WorkflowEmailNotifier = + new WorkflowEmailNotifier(testWid.toLong, userEmail, new URI(sessionUri)) + + // ─── createDashboardUrl ──────────────────────────────────────────────────── + + "createDashboardUrl" should "omit the port when the session URI carries none" in { + val url = notifierFor("http://texera.example.com/dashboard/user/workspace/9";) invokePrivate + createDashboardUrl() + assert(url == s"http://texera.example.com/user/workspace/$testWid";) + } + + it should "omit an explicit port 80" in { + val url = notifierFor("http://texera.example.com:80/dashboard";) invokePrivate + createDashboardUrl() + assert(url == s"http://texera.example.com/user/workspace/$testWid";) + } + + // Note the assertion keeps `http://` even for the https-conventional :443. createDashboardUrl + // hardcodes the scheme and never reads sessionUri.getScheme; that is today's behaviour, not an + // endorsement of it. A fix that preserved the scheme would surface here. + it should "omit an explicit port 443" in { + val url = notifierFor("http://texera.example.com:443/dashboard";) invokePrivate + createDashboardUrl() + assert(url == s"http://texera.example.com/user/workspace/$testWid";) + } + + it should "keep a non-default port" in { + val url = notifierFor("http://localhost:8080/dashboard";) invokePrivate createDashboardUrl() + assert(url == s"http://localhost:8080/user/workspace/$testWid";) + } + + // ─── createEmailSubject / createEmailContent ─────────────────────────────── + + "createEmailSubject" should "carry the workflow name read from the database" in { + val subject = notifierFor("http://texera.example.com";) invokePrivate + createEmailSubject(COMPLETED) + assert(subject == s"[Texera] Workflow $testWorkflowName ($testWid) Status: ${COMPLETED.name}") + } + + "createEmailContent" should "list the database-resolved name, the id, the state and the link" in { + val content = notifierFor("http://texera.example.com:8080/dashboard";) invokePrivate + createEmailContent(FAILED) + + // stripMargin + trim: a broken margin would leave the "|" prefixes and leading blank line. + assert(content.startsWith("Hello,")) + assert(content.contains(s"- Workflow ID: $testWid")) + assert(content.contains(s"- Workflow Name: $testWorkflowName")) + assert(content.contains(s"- State: ${FAILED.name}")) + assert( + content.contains(s"visiting: http://texera.example.com:8080/user/workspace/$testWid";), + s"dashboard link missing from content:\n$content" + ) + assert( + content.linesIterator.exists(line => + line.startsWith("- Timestamp: ") && line.endsWith("(UTC)") + ), + s"no UTC timestamp line in content:\n$content" + ) + } + + // ─── formatTimestamp ─────────────────────────────────────────────────────── + + "formatTimestamp" should "render a fixed instant in UTC, independent of the JVM defaults" in { + val previousLocale = Locale.getDefault + val previousZone = TimeZone.getDefault + // Non-UTC default zone: had the formatter used ZoneId.systemDefault() the clock time below + // would read 06:07 instead of 14:07. + Locale.setDefault(Locale.US) + TimeZone.setDefault(TimeZone.getTimeZone("America/Los_Angeles")) + try { + val rendered = notifierFor("http://texera.example.com";) invokePrivate + formatTimestamp(Instant.parse("2024-03-05T14:07:09Z")) + assert(rendered == "March 5, 2024, 2:07:09 PM (UTC)") + } finally { + TimeZone.setDefault(previousZone) + Locale.setDefault(previousLocale) + } + } + + // ─── isValidEmail ────────────────────────────────────────────────────────── + + "isValidEmail" should "accept a well-formed address" in { + assert(notifierFor("http://texera.example.com";) invokePrivate isValidEmail("[email protected]")) + } + + it should "reject an address with no '@' and one with an empty domain" in { + val notifier = notifierFor("http://texera.example.com";) + assert(!(notifier invokePrivate isValidEmail("user-at-example.com"))) + assert(!(notifier invokePrivate isValidEmail("user@"))) + } + + // ─── shouldSendEmail ─────────────────────────────────────────────────────── + + "shouldSendEmail" should "accept the states that end or suspend an execution" in { + val notifier = notifierFor("http://texera.example.com";) + Seq(COMPLETED, PAUSED, FAILED, KILLED).foreach { state => + assert(notifier.shouldSendEmail(state), s"expected $state to trigger a notification") + } + } + + it should "decline the states an execution merely passes through" in { + val notifier = notifierFor("http://texera.example.com";) + // TERMINATED / UNKNOWN are intentionally left out: whether they should notify is a product + // question, and pinning today's answer would block a harmless change. + val inFlight: Seq[WorkflowAggregatedState] = + Seq(UNINITIALIZED, READY, RUNNING, PAUSING, RESUMING) + inFlight.foreach { state => + assert(!notifier.shouldSendEmail(state), s"expected $state not to trigger a notification") + } + } + + // ─── constructor ─────────────────────────────────────────────────────────── + + "the constructor" should "propagate NotFoundException when the workflow row is missing" in { + val ex = intercept[NotFoundException] { + new WorkflowEmailNotifier( + unseededWid.toLong, + userEmail, + new URI("http://texera.example.com";) + ) + } + assert(ex.getMessage.contains(unseededWid.toString)) + } +}
