From 5ba04ea8bfbcc86933e5ca8418822a1909c64b8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Wed, 2 Sep 2026 13:57:24 +0200 Subject: [PATCH] fix(core): parse rejections say SQL statement, not schema DDL (Story 20.4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both rejection routes of GatewayApi.run(sql) — the Left(ParserError) branch and the thrown-parser route (#250) — now emit 'Error parsing SQL statement []: ' with operation = Some("sql") through one shared builder (GatewayApi.parseRejectionMessage). Each half is rendered on a single line and capped at 200 characters (head + '...' + tail, so the discriminating end of a long generated statement survives); control characters and line separators are collapsed so the text cannot forge a log record or emit an ANSI escape; cuts are surrogate-safe. The thrown route never relays attempt's 'Operation failed: null' (falls back to the cause's class name) and now logs the rejection. statusCode is unchanged on both routes (400 / None). Testkit template gains a DQL rejection case and a batch position-2 case; documentation/client/gateway.md shows the new shape. Verified live on ES 8.18: JavaClientGatewayApiSpec 45/45, JavaClient8ReplGatewayIntegrationSpec 56/56 with the two existing 'Error parsing' assertions untouched. Closes #262 Co-Authored-By: Claude Fable 5 --- .../elastic/client/GatewayApi.scala | 179 ++++++++++- .../client/ParseRejectionMessageSpec.scala | 297 ++++++++++++++++++ documentation/client/gateway.md | 12 +- .../client/GatewayApiIntegrationSpec.scala | 51 ++- 4 files changed, 531 insertions(+), 8 deletions(-) create mode 100644 core/src/test/scala/app/softnetwork/elastic/client/ParseRejectionMessageSpec.scala diff --git a/core/src/main/scala/app/softnetwork/elastic/client/GatewayApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/GatewayApi.scala index 4ebe13d9..beccd0a9 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/GatewayApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/GatewayApi.scala @@ -1819,20 +1819,37 @@ trait GatewayApi extends IndicesApi with ElasticClientHelpers { case Right(statement) => run(statement) case Left(l) => - // parsing error + // Parse rejection. `run` is the front door for DQL, DML AND DDL, so the message must + // not claim a statement class — and it cannot know one: `ParserError` is a bare + // string (see GatewayApi.ParseRejectionPrefix). Echo the statement instead and let + // the reader classify it; BI tools surface this text verbatim (jdbc#35). + // `l.msg` is passed through RAW on purpose: bounding and sanitising it is + // parseRejectionMessage's job, so both routes get it identically. val error = ElasticError( - message = s"Error parsing schema DDL statement: ${l.msg}", + message = GatewayApi.parseRejectionMessage(statement, l.msg), statusCode = Some(400), - operation = Some("schema") + operation = Some("sql") ) logger.error(s"❌ ${error.message}") Future.successful(ElasticFailure(error)) } case ElasticFailure(elasticError) => - // parsing error - Future.successful(ElasticFailure(elasticError.copy(operation = Some("schema")))) + // The other rejection route: `Parser.apply` THREW instead of returning `Left` + // (SoftClient4ES#250 — three `throw ValidationError` sites in `WhereParser`), and + // `ElasticResult.attempt` wrapped it as "Operation failed: " with no status and no + // operation. Relabel it exactly like the `Left` branch so a caller cannot tell the two + // routes apart, and so nothing here changes when #250 folds this route into `Left`. + // `statusCode` is deliberately left as `attempt` set it (None): `attempt` catches ANY + // NonFatal, so an internal fault must not be asserted to be a client-side 400. + val error = elasticError.copy( + message = GatewayApi + .parseRejectionMessage(statement, GatewayApi.parseFailureReason(elasticError)), + operation = Some("sql") + ) + logger.error(s"❌ ${error.message}") + Future.successful(ElasticFailure(error)) } case statements => @@ -1906,6 +1923,158 @@ trait GatewayApi extends IndicesApi with ElasticClientHelpers { object GatewayApi { + /** Prefix of every parse rejection `run(sql: String)` returns. + * + * It says SQL, not "schema DDL" (jdbc#35). `run` is the single front door for DQL, DML '''and''' + * DDL, and the statement that failed to parse has no known kind: `Parser.apply` hands back + * `Left(ParserError(msg: String))` (`sql/.../parser/Parser.scala:1352,1365`) — a bare string + * with no AST, no position and no statement class. So the message names what it can actually + * observe (the statement, and the parser's reason) and nothing it cannot. + * + * The `Error parsing` stem is deliberate, not inherited by accident: it is what + * `ReplGatewayIntegrationSpec` asserts, and it is the wording jdbc#35 itself proposed. + */ + private[client] val ParseRejectionPrefix: String = "Error parsing SQL statement" + + /** Budget for EACH part of a parse-rejection message — the statement excerpt and the parser's + * reason alike. One constant, applied uniformly, one sentence: '''no part of a parse-rejection + * message exceeds MaxExcerpt characters'''. + * + * Bounding only the statement would have been incoherent + * (`feedback_constant_uniform_justification`): the justification is the medium — a narrow BI + * error dialog — and it binds both halves. It is also not hypothetical that the reason needs it. + * `Parser.apply` maps post-parse `validate()` failures to `Left(ParserError(msg))` too, and + * those messages embed whole AST renderings — `sql/.../query/From.scala` repeats `"ON clause + * $this ..."` nine times, and `Parser.scala:312` echoes the '''entire SCRIPT body''' into + * `"Invalid SCRIPT AS expression ($body): $msg"`. Unbounded, one of those buries the diagnosis + * and re-exposes a whole script inside an error dialog. + * + * An excerpt NEVER exceeds this many characters — the ellipsis is inside the budget, not added + * to it, so the constant means what its name says. Whole-message ceiling: prefix + brackets + + * MaxExcerpt + ": " + MaxExcerpt, i.e. under 500 characters for every input. + */ + private[client] val MaxExcerpt: Int = 200 + + /** ASCII on purpose — see the encoding rule in the story's build facts. */ + private[client] val ExcerptEllipsis: String = "..." + + /** How the budget is split when a part is too long: head + ellipsis + tail. + * + * '''A prefix alone would not do the job this story exists for.''' Epic 19 measured 89/89 + * Tableau-emitted statements rejected, and every one of them opens with the same long, fully + * quoted SELECT list — so the first 200 characters identify the TOOL, not the QUERY, and AC-2 + * ("names the offending statement") would be satisfied only on paper. Keeping the tail keeps the + * discriminating part — the FROM / WHERE / GROUP BY, and the region a parse error usually sits + * in. It also defuses the `/* app=Tableau ... */` banner case: block comments are stripped by + * '''neither''' `splitStatements` (which handles `--` only) '''nor''' `Parser.normalize`, so a + * long leading banner would consume a prefix-only excerpt whole and hide the statement + * completely — with a tail, the statement still shows. Hand-typed SQL, the story's other + * audience, is short enough to be echoed whole and is unaffected. 120 + 3 + 77 = 200 exactly. + */ + private[client] val ExcerptHead: Int = 120 + private[client] val ExcerptTail: Int = MaxExcerpt - ExcerptHead - ExcerptEllipsis.length + + /** Runs of whitespace '''and control characters''' collapse to one space. + * + * Not cosmetics — this is the story's one security control. An excerpt is attacker-influenced + * text (it is the caller's own statement) that lands in a log record, in a `SQLException` + * message, and in a BI error dialog. Collapsing newlines is what stops a crafted statement from + * forging a second log line; collapsing the rest of the C0 controls is what stops an ANSI escape + * from reaching the terminal that renders the REPL's error output. Java's `\s` is ASCII-only and + * covers NONE of ESC / NUL / BEL, so `\p{Cntrl}` is required alongside it; U+0085, U+2028 and + * U+2029 are added because renderers treat them as line breaks. Written as `\\u` escapes so the + * source stays ASCII. + */ + private val ExcerptNoise: String = "[\\s\\p{Cntrl}\\u0085\\u2028\\u2029]+" + + /** Never split a surrogate pair when cutting an excerpt. + * + * `String.take` / `substring` count UTF-16 code units, so a naive cut at a fixed offset can emit + * a lone surrogate. That string is not valid UTF-16, and it is re-encoded at least twice on its + * way to the analyst (`SQLException` then the host's own serialisation; a log file's charset). + * Moving the cut by one character is always safe and costs at most one character of the budget. + */ + private def cutBefore(s: String, at: Int): Int = + if (at > 0 && Character.isHighSurrogate(s.charAt(at - 1))) at - 1 else at + + private def cutAfter(s: String, at: Int): Int = + if (at < s.length && Character.isLowSurrogate(s.charAt(at))) at + 1 else at + + /** Single-line, length-bounded rendering of one message part — '''display only'''. + * + * Used for BOTH halves of a parse-rejection message (the statement and the parser's reason), so + * the bound is uniform and neither half can bury the other. + * + * This is NOT `Parser.normalize` and must never be used as one: it is quote-blind and collapses + * whitespace inside string literals too. Nothing parses, splits or matches on its output — and + * nothing may start to, because a statement may legitimately contain `]` (`SELECT a[1] FRM t`, a + * measured rejection) and would make the enclosing `[...]` framing ambiguous to any consumer + * naive enough to try. Stripping `]` was considered and REJECTED: it would corrupt the display + * of valid SQL in order to protect a consumer that must not exist. + * + * TRAP — for the statement half this receives the '''post-split''' statement, not the caller's + * raw input: `splitStatements` has already replaced each `--` comment with a space and consumed + * the separating `;`. Stated here because it is what makes `message should include()` a + * safe assertion only for inputs free of comments, `;` and repeated whitespace. + * + * Post-condition: `excerpt(s).length <= MaxExcerpt`, for every `s`. + */ + private[client] def excerpt(text: String): String = { + val oneLine = text.replaceAll(ExcerptNoise, " ").trim + if (oneLine.length <= MaxExcerpt) oneLine + else { + val head = oneLine.substring(0, cutBefore(oneLine, ExcerptHead)) + val tail = oneLine.substring(cutAfter(oneLine, oneLine.length - ExcerptTail)) + head + ExcerptEllipsis + tail + } + } + + /** Last-resort reason text. A message ending `...]: ` with nothing after the colon is exactly the + * shape AC-2 forbids, so the reason is never allowed to be empty. + */ + private[client] val NoParserReason: String = "no reason reported by the parser" + + /** The reason to quote when the parser THREW rather than returned `Left` (SoftClient4ES#250). + * + * `ElasticResult.attempt` has already flattened the throwable into `message = "Operation failed: + * "`, `cause = Some(ex)`, so the useful text is the cause's own message. + * + * It deliberately does '''not''' fall back to `error.message`. `attempt` interpolates a null + * message straight into its wrapper, so a cause with no message yields the literal `"Operation + * failed: null"` (`result/package.scala:253-266`) — relaying that would violate AC-2 and + * contradict this story's own "the message never says Operation failed" assertion. The fallback + * is the exception's class name instead: strictly more useful, and it makes an internal fault + * visibly different from a grammar rejection at a glance. + * + * Extracted as a named function for one reason: neither the null nor the blank branch is + * reachable through `run` (no parser exception carries a null message today), so inline they + * would ship with zero coverage. + */ + private[client] def parseFailureReason(error: ElasticError): String = { + def nonBlank(s: String): Option[String] = Option(s).map(_.trim).filter(_.nonEmpty) + error.cause + .flatMap(ex => nonBlank(ex.getMessage).orElse(nonBlank(ex.getClass.getName))) + .getOrElse(NoParserReason) + } + + /** The one message builder both parse-rejection routes of `run(sql: String)` go through. + * + * Sharing it is the point: the routes differ only in how the parser failed (returned `Left` vs + * threw, SoftClient4ES#250), and a caller must not be able to tell them apart. It also means + * that when #250 moves the thrown route onto the `Left` branch, no message changes. + * + * It owns BOTH message-shape invariants so neither route can lose one: each half is bounded and + * single-lined by `excerpt`, and the reason half is never empty. The empty guard belongs here + * and not only in `parseFailureReason`, because `ParserError(msg)` carries no non-empty + * invariant either (`Parser.scala:1365`) — the `Left` route can hand over a blank string just as + * easily. + */ + private[client] def parseRejectionMessage(statement: String, reason: String): String = { + val shownReason = excerpt(reason) + val safeReason = if (shownReason.isEmpty) NoParserReason else shownReason + s"$ParseRejectionPrefix [${excerpt(statement)}]: $safeReason" + } + /** Split a normalized SQL string into statements on top-level `;`. * * This replaces `split(";\\s*$")`, which — `$` anchoring to the end of the whole (newline-free) diff --git a/core/src/test/scala/app/softnetwork/elastic/client/ParseRejectionMessageSpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/ParseRejectionMessageSpec.scala new file mode 100644 index 00000000..a14bd32d --- /dev/null +++ b/core/src/test/scala/app/softnetwork/elastic/client/ParseRejectionMessageSpec.scala @@ -0,0 +1,297 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed 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 app.softnetwork.elastic.client + +import akka.actor.ActorSystem +import app.softnetwork.elastic.client.result.{ElasticError, ElasticFailure} +import org.scalatest.BeforeAndAfterAll +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.slf4j.{Logger, LoggerFactory} + +import scala.concurrent.Await +import scala.concurrent.duration._ + +/** jdbc#35 — `GatewayApi.run(sql)` labelled EVERY parse rejection, DQL included, as `"Error parsing + * schema DDL statement: ..."` with `operation = Some("schema")`. A BI tool shows that text + * verbatim in its error dialog (the JDBC driver puts `error.message` straight into the + * `SQLException`), so an analyst who mistyped a SELECT was sent hunting through DDL docs. + * + * These tests assert the SHAPE of the message, never the parser's own reason text: `l.msg` names + * whichever internal production failed last (`string matching regex '(?i)COPY\b' expected but 'C' + * found` for a bad leading keyword, because `copy` is the last alternative of `dmlStatement`) and + * moves with any grammar edit. + */ +class ParseRejectionMessageSpec extends AnyFlatSpec with Matchers with BeforeAndAfterAll { + + implicit val system: ActorSystem = ActorSystem("parse-rejection-message-spec") + + override def afterAll(): Unit = { + system.terminate() + super.afterAll() + } + + private val testLogger: Logger = LoggerFactory.getLogger(getClass) + + // `protected def logger` is NopeClientApi's only abstract member; a parse rejection never reaches + // the client, so no ES, no Docker and no extension lookup is involved. + private val client: ElasticClientApi = new NopeClientApi { + override protected def logger: Logger = testLogger + } + + private def rejectionOf(sql: String): ElasticError = + Await.result(client.run(sql), 10.seconds) match { + case ElasticFailure(error) => error + case other => fail(s"Expected a failure for [$sql], got: $other") + } + + private def assertRejection(sql: String, error: ElasticError): Unit = { + error.message should startWith("Error parsing SQL statement") + error.message should include(sql) + error.message should not include "schema DDL" + error.message should not include "Operation failed" + error.operation shouldBe Some("sql") + // AC-2 has TWO halves and the second one is the easy one to lose: a builder that dropped + // `reason` would satisfy every other assertion in this file. AC-8 forbids pinning the reason's + // TEXT (it is `l.msg` and moves with the grammar), so pin its PRESENCE instead — something + // non-blank must follow the `]: `. + val head = s"Error parsing SQL statement [$sql]: " + error.message should startWith(head) + error.message.substring(head.length).trim should not be empty + () + } + + behavior of "GatewayApi.run parse rejections" + + // THE bug's actual shape: a DQL statement, reported as a schema DDL error. + // `SELECT * FRM users` is chosen because no roadmap item makes it parse — unlike `SELECT 1` + // (#251), backticks or `FROM "t"` (#252), which Epic 21 is expected to accept. + it should "not describe a rejected SELECT as a schema DDL error" in { + val sql = "SELECT * FRM users" + val error = rejectionOf(sql) + assertRejection(sql, error) + error.statusCode shouldBe Some(400) + } + + it should "report a rejected DDL statement with the same neutral wording" in { + val sql = "CREAT TABL missing_keyword" + val error = rejectionOf(sql) + assertRejection(sql, error) + error.statusCode shouldBe Some(400) + } + + it should "report an unsupported statement with the same neutral wording" in { + val sql = "GRANT SELECT ON users TO user1" + val error = rejectionOf(sql) + assertRejection(sql, error) + error.statusCode shouldBe Some(400) + } + + // The OTHER rejection route: SoftClient4ES#250 — `Parser.apply` THROWS `ValidationError` here + // instead of returning `Left` (measured on this exact input; it is jOOQ's rendering of RLIKE). + // The MESSAGE assertions are route-agnostic by design: both routes must produce the same shape. + it should "report a THROWN parse failure with the same neutral wording" in { + val sql = "select id from emp where (name like_regex 'Jo.*')" + assertRejection(sql, rejectionOf(sql)) + } + + // ...but shape assertions alone cannot FAIL if site 2 regresses, because site 1 satisfies them + // too. These two are the route's only discriminators — site 1 always yields `statusCode = + // Some(400)` and no cause — and they pin the two decisions AD-5 and Task 1.3 state explicitly: + // `statusCode` is left exactly as `ElasticResult.attempt` set it, and `copy` preserves the cause. + // Without them a dev can rebuild the error from scratch at site 2, break three recorded decisions, + // and watch every test stay green. + // + // TRAP — this test is EXPECTED to go red when #250 lands and the throw becomes a `Left`. That is + // the signal, not a defect: delete it then (site 1 already covers the shape). Do not weaken it now + // to make it survive a change that has not happened. + it should "leave statusCode and cause exactly as attempt produced them on the thrown route" in { + val error = rejectionOf("select id from emp where (name like_regex 'Jo.*')") + error.statusCode shouldBe None + error.cause shouldBe defined + } + + // The multi-statement branch has no message of its own — `run(statement)` inside the fold is the + // String overload, so it re-enters the single-statement path. The batch is ordered so the FIRST + // statement is the bad one: the fold short-circuits, so the second never runs and no ES call is + // attempted, which is what keeps this spec Docker-free. + // + // HONEST LIMIT, do not oversell this test: with the failure at position 1, "names the FAILING + // statement" and "names the FIRST statement" are the same assertion. What it does prove is that + // the batch path re-enters the two message sites and echoes ONE member, not the whole input. + // AC-4's real property — a rejection at position 2 is reported, with the earlier statement having + // succeeded — needs a statement that actually executes, so it lives in the Docker suite + // (GatewayApiIntegrationSpec, batch position-2 case). + it should "echo only the failing member of a multi-statement batch" in { + val error = rejectionOf("SELECT * FRM users; SELECT * FROM t") + error.message should startWith("Error parsing SQL statement") + error.message should include("SELECT * FRM users") + error.message should not include "SELECT * FROM t" + error.operation shouldBe Some("sql") + } + + // The raw statement carries newlines AND two-space indents; finding the exact single-spaced form + // in the message IS the proof that the collapse happened. The whole-message newline check is + // legitimate too, and worth having: BOTH halves go through `excerpt`, so a parse-rejection + // message is single-line by construction — which is what a log record depends on. + it should "collapse a multi-line statement to one line in the excerpt" in { + val error = rejectionOf("SELECT *\n FRM\n users") + error.message should include("SELECT * FRM users") + error.message should not include "\n" + } + + // Head AND tail, not a prefix: the tail is what tells a Tableau user WHICH of their identically + // prefixed queries failed. `FRM users` is the discriminating suffix here and must survive. + it should "bound the excerpt, mark it truncated, and keep the tail" in { + val padding = "x" * 400 + val error = rejectionOf(s"SELECT $padding FRM users") + error.message should include(GatewayApi.ExcerptEllipsis) + error.message should include("FRM users") + // The 400-char run cannot survive a 200-char excerpt. The whole-message ceiling is exact now + // that BOTH halves are bounded: prefix + brackets + MaxExcerpt + ": " + MaxExcerpt. + error.message should not include padding + error.message.length should be < (2 * GatewayApi.MaxExcerpt + 100) + } + + // Pins the branch this story does NOT touch. + it should "leave the empty-query rejection unchanged" in { + val error = rejectionOf(" ") + error.message shouldBe "Empty SQL query." + error.statusCode shouldBe Some(400) + error.operation shouldBe Some("sql") + } + + behavior of "GatewayApi.excerpt" + + it should "return a short statement unchanged" in { + GatewayApi.excerpt("SELECT 1") shouldBe "SELECT 1" + } + + it should "keep head and tail, with the ASCII ellipsis between them" in { + val long = "H" * 300 + "TAIL" + val excerpt = GatewayApi.excerpt(long) + excerpt should startWith("H" * GatewayApi.ExcerptHead) + excerpt should endWith("TAIL") + excerpt should include(GatewayApi.ExcerptEllipsis) + } + + // The post-condition, stated as an invariant rather than an example: the ellipsis lives INSIDE + // the budget. A `take(n) + "..."` implementation returns n + 3 and fails this. + it should "never exceed MaxExcerpt, at any length around the boundary" in { + for (n <- Seq(0, 1, 199, 200, 201, 202, 500, 5000)) { + val excerpt = GatewayApi.excerpt("a" * n) + withClue(s"n=$n: ") { + excerpt.length should be <= GatewayApi.MaxExcerpt + } + } + GatewayApi.excerpt("a" * 200).length shouldBe 200 // exactly at the cap: no ellipsis + GatewayApi.excerpt("a" * 200) should not include GatewayApi.ExcerptEllipsis + } + + // A cut that lands mid-surrogate emits a lone surrogate — not valid UTF-16, and the excerpt is + // re-encoded at least twice before the analyst sees it. The property is asserted as a UTF-8 + // ROUND TRIP rather than by walking the chars: it is exact (a lone surrogate becomes U+FFFD and + // the strings differ), it cannot itself throw, and it is the failure the analyst would actually + // see. Do NOT assert an exact length here — a guarded cut may hand a character back. + it should "never split a surrogate pair" in { + // U+1F600, one code point, two UTF-16 units. Written as backslash-u escapes so the source + // stays ASCII (the encoding rule in the story's build facts); Scala resolves them before + // lexing. (A bare backslash-u in this very comment would break the 2.12 leg, which still + // processes unicode escapes inside comments.) + val pair = "\uD83D\uDE00" + // Sweep both cuts across a pair boundary: pad 119/120 straddle the head cut, trailing 75/76 + // straddle the tail cut. + for (pad <- 118 to 122; trailing <- 74 to 80) { + val excerpt = GatewayApi.excerpt("a" * pad + pair + "b" * 300 + pair + "c" * trailing) + withClue(s"pad=$pad trailing=$trailing: ") { + val utf8 = java.nio.charset.StandardCharsets.UTF_8 + new String(excerpt.getBytes(utf8), utf8) shouldBe excerpt + excerpt.length should be <= GatewayApi.MaxExcerpt + } + } + } + + // The security control, asserted as a control and not as cosmetics: a crafted statement must not + // be able to forge a log line or emit an ANSI sequence. `\s` alone does NOT cover these. + it should "collapse control characters, not just whitespace" in { + // Written as backslash-u escapes on purpose: a literal ESC / NUL / BEL in a source file is + // invisible to review and to `git diff`. Scala resolves the escapes before lexing, so these + // ARE the characters. + val hostile = + "SELECT" + "\u001b" + "[2J" + "\u0000" + "\u0007" + " 1\nFROM" + "\u0085" + "t" + val excerpt = GatewayApi.excerpt(hostile) + excerpt.exists(c => Character.isISOControl(c)) shouldBe false + excerpt should not include "\u0085" + excerpt shouldBe "SELECT [2J 1 FROM t" + } + + behavior of "GatewayApi.parseRejectionMessage" + + // The reason half is bounded by the SAME constant as the statement half — otherwise a + // `validate()` failure that embeds a whole AST (or a whole SCRIPT body, `Parser.scala:312`) + // buries the diagnosis it was meant to deliver. This assertion is what makes the whole-message + // length bound in the `run` tests meaningful rather than lucky. + it should "bound the parser reason as well as the statement" in { + val message = GatewayApi.parseRejectionMessage("SELECT 1", "R" * 4000) + message should include(GatewayApi.ExcerptEllipsis) + message.length should be < (2 * GatewayApi.MaxExcerpt + 100) + message should startWith("Error parsing SQL statement [SELECT 1]: ") + } + + // `ParserError(msg)` has no non-empty invariant either (`Parser.scala:1365`), so the `Left` route + // can hand over a blank reason exactly as the thrown route can. The message must never end in + // `]: ` with nothing after the colon (AC-2). + it should "never emit an empty reason, whichever route supplied it" in { + GatewayApi.parseRejectionMessage("SELECT 1", " ") shouldBe + s"Error parsing SQL statement [SELECT 1]: ${GatewayApi.NoParserReason}" + GatewayApi.parseRejectionMessage("SELECT 1", "") should not endWith "]: " + } + + behavior of "GatewayApi.parseFailureReason" + + // Branch 1 — the live one: `ElasticResult.attempt` wrapped a thrown parser exception. + it should "prefer the cause's own message over attempt's wrapper" in { + val wrapped = new IllegalStateException("Unbalanced parentheses") + GatewayApi.parseFailureReason( + ElasticError("Operation failed: Unbalanced parentheses", cause = Some(wrapped)) + ) shouldBe "Unbalanced parentheses" + } + + // Branch 2 — unreachable through `run` today, which is exactly why it is tested here. Note what + // it must NOT return: `attempt` interpolates a null message into its own wrapper, so relaying + // `error.message` would put the literal "Operation failed: null" in front of the analyst. + it should "use the cause's class name when its message is null, never attempt's wrapper" in { + val reason = GatewayApi.parseFailureReason( + ElasticError("Operation failed: null", cause = Some(new RuntimeException())) + ) + reason shouldBe classOf[RuntimeException].getName + reason should not include "Operation failed" + } + + // Branch 3 — a blank cause message would render as `[...]: ` with nothing after the colon. + it should "use the cause's class name when its message is blank" in { + GatewayApi.parseFailureReason( + ElasticError("Operation failed:", cause = Some(new IllegalArgumentException(" "))) + ) shouldBe classOf[IllegalArgumentException].getName + } + + // Branch 4 — defensive: `ElasticResult.attempt` always sets a cause, so this cannot arise at + // site 2. It exists so the function is total and can never return an empty reason. + it should "never return an empty reason when there is no cause at all" in { + GatewayApi.parseFailureReason(ElasticError("whatever")) shouldBe GatewayApi.NoParserReason + } +} diff --git a/documentation/client/gateway.md b/documentation/client/gateway.md index 9788aca5..20d6bcdb 100644 --- a/documentation/client/gateway.md +++ b/documentation/client/gateway.md @@ -521,9 +521,19 @@ Example: ```scala gateway.run("BAD SQL") -→ ElasticFailure(ElasticError(message = "Error parsing schema DDL statement: ...")) +→ ElasticFailure( + ElasticError( + message = "Error parsing SQL statement [BAD SQL]: ", + statusCode = Some(400), + operation = Some("sql") + ) + ) ``` +The statement and the parser's reason are each rendered on a single line and capped at +200 characters; a longer one keeps its head and its tail with `...` between them. A `...` inside +the message is therefore a display bound, not the parser's own text. + --- ## Notes diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index e8924a6c..370231ad 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -1486,7 +1486,12 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { val res = client.run(invalidSql).futureValue res.isFailure shouldBe true - res.toEither.left.get.message should include("Error parsing schema DDL statement") + val error = res.toEither.left.get + // jdbc#35 — `run` is the front door for DQL, DML and DDL alike; a rejection must not claim the + // statement was schema DDL, and it must echo what was rejected (BI tools show this verbatim). + error.message should include("Error parsing SQL statement") + error.message should include(invalidSql) + error.message should not include "schema DDL" } // --------------------------------------------------------------------------- @@ -1498,7 +1503,49 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { val res = client.run(unsupportedSql).futureValue res.isFailure shouldBe true - res.toEither.left.get.message should include("Error parsing schema DDL statement") + val error = res.toEither.left.get + error.message should include("Error parsing SQL statement") + error.message should include(unsupportedSql) + error.message should not include "schema DDL" + } + + // --------------------------------------------------------------------------- + // jdbc#35 — a rejected DQL statement is not a schema DDL error + // --------------------------------------------------------------------------- + + it should "not report a rejected SELECT as a schema DDL error" in { + val invalidDql = "SELECT * FRM users" + val res = client.run(invalidDql).futureValue + + res.isFailure shouldBe true + val error = res.toEither.left.get + error.message should include("Error parsing SQL statement") + error.message should include(invalidDql) + error.message should not include "schema DDL" + error.operation shouldBe Some("sql") + error.statusCode shouldBe Some(400) + } + + // --------------------------------------------------------------------------- + // AC-4 — the failing member of a batch is named, at position 2 + // --------------------------------------------------------------------------- + + // This case belongs HERE and not in the core unit spec: the property is "a rejection AFTER a + // statement that actually ran is the one reported", and only a live cluster can make the first + // statement succeed. The unit spec's batch test puts the failure at position 1, where "the + // failing statement" and "the first statement" are indistinguishable. + // `SHOW TABLES LIKE ...` is the cheapest statement this suite already proves works (see the + // `SHOW TABLES LIKE 'show_%'` cases earlier in this file); the pattern is chosen to match + // nothing so the case is independent of every fixture. + it should "name the failing statement of a multi-statement batch" in { + val res = client.run("SHOW TABLES LIKE 'no_such_prefix_%'; SELECT * FRM users").futureValue + + res.isFailure shouldBe true + val error = res.toEither.left.get + error.message should include("Error parsing SQL statement") + error.message should include("SELECT * FRM users") + error.message should not include "SHOW TABLES" + error.operation shouldBe Some("sql") } // ===========================================================================