diff --git a/modules/core/shared/src/main/scala-2/syntax/StringContextOps.scala b/modules/core/shared/src/main/scala-2/syntax/StringContextOps.scala index 0a9069cf2..f91ae5b2e 100644 --- a/modules/core/shared/src/main/scala-2/syntax/StringContextOps.scala +++ b/modules/core/shared/src/main/scala-2/syntax/StringContextOps.scala @@ -16,9 +16,13 @@ class StringContextOps private[skunk](sc: StringContext) { def sql(argSeq: Any*): Any = macro StringContextOps.StringOpsMacros.sql_impl + @deprecated("Use ident\"…\", which preserves the identifier verbatim. For the old case-folding behavior, pass a lower-cased literal.", "1.1.0") def id(): Identifier = macro StringContextOps.StringOpsMacros.identifier_impl + def ident(): Identifier = + macro StringContextOps.StringOpsMacros.ident_impl + /** Construct a constant `Fragment` with no interpolated values. */ def const()(implicit or: Origin): Fragment[Void] = Fragment(sc.parts.toList.map(Left(_)), Void.codec, or) @@ -41,6 +45,12 @@ object StringContextOps { case class Par(n: State[Int, String]) extends Part // n parameters case class Emb(ps: List[Either[String, State[Int, String]]]) extends Part + // Scala 2 drops macro method references before deprecation checking, so `id` expands to this + // deprecated helper to produce a standard warning at the call site. + @deprecated("Use ident\"…\", which preserves the identifier verbatim. For the old case-folding behavior, pass a lower-cased literal.", "1.1.0") + def legacyIdentifier(s: String): Identifier = + Identifier.fromStringLegacy(s).fold(sys.error, identity) + def fragmentFromParts[A](ps: List[Part], enc: Encoder[A], or: Origin): Fragment[A] = Fragment( ps.flatMap { @@ -150,9 +160,17 @@ object StringContextOps { def identifier_impl(): Tree = { val Apply(_, List(Apply(_, List(Literal(Constant(part: String)))))) = c.prefix.tree: @unchecked - Identifier.fromString(part) match { + Identifier.fromStringLegacy(part) match { + case Left(s) => c.abort(c.enclosingPosition, s) + case Right(Identifier(s)) => q"_root_.skunk.syntax.StringContextOps.legacyIdentifier($s)" + } + } + + def ident_impl(): Tree = { + val Apply(_, List(Apply(_, List(Literal(Constant(part: String)))))) = c.prefix.tree: @unchecked + Identifier.fromValue(part) match { case Left(s) => c.abort(c.enclosingPosition, s) - case Right(Identifier(s)) => q"_root_.skunk.data.Identifier.fromString($s).fold(sys.error, identity)" + case Right(Identifier(s)) => q"_root_.skunk.data.Identifier.fromValue($s).fold(sys.error, identity)" } } @@ -165,4 +183,4 @@ trait ToStringContextOps { new StringContextOps(sc) } -object stringcontext extends ToStringContextOps \ No newline at end of file +object stringcontext extends ToStringContextOps diff --git a/modules/core/shared/src/main/scala-3/syntax/StringContextOps.scala b/modules/core/shared/src/main/scala-3/syntax/StringContextOps.scala index b711e3ed1..3b777f41e 100644 --- a/modules/core/shared/src/main/scala-3/syntax/StringContextOps.scala +++ b/modules/core/shared/src/main/scala-3/syntax/StringContextOps.scala @@ -143,8 +143,23 @@ object StringContextOps { import qc.reflect.report sc match { case '{ StringContext(${Varargs(Exprs(Seq(part)))}: _*) } => - Identifier.fromString(part) match { - case Right(Identifier(s)) => '{ Identifier.fromString(${Expr(s)}).fold(sys.error, identity) } + Identifier.fromStringLegacy(part) match { + case Right(Identifier(s)) => '{ Identifier.fromValue(${Expr(s)}).fold(sys.error, identity) } + case Left(s) => + report.error(s) + return '{???} + } + case _ => + report.error(s"Identifiers cannot have interpolated arguments") + return '{???} + } + + def identImpl(sc: Expr[StringContext])(using qc: Quotes): Expr[Identifier] = + import qc.reflect.report + sc match { + case '{ StringContext(${Varargs(Exprs(Seq(part)))}: _*) } => + Identifier.fromValue(part) match { + case Right(Identifier(s)) => '{ Identifier.fromValue(${Expr(s)}).fold(sys.error, identity) } case Left(s) => report.error(s) return '{???} @@ -161,8 +176,13 @@ trait ToStringContextOps { extension (inline sc: StringContext) transparent inline def sql(inline args: Any*): Any = ${ StringContextOps.sqlImpl('sc, 'args) } - extension (inline sc: StringContext) inline def id(): Identifier = - ${ StringContextOps.idImpl('sc) } + extension (inline sc: StringContext) + @deprecated("Use ident\"…\", which preserves the identifier verbatim. For the old case-folding behavior, pass a lower-cased literal.", "1.1.0") + inline def id(): Identifier = + ${ StringContextOps.idImpl('sc) } + + extension (inline sc: StringContext) inline def ident(): Identifier = + ${ StringContextOps.identImpl('sc) } implicit def toStringOps(sc: StringContext): StringContextOps = new StringContextOps(sc) diff --git a/modules/core/shared/src/main/scala/Channel.scala b/modules/core/shared/src/main/scala/Channel.scala index bc7f90ef3..2c10c07bf 100644 --- a/modules/core/shared/src/main/scala/Channel.scala +++ b/modules/core/shared/src/main/scala/Channel.scala @@ -120,10 +120,10 @@ object Channel { new Channel[F, String, String] { val listen: F[Unit] = - proto.execute(Command(s"LISTEN ${name.value}", Origin.unknown, Void.codec)).void + proto.execute(Command(s"LISTEN ${name.sql}", Origin.unknown, Void.codec)).void val unlisten: F[Unit] = - proto.execute(Command(s"UNLISTEN ${name.value}", Origin.unknown, Void.codec)).void + proto.execute(Command(s"UNLISTEN ${name.sql}", Origin.unknown, Void.codec)).void def listen(maxQueued: Int): Stream[F, Notification[String]] = for { @@ -142,7 +142,7 @@ object Channel { def notify(message: String): F[Unit] = // TODO: escape the message - proto.execute(Command(s"NOTIFY ${name.value}, '$message'", Origin.unknown, Void.codec)).void + proto.execute(Command(s"NOTIFY ${name.sql}, '$message'", Origin.unknown, Void.codec)).void } @@ -176,4 +176,4 @@ object Channel { fab.dimap(f)(g) } -} \ No newline at end of file +} diff --git a/modules/core/shared/src/main/scala/data/Identifier.scala b/modules/core/shared/src/main/scala/data/Identifier.scala index a557a21ac..444e8b32e 100644 --- a/modules/core/shared/src/main/scala/data/Identifier.scala +++ b/modules/core/shared/src/main/scala/data/Identifier.scala @@ -9,7 +9,9 @@ import cats.Eq import scala.util.matching.Regex sealed abstract case class Identifier(value: String) { - override def toString: String = value // ok? + lazy val sql: String = Identifier.render(value) + + override def toString: String = sql } object Identifier { @@ -18,11 +20,16 @@ object Identifier { val maxLen = 63 val pat: Regex = "([A-Za-z_][A-Za-z_0-9$]*)".r + private val unquotedPat: Regex = "([a-z_][a-z_0-9$]*)".r implicit val EqIdentifier: Eq[Identifier] = Eq.by(_.value) + @deprecated("Use fromValue, which preserves the identifier value and quotes it when necessary.", "1.1.0") def fromString(s: String): Either[String, Identifier] = + fromStringLegacy(s) + + private[skunk] def fromStringLegacy(s: String): Either[String, Identifier] = s match { case pat(s) => if (s.length > maxLen) @@ -30,10 +37,34 @@ object Identifier { else if (keywords(s.toUpperCase)) Left(s"Illegal identifier: ${s.toUpperCase} is a keyword.") else - Right(new Identifier(s) {}) + // Postgres folds unquoted identifiers to lower case, so we do the same up front. This + // keeps `value` consistent with what Postgres sees and ensures `sql` renders it unquoted. + Right(new Identifier(s.toLowerCase) {}) case _ => Left(s"Malformed identifier: does not match ${pat.regex}") } + def fromValue(s: String): Either[String, Identifier] = + if (s.isEmpty) + Left("Illegal identifier: zero-length delimited identifier.") + else if (s.contains('\u0000')) + Left("Illegal identifier: cannot contain the null byte (\\u0000).") + else { + val byteLen = s.getBytes(java.nio.charset.StandardCharsets.UTF_8).length + if (byteLen > maxLen) + Left(s"Identifier too long: $byteLen bytes (max allowed is $maxLen)") + else + Right(new Identifier(s) {}) + } + + private[data] def render(s: String): String = + s match { + case unquotedPat(_) if !keywords(s.toUpperCase) => s + case _ => quote(s) + } + + private def quote(s: String): String = + "\"" + s.replace("\"", "\"\"") + "\"" + val keywords: Set[String] = Set( "A", "ABORT", "ABS", "ABSENT", "ABSOLUTE", @@ -190,4 +221,4 @@ object Identifier { "YES", "ZONE" ) -} \ No newline at end of file +} diff --git a/modules/core/shared/src/main/scala/net/message/package.scala b/modules/core/shared/src/main/scala/net/message/package.scala index 0fa11d75b..5c1e93b44 100644 --- a/modules/core/shared/src/main/scala/net/message/package.scala +++ b/modules/core/shared/src/main/scala/net/message/package.scala @@ -58,7 +58,7 @@ package object message { module => val identifier: SCodec[Identifier] = utf8z.exmap( - s => Attempt.fromEither(Identifier.fromString(s).leftMap(Err(_))), + s => Attempt.fromEither(Identifier.fromValue(s).leftMap(Err(_))), id => Attempt.successful(id.value) ) @@ -78,4 +78,4 @@ package object message { module => } -} \ No newline at end of file +} diff --git a/modules/docs/src/main/laika/reference/Identifiers.md b/modules/docs/src/main/laika/reference/Identifiers.md index 329d8e448..5be1ad93c 100644 --- a/modules/docs/src/main/laika/reference/Identifiers.md +++ b/modules/docs/src/main/laika/reference/Identifiers.md @@ -1,2 +1,51 @@ +```scala mdoc:invisible +import skunk.data.Identifier +import skunk.implicits._ +``` # Identifiers +`skunk.data.Identifier` represents a Postgres name, such as the name of a table, column, schema, or channel. It keeps the name separate from its SQL representation so that Skunk can validate and escape it correctly. + +## Constructing an Identifier + +Use the `ident"..."` interpolator for names known at compile time. It validates the literal, preserves it verbatim, and rejects invalid identifiers during compilation. + +```scala mdoc:compile-only +val table: Identifier = ident"country" +val column: Identifier = ident"Country.Code" +val keyword: Identifier = ident"SELECT" +``` + +Use `Identifier.fromValue` when the name is available only at run time. + +```scala mdoc:compile-only +def identifier(name: String): Either[String, Identifier] = + Identifier.fromValue(name) +``` + +An identifier must be non-empty, contain no NUL byte, and occupy at most 63 bytes when encoded as UTF-8. The byte limit matters for names containing multibyte characters. + +## SQL Rendering + +Postgres accepts simple lower-case names as bare SQL identifiers. Other names, including mixed-case names, reserved words, and names containing punctuation, must be enclosed in double quotes. `Identifier` handles this distinction automatically. + +The `sql` member returns the SQL-ready representation. It leaves a name bare when that is safe, otherwise it adds double quotes and escapes any double quotes contained in the name. `toString` returns the same representation, while `value` returns the original, unescaped name. + +```scala mdoc:compile-only +val plain = ident"my_table" +val plainValue = plain.value // "my_table" +val plainSql = plain.sql // "my_table" + +val mixedCase = ident"My.Channel" +val mixedCaseValue = mixedCase.value // "My.Channel" +val mixedCaseSql = mixedCase.sql // "\"My.Channel\"" + +val embeddedQuote = ident"""say"hello""" +val embeddedQuoteSql = embeddedQuote.sql // "\"say\"\"hello\"" +``` + +`Channel` uses `sql` internally when issuing `LISTEN`/`UNLISTEN`/`NOTIFY`, so quoted channel names round-trip correctly. + +## Legacy Construction + +The deprecated `Identifier.fromString` method accepts only names matching `[A-Za-z_][A-Za-z_0-9$]*`, rejects keywords, and folds accepted names to lower case. The deprecated `id"..."` interpolator performs the same operation at compile time. New code should use `Identifier.fromValue` or `ident"..."`, which preserve the supplied name. To retain the legacy case-folding behavior, pass an already lower-case value. diff --git a/modules/docs/src/main/laika/tutorial/Channels.md b/modules/docs/src/main/laika/tutorial/Channels.md index 4a1727691..6c0cd611b 100644 --- a/modules/docs/src/main/laika/tutorial/Channels.md +++ b/modules/docs/src/main/laika/tutorial/Channels.md @@ -18,7 +18,7 @@ Use the `channel` method on `Session` to construct a channel. ```scala mdoc:compile-only // assume s: Session[IO] -val ch = s.channel(id"my_channel") // Channel[IO, String, String] +val ch = s.channel(ident"my_channel") // Channel[IO, String, String] ``` Observe the following: @@ -63,7 +63,7 @@ Every `Channel` is also an fs2 `Pipe` that consumes messages. // select all the country names and stream them to the country_names channel. s.prepare(sql"select name from country".query(varchar)).flatMap { ps => ps.stream(Void, 512) - .through(s.channel(id"country_names")) + .through(s.channel(ident"country_names")) .compile .drain } diff --git a/modules/example/src/main/scala/Channel.scala b/modules/example/src/main/scala/Channel.scala index 039bea2be..3739d56e8 100644 --- a/modules/example/src/main/scala/Channel.scala +++ b/modules/example/src/main/scala/Channel.scala @@ -23,7 +23,7 @@ object Channel extends IOApp { def run(args: List[String]): IO[ExitCode] = session.use { s => - s.channel(id"foo") + s.channel(ident"foo") .listen(42) .take(3) .evalMap(n => IO.println(s"⭐️⭐ $n")) diff --git a/modules/example/src/main/scala/Main.scala b/modules/example/src/main/scala/Main.scala index 8b61b6b34..bd822d5b1 100644 --- a/modules/example/src/main/scala/Main.scala +++ b/modules/example/src/main/scala/Main.scala @@ -73,12 +73,12 @@ object Main extends IOApp { st <- s.transactionStatus.get enc <- s.parameters.get.map(_.get("client_encoding")) _ <- IO.println(s"Logged in! Transaction status is $st and client_encoding is $enc") - f2 <- s.channel(id"foo").listen(10).through(anyLinesStdOut).compile.drain.start + f2 <- s.channel(ident"foo").listen(10).through(anyLinesStdOut).compile.drain.start rs <- s.execute(sql"select name, code, indepyear, population from country limit 20".query(country)) _ <- rs.traverse(IO.println) _ <- s.execute(sql"set seed = 0.123".command) _ <- s.execute(sql"set client_encoding = ISO8859_1".command) - _ <- s.channel(id"foo").notify("here is a message") + _ <- s.channel(ident"foo").notify("here is a message") _ <- s.execute(sql"select current_user".query(name)) _ <- s.prepare(q).flatMap(hmm(_)) _ <- s.prepare(in(3)).flatMap { _.stream(List("FRA", "USA", "GAB"), 100).through(anyLinesStdOut).compile.drain } @@ -100,4 +100,3 @@ object Main extends IOApp { } - diff --git a/modules/tests/shared/src/test/scala/ChannelTest.scala b/modules/tests/shared/src/test/scala/ChannelTest.scala index ab3c787fb..a3a183a75 100644 --- a/modules/tests/shared/src/test/scala/ChannelTest.scala +++ b/modules/tests/shared/src/test/scala/ChannelTest.scala @@ -18,7 +18,7 @@ class ChannelTest extends SkunkTest { sessionTest("channel (coverage)") { s => val data = List("foo", "bar", "baz") - val ch0 = s.channel(id"channel_test") + val ch0 = s.channel(ident"channel_test") val ch1 = ch0.mapK(FunctionK.id) val ch2 = Functor[Channel[IO, String, *]].map(ch1)(identity[String]) val ch3 = Contravariant[Channel[IO, *, String]].contramap(ch2)(identity[String]) @@ -41,7 +41,7 @@ class ChannelTest extends SkunkTest { sessionTest("channel with resource (coverage)") { s => val data = List("foo", "bar", "baz") - val ch0 = s.channel(id"channel_test") + val ch0 = s.channel(ident"channel_test") val ch1 = ch0.mapK(FunctionK.id) val ch2 = Functor[Channel[IO, String, *]].map(ch1)(identity[String]) val ch3 = Contravariant[Channel[IO, *, String]].contramap(ch2)(identity[String]) @@ -56,5 +56,17 @@ class ChannelTest extends SkunkTest { } } + sessionTest("channel with quoted identifier round-trips through LISTEN/NOTIFY/UNLISTEN") { s => + val data = List("foo", "bar", "baz") + val ch = s.channel(ident"q_my_queue.INSERT") + + ch.listenR(42).use { r => + for { + _ <- data.traverse_(ch.notify) + d <- r.map(_.value).takeThrough(_ != data.last).compile.toList + _ <- assert(s"channel data $d $data", data == d) + } yield "ok" + } + } } diff --git a/modules/tests/shared/src/test/scala/data/IdentifierTest.scala b/modules/tests/shared/src/test/scala/data/IdentifierTest.scala index d91b407e1..ac96949b9 100644 --- a/modules/tests/shared/src/test/scala/data/IdentifierTest.scala +++ b/modules/tests/shared/src/test/scala/data/IdentifierTest.scala @@ -9,47 +9,148 @@ import skunk.data.Identifier import skunk.implicits._ import cats.syntax.all._ import cats.effect.IO +import scala.annotation.nowarn class IdentifierTest extends ffstest.FTest { - test("valid") { - Identifier.fromString("Identifier") match { + @nowarn("cat=deprecation") + private def legacyFromString(s: String): Either[String, Identifier] = + Identifier.fromString(s) + + test("fromString - valid, folded to lower case") { + legacyFromString("Identifier") match { case Left(err) => fail(err) - case Right(id) => assertEqual("value", id.value, "Identifier") + case Right(id) => assertEqual("value", id.value, "identifier") } } - test("invalid - lexical") { - Identifier.fromString("7_@*#&") match { + test("fromString - invalid lexical") { + legacyFromString("7_@*#&") match { case Left(err) => err.pure[IO] case Right(value) => fail(s"expected error, got $value") } } - test("invalid - too long") { - Identifier.fromString("x" * 100) match { + test("fromString - too long") { + legacyFromString("x" * 100) match { case Left(err) => err.pure[IO] case Right(value) => fail(s"expected error, got $value") } } - test("invalid - too short") { - Identifier.fromString("") match { + test("fromString - empty") { + legacyFromString("") match { case Left(err) => err.pure[IO] case Right(value) => fail(s"expected error, got $value") } } - test("invalid - reserved word") { - Identifier.fromString("select") match { + test("fromString - reserved word") { + legacyFromString("select") match { case Left(err) => err.pure[IO] case Right(value) => fail(s"expected error, got $value") } } - test("toString") { - assertEqual("value", id"foo".toString, "foo") + test("fromValue - valid, preserves case and dots") { + Identifier.fromValue("q_my_queue.INSERT") match { + case Left(err) => fail(err) + case Right(id) => + for { + _ <- assertEqual("value", id.value, "q_my_queue.INSERT") + _ <- assertEqual("sql", id.sql, "\"q_my_queue.INSERT\"") + } yield () + } } -} + test("fromValue - keyword allowed") { + Identifier.fromValue("SELECT") match { + case Left(err) => fail(err) + case Right(id) => + for { + _ <- assertEqual("value", id.value, "SELECT") + _ <- assertEqual("sql", id.sql, "\"SELECT\"") + } yield () + } + } + + test("fromValue - empty rejected") { + Identifier.fromValue("") match { + case Left(err) => err.pure[IO] + case Right(value) => fail(s"expected error, got $value") + } + } + + test("fromValue - null byte rejected") { + Identifier.fromValue("foo" + '\u0000' + "bar") match { + case Left(err) => err.pure[IO] + case Right(value) => fail(s"expected error, got $value") + } + } + + test("fromValue - too long in bytes") { + // "é" is two bytes in UTF-8, so 32 of them is 64 bytes, one over the limit. + Identifier.fromValue("é" * 32) match { + case Left(err) => err.pure[IO] + case Right(value) => fail(s"expected error, got $value") + } + } + test("sql - unquoted when safe") { + assertEqual("sql", (id"foo_bar": @nowarn("cat=deprecation")).sql, "foo_bar") + } + + test("sql - quoted keyword") { + Identifier.fromValue("select") match { + case Left(err) => fail(err) + case Right(id) => assertEqual("sql", id.sql, "\"select\"") + } + } + + test("sql - escapes embedded double quotes") { + Identifier.fromValue("has\"quote") match { + case Left(err) => fail(err) + case Right(id) => assertEqual("sql", id.sql, "\"has\"\"quote\"") + } + } + + test("sql - mixed-case id is folded and renders unquoted") { + // Regression: the (deprecated) `id"…"` interpolator folds to lower case so it round-trips through + // Postgres unchanged, rather than being quoted (which would make it case-sensitive). + val folded = (id"Foo": @nowarn("cat=deprecation")) + for { + _ <- assertEqual("value", folded.value, "foo") + _ <- assertEqual("sql", folded.sql, "foo") + } yield () + } + + test("ident - interpolator preserves case and renders quoted") { + val quoted = ident"q_my_queue.INSERT" + for { + _ <- assertEqual("value", quoted.value, "q_my_queue.INSERT") + _ <- assertEqual("sql", quoted.sql, "\"q_my_queue.INSERT\"") + } yield () + } + + test("ident - interpolator leaves a safe lower-case name unquoted") { + val plain = ident"foo_bar" + for { + _ <- assertEqual("value", plain.value, "foo_bar") + _ <- assertEqual("sql", plain.sql, "foo_bar") + } yield () + } + + test("toString returns the sql form") { + assertEqual("toString", ident"My.Channel".toString, "\"My.Channel\"") + } + + test("Eq compares identifier values") { + val legacy = legacyFromString("foo").toOption.get + val current = Identifier.fromValue("foo").toOption.get + for { + _ <- IO(assert(legacy === current)) + _ <- IO(assert(legacy == current)) + } yield () + } + +}