Support for quoted identifiers#1307
Conversation
matejcerny
force-pushed
the
quoted-identifiers
branch
from
059f5d6 to
a19d05f
Compare
| def asSql: String = if (quoted) s"\"${value.replace("\"", "\"\"")}\"" else value | ||
| override def toString: String = asSql | ||
|
|
||
| override def equals(other: Any): Boolean = other match { |
There was a problem hiding this comment.
Not sure if this can break anything?
There was a problem hiding this comment.
why not add quoted as a constructor parameter instead and rely on the default one?
There was a problem hiding this comment.
it's a sealed class, it should be fine...
(but maybe it's not)
| val identifier: SCodec[Identifier] = | ||
| utf8z.exmap( | ||
| s => Attempt.fromEither(Identifier.fromString(s).leftMap(Err(_))), | ||
| s => Attempt.fromEither(Identifier.fromString(s).orElse(Identifier.fromStringQuoted(s)).leftMap(Err(_))), |
There was a problem hiding this comment.
Any idea if there is a better solution?
matejcerny
force-pushed
the
quoted-identifiers
branch
from
a19d05f to
214acdf
Compare
|
It's a much bigger change than I originally thought 🤷 |
| Identifier.fromStringQuoted(part) match { | ||
| case Right(Identifier(s)) => '{ Identifier.fromStringQuoted(${Expr(s)}).fold(sys.error, identity) } | ||
| case Left(s) => | ||
| report.error(s) |
There was a problem hiding this comment.
there's a variant that aborts and returns Nothing, so you don't have to do '{???} - errorAndAbort
There was a problem hiding this comment.
I mirrored the identifier_impl implementation
| def asSql: String = if (quoted) s"\"${value.replace("\"", "\"\"")}\"" else value | ||
| override def toString: String = asSql | ||
|
|
||
| override def equals(other: Any): Boolean = other match { |
There was a problem hiding this comment.
why not add quoted as a constructor parameter instead and rely on the default one?
matejcerny
force-pushed
the
quoted-identifiers
branch
from
214acdf to
88a1522
Compare
matejcerny
force-pushed
the
quoted-identifiers
branch
from
88a1522 to
586337c
Compare
| 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.asSql}, '$message'", Origin.unknown, Void.codec)).void |
There was a problem hiding this comment.
IMO we need to support Identifiers in interpolation wherever Fragment[Void] is allowed. But that's probably a separate issue.
| def quoted: Boolean = false | ||
| def asSql: String = if (quoted) s"\"${value.replace("\"", "\"\"")}\"" else value |
There was a problem hiding this comment.
Do we actually need to treat quoted identifiers as a separate case? As I understand it (could anyone more familiar with PostgreSQL please confirm this?), the distinction exists only on syntactical level:
- Unquoted identifiers are automatically converted to lowercase.
- Unquoted identifiers can not contain keywords.
- Quoted identifiers are not converted, and can contain almost anything.
Which means that foo_bar is for all intents and purposes the same as "foo_bar".
I propose we do the following (starting from main):
- Change the existing
id"..."syntax to automatically lowercase the literal. Maybe mark this syntax as deprecated, since the same behavior can be achieved if the calling code provides the lowercase literal. - Add a separate syntax that does not change the literal. ( Not sure what to callit.
qid"..."doesn't really work, since quoting becomes an implementation detail). - When interpolating an
Identifierinto SQL, quote it iff it requires quoting (if the value contains uppercase letters, or otherwise breaks the constraints on unquoted identifiers). We can maybe cache the possibly quoted form onIdentifiercreation. - Instead of 3, we can just always quote
Identifiers. It's noisy WRT generated SQL, but a bit easier to implement.
3 participants
implements #1276