search

oyvindberg / typo

Typed Postgresql integration for Scala. Hopes to avoid typos
https://oyvindberg.github.io/typo/
MIT License
99 stars 9 forks source link

Implement `zio-jdbc` backend #57

Closed guizmaii closed 10 months ago

guizmaii commented 10 months ago

Adapted from the doobie backend

Needs:

oyvindberg commented 10 months ago

Thanks @guizmaii - you're fast!

I noticed you found my link to DbLib on twitter. The task is basically to implement that. Here are some pointers:

oyvindberg commented 10 months ago

If you're bothered by the files in .bleep/generated-sources they are created as part of the compilation process by the sourcegen script (scripts.GenHardcodedFiles) in the bleep build. You can comment them out there if needed during development.

I track all generated files in git as a kind of snapshot tests. I also continuously commit them to git as I work and use git diff to preview the effect my changes have. It's a very, very powerful technique for working on code generation code.

GenHardcodedFiles is the fast one, so it runs automatically, and GeneratedAdventureWorks is the slow one which needs a database connection.

guizmaii commented 10 months ago

I track all generated files in git as a kind of snapshot tests. I also continuously commit them to git as I work and use git diff to preview the effect my changes have. It's a very, very powerful technique for working on code generation code.

Yup, we completely agree on this. I do the same thing 🙂

I'm not committing them for now to avoid having a huuuuuuuge PR

oyvindberg commented 10 months ago

One more thing to be aware of in order to not waste time. bleep supports mounting a subset of projects in your IDE, so you would typically just mount for just one scala version and maybe not the test code for anorm/doobie. this way your compile round-trip will be much shorter

oyvindberg commented 10 months ago

We may have to implement our own typeclass like anorm's ParameterMetaData to correctly implement SqlExpr.Const

guizmaii commented 10 months ago

Here's a list of errors I'm blocked on:

oyvindberg commented 10 months ago

I suggest you implement JdbcDecoder for row types by yourself by generating implicit lazy val jdbcDecoder: JdbcDecoder[RowType] = JdbcDecoder.apply(...) instead of using Schema, it'll be very similar to what is done for doobie.Read, see https://github.com/oyvindberg/typo/blob/main/typo/src/scala/typo/internal/codegen/DbLibDoobie.scala#L564

oyvindberg commented 10 months ago

Failed to derive schema for testdb.hardcoded.myschema.Sector. Can only derive Schema for case class or sealed trait

same as last comment, I suggest you try to skip Schema and generate what you need directly. If you do need Schema for something you can add it in stringEnumInstances

oyvindberg commented 10 months ago

could not find implicit value for parameter setter: zio.jdbc.SqlFragment.Setter[adventureworks.customtypes.TypoLocalDateTime]

This is a custom type (called typo types in the documentation, I'll settle on one soon), implemented in customTypeInstances. The essence is that it's a mapping where one type is passed through JDBC, and another type is exposed through the generated code. For instance TypoInstant is passed as strings to make arrays work.

guizmaii commented 10 months ago

Thanks a lot for your help @oyvindberg 🙏

oyvindberg commented 10 months ago

Failed to derive schema for Array[String]. Can only derive Schema for case class or sealed trait

It looks like zio-jdbc does not support arrays. Kind of makes sense, all the other database libraries expose custom postgres integration modules to handle postgres-specific things like arrays. That's likely also why they haven't added sqlType as a string inside the typeclasses yet (remember I hinted at adding a ParameterMetaData typeclass above). With postgres you need this sqlType string to construct empty arrays, for instance.

I suggest you do arrays last, I'll help out with the details there unless you have an idea of the problem space.

In order to make things compile you can add a bogus implementation of JdbcDecoder[Array[T]] and the encoder in missingInstances which throws an exception on usage. It'll look a lot like this generic instance.

missingInstances are really for things which could be upstreamed to the database libraries, but I didn't want that roundtrip until things settled down in typo.

oyvindberg commented 10 months ago

Thanks a lot for your help @oyvindberg 🙏

Thanks for taking this on, bet you got a bigger challenge than you signed up for, ha? :D

guizmaii commented 10 months ago

Thanks for taking this on, bet you got a bigger challenge than you signed up for, ha? :D

How do you know? 😂

guizmaii commented 10 months ago

Things I'm blocked on:

oyvindberg commented 10 months ago

CustomCreditcardId and FirstName are manually written! :D

Sorry for mixing so many things in one test folder instead of small narrow unit tests, but it has been important for me to test as much as possible with little effort. Also all this code needs to be compiled in order to be tested, so here we are

guizmaii commented 10 months ago

CustomCreditcardId and FirstName are manually written! :D

Ahah! Thanks Easy to fix then :)

