Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/doobie-core/src/main/scala/DoobieMapping.scala
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ trait DoobieMappingLike[F[_]] extends Mapping[F] with SqlMappingLike[F] {
implicit tableName: TableName,
typeName: TypeName[T],
pos: SourcePos): ColumnRef =
ColumnRef(tableName.name, colName, (codec, nullable), typeName.value, pos)
ColumnRef(tableName, colName, (codec, nullable), typeName.value, pos)

implicit def Fragments: SqlFragment[Fragment] =
new SqlFragment[Fragment] {
Expand Down
5 changes: 4 additions & 1 deletion modules/doobie-mssql/src/main/scala/DoobieMSSqlMapping.scala
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ trait DoobieMSSqlMappingLike[F[_]] extends DoobieMappingLike[F] with SqlMappingL

def encapsulateUnionBranch(s: SqlSelect): SqlSelect =
if (s.orders.isEmpty) s
else s.toSubquery(s.table.name + "_encaps", Laterality.NotLateral)
else
// The subquery name lands in alias position, so a schema-qualified table name must be
// folded to a bare identifier first (issue #342).
s.toSubquery(s.table.identifier + "_encaps", Laterality.NotLateral)

def mkLateral(inner: Boolean): Laterality =
Laterality.Apply(inner)
Expand Down
8 changes: 8 additions & 0 deletions modules/doobie-pg/src/test/scala/DoobiePgSuites.scala
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,14 @@ final class ProjectionSuite extends DoobiePgDatabaseSuite with SqlProjectionSuit
lazy val mapping = new DoobiePgTestMapping(transactor) with SqlProjectionMapping[IO]
}

final class QualifiedNamesSuite extends DoobiePgDatabaseSuite with SqlQualifiedNamesSuite {
lazy val mapping = new DoobiePgTestMapping(transactor) with SqlQualifiedNamesMapping[IO]
}

final class TableNameSuite extends DoobiePgDatabaseSuite with SqlTableNameSuite {
lazy val mapping = new DoobiePgTestMapping(transactor) with SqlQualifiedNamesMapping[IO]
}

final class RecursiveInterfacesSuite
extends DoobiePgDatabaseSuite
with SqlRecursiveInterfacesSuite {
Expand Down
8 changes: 8 additions & 0 deletions modules/skunk/js-jvm/src/test/scala/SkunkSuites.scala
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,14 @@ final class ProjectionSuite extends SkunkDatabaseSuite with SqlProjectionSuite {
lazy val mapping = new SkunkTestMapping(pool) with SqlProjectionMapping[IO]
}

final class QualifiedNamesSuite extends SkunkDatabaseSuite with SqlQualifiedNamesSuite {
lazy val mapping = new SkunkTestMapping(pool) with SqlQualifiedNamesMapping[IO]
}

final class TableNameSuite extends SkunkDatabaseSuite with SqlTableNameSuite {
lazy val mapping = new SkunkTestMapping(pool) with SqlQualifiedNamesMapping[IO]
}

final class RecursiveInterfacesSuite
extends SkunkDatabaseSuite
with SqlRecursiveInterfacesSuite {
Expand Down
2 changes: 1 addition & 1 deletion modules/skunk/shared/src/main/scala/SkunkMapping.scala
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ trait SkunkMappingLike[F[_]] extends Mapping[F] with SqlPgMappingLike[F] { outer
typeName: NullableTypeName[T],
isNullable: IsNullable[T],
pos: SourcePos): ColumnRef =
ColumnRef(tableName.name, colName, (codec, isNullable.isNullable), typeName.value, pos)
ColumnRef(tableName, colName, (codec, isNullable.isNullable), typeName.value, pos)

// We need to demonstrate that our `Fragment` type has certain compositional properties.
implicit def Fragments: SqlFragment[AppliedFragment] =
Expand Down
95 changes: 74 additions & 21 deletions modules/sql-core/src/main/scala/SqlMapping.scala
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,33 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
def orderToFragment(col: Fragment, ascending: Boolean, nullsLast: Boolean): Fragment
def nullsHigh: Boolean

case class TableName(name: String)
/**
* The name of a SQL table, split into an optional schema qualifier and a local name.
*
* A table's raw SQL name plays two distinct roles depending on where it's used: a reference
* (`sqlRef`, schema-qualified, e.g. "public.country" — legal in a FROM/JOIN clause) and a
* bare identifier (`identifier`, e.g. "public_country" — required wherever an alias or
* synthesized name is minted, since a dot is not a legal identifier character). Keeping both
* derived from one structured value means a call site that needs the identifier form reaches
* for `.identifier` and can't accidentally reach for the raw, possibly-dotted `.sqlRef`
* instead (issue #342).
*/
case class TableName(schema: Option[String], name: String) {
def sqlRef: String = schema.fold(name)(s => s"$s.$name")
def identifier: String = schema.fold(name)(s => s"${s.replace('.', '_')}_$name")
override def toString: String = sqlRef
}
object TableName {
def apply(raw: String): TableName =
raw.lastIndexOf('.') match {
case -1 => TableName(None, raw)
case i =>
val (schema, dotName) = raw.splitAt(i)
TableName(Some(schema), dotName.tail)
}
val rootName = "<root>"
val rootTableName = TableName(rootName)
def isRoot(table: String): Boolean = table == rootName
val rootTableName = TableName(None, rootName)
def isRoot(table: TableName): Boolean = table == rootTableName
}
class TableDef(name: String) {
implicit val tableName: TableName = TableName(name)
Expand All @@ -87,7 +109,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
* used to construct `SqlColumns`.
*/
case class ColumnRef(
table: String,
table: TableName,
column: String,
codec: Codec,
scalaTypeName: String,
Expand Down Expand Up @@ -144,7 +166,10 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
case Some(alias) => (this, alias)
case None =>
if (seenTables(table.name)) {
val alias = s"${table.name}_alias_$next"
// An alias must be a bare identifier, so a qualified name like "public.country"
// cannot seed it verbatim (issue #342); uniqueness is preserved by the counter,
// which is shared across table and column aliases.
val alias = s"${table.identifier}_alias_$next"
val newState =
copy(
next = next + 1,
Expand Down Expand Up @@ -1308,6 +1333,12 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
*/
def name: String

/**
* A bare-identifier-safe form of this `TableExpr`'s name, for use in alias and synthesized
* subquery name positions where a dot is not legal (issue #342).
*/
def identifier: String

/**
* Is the supplied column an immediate component of this `TableExpr`?
*/
Expand Down Expand Up @@ -1354,14 +1385,17 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
/**
* Table expression corresponding to a possibly aliased table
*/
case class TableRef(context: Context, name: String) extends TableExpr {
case class TableRef(context: Context, tableName: TableName) extends TableExpr {
def name: String = tableName.sqlRef
def identifier: String = tableName.identifier

def owns(col: SqlColumn): Boolean = isSameOwner(col.owner)
def contains(other: ColumnOwner): Boolean = isSameOwner(other)

def findNamedOwner(col: SqlColumn): Option[TableExpr] =
if (this == col.owner) Some(this) else None

def isRoot: Boolean = TableName.isRoot(name)
def isRoot: Boolean = TableName.isRoot(tableName)

def isUnion: Boolean = false

Expand Down Expand Up @@ -1429,6 +1463,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
if (this == col.owner) Some(this) else subquery.findNamedOwner(col)

def isRoot: Boolean = false
def identifier: String = name

def isUnion: Boolean = subquery.isUnion

Expand Down Expand Up @@ -1463,6 +1498,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
if (this == col.owner) Some(this) else withQuery.findNamedOwner(col)

def isRoot: Boolean = false
def identifier: String = name

def isUnion: Boolean = withQuery.isUnion

Expand Down Expand Up @@ -1493,6 +1529,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
assert(!underlying.isInstanceOf[WithRef] || noalias)

def name = alias.getOrElse(underlying.name)
def identifier: String = alias.getOrElse(underlying.identifier)

def owns(col: SqlColumn): Boolean = col.owner.isSameOwner(this) || underlying.owns(col)
def contains(other: ColumnOwner): Boolean =
Expand Down Expand Up @@ -2297,7 +2334,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
def isDistinct: Boolean = distinct.nonEmpty

override def isSameOwner(other: ColumnOwner): Boolean =
other.isSameOwner(TableRef(context, table.name))
other.isSameOwner(TableRef(context, TableName(table.name)))

def owns(col: SqlColumn): Boolean = cols.contains(col) || owns0(col)
def contains(other: ColumnOwner): Boolean =
Expand Down Expand Up @@ -2346,8 +2383,10 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
* joins
*/
def syntheticName(suffix: String): String = {
val joinNames = joins.map(_.child.name)
(table.name :: joinNames).mkString("_").take(50 - suffix.length) + suffix
// Synthesized names are used as subquery aliases, so they must be bare identifiers
// even when built from schema-qualified table names (issue #342).
val joinNames = joins.map(_.child.identifier)
(table.identifier :: joinNames).mkString("_").take(50 - suffix.length) + suffix
}

/**
Expand Down Expand Up @@ -2451,9 +2490,11 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
val finalJoin = lastJoin.toSqlJoin(lastJoinParentTable, base.table, inner)
finalJoin :: Nil
} else {
// On the mergeable branch base.table is the raw TableRef, so its name may be
// schema-qualified and must be folded before use in alias position (#342).
val assocTable = TableExpr.DerivedTableRef(
context,
Some(base.table.name + "_assoc"),
Some(base.table.identifier + "_assoc"),
base.table,
true)
val assocJoin = lastJoin.toSqlJoin(lastJoinParentTable, assocTable, inner)
Expand Down Expand Up @@ -3368,7 +3409,9 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
for {
withFilter0 <- withFilter
table <- parentTableForType(context)
sel <- withFilter0.toSubquery(table.name)
// The subquery name lands in alias position, so it must be a bare
// identifier even for a schema-qualified table (issue #342).
sel <- withFilter0.toSubquery(table.identifier)
res <- sel.addFilterOrderByOffsetLimit(
None,
orderBy,
Expand Down Expand Up @@ -4627,7 +4670,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
val tables = allTables(List(om))
val split = tables.sizeCompare(1) > 0
if (!split) Nil
else List(SplitObjectTypeMapping(om, tables))
else List(SplitObjectTypeMapping(om, tables.map(_.sqlRef)))
}

def checkSuperInterfaces(om: ObjectMapping): List[ValidationFailure] = {
Expand All @@ -4639,7 +4682,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
val tables = allTables(allMappings)
val split = tables.sizeCompare(1) > 0
if (!split) Nil
else List(SplitInterfaceTypeMapping(om, allMappings, tables))
else List(SplitInterfaceTypeMapping(om, allMappings, tables.map(_.sqlRef)))
}

def checkUnionMembers(om: ObjectMapping): List[ValidationFailure] = {
Expand All @@ -4649,7 +4692,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
val tables = allTables(allMappings)
val split = tables.sizeCompare(1) > 0
if (!split) Nil
else List(SplitUnionTypeMapping(om, allMappings, tables))
else List(SplitUnionTypeMapping(om, allMappings, tables.map(_.sqlRef)))

case _ => Nil
}
Expand Down Expand Up @@ -4774,7 +4817,14 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
} yield {
val childTables = allTables(List(com))
if (parentTables.sameElements(childTables)) Nil
else List(SplitEmbeddedObjectTypeMapping(om, fm, com, parentTables, childTables))
else
List(
SplitEmbeddedObjectTypeMapping(
om,
fm,
com,
parentTables.map(_.sqlRef),
childTables.map(_.sqlRef)))
}).getOrElse(Nil)
}

Expand Down Expand Up @@ -4810,8 +4860,8 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
InconsistentJoinConditions(
om,
fm,
j.conditions.map(_._1.table).distinct,
j.conditions.map(_._2.table).distinct)
j.conditions.map(_._1.table.sqlRef).distinct,
j.conditions.map(_._2.table.sqlRef).distinct)
}

val serConsistent = {
Expand All @@ -4831,9 +4881,12 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
if (headIsParent && lastIsChild && consistentChain) Nil
else {
val path = nonEmptyJoins.map(j =>
(j.conditions.head._1.table, j.conditions.last._2.table))
(
j.conditions.head._1.table.sqlRef,
j.conditions.last._2.table.sqlRef))

List(MisalignedJoins(om, fm, parentTable, childTable, path))
List(
MisalignedJoins(om, fm, parentTable.sqlRef, childTable.sqlRef, path))
}
}
}
Expand All @@ -4849,7 +4902,7 @@ trait SqlMappingLike[F[_]] extends CirceMappingLike[F] with SqlModule[F] { self
}
}

private def allTables(oms: List[ObjectMapping]): List[String] =
private def allTables(oms: List[ObjectMapping]): List[TableName] =
oms
.flatMap(_.fieldMappings.flatMap {
case SqlField(_, columnRef, _, _, _, _) => List(columnRef.table)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ trait SqlMappingValidatorInvalidSuite extends CatsEffectSuite {
fm.fieldName,
SchemaRenderer.renderType(field.tpe),
field.tpe.isNullable,
columnRef.table,
columnRef.table.sqlRef,
columnRef.column,
colIsNullable))
case _ => None
Expand All @@ -63,7 +63,7 @@ trait SqlMappingValidatorInvalidSuite extends CatsEffectSuite {
om.tpe.name,
fm.fieldName,
SchemaRenderer.renderType(field.tpe),
columnRef.table,
columnRef.table.sqlRef,
columnRef.column,
columnRef.scalaTypeName))
case _ => None
Expand All @@ -80,7 +80,7 @@ trait SqlMappingValidatorInvalidSuite extends CatsEffectSuite {
om.tpe.name,
fm.fieldName,
SchemaRenderer.renderType(field.tpe),
columnRef.table,
columnRef.table.sqlRef,
columnRef.column,
columnRef.scalaTypeName))
case _ => None
Expand Down
Loading
Loading