json array implicits for doobie #132

salamonpavel · 2024-07-09T13:15:42Z

Introduced package for postgres related implicits for working with Circe's Json.
Closes #111

github-actions · 2024-07-09T13:17:19Z

JaCoCo `slick` module code coverage report - scala 2.13.11

Overall Project	87.73%	🍏

There is no coverage information present for the Files changed

github-actions · 2024-07-10T06:20:37Z

JaCoCo `core` module code coverage report - scala 2.13.11

Overall Project	57.44%	🍏

There is no coverage information present for the Files changed

github-actions · 2024-07-10T06:20:38Z

JaCoCo `doobie` module code coverage report - scala 2.13.11

Overall Project	70.6% `-16.46%`	🍏
Files changed	56.19%	❌

File	Coverage
package.scala	42.04% `-59.18%`	❌

lsulak · 2024-07-10T10:43:40Z

doobie/src/main/scala/za/co/absa/db/fadb/doobie/postgres/circe/implicits/package.scala

+      .temap(pgArray => pgArrayToListOfJson(pgArray))
+  }
+
+  implicit val jsonbArrayPut: Put[List[Json]] = {


what about JSONB get utils?

It seems to work for both :D

really?

because the GET implements json[]:

implicit val jsonArrayGet: Get[List[Json]] = { Get.Advanced .other[PgArray]( NonEmptyList.of("json[]") ) .temap(pgArray => pgArrayToListOfJson(pgArray)) }

perhaps for the sake of consistency in the naming we should either: implement the second one with jsonb[] or name this one somehow so that it's understandable that it's also for JSONB GET?

or just something like this :D

implicit val jsonbArrayGet: Get[List[Json]] = jsonArrayGet

I would just mention it in the comments (documentation) of the classes. Which btw, is missing somewhat 👼

discussed on a call

implemented like this after discussion on a call

// to be used for both json[] and jsonb[] as it handles well both and we want to avoid collision // when resolving implicits implicit val jsonOrJsonbArrayGet: Get[List[Json]] = { Get.Advanced .other[PgArray]( NonEmptyList.of("json[]") ) .temap(pgArray => pgArrayToListOfJson(pgArray)) }

Somwhat longer but acceptable, IMHO.

lsulak · 2024-07-10T10:45:27Z

doobie/src/main/scala/za/co/absa/db/fadb/doobie/postgres/circe/implicits/package.scala

+
+import scala.util.{Failure, Success, Try}
+
+package object implicits {


also, what about JSON/JSONB put and get? (these here are List of JSON/JSONB, so I'm talking about the non-array variant)

the non-array variant is already available, see the docs https://github.com/tpolecat/doobie/blob/v1.0.0-RC2/modules/docs/src/main/mdoc/docs/15-Extensions-PostgreSQL.md

hmm okay! Thanks

I've made it available also in the implicits object so we can import everything from the same place.

lsulak · 2024-07-10T10:52:14Z

demo_database/src/main/postgres/integration/V1.2.18__retrieve_actors_jsonb.sql

+CREATE OR REPLACE FUNCTION integration.retrieve_actors_jsonb(idUntil INT)
+    RETURNS TABLE(actors_jsonb JSONB[]) AS $$
+BEGIN
+    RETURN QUERY SELECT a.actors_jsonb FROM integration.actors_json_seq AS a WHERE id <= idUntil;


I like the <= idea :)

benedeki · 2024-07-11T01:23:00Z

doobie/src/test/scala/za/co/absa/db/fadb/doobie/JsonArrayIntegrationTests.scala

+        val actorsAsJsonList = values.map(_.asJson)
+        Seq(
+          {
+            import za.co.absa.db.fadb.doobie.postgres.circe.implicits.jsonArrayPut


Isn't it enough now, to import once and on the top level?

It would be enough to import it once on the top of the file alongside other imports had we had only json[] or jsonb[]. In this case though we have both and given the way implicits are resolved by their types the compiler is unable to figure out which one to use. That's why in this particular case the imports have to be scope using the curly braces for individual query fragments.

Maybe that's also the reason why doobie doesn't support the array variants by default...

Ooh, I missed that they a different import (the one letter difference). 😊
Tiny thing, if you would want to add an explanatory comment why the import is done like this

lsulak

I think it's good to go, last few details we discussed on a call. Once the CI checks pass, I'm happy for you to merge it (or wait for someone else from curation for additional review if you want)

benedeki

I believe it's merge-able.
Unless you want to add the code comments, particularly ScalaDoc to the implicits.

benedeki · 2024-07-11T08:49:50Z

doobie/src/test/scala/za/co/absa/db/fadb/doobie/JsonArrayIntegrationTests.scala

+        val actorsAsJsonList = values.map(_.asJson)
+        Seq(
+          {
+            import za.co.absa.db.fadb.doobie.postgres.circe.implicits.jsonArrayPut


Ooh, I missed that they a different import (the one letter difference). 😊
Tiny thing, if you would want to add an explanatory comment why the import is done like this

benedeki · 2024-07-11T08:55:38Z

doobie/src/main/scala/za/co/absa/db/fadb/doobie/postgres/circe/implicits/package.scala

+      .temap(pgArray => pgArrayToListOfJson(pgArray))
+  }
+
+  implicit val jsonbArrayPut: Put[List[Json]] = {


I would just mention it in the comments (documentation) of the classes. Which btw, is missing somewhat 👼

benedeki · 2024-07-11T08:56:32Z

.github/workflows/jacoco_report.yml

@@ -69,7 +69,7 @@ jobs:
          token: ${{ secrets.GITHUB_TOKEN }}
          min-coverage-overall: ${{ env.coverage-overall }}
          min-coverage-changed-files: ${{ env.coverage-changed-files }}
-          title: JaCoCo `model` module code coverage report - scala ${{ env.scalaLong }}
+          title: JaCoCo `core` module code coverage report - scala ${{ env.scalaLong }}


Thanks for this fix, good boy scout 👍

benedeki · 2024-07-11T10:07:03Z

doobie/src/main/scala/za/co/absa/db/fadb/doobie/postgres/circe/implicits/package.scala

+      .temap(pgArray => pgArrayToListOfJson(pgArray))
+  }
+
+  implicit val jsonbArrayPut: Put[List[Json]] = {


Somwhat longer but acceptable, IMHO.

salamonpavel · 2024-07-12T08:24:51Z

Release notes:

Introduced implicits for storing and retrieving json and jsonb arrays from postgres using Circe's Json.

json array implicits for doobie

56889c0

salamonpavel requested review from benedeki, dk1844, jakipatryk, lsulak, Zejnilovic and miroslavpojer as code owners July 9, 2024 13:15

filename change

f16a2ff

salamonpavel added 7 commits July 9, 2024 15:17

Merge branch 'master' into feature/111-implicits

aa24a0a

license

487d18c

refactoring

bfb5d70

refactoring

e8d8372

refactoring

f0cb4a6

refactoring

5493ab5

fix titles for jacoco reports

e425aff

lsulak reviewed Jul 10, 2024

View reviewed changes

benedeki reviewed Jul 11, 2024

View reviewed changes

jsonOrJsonbArrayGet

f7b6bdd

lsulak previously approved these changes Jul 11, 2024

View reviewed changes

jsonOrJsonbArrayGet

b02ef12

salamonpavel dismissed lsulak’s stale review via b02ef12 July 11, 2024 09:06

lsulak previously approved these changes Jul 11, 2024

View reviewed changes

benedeki previously approved these changes Jul 11, 2024

View reviewed changes

make default implicits available via the same package

4f516ab

salamonpavel dismissed benedeki’s stale review via 4f516ab July 11, 2024 10:20

salamonpavel dismissed lsulak’s stale review via 4f516ab July 11, 2024 10:20

lsulak previously approved these changes Jul 11, 2024

View reviewed changes

minors

d255f6e

salamonpavel dismissed lsulak’s stale review via d255f6e July 11, 2024 10:53

lsulak approved these changes Jul 11, 2024

View reviewed changes

AbsaOSS deleted a comment from github-actions bot Jul 12, 2024

lsulak merged commit 18b2106 into master Jul 12, 2024
6 of 7 checks passed

lsulak deleted the feature/111-implicits branch July 12, 2024 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

json array implicits for doobie #132

json array implicits for doobie #132

salamonpavel commented Jul 9, 2024

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 10, 2024 •

edited

Loading

lsulak Jul 10, 2024

salamonpavel Jul 11, 2024

lsulak Jul 11, 2024

lsulak Jul 11, 2024

benedeki Jul 11, 2024

lsulak Jul 11, 2024

salamonpavel Jul 11, 2024 •

edited

Loading

benedeki Jul 11, 2024

lsulak Jul 10, 2024

salamonpavel Jul 11, 2024

lsulak Jul 11, 2024

salamonpavel Jul 11, 2024

lsulak Jul 10, 2024

benedeki Jul 11, 2024

salamonpavel Jul 11, 2024 •

edited

Loading

benedeki Jul 11, 2024

lsulak left a comment

benedeki left a comment

benedeki Jul 11, 2024

benedeki Jul 11, 2024

benedeki Jul 11, 2024

benedeki Jul 11, 2024

salamonpavel commented Jul 12, 2024


		import scala.util.{Failure, Success, Try}

		package object implicits {

json array implicits for doobie #132

json array implicits for doobie #132

Conversation

salamonpavel commented Jul 9, 2024

github-actions bot commented Jul 9, 2024 • edited Loading

JaCoCo slick module code coverage report - scala 2.13.11

github-actions bot commented Jul 10, 2024 • edited Loading

JaCoCo core module code coverage report - scala 2.13.11

github-actions bot commented Jul 10, 2024 • edited Loading

JaCoCo doobie module code coverage report - scala 2.13.11

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

salamonpavel Jul 11, 2024 • 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

salamonpavel Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsulak left a comment

Choose a reason for hiding this comment

benedeki left a 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

salamonpavel commented Jul 12, 2024

github-actions bot commented Jul 9, 2024 •

edited

Loading

JaCoCo `slick` module code coverage report - scala 2.13.11

github-actions bot commented Jul 10, 2024 •

edited

Loading

JaCoCo `core` module code coverage report - scala 2.13.11

github-actions bot commented Jul 10, 2024 •

edited

Loading

JaCoCo `doobie` module code coverage report - scala 2.13.11

salamonpavel Jul 11, 2024 •

edited

Loading

salamonpavel Jul 11, 2024 •

edited

Loading