guizmaii commented 10 months ago

I suggest you do arrays last, I'll help out with the details there unless you have an idea of the problem space.

I have no idea 😕

oyvindberg commented 10 months ago

As for the Map[String, String], you can map it to a Java Map and put it into a PreparedStatement with setObject. IOW the postgres driver supports it directly

guizmaii commented 10 months ago

My implementation of customTypeInstances is very ad-hoc. It can be improved later

guizmaii commented 10 months ago

Left for tomorrow:

guizmaii commented 10 months ago

IOW the postgres driver supports it directly

@oyvindberg How can we know the data structures supported by the driver?

oyvindberg commented 10 months ago

Left for tomorrow:

  • [ ] JdbcDecoder[List[adventureworks.customtypes.TypoPoint]] 
    could not find implicit value for parameter decoder: zio.jdbc.JdbcDecoder[List[adventureworks.customtypes.TypoPoint]] [value => typo/typo-tester-zio-jdbc/generated-and-checked-in/adventureworks/customtypes/TypoPath.scala:29:19 until 29:47]

This is very likely wrong because custom type instances are implemented incorrectly. It should not be a product type, but refer to fromTypo - check Doobie implementation. Crucially they always refer to one column in postgres, even though there may be a list encoded inside the values in that column

oyvindberg commented 10 months ago

IOW the postgres driver supports it directly

@oyvindberg How can we know the data structures supported by the driver?

Only by inspecting the source code I'm afraid. I help dig it up later if needed

oyvindberg commented 10 months ago

I pushed a commit with custom types now, see commit text for some details.

there is still arrays missing, and it's untested but it may work

guizmaii commented 10 months ago

It compiles! 🎉

image
oyvindberg commented 10 months ago

Woho!

guizmaii commented 10 months ago

It still needs this PR https://github.com/zio/zio-jdbc/pull/171 to compile in CI I'll implement the test and ask for a review from ZIO people

oyvindberg commented 10 months ago

I'll look at this more tonight hopefully, but will share some observations as I come across them until then 👍

pushed a commit now to build with a published snapshot

oyvindberg commented 10 months ago 
  def insert(unsaved: PersonRow): ZIO[ZConnection, Throwable, UpdateResult[PersonRow]]
  def insert(unsaved: PersonRowUnsaved): ZIO[ZConnection, Throwable, UpdateResult[PersonRow]]

can we drop UpdateResult here and just return the row? We know there will 1 changed row

oyvindberg commented 10 months ago

does withConnection work for you? it appears to be hanging when trying to run the tests

guizmaii commented 10 months ago

does withConnection work for you? it appears to be hanging when trying to run the tests

Yes, it's hanging. I probably have made a mistake there. I'm finishing to write the tests on the zio-jdbc PR and I'll fix this

oyvindberg commented 10 months ago

there is something seriously wrong with this fragment data type/interpolator.

sql"${unsaved.name}" ++ sql"::varchar" is not the same as sql"${unsaved.name}::varchar"

guizmaii commented 10 months ago

there is something seriously wrong with this fragment data type/interpolator.

Where does it come from?

guizmaii commented 10 months ago

All PRs in zio-jdbc are merged and I updated the snapshot version in the project. The PR should compile on your computer too now 🙂

oyvindberg commented 10 months ago

It already did actually, but great job on that anyway! :)

When you fix withConnection you'll discover that the tests are broken with invalid sql. The first thing I looked into brought me to what I assume is a bug in the zio-jdbc sql interpolator

guizmaii commented 10 months ago

When you fix withConnection you'll discover that the tests are broken with invalid sql. The first thing I looked into brought me to what I assume is a bug in the zio-jdbc sql interpolator

Yeah, I wasn't expecting them to pass on the first try 😄 I'll have a look tomorrow. It's late here

guizmaii commented 10 months ago

does withConnection work for you? it appears to be hanging when trying to run the tests

Fixed. The connection info were wrong and the default config of the ZConnectioPool were retrying forever. See https://github.com/zio/zio-jdbc/pull/174

guizmaii commented 10 months ago

