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

added support for column and table names in repos #8

Merged

Conversation

lbialy
Copy link
Contributor

@lbialy lbialy commented Jun 29, 2023

This PR introduces column names and table name into ImmutableRepo and Repo allowing user to build more future-proof queries (addresses #4). For this to work it was also necessary to modify the logic of sql interpolator so that it works in two stages (it was initially considering everything that went into the interpolation as value input to be interpolated into PreparedStatement) - first it interpolates identifiers into the SQL query string and only then, after removing identifier type interpolation values goes to value embedding. I have also found out that despite table entity classes needing derives DbCodec it was impossible to use these classes in interpolation so I have fixed that in one breath and now it's completely legit to write sql"INSERT INTO $table $insertColumns VALUES ($p)" where p is a case class instance for which there's a DbCodec instance. This in turn replaces the idea for passing of the parameter strings (useless anyhow, it would require custom handling to properly fire DbCodec on such query anyhow, current solution is safer) mentioned in #4. There are tests for all of the supported databases but I have a M-macbook and can't run Oracle so fingers crossed that it doesn't blow up in CI.

@AugustNagro
Copy link
Owner

now it's completely legit to write sql"INSERT INTO $table $insertColumns VALUES ($p)" where p is a case class instance for which there's a DbCodec instance

Ok.. that's awesome.

can't run Oracle

I tested and it's green.

I've thought a little bit more about the goal of 'future-proof queries'.

The first thing is that future-proof queries have the benefit of greatly reducing boilerplate. They also minimize the need to make dozens of changes should a column name change.

A downside is that it's much more work to copy-paste into a SQL editor like DbBeaver. One other problem with the current implementation is that it doesn't totally eliminate the need to write sql identifiers. For example, top_speed in the added test:

val sql = sql"SELECT ${*} FROM $table WHERE top_speed > $speed"

Queries having multiple joins would be even worse. Also, summoning RepoDefault instances for the other tables in every method is probably bad for performance, since the deriving method is complex.

If we're going to offer a future-proof query dsl, we should go all-in.

I propose the following api:

case class User(id: Long, firstName: String, age: Int) derives DbCodec

case class UserCreator(firstName: String, age: Int) derives DbCodec

object Schema:
  val user = DbSchema[UserCreator, User, Long]

object UserRepo extends Repo[UserCreator, User, Long]:
    def firstNamesForLast(lastName: String)(using DbCon): Vector[String] =
    import Schema.user
    sql"""
      SELECT DISTINCT ${user.firstName}
      FROM $user
      WHERE ${user.lastName} = $lastName
    """.run

   def insertIgnoreDuplicates(insert: ProjectInsert)(using DbCon): Int =
      import Schema.projects
      sql"""
         INSERT OR IGNORE INTO $projects ${projects.insertCols}
         VALUES ($insert)
         """.update.run()

You might find it interesting that I partially implemented this many months ago:

https://github.com/AugustNagro/magnum/blob/d4eade8f17f4f46c8de8ad044358d210338723d7/README.md

It works by using structural types + transparent macro.

But I found that in my projects, renaming columns / tables happens very rarely. Meanwhile, having to copy queries to sql-editors like DbBeaver is super common. I also hadn't discovered the 'given RepoDefaults' pattern yet. So it was more verbose. One other downside is that IDEA doesn't support auto-completion for the structural accessors like userSchema.lastName (although metals does). It also doesn't work if you add an explicit type to val user since it loses the structural type.

So I'm pretty torn on the overall pros/cons. But there are many situations where having future-proof statements is a big win. So lets have it and let users decide.

When you address the minor comments I will merge your PR, then this weekend I'll do some refactoring and bring back DbSchema. Then I'll request you to review that PR.

@AugustNagro
Copy link
Owner

Actually, no need to make changes. I'll just merge now, then make the PR as I said.

@AugustNagro AugustNagro merged commit 58f2529 into AugustNagro:master Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants