Transactor work, add zio-test, and bump zio #95

russwyte · 2025-01-18T17:36:50Z

May at least partially address #84

Adds some Transactor refactoring for discussion.

russwyte · 2025-01-18T20:31:18Z

I think this module's tests should only test the functionality of the Transactor... not all of the core functionality. 🤷‍♂️ All we really need to know is that the Transactor produced expected results on success or failure etc. I feel like the scope of the tests is far too broad for this tiny sub module.

russwyte · 2025-01-18T20:45:52Z

What is Transactor was a trait with a wrapper type? That would make it simple to create transactors for says Kyo/Cats effect etc...

russwyte · 2025-01-18T21:12:31Z

What I mean is - we could take good advantage of higher kinded types:

trait TransactorOps[T[_]]:
  def connect[A](f: DbCon ?=> A): T[A]
  def transact[A](f: DbTx ?=> A): T[A]

type Plain[A] = A
object Plain:
  given makePlain[A]: Conversion[A, Plain[A]] = a => a: Plain[A]

final case class Transactor(
    private val dataSource: DataSource,
    private val sqlLogger: SqlLogger = SqlLogger.Default,
    private val connectionConfig: Connection => Unit = con => ()
) extends TransactorOps[Plain]:

  def withSqlLogger(sqlLogger: SqlLogger) = copy(sqlLogger = sqlLogger)

  def withConnectionConfig(connectionConfig: Connection => Unit) =
    copy(connectionConfig = connectionConfig)

  def connect[T](f: DbCon ?=> T): T =
    Using.resource(dataSource.getConnection): con =>
      connectionConfig(con)
      f(using DbCon(con, sqlLogger))

  def transact[T](f: DbTx ?=> T): T =
    Using.resource(dataSource.getConnection): con =>
      connectionConfig(con)
      con.setAutoCommit(false)
      try
        val res = f(using DbTx(con, sqlLogger))
        con.commit()
        res
      catch
        case t =>
          con.rollback()
          throw t
end Transactor

russwyte · 2025-01-18T23:13:06Z

I added an experimental refactoring to demo a way to enforce operational consistency at a type level between Transactor implementations using the TransactorOps trait. Mostly for discussion 🗨️

build.sbt

guizmaii · 2025-01-19T00:39:32Z

build.sbt

+      "dev.zio" %% "zio-test" % "2.1.14" % Test,
+      "dev.zio" %% "zio-test-sbt" % "2.1.14" % Test,
+      "dev.zio" %% "zio-test-magnolia" % "2.1.14" % Test,
+      "org.testcontainers" % "postgresql" % "1.20.4" % Test,


🟡 Why not keeping testcontainers-scala-postgresql? 🤔

Totally could - this is just a pattern I am used to and my approach gives more direct control over the container scope (ZIO scope).

guizmaii · 2025-01-19T00:42:29Z

magnum-zio/src/main/scala/com/augustnagro/magnum/magzio/Transactor.scala

-    connectionConfig: Connection => Unit,
-    semaphore: Option[Semaphore]
-):
+final case class Transactor(


🟡 IMO case class represents data structures. This isn't a data structure so we don't need case here

Suggested change

final case class Transactor(

final class Transactor(

In ZIO most services are case classes in zio libraries. Services sort of are data structures. If you look in the Zionomicon and the ZIO layer documentation you will see good reasons for them to be case classes. Still it is not strictly required. One advantage is maintenance. Case class provides copy which is more ergonomic than needing to call a full constructor. Still this is not that important to me.

Yeah, I am thinking for a long time to change this documentation. I should have done it before.

IMO This is not a big issue at all - I am happy to use ordinary classes in cases like this.

guizmaii · 2025-01-19T00:45:40Z

magnum-zio/src/main/scala/com/augustnagro/magnum/magzio/Transactor.scala


-  def connect[A](f: DbCon ?=> A)(using Trace): Task[A] =


🔴 I think the need to remove the (using Trace) parameter here shows the limit of the common trait idea. I don't really agree with the idea. I think leaving each integration free of bringing the best integration to its environment without constraints is the best approach.

Why would we need this trait? It's not like, as a Magnum user, you'd use it anyway to abstract over any Transator implementation.

As a library author you would use it to generate a test kit - such that the tests are written to the trait and not to the impls - that makes it easier to plug the impls into the core tests without rewriting them.
To the library user they need not even know about the trait - notice the library still works just as it did before - a user need not care. But library maintainers often just want one set of core tests.

When I look at most ZIO services - I don't see (using trace) anywhere on service methods - I am not sure we need it at all. If we find we do - I can think of a couple of ways to support it.

guizmaii · 2025-01-19T00:45:54Z

magnum-zio/src/main/scala/com/augustnagro/magnum/magzio/Transactor.scala

-      connectionConfig,
-      semaphore
-    )
+    copy(sqlLogger = sqlLogger)


This is a benefit of services as case classes.

indeed but I still prefer it not to be a case class

No worries - I can respect that.

guizmaii · 2025-01-19T00:46:20Z

magnum-zio/src/main/scala/com/augustnagro/magnum/magzio/Transactor.scala

+    private val dataSource: DataSource,
+    private val sqlLogger: SqlLogger,
+    private val connectionConfig: Connection => Unit,
+    private val semaphore: Option[Semaphore]


Indeed these should be private 🙂

They are already private:

https://docs.scala-lang.org/tour/classes.html#:~:text=Parameters%20without%20val%20or%20var%20are%20private%20values%2C%20visible%20only%20within%20the%20class.

guizmaii · 2025-01-19T00:49:41Z

magnum-zio/src/main/scala/com/augustnagro/magnum/magzio/Transactor.scala

-    *   Number of threads in your connection pool. This helps magzio be more
-    *   memory efficient by limiting the number of blocking pool threads used.
-    *   Not needed if using the ZIO virtual-thread based blocking executor
    * @return
-    *   Transactor UIO
+    *   The transactor layer
    */
-  def layer(
-      dataSource: DataSource,
-      sqlLogger: SqlLogger,
-      connectionConfig: Connection => Unit,
-      maxBlockingThreads: Option[Int]
-  ): ULayer[Transactor] =
-    ZLayer.fromZIO {
-      ZIO
-        .fromOption(maxBlockingThreads)
-        .flatMap(threads => Semaphore.make(threads))
-        .unsome
-        .map(semaphoreOpt =>
-          new Transactor(
-            dataSource,
-            sqlLogger,
-            connectionConfig,
-            semaphoreOpt
-          )
-        )
+  def configured(
+      sqlLogger: SqlLogger = SqlLogger.Default,
+      connectionConfig: Connection => Unit = noOpConnectionConfig,
+      maxBlockingThreads: Int = -1
+  ) =
+    val semaphore = ZLayer {
+      if maxBlockingThreads < 1 then ZIO.succeed(None)
+      else Semaphore.make(maxBlockingThreads).map(Some(_))
    }
+    defaultLogger ++ defaultConnectionConfig ++ semaphore >>> layer

-  /** Construct a Transactor
-    *
-    * @param dataSource
-    *   Datasource to be used
-    * @param sqlLogger
-    *   Logging configuration
-    * @param connectionConfig
-    *   Customize the underlying JDBC Connections
-    * @return
-    *   Transactor UIO
-    */
-  def layer(
-      dataSource: DataSource,
-      sqlLogger: SqlLogger,
-      connectionConfig: Connection => Unit
-  ): ULayer[Transactor] =
-    layer(
-      dataSource,
-      sqlLogger,
-      connectionConfig,
-      None
-    )
-
-  /** Construct a Transactor
-    *
-    * @param dataSource
-    *   Datasource to be used
-    * @param sqlLogger
-    *   Logging configuration
-    * @return
-    *   Transactor UIO
-    */
-  def layer(dataSource: DataSource, sqlLogger: SqlLogger): ULayer[Transactor] =
-    layer(dataSource, sqlLogger, noOpConnectionConfig, None)
-
-  /** Construct a Transactor
-    *
-    * @param dataSource
-    *   Datasource to be used
-    * @return
-    *   Transactor UIO
-    */
-  def layer(dataSource: DataSource): ULayer[Transactor] =
-    layer(dataSource, SqlLogger.Default, noOpConnectionConfig, None)
-
-  /** Construct a Transactor
-    *
-    * @param dataSource
-    *   Datasource to be used
-    * @param connectionConfig
-    *   Customize the underlying JDBC Connections
-    * @return
-    *   Transactor UIO
+  /** The default transactor layer with all dependencies satisfied except for
+    * the DataSource.
    */
-  def layer(
-      dataSource: DataSource,
-      connectionConfig: Connection => Unit
-  ): ULayer[Transactor] =
-    layer(dataSource, SqlLogger.Default, connectionConfig, None)
+  val default: ZLayer[DataSource, Nothing, Transactor] =
+    defaultLogger ++ defaultConnectionConfig ++ defaultSemaphore >>> layer


🔴 I don't understand these changes.
I'd prefer we keep as it was.

1 issue amongst others in your code: the configured function takes a sqlLogger: SqlLogger = SqlLogger.Default, parameter but never use it

Let me refine that a bit - this was sort of a half baked concept.... :)

guizmaii · 2025-01-19T00:51:27Z

@russwyte Gave some feedback. IMO, it'd be better if you separate the migration to zio-test and the trait idea into 2 separate PRs

russwyte · 2025-01-19T13:31:34Z

I tend to agree that this PR is over-broad. 😅

I am happy to close this PR as I mostly just wanted to learn by example and discussion.

A couple of notes on why I started this weekend coding blitz and some learning I got from it.

The idea of a test kit @AugustNagro had is a great one. The trait in this PR came about in trying to imagine a way to create such a test kit without impacting the library user at all. The library users see exactly the same API as before - but library maintainers have more universally applicable Transactor trait for tests - or even conversion from one typeclass to another.
IMO Right now the in ZIO module (excellent work @guizmaii) the tests seem to me way too heavy and duplicative - it should only test the core methods connect and transact - other tests should be done across transactors using a core testkit that completely exercises the core API.

Thanks for this opportunity to learn! Great work on this so far!

russwyte · 2025-01-19T20:17:57Z

magnum-zio/src/test/scala/com/augustnagro/magnum/magzio/GenericSpec.scala

@guizmaii @AugustNagro This what I was trying to facilitate with the trait. I needed only write the specs once to test them against both transactors.

russwyte · 2025-01-20T04:41:40Z

Soon I intend to refine my idea into a new "test" (testkit) module that tests everything (like the root project) against any TransactorOps type class. This could make this library widely accessible - from those who want "pure" Scala to those who use any sort of effect or typeclass F[_]

russwyte · 2025-01-20T14:06:58Z

Closing in favor of a more focused testing PR to come later

AugustNagro · 2025-02-01T23:23:13Z

Very interesting @russwyte and that's an awesome end result. I would agree with Jules to the extent that we should try to see if we can scrap the test boilerplate while keeping the Transactor definition the same, since

We shouldn't have to change impl just to simplify tests
It makes sense to have Transactor be a trait if we expect users will Mock it, but users should mock at the respository level instead (or not mock at all if possible)

However, to reduce the scope of your first contribution, can you create a PR with just the change to zio-test? That would be very helpful in and of itself.

russwyte added 4 commits January 18, 2025 11:34

use zio-test and bump zio

5b46763

dead code

5bc0855

dead code

219ebfb

make tests more modular

7b6913e

clarify

f07d8ef

russwyte added 3 commits January 18, 2025 16:53

experimental refactoring of Transactor - will revert if desired

60132fb

cleanup

dd3f0be

0 makes no sense

669d4ac

russwyte changed the title ~~Use zio-test and bump zio~~ Use Transactor tweaks zio-test and bump zio Jan 18, 2025

guizmaii reviewed Jan 19, 2025

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

guizmaii reviewed Jan 19, 2025

View reviewed changes

russwyte changed the title ~~Use Transactor tweaks zio-test and bump zio~~ Transactor work, add zio-test, and bump zio Jan 19, 2025

add GenericSpec for discussion

d9b0ab1

russwyte commented Jan 19, 2025

View reviewed changes

russwyte added 3 commits January 19, 2025 14:30

fix layers

6ce94e7

not needed

a18ef22

not needed

ae07ae4

russwyte closed this Jan 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transactor work, add zio-test, and bump zio #95

Transactor work, add zio-test, and bump zio #95

russwyte commented Jan 18, 2025 •

edited

Loading

russwyte commented Jan 18, 2025 •

edited

Loading

russwyte commented Jan 18, 2025

russwyte commented Jan 18, 2025 •

edited

Loading

russwyte commented Jan 18, 2025

guizmaii Jan 19, 2025 •

edited

Loading

russwyte Jan 19, 2025

guizmaii Jan 19, 2025 •

edited

Loading

russwyte Jan 19, 2025

guizmaii Jan 19, 2025

russwyte Jan 20, 2025

guizmaii Jan 19, 2025 •

edited

Loading

russwyte Jan 19, 2025

russwyte Jan 19, 2025

guizmaii Jan 19, 2025

russwyte Jan 19, 2025

guizmaii Jan 19, 2025

russwyte Jan 20, 2025

guizmaii Jan 19, 2025

AugustNagro Feb 1, 2025

guizmaii Jan 19, 2025 •

edited

Loading

russwyte Jan 19, 2025

guizmaii commented Jan 19, 2025

russwyte commented Jan 19, 2025 •

edited

Loading

russwyte Jan 19, 2025

russwyte commented Jan 20, 2025 •

edited

Loading

russwyte commented Jan 20, 2025

AugustNagro commented Feb 1, 2025

Transactor work, add zio-test, and bump zio #95

Transactor work, add zio-test, and bump zio #95

Conversation

russwyte commented Jan 18, 2025 • edited Loading

russwyte commented Jan 18, 2025 • edited Loading

russwyte commented Jan 18, 2025

russwyte commented Jan 18, 2025 • edited Loading

russwyte commented Jan 18, 2025

guizmaii Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii commented Jan 19, 2025

russwyte commented Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

russwyte commented Jan 20, 2025 • edited Loading

russwyte commented Jan 20, 2025

AugustNagro commented Feb 1, 2025

russwyte commented Jan 18, 2025 •

edited

Loading

russwyte commented Jan 18, 2025 •

edited

Loading

russwyte commented Jan 18, 2025 •

edited

Loading

guizmaii Jan 19, 2025 •

edited

Loading

guizmaii Jan 19, 2025 •

edited

Loading

guizmaii Jan 19, 2025 •

edited

Loading

guizmaii Jan 19, 2025 •

edited

Loading

russwyte commented Jan 19, 2025 •

edited

Loading

russwyte commented Jan 20, 2025 •

edited

Loading