Identified issues still present:

This is most probably why the EmployeeTest fails

oyvindberg commented 10 months ago

all the tests are written with the assumption that transactions are rolled back. if you have 34 rows it means they are not.

can you recreate the database and rerun tests and see which errors persist?

oyvindberg commented 10 months ago

The ArrayTest.not nulls test fails with None did not equal Some(List()) (ArrayTest.scala:256)

  implicit lazy val jdbcArraySetter: Setter[Array[TypoHStore]] = Setter.forSqlType((ps, i, v) =>
    if (v.isEmpty) ps.setNull(i, Types.ARRAY)
    else

empty array should not be null, just delete the if and keep the else clause

oyvindberg commented 10 months ago

pushed a commit which changes when we disable auto-commit for tests

oyvindberg commented 10 months ago

In EmployeeRepoImpl, the selectByIds operating on an Array[BusinessentityId] doesn't generate the correct SQL query

pushed a fix for this, was missing some interpolation

oyvindberg commented 10 months ago

ArrayTest.work test fails with org.postgresql.util.PSQLException: The column index is out of range: 42, number of columns: 41.

this will require a bit more digging. Parts of jdbc is 1-indexed IIRC, maaybe zio-jdbc uses zero indexing for that. Gonna have to dig a bit more into this

oyvindberg commented 10 months ago

SqlInterpolator does not have stripMargin, that breaks some DSL usage. I'll make the generated sql a bit uglier for now

oyvindberg commented 10 months ago

SqlFragment.where seems broken. we're getting queries like this:

typo-db-1  | 2023-10-27 18:25:17.174 UTC [4107] ERROR:  syntax error at or near "=" at character 137
typo-db-1  | 2023-10-27 18:25:17.174 UTC [4107] STATEMENT:  select "user_id", "name", "last_name", "email"::text, "password", "created_at"::text, "verified_on"::text from public.usersWHEREuser_id = $1
oyvindberg commented 10 months ago

something is wrong with the column index computation, at least when used through the DSL.

Look at this monster query:

Sql(select product1.productid, product1.name, product1.productnumber, product1.makeflag, product1.finishedgoodsflag, product1.color, product1.safetystocklevel, product1.reorderpoint, product1.standardcost, product1.listprice, product1.size, product1.sizeunitmeasurecode, product1.weightunitmeasurecode, product1.weight, product1.daystomanufacture, product1.productline, product1.class, product1.style, product1.productsubcategoryid, product1.productmodelid, product1.sellstartdate, product1.sellenddate, product1.discontinueddate, product1.rowguid, product1.modifieddate, unitmeasure2.unitmeasurecode, unitmeasure2.name, unitmeasure2.modifieddate, productmodel3.productmodelid, productmodel3.name, productmodel3.catalogdescription, productmodel3.instructions, productmodel3.rowguid, productmodel3.modifieddate
                  from (
                    select "productid", "name", "productnumber", "makeflag", "finishedgoodsflag", "color", "safetystocklevel", "reorderpoint", "standardcost", "listprice", "size", "sizeunitmeasurecode", "weightunitmeasurecode", "weight", "daystomanufacture", "productline", "class", "style", "productsubcategoryid", "productmodelid", "sellstartdate"::text, "sellenddate"::text, "discontinueddate"::text, "rowguid", "modifieddate"::text from production.product WHERE class = ? AND daystomanufacture > ? OR daystomanufacture <= ? AND productline = ? 
                  ) product1
                  left join (
                  select "unitmeasurecode", "name", "modifieddate"::text from production.unitmeasure WHERE name LIKE ? 
                  ) unitmeasure2 on product1.sizeunitmeasurecode = unitmeasure2.unitmeasurecode
                  left join (
                  select "productmodelid", "name", "catalogdescription", "instructions", "rowguid", "modifieddate"::text from production.productmodel
                  ) productmodel3 on product1.productmodelid = productmodel3.productmodelid
                   WHERE product1.productmodelid = productmodel3.productmodelid  ORDER BY product1.productmodelid ASC ,productmodel3.name DESC NULLS FIRST , H , 25, 0, foo, name%)

One of the ProductTest cases fails when trying to decode a UnitmeasureRow from columnIndex 2. I suppose we need to return the number of consumed columns somewhere

oyvindberg commented 10 months ago

zio.jdbc.SqlFragment.Setter.arraySetter does absolutely the wrong thing for us. it creates one sql parameter for each value in the array. Other libraries like doobie provide this functionality for List and other scala sequence types. It'd be nice if zio-jdbc could reserve the usage of Array for passing actual arrays as parameters.

guizmaii commented 10 months ago

something is wrong with the column index computation, at least when used through the DSL.

Code at the origin of this invalid SQL code (just to document it):

        query = productRepo.select
          .where(_.`class` === "H ")
          .where(x => x.daystomanufacture > 25 or x.daystomanufacture <= 0)
          .where(x => x.productline === "foo")
          .join(unitmeasureRepo.select.where(_.name.like("name%")))
          .on { case (p, um) => p.sizeunitmeasurecode === um.unitmeasurecode }
          .join(projectModelRepo.select)
          .leftOn { case ((product, _), productModel) => product.productmodelid === productModel.productmodelid }
          .where { case ((product, _), productModel) => product.productmodelid === productModel(_.productmodelid) }
          .orderBy { case ((product, _), _) => product.productmodelid.asc }
          .orderBy { case ((_, _), productModel) => productModel(_.name).desc.withNullsFirst }

Edit: I debugged this query and it seems to be working fine. Giving these results when executed:

Chunk(((ProductRow(ProductId(5),Name(name),productnumber,Flag(true),Flag(true),Some(color),TypoShort(16),TypoShort(18),20,22,None,Some(UnitmeasureId(kgg)),Some(UnitmeasureId(kgg)),Some(1.00),26,Some(T ),Some(H ),Some(W ),Some(ProductsubcategoryId(5)),Some(ProductmodelId(8)),TypoLocalDateTime(2023-10-29T12:28:27),Some(TypoLocalDateTime(2023-11-07T12:28:27)),Some(TypoLocalDateTime(2024-02-05T12:28:27)),TypoUUID(49461518-45a1-4fe5-b974-f7130feebc8e),TypoLocalDateTime(2023-10-27T12:28:27.731710)),UnitmeasureRow(UnitmeasureId(kgg),Name(name),TypoLocalDateTime(2023-10-28T12:28:27.542541))),Some(ProductmodelRow(ProductmodelId(8),Name(name),Some(TypoXml(<xml/>)),Some(TypoXml(<instructions/>)),TypoUUID(f7eeb3a8-756b-11ee-be21-0242ac120002),TypoLocalDateTime(2023-10-28T12:28:27.542541)))))
guizmaii commented 10 months ago

Here's the request failing the ProductTest:

Sql(UPDATE production.product name = substring(upper(reverse(name)) || ?, ?, ?)::varcharlistprice = ?::numericreorderpoint = reorderpoint + ?::int2  WHERE coalesce(productid = ?, ?)  returning "productid", "name", "productnumber", "makeflag", "finishedgoodsflag", "color", "safetystocklevel", "reorderpoint", "standardcost", "listprice", "size", "sizeunitmeasurecode", "weightunitmeasurecode", "weight", "daystomanufacture", "productline", "class", "style", "productsubcategoryid", "productmodelid", "sellstartdate"::text, "sellenddate"::text, "discontinueddate"::text, "rowguid", "modifieddate"::text, flaff, 2, 4, 2, TypoShort(22), 6, false)

The code origin of this query being:

        update = productRepo.update
          .setComputedValue(_.name)(p => (p.reverse.upper || Name("flaff")).substring(2, 4))
          .setValue(_.listprice)(BigDecimal(2))
          .setComputedValue(_.reorderpoint)(_ plus TypoShort(22))
//          .setComputedValue(_.sizeunitmeasurecode)(_ => Some(unitmeasure.unitmeasurecode))
          .where(_.productid === saved1.productid)

        _ <- ZIO.succeed(update.sql(returning = true).foreach(println(_)))

Seems to be missing some , between the updated fields Also, the syntax is incorrect. Should be:

UPDATE table
SET ...

we miss the ... table SET ... part apparently

Edit: This update issue is fixed now. The test is still failling but with a different error:

📗 BSP: - pg *** FAILED ***
📗 BSP:   org.postgresql.util.PSQLException: No value specified for parameter 10.

🙂

oyvindberg commented 10 months ago

I created https://github.com/zio/zio-jdbc/pull/180 and https://github.com/zio/zio-jdbc/pull/181 . First is a blocker, second will allow us to generate nicer code, as well as improving zio-jdbc surface