Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NamingStrategy override of querySchema #1560

Merged
merged 2 commits into from
Aug 15, 2019
Merged
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
11 changes: 5 additions & 6 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import java.io.{File => JFile}

enablePlugins(TutPlugin)

val CodegenTag = Tags.Tag("CodegenTag")
(concurrentRestrictions in Global) += Tags.exclusive(CodegenTag)

lazy val baseModules = Seq[sbt.ClasspathDep[sbt.ProjectReference]](
`quill-core-jvm`, `quill-core-js`, `quill-sql-jvm`, `quill-sql-js`, `quill-monix`
)
Expand Down Expand Up @@ -135,16 +138,12 @@ lazy val `quill-codegen-jdbc` =
.dependsOn(`quill-codegen` % "compile->compile;test->test")
.dependsOn(`quill-jdbc` % "compile->compile;test->test")


val codegen = taskKey[Seq[File]]("Run Code Generation Phase for Integration Testing")

lazy val `quill-codegen-tests` =
(project in file("quill-codegen-tests"))
.settings(commonSettings: _*)
.settings(
libraryDependencies += "org.scala-lang" % "scala-compiler" % scalaVersion.value % Test,
fork in Test := true,
(sourceGenerators in Test) += (codegen in Test),
(excludeFilter in unmanagedSources) := excludePathsIfOracle {
(unmanagedSourceDirectories in Test).value.map { dir =>
(dir / "io" / "getquill" / "codegen" / "OracleCodegenTestCases.scala").getCanonicalPath
Expand All @@ -153,7 +152,7 @@ lazy val `quill-codegen-tests` =
(dir / "io" / "getquill" / "codegen" / "util" / "WithOracleContext.scala").getCanonicalPath
}
},
(codegen in Test) := {
(sourceGenerators in Test) += Def.task {
def recrusiveList(file:JFile): List[JFile] = {
if (file.isDirectory)
Option(file.listFiles()).map(_.flatMap(child=> recrusiveList(child)).toList).toList.flatten
Expand All @@ -180,7 +179,7 @@ lazy val `quill-codegen-tests` =
s
)
recrusiveList(fileDir)
}
}.tag(CodegenTag)
)
.dependsOn(`quill-codegen-jdbc` % "compile->test")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ trait CqlIdiom extends Idiom {

implicit def propertyTokenizer(implicit valueTokenizer: Tokenizer[Value], identTokenizer: Tokenizer[Ident], strategy: NamingStrategy): Tokenizer[Property] =
Tokenizer[Property] {
case Property(_, name) => strategy.column(name).token
case Property.Opinionated(_, name, renameable) => strategy.column(name).token
}

implicit def valueTokenizer(implicit strategy: NamingStrategy): Tokenizer[Value] = Tokenizer[Value] {
Expand Down Expand Up @@ -187,7 +187,8 @@ trait CqlIdiom extends Idiom {
}

implicit def entityTokenizer(implicit strategy: NamingStrategy): Tokenizer[Entity] = Tokenizer[Entity] {
case Entity(name, properties) => strategy.table(name).token
case Entity.Opinionated(name, properties, renameable) =>
renameable.fixedOr(name.token)(strategy.table(name).token)
}

implicit def traversableTokenizer(implicit strategy: NamingStrategy): Tokenizer[TraversableOperation] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import io.getquill.codegen.util.TryOps._
import org.slf4j.LoggerFactory

import scala.concurrent.duration.Duration
import scala.concurrent.{ Await, Future }
import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.Await

object SchemaNames {
val simpleSnake = `schema_snakecase`
Expand All @@ -26,17 +25,10 @@ object CodegenTestCaseRunner {
if (args.drop(1).contains("all")) ConfigPrefix.all
else args.drop(1).map(ConfigPrefix.fromValue(_).orThrow).toList

// Need to generate files for each database test-case by test-case (since auto-commit is used)
// but can do cases from multiple databases at the same time
val results =
prefixes.map(prefix =>
Future {
val generatedFiles = apply(prefix, path)
generatedFiles.foreach(f => logger.info(s"${prefix} | ${f}"))
})

Await.result(Future.sequence(results), Duration.Inf)
()
prefixes.foreach(prefix => {
val generatedFiles = apply(prefix, path)
generatedFiles.foreach(f => logger.info(s"${prefix} | ${f}"))
})
}

def apply(dbPrefix: ConfigPrefix, path: String) = {
Expand Down
17 changes: 12 additions & 5 deletions quill-core/src/main/scala/io/getquill/MirrorIdiom.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.getquill

import io.getquill.ast.Renameable.{ ByStrategy, Fixed }
import io.getquill.ast._
import io.getquill.context.CanReturnClause
import io.getquill.idiom.{ Idiom, SetContainsToken, Statement }
Expand Down Expand Up @@ -63,11 +64,11 @@ trait MirrorIdiomBase extends Idiom {

implicit def queryTokenizer(implicit liftTokenizer: Tokenizer[Lift]): Tokenizer[Query] = Tokenizer[Query] {

case Entity(name, Nil) => stmt"querySchema(${s""""$name"""".token})"
case Entity.Opinionated(name, Nil, renameable) => stmt"${tokenizeName("querySchema", renameable).token}(${s""""$name"""".token})"

case Entity(name, prop) =>
case Entity.Opinionated(name, prop, renameable) =>
val properties = prop.map(p => stmt"""_.${p.path.mkStmt(".")} -> "${p.alias.token}"""")
stmt"querySchema(${s""""$name"""".token}, ${properties.token})"
stmt"${tokenizeName("querySchema", renameable).token}(${s""""$name"""".token}, ${properties.token})"

case Filter(source, alias, body) =>
stmt"${source.token}.filter(${alias.token} => ${body.token})"
Expand Down Expand Up @@ -176,9 +177,15 @@ trait MirrorIdiomBase extends Idiom {
case o => stmt"${o.toString.token}"
}

def tokenizeName(name: String, renameable: Renameable) =
renameable match {
case ByStrategy => name
case Fixed => s"`${name}`"
}

implicit def propertyTokenizer(implicit liftTokenizer: Tokenizer[Lift]): Tokenizer[Property] = Tokenizer[Property] {
case Property(ExternalIdent(_), name) => stmt"${name.token}"
case Property(ref, name) => stmt"${scopedTokenizer(ref)}.${name.token}"
case Property.Opinionated(ExternalIdent(_), name, renameable) => stmt"${tokenizeName(name, renameable).token}"
case Property.Opinionated(ref, name, renameable) => stmt"${scopedTokenizer(ref)}.${tokenizeName(name, renameable).token}"
}

implicit val valueTokenizer: Tokenizer[Value] = Tokenizer[Value] {
Expand Down
138 changes: 135 additions & 3 deletions quill-core/src/main/scala/io/getquill/ast/Ast.scala
Original file line number Diff line number Diff line change
@@ -1,13 +1,31 @@
package io.getquill.ast

import io.getquill.NamingStrategy

//************************************************************

sealed trait Ast {

/**
* Return a copy of this AST element with any opinions that it may have set to their neutral position.
* Return the object itself if it has no opinions.
*/
def neutral: Ast = this

/**
* Set all opinions of this element and every element in the subtree to the neutral position.
*/
final def neutralize: Ast = new StatelessTransformer {
override def apply(a: Ast) =
super.apply(a.neutral)
}.apply(this)

override def toString = {
import io.getquill.MirrorIdiom._
import io.getquill.idiom.StatementInterpolator._
implicit def liftTokenizer: Tokenizer[Lift] =
Tokenizer[Lift](_ => stmt"?")
implicit val namingStrategy: NamingStrategy = io.getquill.Literal
this.token.toString
}
}
Expand All @@ -16,7 +34,51 @@ sealed trait Ast {

sealed trait Query extends Ast

case class Entity(name: String, properties: List[PropertyAlias]) extends Query
/**
* Entities represent the actual tables/views being selected.
* Typically, something like:
* <pre>`SELECT p.name FROM People p`</pre> comes from
* something like:
* <pre>`Map(Entity("People", Nil), Ident("p"), Property(Ident(p), "name"))`.</pre>
* When you define a `querySchema`, the fields you mention inside become `PropertyAlias`s.
* For example something like:
* <pre>`querySchema[Person]("t_person", _.name -> "s_name")`</pre>
* Becomes something like:
* <pre>`Entity("t_person", List(PropertyAlias(List("name"), "s_name"))) { def renameable = Fixed }`</pre>
* Note that Entity has an Opinion called `renameable` which will be the value `Fixed` when a `querySchema` is specified.
* That means that even if the `NamingSchema` is `UpperCase`, the resulting query will select `t_person` as opposed
* to `T_PERSON` or `Person`.
*/
case class Entity(name: String, properties: List[PropertyAlias]) extends Query {
// Technically this should be part of the Entity case class but due to the limitations of how
// scala creates companion objects, the apply/unapply wouldn't be able to work correctly.
def renameable: Renameable = Renameable.neutral

override def neutral: Entity =
new Entity(name, properties)

override def equals(that: Any) =
that match {
case e: Entity => (e.name, e.properties, e.renameable) == ((name, properties, renameable))
case _ => false
}

override def hashCode = (name, properties, renameable).hashCode()
}

object Entity {
def apply(name: String, properties: List[PropertyAlias]) = new Entity(name, properties)
def unapply(e: Entity) = Some((e.name, e.properties))

object Opinionated {
def apply(name: String, properties: List[PropertyAlias], renameableNew: Renameable) =
new Entity(name, properties) {
override def renameable: Renameable = renameableNew
}
def unapply(e: Entity) =
Some((e.name, e.properties, e.renameable))
}
}

case class PropertyAlias(path: List[String], alias: String)

Expand Down Expand Up @@ -70,10 +132,80 @@ case class Function(params: List[Ident], body: Ast) extends Ast
case class Ident(name: String) extends Ast

// Like identity but is but defined in a clause external to the query. Currently this is used
// for 'returning' clauses to define properties being retruned.
// for 'returning' clauses to define properties being returned.
case class ExternalIdent(name: String) extends Ast

case class Property(ast: Ast, name: String) extends Ast
/**
* An Opinion represents a piece of data that needs to be propagated through AST transformations but is not directly
* related to how ASTs are transformed in most stages. For instance, `Renameable` controls how columns are named (i.e. whether to use a
* `NamingStrategy` or not) after most of the SQL transformations are done. Some transformations (e.g. `RenameProperties`
* will use `Opinions` or even modify them so that the correct kind of query comes out at the end of the normalizations.
* That said, Opinions should be transparent in most steps of the normalization. In some cases e.g. `BetaReduction`,
* AST elements need to be neutralized (i.e. set back to defaults in the entire tree) so that this works correctly.
*/
sealed trait Opinion[T]
sealed trait OpinionValues[T <: Opinion[T]] {
def neutral: T
}

sealed trait Renameable extends Opinion[Renameable] {
def fixedOr[T](plain: T)(otherwise: T) =
this match {
case Renameable.Fixed => plain
case _ => otherwise
}
}
object Renameable extends OpinionValues[Renameable] {
case object Fixed extends Renameable with Opinion[Renameable]
case object ByStrategy extends Renameable with Opinion[Renameable]

override def neutral: Renameable = ByStrategy
}

/**
* Properties generally represent column selection from a table or invocation of some kind of method from
* some other object. Typically, something like
* <pre>`SELECT p.name FROM People p`</pre> comes from
* something like
* <pre>`Map(Entity("People"), Ident("p"), Property(Ident(p), "name"))`</pre>
* Properties also have
* an Opinion about how the `NamingStrategy` affects their name. For example something like
* `Property.Opinionated(Ident(p), "s_name", Fixed)` will become `p.s_name` even if the `NamingStrategy` is `UpperCase`
* (whereas `Property(Ident(p), "s_name")` would become `p.S_NAME`). When Property is constructed without `Opinionated`
* being used, the default opinion `ByStrategy` is used.
*/
case class Property(ast: Ast, name: String) extends Ast {
// Technically this should be part of the Property case class but due to the limitations of how
// scala creates companion objects, the apply/unapply wouldn't be able to work correctly.
def renameable: Renameable = Renameable.neutral

override def neutral: Property =
new Property(ast, name) {
override def renameable = Renameable.neutral
}

override def equals(that: Any) =
that match {
case p: Property => (p.ast, p.name, p.renameable) == ((ast, name, renameable))
case _ => false
}

override def hashCode = (ast, name, renameable).hashCode()
}

object Property {
def apply(ast: Ast, name: String) = new Property(ast, name)
def unapply(p: Property) = Some((p.ast, p.name))

object Opinionated {
def apply(ast: Ast, name: String, renameableNew: Renameable) =
new Property(ast, name) {
override def renameable: Renameable = renameableNew
}
def unapply(p: Property) =
Some((p.ast, p.name, p.renameable))
}
}

sealed trait OptionOperation extends Ast
case class OptionFlatten(ast: Ast) extends OptionOperation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,9 @@ trait StatefulTransformer[T] {

def apply(e: Property): (Property, StatefulTransformer[T]) =
e match {
case Property(a, b) =>
case Property.Opinionated(a, b, renameable) =>
val (at, att) = apply(a)
(Property(at, b), att)
(Property.Opinionated(at, b, renameable), att)
}

def apply(e: Operation): (Operation, StatefulTransformer[T]) =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ trait StatelessTransformer {

def apply(e: Property): Property =
e match {
case Property(a, name) => Property(apply(a), name)
case Property.Opinionated(a, name, renameable) => Property.Opinionated(apply(a), name, renameable)
}

def apply(e: Operation): Operation =
Expand Down
10 changes: 8 additions & 2 deletions quill-core/src/main/scala/io/getquill/dsl/DynamicQueryDSL.scala
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package io.getquill.dsl

import io.getquill.ast.Renameable.Fixed

import scala.language.implicitConversions
import scala.language.experimental.macros
import io.getquill.ast._

import scala.reflect.macros.whitebox.{ Context => MacroContext }
import io.getquill.util.Messages._

import scala.annotation.tailrec
import scala.util.DynamicVariable
import scala.reflect.ClassTag
Expand Down Expand Up @@ -60,7 +64,9 @@ trait DynamicQueryDsl {
implicit def toQuoted[T <: Action[_]](q: DynamicAction[T]): Quoted[T] = q.q

def dynamicQuery[T](implicit t: ClassTag[T]): DynamicEntityQuery[T] =
dynamicQuerySchema(t.runtimeClass.getName.split('.').last.split('$').last)
Copy link
Collaborator Author

@deusaquilus deusaquilus Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamicQuerySchema method needs to use Fixed naming strategy since it's the equivalent of querySchema. Need to have a separate invocation of Entity here.

DynamicEntityQuery(splice[EntityQuery[T]](
Entity(t.runtimeClass.getName.split('.').last.split('$').last, Nil)
))

case class DynamicAlias[T](property: Quoted[T] => Quoted[Any], name: String)

Expand Down Expand Up @@ -103,7 +109,7 @@ trait DynamicQueryDsl {

PropertyAlias(path(alias.property(splice[T](Ident("v"))).ast), alias.name)
}
DynamicEntityQuery(splice[EntityQuery[T]](Entity(entity, aliases.toList)))
DynamicEntityQuery(splice[EntityQuery[T]](Entity.Opinionated(entity, aliases.toList, Fixed)))
}

private[this] val nextIdentId = new DynamicVariable(0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class MetaDslMacro(val c: MacroContext) extends ValueComputation {
q"""
new ${c.prefix}.SchemaMeta[$t] {
private[this] val _entity =
${c.prefix}.quote(${c.prefix}.querySchema[$t](${t.tpe.typeSymbol.name.decodedName.toString}))
${c.prefix}.quote(${c.prefix}.impliedQuerySchema[$t](${t.tpe.typeSymbol.name.decodedName.toString}))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to have a separatae impliedQuerySchema method so that the parser knows to not to set the resulting Entity class's renamable field to Fixed. Basically, now all querySchema elements will have their Entity.renameable set to Fixed and impliedQuerySchema elements will have their Entity.renameable set to ByStrategy.

def entity = _entity
}
"""
Expand Down
3 changes: 3 additions & 0 deletions quill-core/src/main/scala/io/getquill/dsl/QueryDsl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ private[getquill] trait QueryDsl {
@compileTimeOnly(NonQuotedException.message)
def querySchema[T](entity: String, columns: (T => (Any, String))*): EntityQuery[T] = NonQuotedException()

@compileTimeOnly(NonQuotedException.message)
def impliedQuerySchema[T](entity: String, columns: (T => (Any, String))*): EntityQuery[T] = NonQuotedException()
Copy link
Collaborator Author

@deusaquilus deusaquilus Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use querySchema for places where a schema needs to be implicitly inferred (e.g. insert/update/delete) because when querySchema goes into the parser, the parser thinks it's an entity with Entity.renameable = Fixed which is wrong (when insert/update/delete are used).


implicit class NullableColumnExtensions[A](o: Option[A]) {
@compileTimeOnly(NonQuotedException.message)
def getOrNull: A =
Expand Down
4 changes: 2 additions & 2 deletions quill-core/src/main/scala/io/getquill/norm/ApplyMap.scala
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ object ApplyMap {
case GroupBy(DetachableMap(a, b, c), d, e) =>
val er = BetaReduction(e, d -> c)
val x = Ident("x")
val x1 = Property(Ident("x"), "_1")
val x2 = Property(Ident("x"), "_2")
val x1 = Property(Ident("x"), "_1") // These introduced property should not be renamed
val x2 = Property(Ident("x"), "_2") // due to any naming convention.
val body = Tuple(List(x1, Map(x2, b, c)))
Some(Map(GroupBy(a, b, er), x, body))

Expand Down
Loading